Re: [PATCH 11/12] virtio-scsi: add iothread-vq-mapping parameter

[Kevin Wolf <kwolf@redhat.com>]

Am 10.03.2025 um 15:37 hat Peter Krempa geschrieben:
> On Mon, Mar 10, 2025 at 15:33:02 +0100, Kevin Wolf wrote:
> > Am 13.02.2025 um 19:00 hat Stefan Hajnoczi geschrieben:
> > > Allow virtio-scsi virtqueues to be assigned to different IOThreads. This
> > > makes it possible to take advantage of host multi-queue block layer
> > > scalability by assigning virtqueues that have affinity with vCPUs to
> > > different IOThreads that have affinity with host CPUs. The same feature
> > > was introduced for virtio-blk in the past:
> > > https://developers.redhat.com/articles/2024/09/05/scaling-virtio-blk-disk-io-iothread-virtqueue-mapping
> > > 
> > > Here are fio randread 4k iodepth=64 results from a 4 vCPU guest with an
> > > Intel P4800X SSD:
> > > iothreads IOPS
> > > ------------------------------
> > > 1         189576
> > > 2         312698
> > > 4         346744
> > > 
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > As Peter already noted, the interface is a bit confusing in that it
> > considers the control and event queue just normal queues like any other
> > and you need to specify a mapping for them, too (even though you
> > probably don't care about them).
> > 
> > I wonder if it wouldn't be better to use the iothread-vq-mapping
> > property only for command queues and to have separate properties for the
> > event and control queue. I think this would be less surprising to users.
> 
> In the v2 of libvirt's patches I've proposed:
> 
>         <driver queues='3'>
>           <iothreads>
>             <iothread id='2'>
>               <queue id='ctrl'/>
>               <queue id='event'/>
>               <queue id='1'/>
>             </iothread>
>             <iothread id='3'>
>               <queue id='0'/>
>               <queue id='2'/>
>             </iothread>
>           </iothreads>
>         </driver>
> 
> To map the queues by name explicitly so that it's clear what's
> happening.
> 
> In my proposed it auto-translates ctrl and event into 0 and 1 and the
> command queues into N+2.

Note that if I understand patch 12 correctly, the 'ctrl' queue setting
will never actually take effect. So libvirt probably shouln't even offer
it (and neither should QEMU).

> > It would also allow you to use the round robin allocation for command
> > queues while using a different setting for the special queues - in
> > particular, the event queue is currently no_poll, which disables polling
> > for the whole AioContext, so you probably want to have it just anywhere
> > else, but not in the iothreads you use for command queues. This should
> > probably also be the default.
> 
> This sounds like an important bit of information. If that stays like
> this I think libvirt should also document this.
> 
> The proposed libvirt patch also words the recommendation to use the
> round-robin approach unless specific needs arise so if qemu did the
> correct thing here it would be great.

Yes, I consider this a QEMU bug that should be fixed. It's no_poll not
in the sense that we must use the eventfd because we can't otherwise
figure out if it's ready, but that we don't usually care about new
things being ready in the queue.

But if we always tie the event queue to the main loop, too, it would
already be worked around for most cases - the main loop generally won't
be able to poll anyway because of other fd handlers that don't support
polling.

Kevin