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 <stefa...@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
signature.asc
Description: PGP signature
