This is a preview of the iothread-vq-mapping parameter that assigns virtqueues
to IOThreads. The syntax is implemented but multiple IOThreads are not actually
supported yet. The purpose of this RFC is to reach agreement on the syntax and
to prepare for libvirt support.

virtio-blk and virtio-scsi devices will need a way to specify the
mapping between IOThreads and virtqueues. At the moment all virtqueues
are assigned to a single IOThread or the main loop. This single thread
can be a CPU bottleneck, so it is necessary to allow finer-grained
assignment to spread the load.

This series introduces command-line syntax for the new iothread-vq-mapping
property is as follows:

  --device '{"driver":"virtio-blk-pci","iothread-vq-mapping":[{"iothread":"iothread0","vqs":[0,1,2]},...]},...'

IOThreads are specified by name and virtqueues are specified by 0-based
index.

It will be common to simply assign virtqueues round-robin across a set
of IOThreads. A convenient syntax that does not require specifying
individual virtqueue indices is available:

  --device '{"driver":"virtio-blk-pci","iothread-vq-mapping":[{"iothread":"iothread0"},{"iothread":"iothread1"},...]},...'

There is no way to reassign virtqueues at runtime and I expect that to be a
very rare requirement.

Perhaps libvirt only needs to support round-robin because specifying individual
virtqueues is very specific and probably only useful for low-level performance
investigation. The libvirt domain XML syntax for this could be:

  <driver name='qemu' type='qcow2'>
    <iothreads>
      <iothread id="1"/>
      <iothread id="2"/>
      <iothread id="3"/>
    </iothreads>
    ...
  </driver>

and that would generate this QEMU command-line snippet:

  "iothread-vq-mapping":[{"iothread":"iothread1"},{"iothread":"iothread2"},{"iothread":"iothread3"}]

Note that JSON --device syntax is required for the iothread-vq-mapping
parameter because it's non-scalar.

What do you think?

Stefan Hajnoczi (3):
  qdev-properties: alias all object class properties
  qdev: add IOThreadVirtQueueMappingList property type
  virtio-blk: add iothread-vq-mapping parameter

 qapi/virtio.json                    | 30 +++++++++++
 include/hw/qdev-properties-system.h |  4 ++
 include/hw/virtio/virtio-blk.h      |  2 +
 hw/block/virtio-blk.c               | 78 +++++++++++++++++++++++++++++
 hw/core/qdev-properties-system.c    | 47 +++++++++++++++++
 hw/core/qdev-properties.c           | 18 ++++---
 6 files changed, 171 insertions(+), 8 deletions(-)

-- 
2.39.0

On 1/18/23 20:47, Stefan Hajnoczi wrote:
> This is a preview of the iothread-vq-mapping parameter that assigns virtqueues
> to IOThreads. The syntax is implemented but multiple IOThreads are not actually
> supported yet. The purpose of this RFC is to reach agreement on the syntax and
> to prepare for libvirt support.
> 
> virtio-blk and virtio-scsi devices will need a way to specify the
> mapping between IOThreads and virtqueues. At the moment all virtqueues
> are assigned to a single IOThread or the main loop. This single thread
> can be a CPU bottleneck, so it is necessary to allow finer-grained
> assignment to spread the load.
> 
> This series introduces command-line syntax for the new iothread-vq-mapping
> property is as follows:
> 
>   --device '{"driver":"virtio-blk-pci","iothread-vq-mapping":[{"iothread":"iothread0","vqs":[0,1,2]},...]},...'
> 
> IOThreads are specified by name and virtqueues are specified by 0-based
> index.
> 
> It will be common to simply assign virtqueues round-robin across a set
> of IOThreads. A convenient syntax that does not require specifying
> individual virtqueue indices is available:
> 
>   --device '{"driver":"virtio-blk-pci","iothread-vq-mapping":[{"iothread":"iothread0"},{"iothread":"iothread1"},...]},...'
> 
> There is no way to reassign virtqueues at runtime and I expect that to be a
> very rare requirement.
> 
> Perhaps libvirt only needs to support round-robin because specifying individual
> virtqueues is very specific and probably only useful for low-level performance
> investigation. The libvirt domain XML syntax for this could be:
> 
>   <driver name='qemu' type='qcow2'>
>     <iothreads>
>       <iothread id="1"/>
>       <iothread id="2"/>
>       <iothread id="3"/>
>     </iothreads>
>     ...
>   </driver>

Just for completeness, this how disk XML looks now:

  <disk type='file' device='disk'>
    <driver name='qemu' type='qcow2' queues='4' iothread='1'/>
    <source file='/tmp/data.qcow'/>
    <target dev='vda' bus='virtio'/>
    <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
  </disk>

