Re: [PATCH 1/2] virtio: use blk_io_plug_call() in virtio_irqfd_notify()

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Wed, Aug 16, 2023 at 08:30:58PM +0200, Ilya Maximets wrote:
> On 8/16/23 17:30, Stefan Hajnoczi wrote:
> > On Wed, Aug 16, 2023 at 03:36:32PM +0200, Ilya Maximets wrote:
> >> On 8/15/23 14:08, Stefan Hajnoczi wrote:
> >>> virtio-blk and virtio-scsi invoke virtio_irqfd_notify() to send Used
> >>> Buffer Notifications from an IOThread. This involves an eventfd
> >>> write(2) syscall. Calling this repeatedly when completing multiple I/O
> >>> requests in a row is wasteful.
> >>
> >> Hi, Stefan.  This is an interesting change!
> >>
> >> There is more or less exactly the same problem with fast network backends
> >> and I was playing around with similar ideas in this area while working on
> >> af-xdp network backend recently.  Primarily, implementation of the Rx BH
> >> for virtio-net device and locking the RCU before passing packets from the
> >> backend to the device one by one.
> >>
> >>>
> >>> Use the blk_io_plug_call() API to batch together virtio_irqfd_notify()
> >>> calls made during Linux AIO (aio=native) or io_uring (aio=io_uring)
> >>> completion processing. Do not modify the thread pool (aio=threads) to
> >>> avoid introducing a dependency from util/ onto the block layer.
> >>
> >> But you're introducing a dependency from generic virtio code onto the
> >> block layer in this patch.  This seem to break the module abstraction.
> >>
> >> It looks like there are 2 options to resolve the semantics issue here:
> > 
> > Yes, it's a layering violation.
> > 
> >>
> >> 1. Move virtio_notify_irqfd() from virtio.c down to the block layer.
> >>    Block layer is the only user, so that may be justified, but it
> >>    doesn't seem like a particularly good solution.  (I'm actually not
> >>    sure why block devices are the only ones using this function...)
> > 
> > Yes, this is the easiest way to avoid the layering violation for now.
> > 
> > The virtio_notify_irqfd() API is necessary when running in an IOThread
> > because the normal QEMU irq API must run under the Big QEMU Lock. Block
> > devices are the only ones that raise interrupts from IOThreads at the
> > moment.
> 
> Ack.  Thanks for explanation!
> 
> > 
> >>
> >> 2. Move and rename the block/plug library somewhere generic.  The plug
> >>    library seems to not have any dependencies on a block layer, other
> >>    than a name, so it should not be hard to generalize it (well, the
> >>    naming might be hard).
> > 
> > Yes, it should be possible to make it generic quite easily. I will give
> > this a try in the next version of the patch.
> 
> OK.  Sounds good to me.
> 
> > 
> >> In general, while looking at the plug library, it seems to do what is
> >> typically implemented via RCU frameworks - the delayed function call.
> >> The only difference is that RCU doesn't check for duplicates and the
> >> callbacks are global.  Should not be hard to add some new functionality
> >> to RCU framework in order to address these, e.g. rcu_call_local() for
> >> calls that should be executed once the current thread exits its own
> >> critical section.
> > 
> > This rcu_call_local() idea is unrelated to Read Copy Update, so I don't
> > think it should be part of the RCU API.
> 
> Agreed.
> 
> > Another deferred function call mechanism is QEMUBH. It already supports
> > coalescing. However, BHs are invoked once per AioContext event loop
> > iteration and there is no way invoke the BH earlier. Also the BH pointer
> > needs to be passed to every function that wishes to schedule a deferred
> > call, which can be tedious (e.g. block/linux-aio.c should defer the
> > io_submit(2) syscall until the end of virtio-blk request processing -
> > there are a lot of layers of code between those two components).
> > 
> >>
> >> Using RCU for non-RCU-protected things might be considered as an abuse.
> >> However, we might solve two issues in one shot if instead of entering
> >> blk_io_plug/unplug section we will enter an RCU critical section and
> >> call callbacks at the exit.  The first issue is the notification batching
> >> that this patch is trying to fix, the second is an excessive number of
> >> thread fences on RCU exits every time virtio_notify_irqfd() and other
> >> virtio functions are invoked.  The second issue can be avoided by using
> >> RCU_READ_LOCK_GUARD() in completion functions.  Not sure if that will
> >> improve performance, but it definitely removes a lot of noise from the
> >> perf top for network backends.  This makes the code a bit less explicit
> >> though, the lock guard will definitely need a comment.  Though, the reason
> >> for blk_io_plug() calls is not fully clear for a module code alone either.
> > 
> > util/aio-posix.c:run_poll_handlers() has a top-level
> > RCU_READ_LOCK_GUARD() for this reason.
> 
> Nice, didn't know that.
> 
> > Maybe we should do the same
> > around aio_bh_poll() + aio_dispatch_ready_handlers() in
> > util/aio-posix.c:aio_poll()? The downside is that latency-sensitive
> > call_rcu() callbacks perform worse.
> 
> "latency-sensitive call_rcu() callback" is a bit of an oxymoron.
> There is no real way to tell when the other thread will exit the
> critical section.  But I'm not familiar with the code enough to
> make a decision here.

