thr3ads.net - Linux Virtualization - [PATCH RFC] virtio-pci: share config interrupt between virtio devices [Sep 2014]

If this information is useful, please help other people find it:
Share via:

Amos Kong

2014-Sep-01 05:41 UTC

[PATCH RFC] virtio-pci: share config interrupt between virtio devices

One VM only has 128 msix interrupt, virtio-config interrupt
has less workload. This patch shares one normal interrupt
for configuration between virtio devices.

Signed-off-by: Amos Kong <akong at redhat.com>
---
 drivers/virtio/virtio_pci.c | 41 ++++++++++++++++-------------------------
 1 file changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 3d1463c..b1263b3 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -52,6 +52,7 @@ struct virtio_pci_device
 	/* Name strings for interrupts. This size should be enough,
 	 * and I'm too lazy to allocate each name separately. */
 	char (*msix_names)[256];
+	char config_msix_name[256];
 	/* Number of available vectors */
 	unsigned msix_vectors;
 	/* Vectors allocated, excluding per-vq vectors if any */
@@ -282,12 +283,6 @@ static void vp_free_vectors(struct virtio_device *vdev)
 			free_cpumask_var(vp_dev->msix_affinity_masks[i]);
 
 	if (vp_dev->msix_enabled) {
-		/* Disable the vector used for configuration */
-		iowrite16(VIRTIO_MSI_NO_VECTOR,
-			  vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
-		/* Flush the write out to device */
-		ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
-
 		pci_disable_msix(vp_dev->pci_dev);
 		vp_dev->msix_enabled = 0;
 	}
@@ -339,24 +334,18 @@ static int vp_request_msix_vectors(struct virtio_device
*vdev, int nvectors,
 		goto error;
 	vp_dev->msix_enabled = 1;
 
-	/* Set the vector used for configuration */
-	v = vp_dev->msix_used_vectors;
-	snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
+	/* Set shared IRQ for configuration */
+	snprintf(vp_dev->config_msix_name, sizeof(*vp_dev->msix_names),
 		 "%s-config", name);
-	err = request_irq(vp_dev->msix_entries[v].vector,
-			  vp_config_changed, 0, vp_dev->msix_names[v],
+	err = request_irq(vp_dev->pci_dev->irq,
+			  vp_config_changed,
+			  IRQF_SHARED,
+			  vp_dev->config_msix_name,
 			  vp_dev);
-	if (err)
-		goto error;
-	++vp_dev->msix_used_vectors;
-
-	iowrite16(v, vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
-	/* Verify we had enough resources to assign the vector */
-	v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
-	if (v == VIRTIO_MSI_NO_VECTOR) {
-		err = -EBUSY;
+	if (!err)
+		vp_dev->intx_enabled = 1;
+	else
 		goto error;
-	}
 
 	if (!per_vq_vectors) {
 		/* Shared vector for all VQs */
@@ -535,14 +524,16 @@ static int vp_try_to_find_vqs(struct virtio_device *vdev,
unsigned nvqs,
 			goto error_request;
 	} else {
 		if (per_vq_vectors) {
-			/* Best option: one for change interrupt, one per vq. */
-			nvectors = 1;
+			/* Best option: one normal interrupt for change,
+			   one msix per vq. */
+			nvectors = 0;
 			for (i = 0; i < nvqs; ++i)
 				if (callbacks[i])
 					++nvectors;
 		} else {
-			/* Second best: one for change, shared for all vqs. */
-			nvectors = 2;
+			/* Second best: one normal interrupt for
+			   change, share one msix for all vqs. */
+			nvectors = 1;
 		}
 
 		err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
-- 
1.9.3

Michael S. Tsirkin

2014-Sep-01 06:37 UTC

head link

[PATCH RFC] virtio-pci: share config interrupt between virtio devices

On Mon, Sep 01, 2014 at 01:41:54PM +0800, Amos Kong
wrote:> One VM only has 128 msix interrupt, virtio-config interrupt
> has less workload. This patch shares one normal interrupt
normal == INT#x? Please don't call it normal. The proper name is
"legacy INT#x".

So you are trying to use legacy INT#x at the same time
with MSI-X?  This does not work: the PCI spec says:
	While enabled for MSI or MSI-X
	operation, a function is prohibited from using its INTx# pin (if
	implemented) to request
	service (MSI, MSI-X, and INTx# are mutually exclusive).

does the patch work for you? If it does it might be a (minor) spec
violation in kvm.

Besides, INT#x really leads to terrible performance because
sharing is forced even if there aren't many devices.

Why do we need INT#x?
How about setting IRQF_SHARED for the config interrupt
while using MSI-X? You'd have to read ISR to check that the interrupt was
intended for your device.

> for configuration between virtio devices.
> 
> Signed-off-by: Amos Kong <akong at redhat.com>
> ---
>  drivers/virtio/virtio_pci.c | 41 ++++++++++++++++-------------------------
>  1 file changed, 16 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 3d1463c..b1263b3 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -52,6 +52,7 @@ struct virtio_pci_device
>  	/* Name strings for interrupts. This size should be enough,
>  	 * and I'm too lazy to allocate each name separately. */
>  	char (*msix_names)[256];
> +	char config_msix_name[256];
>  	/* Number of available vectors */
>  	unsigned msix_vectors;
>  	/* Vectors allocated, excluding per-vq vectors if any */
> @@ -282,12 +283,6 @@ static void vp_free_vectors(struct virtio_device
*vdev)
>  			free_cpumask_var(vp_dev->msix_affinity_masks[i]);
>  
>  	if (vp_dev->msix_enabled) {
> -		/* Disable the vector used for configuration */
> -		iowrite16(VIRTIO_MSI_NO_VECTOR,
> -			  vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> -		/* Flush the write out to device */
> -		ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> -
>  		pci_disable_msix(vp_dev->pci_dev);
>  		vp_dev->msix_enabled = 0;
>  	}
> @@ -339,24 +334,18 @@ static int vp_request_msix_vectors(struct
virtio_device *vdev, int nvectors,
>  		goto error;
>  	vp_dev->msix_enabled = 1;
>  
> -	/* Set the vector used for configuration */
> -	v = vp_dev->msix_used_vectors;
> -	snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
> +	/* Set shared IRQ for configuration */
> +	snprintf(vp_dev->config_msix_name, sizeof(*vp_dev->msix_names),
>  		 "%s-config", name);
> -	err = request_irq(vp_dev->msix_entries[v].vector,
> -			  vp_config_changed, 0, vp_dev->msix_names[v],
> +	err = request_irq(vp_dev->pci_dev->irq,
> +			  vp_config_changed,
> +			  IRQF_SHARED,
> +			  vp_dev->config_msix_name,
>  			  vp_dev);
> -	if (err)
> -		goto error;
> -	++vp_dev->msix_used_vectors;
> -
> -	iowrite16(v, vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> -	/* Verify we had enough resources to assign the vector */
> -	v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> -	if (v == VIRTIO_MSI_NO_VECTOR) {
> -		err = -EBUSY;
> +	if (!err)
> +		vp_dev->intx_enabled = 1;
> +	else
>  		goto error;
> -	}
>  
>  	if (!per_vq_vectors) {
>  		/* Shared vector for all VQs */
> @@ -535,14 +524,16 @@ static int vp_try_to_find_vqs(struct virtio_device
*vdev, unsigned nvqs,
>  			goto error_request;
>  	} else {
>  		if (per_vq_vectors) {
> -			/* Best option: one for change interrupt, one per vq. */
> -			nvectors = 1;
> +			/* Best option: one normal interrupt for change,
> +			   one msix per vq. */
> +			nvectors = 0;
>  			for (i = 0; i < nvqs; ++i)
>  				if (callbacks[i])
>  					++nvectors;
>  		} else {
> -			/* Second best: one for change, shared for all vqs. */
> -			nvectors = 2;
> +			/* Second best: one normal interrupt for
> +			   change, share one msix for all vqs. */
> +			nvectors = 1;
>  		}
>  
>  		err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
> -- 
> 1.9.3

Amos Kong

2014-Sep-01 07:58 UTC

head link

[PATCH RFC] virtio-pci: share config interrupt between virtio devices

On Mon, Sep 01, 2014 at 09:37:30AM +0300, Michael S. Tsirkin
wrote:>
Hi Michael,
> On Mon, Sep 01, 2014 at 01:41:54PM +0800, Amos Kong wrote:
> > One VM only has 128 msix interrupt, virtio-config interrupt
> > has less workload. This patch shares one normal interrupt
Thanks for your quick reply.
> normal == INT#x? Please don't call it normal. The proper name is
> "legacy INT#x". 
OK
> So you are trying to use legacy INT#x at the same time
> with MSI-X?  This does not work: the PCI spec says:
> 	While enabled for MSI or MSI-X
> 	operation, a function is prohibited from using its INTx# pin (if
> 	implemented) to request
> 	service (MSI, MSI-X, and INTx# are mutually exclusive).
It means we can't use interrupt and triggered-interrupt together for
one PCI device. I will study this problem.
 > does the patch work for you? If it does it might be a (minor) spec
> violation in kvm.
I did some basic testing (multiple nics, scp, ping, etc), it works.
 > Besides, INT#x really leads to terrible performance because
> sharing is forced even if there aren't many devices.
> 
> Why do we need INT#x?
> How about setting IRQF_SHARED for the config interrupt
> while using MSI-X?
> You'd have to read ISR to check that the interrupt was
> intended for your device. 
I have a draft patch to share one MSI-X for all virtio-config, it has
some problem in hotplugging devices. I will continue this way.
 > > for configuration between virtio devices.
> > 
> > Signed-off-by: Amos Kong <akong at redhat.com>
> > ---
> >  drivers/virtio/virtio_pci.c | 41
++++++++++++++++-------------------------
> >  1 file changed, 16 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> > index 3d1463c..b1263b3 100644
> > --- a/drivers/virtio/virtio_pci.c
> > +++ b/drivers/virtio/virtio_pci.c
> > @@ -52,6 +52,7 @@ struct virtio_pci_device
> >  	/* Name strings for interrupts. This size should be enough,
> >  	 * and I'm too lazy to allocate each name separately. */
> >  	char (*msix_names)[256];
> > +	char config_msix_name[256];
> >  	/* Number of available vectors */
> >  	unsigned msix_vectors;
> >  	/* Vectors allocated, excluding per-vq vectors if any */
> > @@ -282,12 +283,6 @@ static void vp_free_vectors(struct virtio_device
*vdev)
> >  			free_cpumask_var(vp_dev->msix_affinity_masks[i]);
> >  
> >  	if (vp_dev->msix_enabled) {
> > -		/* Disable the vector used for configuration */
> > -		iowrite16(VIRTIO_MSI_NO_VECTOR,
> > -			  vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> > -		/* Flush the write out to device */
> > -		ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> > -
> >  		pci_disable_msix(vp_dev->pci_dev);
> >  		vp_dev->msix_enabled = 0;
> >  	}
> > @@ -339,24 +334,18 @@ static int vp_request_msix_vectors(struct
virtio_device *vdev, int nvectors,
> >  		goto error;
> >  	vp_dev->msix_enabled = 1;
> >  
> > -	/* Set the vector used for configuration */
> > -	v = vp_dev->msix_used_vectors;
> > -	snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
> > +	/* Set shared IRQ for configuration */
> > +	snprintf(vp_dev->config_msix_name,
sizeof(*vp_dev->msix_names),
> >  		 "%s-config", name);
> > -	err = request_irq(vp_dev->msix_entries[v].vector,
> > -			  vp_config_changed, 0, vp_dev->msix_names[v],
> > +	err = request_irq(vp_dev->pci_dev->irq,
> > +			  vp_config_changed,
> > +			  IRQF_SHARED,
> > +			  vp_dev->config_msix_name,
> >  			  vp_dev);
> > -	if (err)
> > -		goto error;
> > -	++vp_dev->msix_used_vectors;
> > -
> > -	iowrite16(v, vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> > -	/* Verify we had enough resources to assign the vector */
> > -	v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> > -	if (v == VIRTIO_MSI_NO_VECTOR) {
> > -		err = -EBUSY;
> > +	if (!err)
> > +		vp_dev->intx_enabled = 1;
> > +	else
> >  		goto error;
> > -	}
> >  
> >  	if (!per_vq_vectors) {
> >  		/* Shared vector for all VQs */
> > @@ -535,14 +524,16 @@ static int vp_try_to_find_vqs(struct
virtio_device *vdev, unsigned nvqs,
> >  			goto error_request;
> >  	} else {
> >  		if (per_vq_vectors) {
> > -			/* Best option: one for change interrupt, one per vq. */
> > -			nvectors = 1;
> > +			/* Best option: one normal interrupt for change,
> > +			   one msix per vq. */
> > +			nvectors = 0;
> >  			for (i = 0; i < nvqs; ++i)
> >  				if (callbacks[i])
> >  					++nvectors;
> >  		} else {
> > -			/* Second best: one for change, shared for all vqs. */
> > -			nvectors = 2;
> > +			/* Second best: one normal interrupt for
> > +			   change, share one msix for all vqs. */
> > +			nvectors = 1;
> >  		}
> >  
> >  		err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
> > -- 
> > 1.9.3
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
			Amos.

Stefan Fritsch

2014-Sep-18 19:18 UTC

head link

[PATCH RFC] virtio-pci: share config interrupt between virtio devices

On Monday 01 September 2014 09:37:30, Michael S. Tsirkin
wrote:> Why do we need INT#x?
> How about setting IRQF_SHARED for the config interrupt
> while using MSI-X? You'd have to read ISR to check that the
> interrupt was intended for your device.
The virtio 0.9.5 spec says that ISR is "unused" when in MSI-X mode. I 
don't think that you can depend on the device to set the configuration 
changed bit.

The virtio 1.0 spec seems to have fixed that.

Apparently Analagous Threads

Search for more apparently analagous threads

Linux Virtualization - Sep 2014 - [PATCH RFC] virtio-pci: share config interrupt between virtio devices

[PATCH RFC] virtio-pci: share config interrupt between virtio devices

[PATCH RFC] virtio-pci: share config interrupt between virtio devices

[PATCH RFC] virtio-pci: share config interrupt between virtio devices

[PATCH RFC] virtio-pci: share config interrupt between virtio devices

Apparently Analagous Threads