| Message ID | 20240202153158.788922-4-hreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio: Re-enable notifications after drain | expand |
02.02.2024 18:31, Hanna Czenczek : > Commit d3f6f294aeadd5f88caf0155e4360808c95b3146 ("virtio-blk: always set > ioeventfd during startup") has made virtio_blk_start_ioeventfd() always > kick the virtqueue (set the ioeventfd), regardless of whether the BB is > drained. That is no longer necessary, because attaching the host > notifier will now set the ioeventfd, too; this happens either > immediately right here in virtio_blk_start_ioeventfd(), or later when > the drain ends, in virtio_blk_ioeventfd_attach(). > > With event_notifier_set() removed, the code becomes the same as the one > in virtio_blk_ioeventfd_attach(), so we can reuse that function. The mentioned comit is v8.2.0-812-gd3f6f294ae, - ie, past 8.2. Is this new change still relevant for stable? Thanks, /mjt
On 09.02.24 15:38, Michael Tokarev wrote: > 02.02.2024 18:31, Hanna Czenczek : >> Commit d3f6f294aeadd5f88caf0155e4360808c95b3146 ("virtio-blk: always set >> ioeventfd during startup") has made virtio_blk_start_ioeventfd() always >> kick the virtqueue (set the ioeventfd), regardless of whether the BB is >> drained. That is no longer necessary, because attaching the host >> notifier will now set the ioeventfd, too; this happens either >> immediately right here in virtio_blk_start_ioeventfd(), or later when >> the drain ends, in virtio_blk_ioeventfd_attach(). >> >> With event_notifier_set() removed, the code becomes the same as the one >> in virtio_blk_ioeventfd_attach(), so we can reuse that function. > > The mentioned comit is v8.2.0-812-gd3f6f294ae, - ie, past 8.2. > Is this new change still relevant for stable? Sorry again. :/ This patch is a clean-up patch that won’t apply to 8.2. Now, 8.2 does have basically the same logic as described in the patch message (d3f6f294aea restored it after it was broken), so a similar patch could be made for it (removing the event_notifier_set() from virtio_blk_data_plane_start()), but whether we kick the virtqueues once or twice on start-up probably won’t make a difference, certainly not in terms of correctness. Hanna
09.02.2024 20:11, Hanna Czenczek : >> The mentioned comit is v8.2.0-812-gd3f6f294ae, - ie, past 8.2. >> Is this new change still relevant for stable? > > Sorry again. :/ There's nothing to be sorry about here - it's regular work, and is quite good at it, - I just asked to be sure, maybe I misunderstood something. > This patch is a clean-up patch that won’t apply to 8.2. Now, 8.2 does have basically the same logic as described in the patch > message (d3f6f294aea restored it after it was broken), so a similar patch could be made for it (removing the event_notifier_set() from > virtio_blk_data_plane_start()), but whether we kick the virtqueues once or twice on start-up probably won’t make a difference, certainly not in terms > of correctness. Ok, excellent, this makes good sense now. I'm not including this one in stable-8.2 :) Thank you very much for the excellent work and the clarification! /mjt
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 227d83569f..22b8eef69b 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -37,6 +37,8 @@ #include "hw/virtio/virtio-blk-common.h" #include "qemu/coroutine.h" +static void virtio_blk_ioeventfd_attach(VirtIOBlock *s); + static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq, VirtIOBlockReq *req) { @@ -1808,17 +1810,14 @@ static int virtio_blk_start_ioeventfd(VirtIODevice *vdev) s->ioeventfd_started = true; smp_wmb(); /* paired with aio_notify_accept() on the read side */ - /* Get this show started by hooking up our callbacks */ - for (i = 0; i < nvqs; i++) { - VirtQueue *vq = virtio_get_queue(vdev, i); - AioContext *ctx = s->vq_aio_context[i]; - - /* Kick right away to begin processing requests already in vring */ - event_notifier_set(virtio_queue_get_host_notifier(vq)); - - if (!blk_in_drain(s->conf.conf.blk)) { - virtio_queue_aio_attach_host_notifier(vq, ctx); - } + /* + * Get this show started by hooking up our callbacks. If drained now, + * virtio_blk_drained_end() will do this later. + * Attaching the notifier also kicks the virtqueues, processing any requests + * they may already have. + */ + if (!blk_in_drain(s->conf.conf.blk)) { + virtio_blk_ioeventfd_attach(s); } return 0;
Commit d3f6f294aeadd5f88caf0155e4360808c95b3146 ("virtio-blk: always set ioeventfd during startup") has made virtio_blk_start_ioeventfd() always kick the virtqueue (set the ioeventfd), regardless of whether the BB is drained. That is no longer necessary, because attaching the host notifier will now set the ioeventfd, too; this happens either immediately right here in virtio_blk_start_ioeventfd(), or later when the drain ends, in virtio_blk_ioeventfd_attach(). With event_notifier_set() removed, the code becomes the same as the one in virtio_blk_ioeventfd_attach(), so we can reuse that function. Signed-off-by: Hanna Czenczek <hreitz@redhat.com> --- hw/block/virtio-blk.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)