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.

[Paolo Bonzini]

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 <address@hidden>
> ---
>  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