Re: [Qemu-devel] [PATCH v2 2/4] aio: add polling mode to AioContext

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4103 bytes --]

On Wed, Nov 16, 2016 at 07:14:47PM +0100, Paolo Bonzini wrote:
> 
> 
> On 16/11/2016 18:47, Stefan Hajnoczi wrote:
> > The AioContext event loop uses ppoll(2) or epoll_wait(2) to monitor file
> > descriptors or until a timer expires.  In cases like virtqueues, Linux
> > AIO, and ThreadPool it is technically possible to wait for events via
> > polling (i.e. continuously checking for events without blocking).
> > 
> > Polling can be faster than blocking syscalls because file descriptors,
> > the process scheduler, and system calls are bypassed.
> > 
> > The main disadvantage to polling is that it increases CPU utilization.
> > In classic polling configuration a full host CPU thread might run at
> > 100% to respond to events as quickly as possible.  This patch implements
> > a timeout so we fall back to blocking syscalls if polling detects no
> > activity.  After the timeout no CPU cycles are wasted on polling until
> > the next event loop iteration.
> > 
> > This patch implements an experimental polling mode that can be
> > controlled with the QEMU_AIO_POLL_MAX_NS=<nanoseconds> environment
> > variable.  The aio_poll() event loop function will attempt to poll
> > instead of using blocking syscalls.
> > 
> > The run_poll_handlers_begin() and run_poll_handlers_end() trace events
> > are added to aid performance analysis and troubleshooting.  If you need
> > to know whether polling mode is being used, trace these events to find
> > out.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  aio-posix.c         | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  async.c             |  11 +++++-
> >  include/block/aio.h |   3 ++
> >  trace-events        |   4 ++
> >  4 files changed, 123 insertions(+), 2 deletions(-)
> 
> Nice!
> 
> 
> > diff --git a/aio-posix.c b/aio-posix.c
> > index 4379c13..5e5a561 100644
> > --- a/aio-posix.c
> > +++ b/aio-posix.c
> > @@ -18,6 +18,8 @@
> >  #include "block/block.h"
> >  #include "qemu/queue.h"
> >  #include "qemu/sockets.h"
> > +#include "qemu/cutils.h"
> > +#include "trace.h"
> >  #ifdef CONFIG_EPOLL_CREATE1
> >  #include <sys/epoll.h>
> >  #endif
> > @@ -27,12 +29,16 @@ struct AioHandler
> >      GPollFD pfd;
> >      IOHandler *io_read;
> >      IOHandler *io_write;
> > +    AioPollFn *io_poll;
> >      int deleted;
> >      void *opaque;
> >      bool is_external;
> >      QLIST_ENTRY(AioHandler) node;
> >  };
> >  
> > +/* How long to poll AioPollHandlers before monitoring file descriptors */
> > +static int64_t aio_poll_max_ns;
> > +
> >  #ifdef CONFIG_EPOLL_CREATE1
> >  
> >  /* The fd number threashold to switch to epoll */
> > @@ -206,11 +212,12 @@ void aio_set_fd_handler(AioContext *ctx,
> >      AioHandler *node;
> >      bool is_new = false;
> >      bool deleted = false;
> > +    int poll_disable_cnt = 0;
> 
> poll_disable_cnt = !io_poll - !node->io_poll
> 
> ?  Not the most readable thing, but effective...
> 
> >      node = find_aio_handler(ctx, fd);

Taking into account creation of new nodes:

if (node) {
    poll_disable_cnt = !io_poll - !node->io_poll;
} else {
    poll_disable_cnt = !io_poll;
}

This does shorten the code quite a bit so I'll use it in the next
version.

> > +static bool run_poll_handlers(AioContext *ctx, int64_t max_ns)
> > +{
> > +    bool progress = false;
> > +    int64_t end_time;
> > +
> > +    assert(ctx->notify_me);
> > +    assert(ctx->walking_handlers > 0);
> > +    assert(ctx->poll_disable_cnt == 0);
> > +
> > +    trace_run_poll_handlers_begin(ctx, max_ns);
> > +
> > +    end_time = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + max_ns;
> > +
> > +    do {
> > +        AioHandler *node;
> > +
> > +        /* Bail if aio_notify() was called (e.g. BH was scheduled) */
> > +        if (atomic_read(&ctx->notified)) {
> > +            progress = true;
> > +            break;
> > +        }
> 
> This can be done in event_notifier_dummy_poll.

Nice idea, then the "dummy" function can serve a real purpose!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]