| Message ID | 1259137008-9669-2-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
Sorry, I missed this the first time this was posted, and I see this in staging now. Gerd, could you please explain the motivation for this patch? I assumed console/baloon interrupts are not performance critical, so would we not be better off using a shared interrupt for these, reserving MSI vectors for where performance matters? On Wed, Nov 25, 2009 at 09:16:48AM +0100, Gerd Hoffmann wrote: > Enable MSI-X for virtio-console-pci and virtio-balloon-pci. > Add entries to the compatibility machine types so MSI-X will > be disabled for pc-0.10 and pc-0.11. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > hw/pc.c | 16 ++++++++++++++++ > hw/virtio-pci.c | 11 +++++++++++ > 2 files changed, 27 insertions(+), 0 deletions(-) > > diff --git a/hw/pc.c b/hw/pc.c > index fdaa52c..cb78923 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -1297,6 +1297,14 @@ static QEMUMachine pc_machine_v0_11 = { > .driver = "virtio-blk-pci", > .property = "vectors", > .value = stringify(0), > + },{ > + .driver = "virtio-balloon-pci", > + .property = "vectors", > + .value = stringify(0), > + },{ > + .driver = "virtio-console-pci", > + .property = "vectors", > + .value = stringify(0), > }, > { /* end of list */ } > } > @@ -1324,6 +1332,14 @@ static QEMUMachine pc_machine_v0_10 = { > .driver = "virtio-blk-pci", > .property = "vectors", > .value = stringify(0), > + },{ > + .driver = "virtio-balloon-pci", > + .property = "vectors", > + .value = stringify(0), > + },{ > + .driver = "virtio-console-pci", > + .property = "vectors", > + .value = stringify(0), > }, > { /* end of list */ } > }, > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > index aebcf9d..cb8ab21 100644 > --- a/hw/virtio-pci.c > +++ b/hw/virtio-pci.c > @@ -483,10 +483,13 @@ static int virtio_console_init_pci(PCIDevice *pci_dev) > if (!vdev) { > return -1; > } > + vdev->nvectors = proxy->nvectors; > virtio_init_pci(proxy, vdev, > PCI_VENDOR_ID_REDHAT_QUMRANET, > PCI_DEVICE_ID_VIRTIO_CONSOLE, > proxy->class_code, 0x00); > + /* make the actual value visible */ > + proxy->nvectors = vdev->nvectors; > return 0; > } > > @@ -531,11 +534,14 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev) > VirtIODevice *vdev; > > vdev = virtio_balloon_init(&pci_dev->qdev); > + vdev->nvectors = proxy->nvectors; > virtio_init_pci(proxy, vdev, > PCI_VENDOR_ID_REDHAT_QUMRANET, > PCI_DEVICE_ID_VIRTIO_BALLOON, > PCI_CLASS_MEMORY_RAM, > 0x00); > + /* make the actual value visible */ > + proxy->nvectors = vdev->nvectors; > return 0; > } > > @@ -569,6 +575,7 @@ static PCIDeviceInfo virtio_info[] = { > .init = virtio_console_init_pci, > .exit = virtio_exit_pci, > .qdev.props = (Property[]) { > + DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), > DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0), > DEFINE_PROP_END_OF_LIST(), > }, > @@ -579,6 +586,10 @@ static PCIDeviceInfo virtio_info[] = { > .init = virtio_balloon_init_pci, > .exit = virtio_exit_pci, > .qdev.reset = virtio_pci_reset, > + .qdev.props = (Property[]) { > + DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3), > + DEFINE_PROP_END_OF_LIST(), > + }, > },{ > /* end of list */ > } > -- > 1.6.2.5 > >
On 12/07/09 10:37, Michael S. Tsirkin wrote: > > Sorry, I missed this the first time this was posted, and I see this in > staging now. Gerd, could you please explain the motivation for this > patch? > > I assumed console/baloon interrupts are not performance critical, so > would we not be better off using a shared interrupt for these, > reserving MSI vectors for where performance matters? The motivation was to move them away from the ioapic, to reduce irq sharing of *other* devices which are connected to the ioapic too (i.e. usb, e1000, lsi, ...) I'm aware that these are not performance-critical. I've even tried to use 'vectors=1' because of that. I expected that would make them use MSI-X, but a single IRQ line only. Didn't work though. Intentional? cheers, Gerd
On Mon, Dec 07, 2009 at 11:32:36AM +0100, Gerd Hoffmann wrote: > On 12/07/09 10:37, Michael S. Tsirkin wrote: >> >> Sorry, I missed this the first time this was posted, and I see this in >> staging now. Gerd, could you please explain the motivation for this >> patch? >> >> I assumed console/baloon interrupts are not performance critical, so >> would we not be better off using a shared interrupt for these, >> reserving MSI vectors for where performance matters? > > The motivation was to move them away from the ioapic, to reduce irq > sharing of *other* devices which are connected to the ioapic too (i.e. > usb, e1000, lsi, ...) Let's convert these to MSI instead? This will likely pay off long term, e.g. with nested virtualization. > > I'm aware that these are not performance-critical. I've even tried to > use 'vectors=1' because of that. I expected that would make them use > MSI-X, but a single IRQ line only. Didn't work though. Intentional? So it's even worse, we are using up 2 vectors per device? Ugh ... vectors=1 currently will make guest fall back on regular interrupts, because IRQ field is undefined when MSI is used, so there is no way to distinguish between vq interrupt and config change. This last thing is important because it allows fastpath injection of MSI interrupts directly from kernel without notifying qemu to update IRQ field. Maybe we can define something special for a single vector, but this is not a use case I considered so will need guest changes ... > cheers, > Gerd
On 12/07/09 11:59, Michael S. Tsirkin wrote: >> The motivation was to move them away from the ioapic, to reduce irq >> sharing of *other* devices which are connected to the ioapic too (i.e. >> usb, e1000, lsi, ...) > > Let's convert these to MSI instead? > This will likely pay off long term, > e.g. with nested virtualization. Works only of the real hardware we emulate can do MSI-X too, otherwise guests simply wouldn't use it. I think the only case where this could work out is e1000, newer revisions can do MSI-X. Maybe also the upcoming megasas emulation Hannes is working on. >> I'm aware that these are not performance-critical. I've even tried to >> use 'vectors=1' because of that. I expected that would make them use >> MSI-X, but a single IRQ line only. Didn't work though. Intentional? > > So it's even worse, we are using up 2 vectors per device? Ugh ... Yes. Three for balloon, but that can easily changed to two. > no way to distinguish between vq interrupt > and config change. This last thing is important > because it allows fastpath injection of MSI > interrupts directly from kernel without > notifying qemu to update IRQ field. Ah, *that* is the reason for the separate config interrupt. Does the in-kernel injection matter for balloon+console? I'd expect only virtio-net needs that when it is configured with vhost? cheers Gerd
On Mon, Dec 07, 2009 at 12:24:02PM +0100, Gerd Hoffmann wrote: > On 12/07/09 11:59, Michael S. Tsirkin wrote: >>> The motivation was to move them away from the ioapic, to reduce irq >>> sharing of *other* devices which are connected to the ioapic too (i.e. >>> usb, e1000, lsi, ...) >> >> Let's convert these to MSI instead? >> This will likely pay off long term, >> e.g. with nested virtualization. > > Works only of the real hardware we emulate can do MSI-X too, otherwise > guests simply wouldn't use it. Sure. Naturally, improving IRQ routing so that e.g. lsi and usb do not share an interrupt would also be a good idea regardless. Also - why not simply use virtio? I assume you are talking about guests with virtio support otherwise MSI support in virtio won't matter for them. > I think the only case where this could > work out is e1000, newer revisions can do MSI-X. Maybe also the > upcoming megasas emulation Hannes is working on. Hmm. I would expect high-end storage to support MSI-X as well. Could you point me to linux drivers for devices we emulate so that I can check? >>> I'm aware that these are not performance-critical. I've even tried to >>> use 'vectors=1' because of that. I expected that would make them use >>> MSI-X, but a single IRQ line only. Didn't work though. Intentional? >> >> So it's even worse, we are using up 2 vectors per device? Ugh ... > > Yes. Three for balloon, but that can easily changed to two. Yes, please, make this change for 0.12. >> no way to distinguish between vq interrupt >> and config change. This last thing is important >> because it allows fastpath injection of MSI >> interrupts directly from kernel without >> notifying qemu to update IRQ field. > > Ah, *that* is the reason for the separate config interrupt. Does the > in-kernel injection matter for balloon+console? No, but changing this will need updating guests. And if we do update guests anyway, I would suggest that we put IRQ field in guest memory, with an atomic set, and guest would get it with test and clear, so that we get a generic interface and not something specific for console/baloon. Naturally, this needs a feature bit, and it won't happen in 0.12/2.6.33 timeframe. > I'd expect only > virtio-net needs that when it is configured with vhost? > > cheers > Gerd Any other device will need the same if it has a kernel backend.
On 12/07/09 14:03, Michael S. Tsirkin wrote: > Also - why not simply use virtio? I assume you are talking about > guests with virtio support otherwise MSI support in virtio > won't matter for them. Point. Guests with virtio-console+balloon most likely can use virtio for storage+net too, so nothing performance-critical is left in ioapic irq space and IRQ sharing for the leftovers (just usb I think) shouldn't be a big issue. >> Yes. Three for balloon, but that can easily changed to two. > > Yes, please, make this change for 0.12. We can also simply drop this ... > No, but changing this will need updating guests. ... or postphone until we have the "one MSI-X vector" support working. cheers, Gerd
On Mon, Dec 07, 2009 at 02:16:33PM +0100, Gerd Hoffmann wrote: > On 12/07/09 14:03, Michael S. Tsirkin wrote: >> Also - why not simply use virtio? I assume you are talking about >> guests with virtio support otherwise MSI support in virtio >> won't matter for them. > > Point. Guests with virtio-console+balloon most likely can use virtio > for storage+net too, so nothing performance-critical is left in ioapic > irq space and IRQ sharing for the leftovers (just usb I think) shouldn't > be a big issue. > >>> Yes. Three for balloon, but that can easily changed to two. >> >> Yes, please, make this change for 0.12. > > We can also simply drop this ... > >> No, but changing this will need updating guests. > > ... or postphone until we have the "one MSI-X vector" support working. OK. Anthony, could you drop this from staging for now please? Gerd, any idea whether "one MSI-X vector" feature is worth pursuing?
On 12/07/09 15:13, Michael S. Tsirkin wrote:
> Gerd, any idea whether "one MSI-X vector" feature is worth pursuing?
I'd rate it low priority. First because virtio-enabled guests can use
virtio-net+blk. Also because with the upcoming p35 support we'll get a
more modern pc emulation including a ioapic with all 24 IRQ lines being
wired up, which will help reducing IRQ sharing too.
MSI support (no -X) could be more intresting (for emulated devices) as
it is older and thus support is more common. My T60 for example has no
device with MSI-X support but 9 with MSI support. Linux turns on MSI
for 7 of them (4 PCIe ports, e1000, ahci, iwl3945).
cheers,
Gerd
On Mon, Dec 07, 2009 at 03:30:09PM +0100, Gerd Hoffmann wrote: > On 12/07/09 15:13, Michael S. Tsirkin wrote: >> Gerd, any idea whether "one MSI-X vector" feature is worth pursuing? > > I'd rate it low priority. First because virtio-enabled guests can use > virtio-net+blk. Also because with the upcoming p35 support we'll get a > more modern pc emulation including a ioapic with all 24 IRQ lines being > wired up, which will help reducing IRQ sharing too. > > MSI support (no -X) could be more intresting (for emulated devices) as > it is older and thus support is more common. My T60 for example has no > device with MSI-X support but 9 with MSI support. Linux turns on MSI > for 7 of them (4 PCIe ports, e1000, ahci, iwl3945). > > cheers, > Gerd Yes, but what matters is guest support I think. On windows it seems that both msix and msi support were added simulataneously. It won't be hard to add MSI support, but let's determine when is it needed.
diff --git a/hw/pc.c b/hw/pc.c index fdaa52c..cb78923 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -1297,6 +1297,14 @@ static QEMUMachine pc_machine_v0_11 = { .driver = "virtio-blk-pci", .property = "vectors", .value = stringify(0), + },{ + .driver = "virtio-balloon-pci", + .property = "vectors", + .value = stringify(0), + },{ + .driver = "virtio-console-pci", + .property = "vectors", + .value = stringify(0), }, { /* end of list */ } } @@ -1324,6 +1332,14 @@ static QEMUMachine pc_machine_v0_10 = { .driver = "virtio-blk-pci", .property = "vectors", .value = stringify(0), + },{ + .driver = "virtio-balloon-pci", + .property = "vectors", + .value = stringify(0), + },{ + .driver = "virtio-console-pci", + .property = "vectors", + .value = stringify(0), }, { /* end of list */ } }, diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index aebcf9d..cb8ab21 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -483,10 +483,13 @@ static int virtio_console_init_pci(PCIDevice *pci_dev) if (!vdev) { return -1; } + vdev->nvectors = proxy->nvectors; virtio_init_pci(proxy, vdev, PCI_VENDOR_ID_REDHAT_QUMRANET, PCI_DEVICE_ID_VIRTIO_CONSOLE, proxy->class_code, 0x00); + /* make the actual value visible */ + proxy->nvectors = vdev->nvectors; return 0; } @@ -531,11 +534,14 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev) VirtIODevice *vdev; vdev = virtio_balloon_init(&pci_dev->qdev); + vdev->nvectors = proxy->nvectors; virtio_init_pci(proxy, vdev, PCI_VENDOR_ID_REDHAT_QUMRANET, PCI_DEVICE_ID_VIRTIO_BALLOON, PCI_CLASS_MEMORY_RAM, 0x00); + /* make the actual value visible */ + proxy->nvectors = vdev->nvectors; return 0; } @@ -569,6 +575,7 @@ static PCIDeviceInfo virtio_info[] = { .init = virtio_console_init_pci, .exit = virtio_exit_pci, .qdev.props = (Property[]) { + DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0), DEFINE_PROP_END_OF_LIST(), }, @@ -579,6 +586,10 @@ static PCIDeviceInfo virtio_info[] = { .init = virtio_balloon_init_pci, .exit = virtio_exit_pci, .qdev.reset = virtio_pci_reset, + .qdev.props = (Property[]) { + DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3), + DEFINE_PROP_END_OF_LIST(), + }, },{ /* end of list */ }
Enable MSI-X for virtio-console-pci and virtio-balloon-pci. Add entries to the compatibility machine types so MSI-X will be disabled for pc-0.10 and pc-0.11. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/pc.c | 16 ++++++++++++++++ hw/virtio-pci.c | 11 +++++++++++ 2 files changed, 27 insertions(+), 0 deletions(-)