Re: [PATCH v4 4/4] virtio-blk: add iothread-vq-mapping parameter

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Thu, Dec 21, 2023 at 02:40:19PM +0100, Kevin Wolf wrote:
> Am 20.12.2023 um 14:47 hat Stefan Hajnoczi geschrieben:
> > Add the iothread-vq-mapping parameter to assign virtqueues to IOThreads.
> > Store the vq:AioContext mapping in the new struct
> > VirtIOBlockDataPlane->vq_aio_context[] field and refactor the code to
> > use the per-vq AioContext instead of the BlockDriverState's AioContext.
> > 
> > Reimplement --device virtio-blk-pci,iothread= and non-IOThread mode by
> > assigning all virtqueues to the IOThread and main loop's AioContext in
> > vq_aio_context[], respectively.
> > 
> > The comment in struct VirtIOBlockDataPlane about EventNotifiers is
> > stale. Remove it.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> > @@ -177,19 +238,13 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
> >  
> >      trace_virtio_blk_data_plane_start(s);
> >  
> > -    r = blk_set_aio_context(s->conf->conf.blk, s->ctx, &local_err);
> > +    r = blk_set_aio_context(s->conf->conf.blk, s->vq_aio_context[0],
> > +                            &local_err);
> >      if (r < 0) {
> >          error_report_err(local_err);
> >          goto fail_aio_context;
> >      }
> 
> This doesn't really have to be an error any more, we'll just submit I/O
> from any thread we want no matter what the home AioContext of the
> BlockBackend is.
> 
> So the only effect the blk_set_aio_context() has is that other users of
> the image try to submit their requests from the same iothread as the
> first virtqueue in the hope that this performs a bit better (maybe less
> lock contention or whatever the idea was?)

Yes, I'll change this.

> > -    /* Kick right away to begin processing requests already in vring */
> > -    for (i = 0; i < nvqs; i++) {
> > -        VirtQueue *vq = virtio_get_queue(s->vdev, i);
> > -
> > -        event_notifier_set(virtio_queue_get_host_notifier(vq));
> > -    }
> > -
> >      /*
> >       * These fields must be visible to the IOThread when it processes the
> >       * virtqueue, otherwise it will think dataplane has not started yet.
> > @@ -206,8 +261,12 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
> >      if (!blk_in_drain(s->conf->conf.blk)) {
> >          for (i = 0; i < nvqs; i++) {
> >              VirtQueue *vq = virtio_get_queue(s->vdev, i);
> > +            AioContext *ctx = s->vq_aio_context[i];
> >  
> > -            virtio_queue_aio_attach_host_notifier(vq, s->ctx);
> > +            /* Kick right away to begin processing requests already in vring */
> > +            event_notifier_set(virtio_queue_get_host_notifier(vq));
> 
> The old code did this also for blk_in_drain() == true. Why don't we need
> it there any more? Should the 'if' move inside the loop just around
> attaching the notifier?

The answer is I'm not 100% sure. Your suggestion is safer, I'll do that.

Stefan

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