It corresponds to the following cmd line:

  -blockdev '{"driver":"file","filename":"/tmp/data.qcow","node-name":"libvirt-3-storage","auto-read-only":true,"discard":"unmap"}' \
  -blockdev '{"node-name":"libvirt-3-format","read-only":false,"driver":"qcow2","file":"libvirt-3-storage"}' \
  -device '{"driver":"virtio-blk-pci","iothread":"iothread1","num-queues":4,"bus":"pci.0","addr":"0x3","drive":"libvirt-3-format","id":"virtio-disk0","bootindex":1}' \

We already have @iothread attribute, so inventing an <iothread/>
subelement is a bit misleading, because if users query which disk uses
iothreads, they need to change their XPATH. Currently they can get away
with:

  //disk[driver/@iothread]/source/@file

but I guess for backwards compatibility, we can put the first iothread
ID into the attribute, e.g.:

  <driver iothread="2">
    <iothread>
     <iothread id="2"/>
     <iothread id="4"/>
    </iothread>
  </driver>


We've done something similar, when introducing multiple listen addresses
for VNC.

Now, an iothread is actually a thread pool. I guess we will never ever
need to assign individual threads from the pool to queues, right?

Michal

On Mon, Feb 06, 2023 at 03:11:21PM +0100, Michal Prívozník wrote:
> On 1/18/23 20:47, Stefan Hajnoczi wrote:
> > This is a preview of the iothread-vq-mapping parameter that assigns virtqueues
> > to IOThreads. The syntax is implemented but multiple IOThreads are not actually
> > supported yet. The purpose of this RFC is to reach agreement on the syntax and
> > to prepare for libvirt support.
> > 
> > virtio-blk and virtio-scsi devices will need a way to specify the
> > mapping between IOThreads and virtqueues. At the moment all virtqueues
> > are assigned to a single IOThread or the main loop. This single thread
> > can be a CPU bottleneck, so it is necessary to allow finer-grained
> > assignment to spread the load.
> > 
> > This series introduces command-line syntax for the new iothread-vq-mapping
> > property is as follows:
> > 
> >   --device '{"driver":"virtio-blk-pci","iothread-vq-mapping":[{"iothread":"iothread0","vqs":[0,1,2]},...]},...'
> > 
> > IOThreads are specified by name and virtqueues are specified by 0-based
> > index.
> > 
> > It will be common to simply assign virtqueues round-robin across a set
> > of IOThreads. A convenient syntax that does not require specifying
> > individual virtqueue indices is available:
> > 
> >   --device '{"driver":"virtio-blk-pci","iothread-vq-mapping":[{"iothread":"iothread0"},{"iothread":"iothread1"},...]},...'
> > 
> > There is no way to reassign virtqueues at runtime and I expect that to be a
> > very rare requirement.
> > 
> > Perhaps libvirt only needs to support round-robin because specifying individual
> > virtqueues is very specific and probably only useful for low-level performance
> > investigation. The libvirt domain XML syntax for this could be:
> > 
> >   <driver name='qemu' type='qcow2'>
> >     <iothreads>
> >       <iothread id="1"/>
> >       <iothread id="2"/>
> >       <iothread id="3"/>
> >     </iothreads>
> >     ...
> >   </driver>
> 
> Just for completeness, this how disk XML looks now:
> 
>   <disk type='file' device='disk'>
>     <driver name='qemu' type='qcow2' queues='4' iothread='1'/>
>     <source file='/tmp/data.qcow'/>
>     <target dev='vda' bus='virtio'/>
>     <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
>   </disk>
> 
> It corresponds to the following cmd line:
> 
>   -blockdev '{"driver":"file","filename":"/tmp/data.qcow","node-name":"libvirt-3-storage","auto-read-only":true,"discard":"unmap"}' \
>   -blockdev '{"node-name":"libvirt-3-format","read-only":false,"driver":"qcow2","file":"libvirt-3-storage"}' \
>   -device '{"driver":"virtio-blk-pci","iothread":"iothread1","num-queues":4,"bus":"pci.0","addr":"0x3","drive":"libvirt-3-format","id":"virtio-disk0","bootindex":1}' \
> 
> We already have @iothread attribute, so inventing an <iothread/>
> subelement is a bit misleading, because if users query which disk uses
> iothreads, they need to change their XPATH. Currently they can get away
> with:
> 
>   //disk[driver/@iothread]/source/@file
> 
> but I guess for backwards compatibility, we can put the first iothread
> ID into the attribute, e.g.:
> 
>   <driver iothread="2">
>     <iothread>
>      <iothread id="2"/>
>      <iothread id="4"/>
>     </iothread>
>   </driver>
> 
> 
> We've done something similar, when introducing multiple listen addresses
> for VNC.
> 
> Now, an iothread is actually a thread pool. I guess we will never ever
> need to assign individual threads from the pool to queues, right?

QEMU will have the ability to assign an individual virtqueue to a
specific IOThread.

At the moment I believe no one will need that much control. Users
probably just want to round-robin threads for virtio-blk and virtio-scsi
devices.

I think it's fine for libvirt domain XML to only support round-robin for
virtio-blk and virtio-scsi.

Stefan