Re: [Qemu-devel] How many msi-x vectors should be allocated for the virtio-serial device?

Re: [Qemu-devel] How many msi-x vectors should be allocated for the virtio-serial device?

[Paolo Bonzini]

Il 31/01/2013 12:25, Michael S. Tsirkin ha scritto:
> On Thu, Jan 31, 2013 at 10:13:11AM +0200, Gal Hammer wrote:
>> Hi,
>>
>> How many msi-x vectors should be allocated for the virtio-serial device?
>>
>> I'm asking this as it seems that a proposed patch
>> (http://lists.gnu.org/archive/html/qemu-devel/2011-12/msg02094.html)
>> was not accepted and I re-encountered this issue while trying to
>> upgrade the virtio-serial's Windows driver to use msi-x vectors.
>>
>> The virtio-serial's Linux module tries to use one vector per
>> virtqueue (every serial port have two virtqueues) with a fall back
>> to using only two vectors
>> (http://lxr.linux.no/linux+v3.7.2/drivers/virtio/virtio_pci.c#L539).
>> The problem is that qemu's virtio-pci device allocate less vectors
>> than the modules expects. So, for example, if a serial device have
>> 16 ports, 17 vectors are allocated. The module tries to use 34
>> vectors, fails and choose to use only 2, leaving 15 unused vectors.
>>
>> Is it possible to increase the vectors number from
>> "proxy->serial.max_virtserial_ports + 1" to
>> "(proxy->serial.max_virtserial_ports + 1) * 2"?
>>
>> Thanks,
>>
>>     Gal.
> 
> Allocated MSI-X vectors on x86 are a limited resource.  Let's not waste
> them. From what you say virtio serial works fine with two
> vectors and I do not see any reason to let us use more
> until someone can show an important workload where this helps.
> 
> So I think we need something like the below, but we also
> need to handle 1.3 and older compatibility so the
> patch below shouldn't be applied. Want to try
> writing up the complete patch?

I think the patch should instead be something like

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 9abbcdf..24e8232 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -1154,7 +1154,7 @@ static const TypeInfo virtio_net_info = {
 
 static Property virtio_serial_properties[] = {
     DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
-    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, DEV_NVECTORS_UNSPECIFIED),
+    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
     DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
     DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31),

plus the backwards-compatibility stuff.

Paolo

> -->
> 
> serial: use 2 vectors by default
> 
> Guests only utilize 2 vectors and this seems to be enough,
> so let's only allocate as much.
> serial is likely not so performance intensive to require more,
> but if yes users can always override the nvectors property.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> ---
> 
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 9abbcdf..bb0f60e 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -975,8 +975,7 @@ static int virtio_serial_init_pci(PCIDevice *pci_dev)
>      if (!vdev) {
>          return -1;
>      }
> -    vdev->nvectors = proxy->nvectors == DEV_NVECTORS_UNSPECIFIED
> -                                        ? proxy->serial.max_virtserial_ports + 1
> +    vdev->nvectors = proxy->nvectors == DEV_NVECTORS_UNSPECIFIED ? 2
>                                          : proxy->nvectors;
>      virtio_init_pci(proxy, vdev);
>      proxy->nvectors = vdev->nvectors;
> 

Re: [Qemu-devel] How many msi-x vectors should be allocated for the virtio-serial device?

[Michael S. Tsirkin]

On Sat, Feb 02, 2013 at 12:01:52AM +0100, Paolo Bonzini wrote:
> Il 31/01/2013 12:25, Michael S. Tsirkin ha scritto:
> > On Thu, Jan 31, 2013 at 10:13:11AM +0200, Gal Hammer wrote:
> >> Hi,
> >>
> >> How many msi-x vectors should be allocated for the virtio-serial device?
> >>
> >> I'm asking this as it seems that a proposed patch
> >> (http://lists.gnu.org/archive/html/qemu-devel/2011-12/msg02094.html)
> >> was not accepted and I re-encountered this issue while trying to
> >> upgrade the virtio-serial's Windows driver to use msi-x vectors.
> >>
> >> The virtio-serial's Linux module tries to use one vector per
> >> virtqueue (every serial port have two virtqueues) with a fall back
> >> to using only two vectors
> >> (http://lxr.linux.no/linux+v3.7.2/drivers/virtio/virtio_pci.c#L539).
> >> The problem is that qemu's virtio-pci device allocate less vectors
> >> than the modules expects. So, for example, if a serial device have
> >> 16 ports, 17 vectors are allocated. The module tries to use 34
> >> vectors, fails and choose to use only 2, leaving 15 unused vectors.
> >>
> >> Is it possible to increase the vectors number from
> >> "proxy->serial.max_virtserial_ports + 1" to
> >> "(proxy->serial.max_virtserial_ports + 1) * 2"?
> >>
> >> Thanks,
> >>
> >>     Gal.
> > 
> > Allocated MSI-X vectors on x86 are a limited resource.  Let's not waste
> > them. From what you say virtio serial works fine with two
> > vectors and I do not see any reason to let us use more
> > until someone can show an important workload where this helps.
> > 
> > So I think we need something like the below, but we also
> > need to handle 1.3 and older compatibility so the
> > patch below shouldn't be applied. Want to try
> > writing up the complete patch?
> 
> I think the patch should instead be something like
> 
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 9abbcdf..24e8232 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -1154,7 +1154,7 @@ static const TypeInfo virtio_net_info = {
>  
>  static Property virtio_serial_properties[] = {
>      DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
> -    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, DEV_NVECTORS_UNSPECIFIED),
> +    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
>      DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
>      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
>      DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31),
> 
> plus the backwards-compatibility stuff.
> 
> Paolo

Makes sense, but the logic in virtio_serial_init_pci is
then dead code and should go away.

> > -->
> > 
> > serial: use 2 vectors by default
> > 
> > Guests only utilize 2 vectors and this seems to be enough,
> > so let's only allocate as much.
> > serial is likely not so performance intensive to require more,
> > but if yes users can always override the nvectors property.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > ---
> > 
> > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> > index 9abbcdf..bb0f60e 100644
> > --- a/hw/virtio-pci.c
> > +++ b/hw/virtio-pci.c
> > @@ -975,8 +975,7 @@ static int virtio_serial_init_pci(PCIDevice *pci_dev)
> >      if (!vdev) {
> >          return -1;
> >      }
> > -    vdev->nvectors = proxy->nvectors == DEV_NVECTORS_UNSPECIFIED
> > -                                        ? proxy->serial.max_virtserial_ports + 1
> > +    vdev->nvectors = proxy->nvectors == DEV_NVECTORS_UNSPECIFIED ? 2
> >                                          : proxy->nvectors;
> >      virtio_init_pci(proxy, vdev);
> >      proxy->nvectors = vdev->nvectors;
> >