Re: [Qemu-devel] [PATCH 1/2] ISCSI: redo how we set up the events to only call qemu_aio_set_fd_handler() and qemu_notify_event() if something has changed.

[ronnie sahlberg <ronniesahlberg@gmail.com>]

On Fri, May 25, 2012 at 7:59 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 25/05/2012 09:43, Ronnie Sahlberg ha scritto:
>> Also first call out to the socket write functions director, and only set up the write event if the socket is full.
>>
>> This means that we will only need to invoke these two functions very rarely which will improve performance.
>>
>> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> ---
>>  block/iscsi.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++----------
>>  1 files changed, 46 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index ed1ad7b..9642ee6 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -40,6 +40,8 @@ typedef struct IscsiLun {
>>      int lun;
>>      int block_size;
>>      unsigned long num_blocks;
>> +    IOHandler *read_handler;
>> +    IOHandler *write_handler;
>>  } IscsiLun;
>>
>>  typedef struct IscsiAIOCB {
>> @@ -105,18 +107,50 @@ static void
>>  iscsi_set_events(IscsiLun *iscsilun)
>>  {
>>      struct iscsi_context *iscsi = iscsilun->iscsi;
>> +    int need_set_fd = 0;
>> +    int need_notify_event = 0;
>>
>> -    qemu_aio_set_fd_handler(iscsi_get_fd(iscsi), iscsi_process_read,
>> -                           (iscsi_which_events(iscsi) & POLLOUT)
>> -                           ? iscsi_process_write : NULL,
>> -                           iscsi_process_flush, iscsilun);
>> -
>> -    /* If we just added the event for writeable we must call
>> -       and the socket is already writeable the callback might
>> -       not be invoked until after a short delay unless we call
>> -       qemu_notify_event().
>> +    /* Try to write as much as we can to the socket
>> +     * without setting up an event
>>       */
>> -    qemu_notify_event();
>> +    if (iscsi_which_events(iscsi) & POLLOUT) {
>> +        iscsi_process_write(iscsilun);
>> +    }
>> +
>> +    if (iscsilun->read_handler == NULL) {
>> +        iscsilun->read_handler = iscsi_process_read;
>> +        need_set_fd = 1;
>> +        need_notify_event = 1;
>> +    }
>
> This is not needed, just pass iscsi_process_read unconditionally to
> qemu_aio_set_fd_handler.
>
>> +    if (iscsi_which_events(iscsi) & POLLOUT) {
>> +        if (iscsilun->write_handler == NULL) {
>> +            iscsilun->write_handler = iscsi_process_write;
>> +            need_set_fd = 1;
>> +            need_notify_event = 1;
>> +        }
>> +    } else {
>> +        if (iscsilun->write_handler != NULL) {
>> +            iscsilun->write_handler = NULL;
>> +            need_set_fd = 1;
>> +        }
>> +    }
>> +
>> +    if (need_set_fd) {
>> +        qemu_aio_set_fd_handler(iscsi_get_fd(iscsi),
>> +                            iscsilun->read_handler,
>> +                            iscsilun->write_handler,
>> +                            iscsi_process_flush,
>> +                            iscsilun);
>> +
>> +        /* If we just added an event that may trigger almost immediately
>> +           the callback might not be invoked until after a short
>> +           delay unless we call qemu_notify_event().
>> +         */
>> +        if (need_notify_event) {
>> +            qemu_notify_event();
>> +        }
>> +    }
>
> What about this:
>
>    /* We always register a read handler.  */
>    ev = POLLIN;
>    ev |= iscsi_which_events(iscsi);
>    if (ev != iscsilun->events) {
>        qemu_aio_set_fd_handler(iscsi_get_fd(iscsi),
>                       iscsi_process_read,
>                       (ev & POLLOUT) ? iscsi_process_write : NULL,
>                       iscsi_process_flush,
>                       iscsilun);
>
>    }
>
>    /* If we just added an event, the callback might be delayed
>     * unless we call qemu_notify_event().
>     */
>    if (ev & ~iscsilun->events) {
>        qemu_notify_event();
>    }
>    iscsilun->events = ev;
>
> ?
>
> Paolo

Yes. Even better.

Will you check that in  or should I send a new patch with your suggestion?