I see your point :).

Another issue occurred to me: drain_call_rcu() is invoked by some QEMU
monitor commands and the monitor runs in QEMU's event loop.
drain_call_rcu() waits for the call_rcu thread, which needs
synchronize_rcu() to finish, so I guess it will hang if called with the
RCU read lock held. There is the potential for a deadlock here. It might
be solvable.

> 
> >>
> >> I'm not sure what is the best way forward.  I'm trying to figure out
> >> that myself, since a similar solution will in the end be needed for
> >> network backends and so it might be better to have something more generic.
> >> What do you think?
> > 
> > I suggest moving blk_io_plug() to util/ and renaming it to
> > defer_begin/defer_end()/defer_call(). The interface and implementation
> > can also be improved as needed for other users.
> 
> Sounds good to me.
> 
> > 
> >>
> >>>
> >>> Behavior is unchanged for emulated devices that do not use blk_io_plug()
> >>> since blk_io_plug_call() immediately invokes the callback when called
> >>> outside a blk_io_plug()/blk_io_unplug() region.
> >>>
> >>> fio rw=randread bs=4k iodepth=64 numjobs=8 IOPS increases by ~9% with a
> >>> single IOThread and 8 vCPUs. iodepth=1 decreases by ~1% but this could
> >>> be noise. Detailed performance data and configuration specifics are
> >>> available here:
> >>> https://gitlab.com/stefanha/virt-playbooks/-/tree/blk_io_plug-irqfd
> >>>
> >>> This duplicates the BH that virtio-blk uses for batching. The next
> >>> commit will remove it.
> >>
> >> I'm likely missing something, but could you explain why it is safe to
> >> batch unconditionally here?  The current BH code, as you mentioned in
> >> the second patch, is only batching if EVENT_IDX is not set.
> >> Maybe worth adding a few words in the commit message for people like
> >> me, who are a bit rusty on QEMU/virtio internals. :)
> > 
> > The BH is condition on EVENT_IDX not for correctness/safety, but for
> > performance.
> 
> I see.  Thanks!
> Do you think using deferred call in the non-irqfd virtio_notify()
> will make sense?  Not in this patch set, but in general, e.g. for
> a future use from a network backend.

Yes, a deferred call may improve performance in the non-irqfd case too.

