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 <address@hidden>
> 
> ---
> 
> 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;
>