1. 22 7月, 2015 3 次提交
    • P
      AioContext: optimize clearing the EventNotifier · 05e514b1
      Paolo Bonzini 提交于
      It is pretty rare for aio_notify to actually set the EventNotifier.  It
      can happen with worker threads such as thread-pool.c's, but otherwise it
      should never be set thanks to the ctx->notify_me optimization.  The
      previous patch, unfortunately, added an unconditional call to
      event_notifier_test_and_clear; now add a userspace fast path that
      avoids the call.
      
      Note that it is not possible to do the same with event_notifier_set;
      it would break, as proved (again) by the included formal model.
      
      This patch survived over 3000 reboots on aarch64 KVM.
      Signed-off-by: NPaolo Bonzini <pbonzini@redhat.com>
      Reviewed-by: NFam Zheng <famz@redhat.com>
      Tested-by: NRichard W.M. Jones <rjones@redhat.com>
      Message-id: 1437487673-23740-7-git-send-email-pbonzini@redhat.com
      Signed-off-by: NStefan Hajnoczi <stefanha@redhat.com>
      05e514b1
    • P
      AioContext: fix broken placement of event_notifier_test_and_clear · 21a03d17
      Paolo Bonzini 提交于
      event_notifier_test_and_clear must be called before processing events.
      Otherwise, an aio_poll could "eat" the notification before the main
      I/O thread invokes ppoll().  The main I/O thread then never wakes up.
      This is an example of what could happen:
      
         i/o thread       vcpu thread                     worker thread
         ---------------------------------------------------------------------
         lock_iothread
         notify_me = 1
         ...
         unlock_iothread
                                                           bh->scheduled = 1
                                                           event_notifier_set
                          lock_iothread
                          notify_me = 3
                          ppoll
                          notify_me = 1
                          aio_dispatch
                           aio_bh_poll
                            thread_pool_completion_bh
                                                           bh->scheduled = 1
                                                           event_notifier_set
                           node->io_read(node->opaque)
                            event_notifier_test_and_clear
         ppoll
         *** hang ***
      
      "Tracing" with qemu_clock_get_ns shows pretty much the same behavior as
      in the previous bug, so there are no new tricks here---just stare more
      at the code until it is apparent.
      
      One could also use a formal model, of course.  The included one shows
      this with three processes: notifier corresponds to a QEMU thread pool
      worker, temporary_waiter to a VCPU thread that invokes aio_poll(),
      waiter to the main I/O thread.  I would be happy to say that the
      formal model found the bug for me, but actually I wrote it after the
      fact.
      
      This patch is a bit of a big hammer.  The next one optimizes it,
      with help (this time for real rather than a posteriori :)) from
      another, similar formal model.
      Reported-by: NRichard W. M. Jones <rjones@redhat.com>
      Signed-off-by: NPaolo Bonzini <pbonzini@redhat.com>
      Reviewed-by: NFam Zheng <famz@redhat.com>
      Tested-by: NRichard W.M. Jones <rjones@redhat.com>
      Message-id: 1437487673-23740-6-git-send-email-pbonzini@redhat.com
      Signed-off-by: NStefan Hajnoczi <stefanha@redhat.com>
      21a03d17
    • P
      AioContext: fix broken ctx->dispatching optimization · eabc9779
      Paolo Bonzini 提交于
      This patch rewrites the ctx->dispatching optimization, which was the cause
      of some mysterious hangs that could be reproduced on aarch64 KVM only.
      The hangs were indirectly caused by aio_poll() and in particular by
      flash memory updates's call to blk_write(), which invokes aio_poll().
      Fun stuff: they had an extremely short race window, so much that
      adding all kind of tracing to either the kernel or QEMU made it
      go away (a single printf made it half as reproducible).
      
      On the plus side, the failure mode (a hang until the next keypress)
      made it very easy to examine the state of the process with a debugger.
      And there was a very nice reproducer from Laszlo, which failed pretty
      often (more than half of the time) on any version of QEMU with a non-debug
      kernel; it also failed fast, while still in the firmware.  So, it could
      have been worse.
      
      For some unknown reason they happened only with virtio-scsi, but
      that's not important.  It's more interesting that they disappeared with
      io=native, making thread-pool.c a likely suspect for where the bug arose.
      thread-pool.c is also one of the few places which use bottom halves
      across threads, by the way.
      
      I hope that no other similar bugs exist, but just in case :) I am
      going to describe how the successful debugging went...  Since the
      likely culprit was the ctx->dispatching optimization, which mostly
      affects bottom halves, the first observation was that there are two
      qemu_bh_schedule() invocations in the thread pool: the one in the aio
      worker and the one in thread_pool_completion_bh.  The latter always
      causes the optimization to trigger, the former may or may not.  In
      order to restrict the possibilities, I introduced new functions
      qemu_bh_schedule_slow() and qemu_bh_schedule_fast():
      
           /* qemu_bh_schedule_slow: */
           ctx = bh->ctx;
           bh->idle = 0;
           if (atomic_xchg(&bh->scheduled, 1) == 0) {
               event_notifier_set(&ctx->notifier);
           }
      
           /* qemu_bh_schedule_fast: */
           ctx = bh->ctx;
           bh->idle = 0;
           assert(ctx->dispatching);
           atomic_xchg(&bh->scheduled, 1);
      
      Notice how the atomic_xchg is still in qemu_bh_schedule_slow().  This
      was already debated a few months ago, so I assumed it to be correct.
      In retrospect this was a very good idea, as you'll see later.
      
      Changing thread_pool_completion_bh() to qemu_bh_schedule_fast() didn't
      trigger the assertion (as expected).  Changing the worker's invocation
      to qemu_bh_schedule_slow() didn't hide the bug (another assumption
      which luckily held).  This already limited heavily the amount of
      interaction between the threads, hinting that the problematic events
      must have triggered around thread_pool_completion_bh().
      
      As mentioned early, invoking a debugger to examine the state of a
      hung process was pretty easy; the iothread was always waiting on a
      poll(..., -1) system call.  Infinite timeouts are much rarer on x86,
      and this could be the reason why the bug was never observed there.
      With the buggy sequence more or less resolved to an interaction between
      thread_pool_completion_bh() and poll(..., -1), my "tracing" strategy was
      to just add a few qemu_clock_get_ns(QEMU_CLOCK_REALTIME) calls, hoping
      that the ordering of aio_ctx_prepare(), aio_ctx_dispatch, poll() and
      qemu_bh_schedule_fast() would provide some hint.  The output was:
      
          (gdb) p last_prepare
          $3 = 103885451
          (gdb) p last_dispatch
          $4 = 103876492
          (gdb) p last_poll
          $5 = 115909333
          (gdb) p last_schedule
          $6 = 115925212
      
      Notice how the last call to qemu_poll_ns() came after aio_ctx_dispatch().
      This makes little sense unless there is an aio_poll() call involved,
      and indeed with a slightly different instrumentation you can see that
      there is one:
      
          (gdb) p last_prepare
          $3 = 107569679
          (gdb) p last_dispatch
          $4 = 107561600
          (gdb) p last_aio_poll
          $5 = 110671400
          (gdb) p last_schedule
          $6 = 110698917
      
      So the scenario becomes clearer:
      
         iothread                   VCPU thread
      --------------------------------------------------------------------------
         aio_ctx_prepare
         aio_ctx_check
         qemu_poll_ns(timeout=-1)
                                    aio_poll
                                      aio_dispatch
                                        thread_pool_completion_bh
                                          qemu_bh_schedule()
      
      At this point bh->scheduled = 1 and the iothread has not been woken up.
      The solution must be close, but this alone should not be a problem,
      because the bottom half is only rescheduled to account for rare situations
      (see commit 3c80ca15, thread-pool: avoid deadlock in nested aio_poll()
      calls, 2014-07-15).
      
      Introducing a third thread---a thread pool worker thread, which
      also does qemu_bh_schedule()---does bring out the problematic case.
      The third thread must be awakened *after* the callback is complete and
      thread_pool_completion_bh has redone the whole loop, explaining the
      short race window.  And then this is what happens:
      
                                                            thread pool worker
      --------------------------------------------------------------------------
                                                            <I/O completes>
                                                            qemu_bh_schedule()
      
      Tada, bh->scheduled is already 1, so qemu_bh_schedule() does nothing
      and the iothread is never woken up.  This is where the bh->scheduled
      optimization comes into play---it is correct, but removing it would
      have masked the bug.
      
      So, what is the bug?
      
      Well, the question asked by the ctx->dispatching optimization ("is any
      active aio_poll dispatching?") was wrong.  The right question to ask
      instead is "is any active aio_poll *not* dispatching", i.e. in the prepare
      or poll phases?  In that case, the aio_poll is sleeping or might go to
      sleep anytime soon, and the EventNotifier must be invoked to wake
      it up.
      
      In any other case (including if there is *no* active aio_poll at all!)
      we can just wait for the next prepare phase to pick up the event (e.g. a
      bottom half); the prepare phase will avoid the blocking and service the
      bottom half.
      
      Expressing the invariant with a logic formula, the broken one looked like:
      
         !(exists(thread): in_dispatching(thread)) => !optimize
      
      or equivalently:
      
         !(exists(thread):
                in_aio_poll(thread) && in_dispatching(thread)) => !optimize
      
      In the correct one, the negation is in a slightly different place:
      
         (exists(thread):
               in_aio_poll(thread) && !in_dispatching(thread)) => !optimize
      
      or equivalently:
      
         (exists(thread): in_prepare_or_poll(thread)) => !optimize
      
      Even if the difference boils down to moving an exclamation mark :)
      the implementation is quite different.  However, I think the new
      one is simpler to understand.
      
      In the old implementation, the "exists" was implemented with a boolean
      value.  This didn't really support well the case of multiple concurrent
      event loops, but I thought that this was okay: aio_poll holds the
      AioContext lock so there cannot be concurrent aio_poll invocations, and
      I was just considering nested event loops.  However, aio_poll _could_
      indeed be concurrent with the GSource.  This is why I came up with the
      wrong invariant.
      
      In the new implementation, "exists" is computed simply by counting how many
      threads are in the prepare or poll phases.  There are some interesting
      points to consider, but the gist of the idea remains:
      
      1) AioContext can be used through GSource as well; as mentioned in the
      patch, bit 0 of the counter is reserved for the GSource.
      
      2) the counter need not be updated for a non-blocking aio_poll, because
      it won't sleep forever anyway.  This is just a matter of checking
      the "blocking" variable.  This requires some changes to the win32
      implementation, but is otherwise not too complicated.
      
      3) as mentioned above, the new implementation will not call aio_notify
      when there is *no* active aio_poll at all.  The tests have to be
      adjusted for this change.  The calls to aio_notify in async.c are fine;
      they only want to kick aio_poll out of a blocking wait, but need not
      do anything if aio_poll is not running.
      
      4) nested aio_poll: these just work with the new implementation; when
      a nested event loop is invoked, the outer event loop is never in the
      prepare or poll phases.  The outer event loop thus has already decremented
      the counter.
      Reported-by: NRichard W. M. Jones <rjones@redhat.com>
      Reported-by: NLaszlo Ersek <lersek@redhat.com>
      Signed-off-by: NPaolo Bonzini <pbonzini@redhat.com>
      Reviewed-by: NFam Zheng <famz@redhat.com>
      Tested-by: NRichard W.M. Jones <rjones@redhat.com>
      Message-id: 1437487673-23740-5-git-send-email-pbonzini@redhat.com
      Signed-off-by: NStefan Hajnoczi <stefanha@redhat.com>
      eabc9779
  2. 28 4月, 2015 2 次提交
  3. 10 12月, 2014 1 次提交
  4. 29 8月, 2014 4 次提交
    • P
      AioContext: introduce aio_prepare · a3462c65
      Paolo Bonzini 提交于
      This will be used to implement socket polling on Windows.
      On Windows, select() and g_poll() are completely different;
      sockets are polled with select() before calling g_poll,
      and the g_poll must be nonblocking if select() says a
      socket is ready.
      Signed-off-by: NPaolo Bonzini <pbonzini@redhat.com>
      Signed-off-by: NStefan Hajnoczi <stefanha@redhat.com>
      a3462c65
    • P
      AioContext: export and use aio_dispatch · e4c7e2d1
      Paolo Bonzini 提交于
      So far, aio_poll's scheme was dispatch/poll/dispatch, where
      the first dispatch phase was used only in the GSource case in
      order to avoid a blocking poll.  Earlier patches changed it to
      dispatch/prepare/poll/dispatch, where prepare is aio_compute_timeout.
      
      By making aio_dispatch public, we can remove the first dispatch
      phase altogether, so that both aio_poll and the GSource use the same
      prepare/poll/dispatch scheme.
      
      This patch breaks the invariant that aio_poll(..., true) will not block
      the first time it returns false.  This used to be fundamental for
      qemu_aio_flush's implementation as "while (qemu_aio_wait()) {}" but
      no code in QEMU relies on this invariant anymore.  The return value
      of aio_poll() is now comparable with that of g_main_context_iteration.
      Signed-off-by: NPaolo Bonzini <pbonzini@redhat.com>
      Signed-off-by: NStefan Hajnoczi <stefanha@redhat.com>
      e4c7e2d1
    • P
      AioContext: run bottom halves after polling · 3672fa50
      Paolo Bonzini 提交于
      Make the dispatching phase the same before blocking and afterwards.
      The next patch will make aio_dispatch public and use it directly
      for the GSource case, instead of aio_poll.  aio_poll can then be
      simplified heavily.
      Signed-off-by: NPaolo Bonzini <pbonzini@redhat.com>
      Signed-off-by: NStefan Hajnoczi <stefanha@redhat.com>
      3672fa50
    • P
      AioContext: take bottom halves into account when computing aio_poll timeout · 845ca10d
      Paolo Bonzini 提交于
      Right now, QEMU invokes aio_bh_poll before the "poll" phase
      of aio_poll.  It is simpler to do it afterwards and skip the
      "poll" phase altogether when the OS-dependent parts of AioContext
      are invoked from GSource.  This way, AioContext behaves more
      similarly when used as a GSource vs. when used as stand-alone.
      
      As a start, take bottom halves into account when computing the
      poll timeout.  If a bottom half is ready, do a non-blocking
      poll.  As a side effect, this makes idle bottom halves work
      with aio_poll; an improvement, but not really an important
      one since they are deprecated.
      Signed-off-by: NPaolo Bonzini <pbonzini@redhat.com>
      Signed-off-by: NStefan Hajnoczi <stefanha@redhat.com>
      845ca10d
  5. 09 7月, 2014 2 次提交
    • P
      AioContext: speed up aio_notify · 0ceb849b
      Paolo Bonzini 提交于
      In many cases, the call to event_notifier_set in aio_notify is unnecessary.
      In particular, if we are executing aio_dispatch, or if aio_poll is not
      blocking, we know that we will soon get to the next loop iteration (if
      necessary); the thread that hosts the AioContext's event loop does not
      need any nudging.
      
      The patch includes a Promela formal model that shows that this really
      works and does not need any further complication such as generation
      counts.  It needs a memory barrier though.
      
      The generation counts are not needed because any change to
      ctx->dispatching after the memory barrier is okay for aio_notify.
      If it changes from zero to one, it is the right thing to skip
      event_notifier_set.  If it changes from one to zero, the
      event_notifier_set is unnecessary but harmless.
      Signed-off-by: NPaolo Bonzini <pbonzini@redhat.com>
      Signed-off-by: NKevin Wolf <kwolf@redhat.com>
      0ceb849b
    • P
      block: drop aio functions that operate on the main AioContext · 87f68d31
      Paolo Bonzini 提交于
      The main AioContext should be accessed explicitly via qemu_get_aio_context().
      Most of the time, using it is not the right thing to do.
      Signed-off-by: NPaolo Bonzini <pbonzini@redhat.com>
      Signed-off-by: NKevin Wolf <kwolf@redhat.com>
      87f68d31
  6. 06 12月, 2013 1 次提交
    • S
      aio: make aio_poll(ctx, true) block with no fds · d3fa9230
      Stefan Hajnoczi 提交于
      This patch drops a special case where aio_poll(ctx, true) returns false
      instead of blocking if no file descriptors are waiting on I/O.  Now it
      is possible to block in aio_poll() to wait for aio_notify().
      
      This change eliminates busy waiting.  bdrv_drain_all() used to rely on
      busy waiting to completed throttled I/O requests but this is no longer
      required so we can simplify aio_poll().
      
      Note that aio_poll() still returns false when aio_notify() was used.  In
      other words, stopping a blocking aio_poll() wait is not considered
      making progress.
      
      Adjust test-aio /aio/bh/callback-delete/one which assumed aio_poll(ctx,
      true) would immediately return false instead of blocking.
      Reviewed-by: NAlex Bligh <alex@alex.org.uk>
      Signed-off-by: NStefan Hajnoczi <stefanha@redhat.com>
      d3fa9230
  7. 23 8月, 2013 1 次提交
  8. 19 8月, 2013 2 次提交
    • S
      aio: drop io_flush argument · f2e5dca4
      Stefan Hajnoczi 提交于
      The .io_flush() handler no longer exists and has no users.  Drop the
      io_flush argument to aio_set_fd_handler() and related functions.
      
      The AioFlushEventNotifierHandler and AioFlushHandler typedefs are no
      longer used and are dropped too.
      Reviewed-by: NPaolo Bonzini <pbonzini@redhat.com>
      Signed-off-by: NStefan Hajnoczi <stefanha@redhat.com>
      f2e5dca4
    • S
      aio: stop using .io_flush() · 164a101f
      Stefan Hajnoczi 提交于
      Now that aio_poll() users check their termination condition themselves,
      it is no longer necessary to call .io_flush() handlers.
      
      The behavior of aio_poll() changes as follows:
      
      1. .io_flush() is no longer invoked and file descriptors are *always*
      monitored.  Previously returning 0 from .io_flush() would skip this file
      descriptor.
      
      Due to this change it is essential to check that requests are pending
      before calling qemu_aio_wait().  Failure to do so means we block, for
      example, waiting for an idle iSCSI socket to become readable when there
      are no requests.  Currently all qemu_aio_wait()/aio_poll() callers check
      before calling.
      
      2. aio_poll() now returns true if progress was made (BH or fd handlers
      executed) and false otherwise.  Previously it would return true whenever
      'busy', which means that .io_flush() returned true.  The 'busy' concept
      no longer exists so just progress is returned.
      
      Due to this change we need to update tests/test-aio.c which asserts
      aio_poll() return values.  Note that QEMU doesn't actually rely on these
      return values so only tests/test-aio.c cares.
      
      Note that ctx->notifier, the EventNotifier fd used for aio_notify(), is
      now handled as a special case.  This is a little ugly but maintains
      aio_poll() semantics, i.e. aio_notify() does not count as 'progress' and
      aio_poll() avoids blocking when the user has not set any fd handlers yet.
      
      Patches after this remove .io_flush() handler code until we can finally
      drop the io_flush arguments to aio_set_fd_handler() and friends.
      Reviewed-by: NPaolo Bonzini <pbonzini@redhat.com>
      Signed-off-by: NStefan Hajnoczi <stefanha@redhat.com>
      164a101f
  9. 22 2月, 2013 3 次提交
  10. 17 1月, 2013 1 次提交
    • K
      aio: Fix return value of aio_poll() · 2ea9b58f
      Kevin Wolf 提交于
      aio_poll() must return true if any work is still pending, even if it
      didn't make progress, so that bdrv_drain_all() doesn't stop waiting too
      early. The possibility of stopping early occasionally lead to a failed
      assertion in bdrv_drain_all(), when some in-flight request was missed
      and the function didn't really drain all requests.
      
      In order to make that change, the return value as specified in the
      function comment must change for blocking = false; fortunately, the
      return value of blocking = false callers is only used in test cases, so
      this change shouldn't cause any trouble.
      
      Cc: qemu-stable@nongnu.org
      Signed-off-by: NKevin Wolf <kwolf@redhat.com>
      Signed-off-by: NStefan Hajnoczi <stefanha@redhat.com>
      2ea9b58f
  11. 19 12月, 2012 2 次提交
  12. 30 10月, 2012 10 次提交
  13. 28 9月, 2012 2 次提交
  14. 19 4月, 2012 3 次提交
  15. 14 1月, 2012 1 次提交
  16. 21 8月, 2011 1 次提交
  17. 21 5月, 2010 1 次提交