diff mbox

How many msi-x vectors should be allocated for the virtio-serial device?

Message ID 510C4960.4030201@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini Feb. 1, 2013, 11:01 p.m. UTC 
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


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;
>

Comments

Michael S. Tsirkin Feb. 3, 2013, 12:11 p.m. UTC | #1 
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;
> >
Paolo Bonzini Feb. 4, 2013, 9:42 a.m. UTC | #2 
Il 03/02/2013 13:11, Michael S. Tsirkin ha scritto:
>> >  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.
> 

It won't be dead code for the backwards-compatible machine types (that
use DEV_NVECTORS_UNSPECIFIED).

Paolo
Michael S. Tsirkin Feb. 4, 2013, 9:59 a.m. UTC | #3 
On Mon, Feb 04, 2013 at 10:42:32AM +0100, Paolo Bonzini wrote:
> Il 03/02/2013 13:11, Michael S. Tsirkin ha scritto:
> >> >  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.
> > 
> 
> It won't be dead code for the backwards-compatible machine types (that
> use DEV_NVECTORS_UNSPECIFIED).
> 
> Paolo

Good point. Ack. Want to post this with proper signature etc?
Amit Shah Feb. 27, 2013, 12:35 p.m. UTC | #4 
On (Mon) 04 Feb 2013 [11:59:02], Michael S. Tsirkin wrote:
> On Mon, Feb 04, 2013 at 10:42:32AM +0100, Paolo Bonzini wrote:
> > Il 03/02/2013 13:11, Michael S. Tsirkin ha scritto:
> > >> >  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.
> > > 
> > 
> > It won't be dead code for the backwards-compatible machine types (that
> > use DEV_NVECTORS_UNSPECIFIED).
> > 
> > Paolo
> 
> Good point. Ack. Want to post this with proper signature etc?

Gal, do you want to submit a patch for this?

		Amit
diff mbox

Patch

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),