> > Commit 12c1c7d7cefb4dbffeb5712e75a33e4692f0a76b
> > ("virtio-blk: dataplane: Don't batch notifications if EVENT_IDX is
> > present") argued that VIRTIO's EVENT_IDX feature already provides
> > batching so the BH is not needed in that case.
> > 
> > Actually I'm not sure exactly why deferring virtio_notify_irqfd() helps,
> > but IOPS improves on my machine. One theory:
> > 
> > Linux drivers/virtio/virtio_ring.c:virtqueue_get_buf_ctx_split() (or the
> > end of the irq handler function) updates EVENT_IDX so that QEMU will
> > inject an interrupt again when the next request is completed. If the
> > vCPU and QEMU are running in parallel, then N requests could result in N
> > interrupts when the vCPU updates EVENT_IDX before QEMU completes the
> > next request. Maybe that's why I see better performance when deferring
> > virtio_notify_irqfd().
> 
> Sounds very plausible.  I actually see only latency and interrupt
> count measurements in either 12c1c7d7cefb ("virtio-blk: dataplane:
> Don't batch notifications if EVENT_IDX is present") or 5b2ffbe4d998
> ("virtio-blk: dataplane: notify guest as a batch") as well as in
> mailing list discussions about these changes.  So, it's possible that
> impact of batching on throughput in terms of IOPS was not actually
> measured before (it porobably was, but not published).
> 
> > 
> > (I'm thinking of experimenting with rate limiting irq injection so that
> > it's possible to batch even more efficienctly but I worry that it will
> > be hard to avoid latency vs throughput regressions.)
> > 
> > Stefan
> > 
> >>
> >> Best regards, Ilya Maximets.
> >>
> >>>
> >>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >>> ---
> >>>  block/io_uring.c   |  6 ++++++
> >>>  block/linux-aio.c  |  4 ++++
> >>>  hw/virtio/virtio.c | 10 +++++++++-
> >>>  3 files changed, 19 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/block/io_uring.c b/block/io_uring.c
> >>> index 69d9820928..749cf83934 100644
> >>> --- a/block/io_uring.c
> >>> +++ b/block/io_uring.c
> >>> @@ -124,6 +124,9 @@ static void luring_process_completions(LuringState *s)
> >>>  {
> >>>      struct io_uring_cqe *cqes;
> >>>      int total_bytes;
> >>> +
> >>> +    blk_io_plug();
> >>> +
> >>>      /*
> >>>       * Request completion callbacks can run the nested event loop.
> >>>       * Schedule ourselves so the nested event loop will "see" remaining
> >>> @@ -216,7 +219,10 @@ end:
> >>>              aio_co_wake(luringcb->co);
> >>>          }
> >>>      }
> >>> +
> >>>      qemu_bh_cancel(s->completion_bh);
> >>> +
> >>> +    blk_io_unplug();
> >>>  }
> >>>  
> >>>  static int ioq_submit(LuringState *s)
> >>> diff --git a/block/linux-aio.c b/block/linux-aio.c
> >>> index 561c71a9ae..cef3d6b1c7 100644
> >>> --- a/block/linux-aio.c
> >>> +++ b/block/linux-aio.c
> >>> @@ -204,6 +204,8 @@ static void qemu_laio_process_completions(LinuxAioState *s)
> >>>  {
> >>>      struct io_event *events;
> >>>  
> >>> +    blk_io_plug();
> >>> +
> >>>      /* Reschedule so nested event loops see currently pending completions */
> >>>      qemu_bh_schedule(s->completion_bh);
> >>>  
> >>> @@ -230,6 +232,8 @@ static void qemu_laio_process_completions(LinuxAioState *s)
> >>>       * own `for` loop.  If we are the last all counters droped to zero. */
> >>>      s->event_max = 0;
> >>>      s->event_idx = 0;
> >>> +
> >>> +    blk_io_unplug();
> >>>  }
> >>>  
> >>>  static void qemu_laio_process_completions_and_submit(LinuxAioState *s)
> >>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >>> index 309038fd46..a691e8526b 100644
> >>> --- a/hw/virtio/virtio.c
> >>> +++ b/hw/virtio/virtio.c
> >>> @@ -28,6 +28,7 @@
> >>>  #include "hw/virtio/virtio-bus.h"
> >>>  #include "hw/qdev-properties.h"
> >>>  #include "hw/virtio/virtio-access.h"
> >>> +#include "sysemu/block-backend.h"
> >>>  #include "sysemu/dma.h"
> >>>  #include "sysemu/runstate.h"
> >>>  #include "virtio-qmp.h"
> >>> @@ -2426,6 +2427,13 @@ static bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq)
> >>>      }
> >>>  }
> >>>  
> >>> +/* Batch irqs while inside a blk_io_plug()/blk_io_unplug() section */
> >>> +static void virtio_notify_irqfd_unplug_fn(void *opaque)
> >>> +{
> >>> +    EventNotifier *notifier = opaque;
> >>> +    event_notifier_set(notifier);
> >>> +}
> >>> +
> >>>  void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue *vq)
> >>>  {
> >>>      WITH_RCU_READ_LOCK_GUARD() {
> >>> @@ -2452,7 +2460,7 @@ void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue *vq)
> >>>       * to an atomic operation.
> >>>       */
> >>>      virtio_set_isr(vq->vdev, 0x1);
> >>> -    event_notifier_set(&vq->guest_notifier);
> >>> +    blk_io_plug_call(virtio_notify_irqfd_unplug_fn, &vq->guest_notifier);
> 
> There is a trace call in this function.  Should we move it or add a new
> trace call to the callback?  The trace was printed right before the
> actual notification before, and now the actual notification is delayed
> potentially reducing the usefulness of the trace.
> 
> >>>  }
> >>>  
> >>>  static void virtio_irq(VirtQueue *vq)
> >>
> 

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