RFC: sharing config interrupt between virtio devices for saving MSI

Hi all,

I'm working on this item in Upstream Networking todolist:

| - Sharing config interrupts
|    Support more devices by sharing a single msi vector
|    between multiple virtio devices.
|    (Applies to virtio-blk too).

I have this solution here, and only have draft patches of Solution
1 & 2, let's discuss if solution 3 is feasible.

* We should not introduce perf regression in this change
* little effect is acceptable because we are _sharing_ interrupt

Solution 1:
 static void vp_free_vectors(struct virtio_device *vdev)

On Sat, Apr 19, 2014 at 12:08:15PM +0800, Amos Kong wrote:
> 
> Hi all,
> 
> I'm working on this item in Upstream Networking todolist:
> 
> | - Sharing config interrupts
> |    Support more devices by sharing a single msi vector
> |    between multiple virtio devices.
> |    (Applies to virtio-blk too).
> 
> I have this solution here, and only have draft patches of Solution
> 1 & 2, let's discuss if solution 3 is feasible.
> 
> * We should not introduce perf regression in this change
> * little effect is acceptable because we are _sharing_ interrupt
> 
> Solution 1:
> ==========
> share one LSI interrupt for configuration change of all virtio devices.
> Draft patch: share_lsi_for_all_config.patch
> 
> Solution 2:
> ==========
> share one MSI interrupt for configuration change of all virtio devices.
> Draft patch: share_msix_for_all_config.patch
> 
> Solution 3:
> ==========
> dynamic adjust interrupt policy when device is added or removed
> Current:
> -------
> - try to allocate a MSI to device's config
> - try to allocate a MSI for each vq
> - share one MSI for all vqs of one device
> - share one LSI for config & vqs of one device
> 
> additional dynamic policies:
> ---------------------------
> - if there is no enough MSI, adjust interrupt allocation for returning
>   some MSI
>    - try to find a device that allocated one MSI for each vq, change
>      it to share one MSI for saving the MSI
>    - try to share one MSI for config of all virtio_pci devices
>    - try to share one LSI for config of all devices
> 
> - if device is removed, try to re-allocate freed MSI to existed
>   devices, if they aren't in best status (one MSI for config, one
>   MSI for each vq)
>    - if config of all devices is sharing one LSI, try to upgrade it to MSI
>    - if config of all devices is sharing one MSI, try to allocate one
>      MSI for configuration change of each device
>    - try to find a device that is sharing one MSI for all vqs, try to
>      allocate one MSI for each vq
> 
> 
> 
> BTW, I saw we still notify all vqs even VIRTIO_PCI_ISR_CONFIG bit of
> isr is set, is it necessary?

[Reply myself]

Quote from Virtio-spec:

| 2.4.3 Dealing With Configuration Changes
| Some virtio PCI devices can change the device configuration state, as reflected
| in the virtio header in the PCI configuration space. In this case:
| 
| 1. If MSI-X capability is disabled: an interrupt is delivered and the sec-
| ond highest bit is set in the ISR Status field to indicate that the driver
| should re-examine the configuration space.

| Note that a single interrupt can
| indicate both that one or more virtqueue has been used and that the con-
| figuration space has changed: even if the config bit is set, virtqueues must
| be scanned. <<<<

It seems current code is fine.

 
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 101db3f..176aabc 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -259,9 +259,9 @@ static irqreturn_t vp_interrupt(int irq, void
> *opaque)
>  
>         /* Configuration change?  Tell driver if it wants to know. */
>         if (isr & VIRTIO_PCI_ISR_CONFIG)
> -               vp_config_changed(irq, opaque);
> -
> -       return vp_vring_interrupt(irq, opaque);
> +               return vp_config_changed(irq, opaque);
> +       else
> +               return vp_vring_interrupt(irq, opaque);
>  }
>  
>  static void vp_free_vectors(struct virtio_device *vdev)
> 
> -- 
> 			Amos.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, Apr 19, 2014 at 12:08:15PM +0800, Amos Kong wrote:
> 
> Hi all,
> 
> I'm working on this item in Upstream Networking todolist:
> 
> | - Sharing config interrupts
> |    Support more devices by sharing a single msi vector
> |    between multiple virtio devices.
> |    (Applies to virtio-blk too).
> 
> I have this solution here, and only have draft patches of Solution
> 1 & 2, let's discuss if solution 3 is feasible.
> 
> * We should not introduce perf regression in this change
> * little effect is acceptable because we are _sharing_ interrupt
> 
> Solution 1:
> ==========
> share one LSI interrupt for configuration change of all virtio devices.
> Draft patch: share_lsi_for_all_config.patch
> 
> Solution 2:
> ==========
> share one MSI interrupt for configuration change of all virtio devices.
> Draft patch: share_msix_for_all_config.patch
> 
> Solution 3:
> ==========
> dynamic adjust interrupt policy when device is added or removed
> Current:
> -------
> - try to allocate a MSI to device's config
> - try to allocate a MSI for each vq
> - share one MSI for all vqs of one device
> - share one LSI for config & vqs of one device
> 
> additional dynamic policies:
> ---------------------------
> - if there is no enough MSI, adjust interrupt allocation for returning
>   some MSI
>    - try to find a device that allocated one MSI for each vq, change
>      it to share one MSI for saving the MSI
>    - try to share one MSI for config of all virtio_pci devices
>    - try to share one LSI for config of all devices
> 
> - if device is removed, try to re-allocate freed MSI to existed
>   devices, if they aren't in best status (one MSI for config, one
>   MSI for each vq)
>    - if config of all devices is sharing one LSI, try to upgrade it to MSI
>    - if config of all devices is sharing one MSI, try to allocate one
>      MSI for configuration change of each device
>    - try to find a device that is sharing one MSI for all vqs, try to
>      allocate one MSI for each vq

Hi Michael, Alex

Any thoughts about this ? :)


Thanks, Amos


> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 101db3f..5ba348d 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -302,6 +302,8 @@ static void vp_free_vectors(struct virtio_device *vdev)
>  	vp_dev->msix_affinity_masks = NULL;
>  }
>  
> +//static msix_entry *config_msix_entry;
> +static char config_msix_name[100];
>  static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
>  				   bool per_vq_vectors)
>  {
> @@ -341,14 +343,13 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
>  
>  	/* Set the vector used for configuration */
>  	v = vp_dev->msix_used_vectors;
> -	snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
> +	snprintf(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],
> -			  vp_dev);
> +	err = request_irq(vp_dev->pci_dev->irq, vp_config_changed, IRQF_SHARED, config_msix_name, vp_dev);
>  	if (err)
>  		goto error;
> -	++vp_dev->msix_used_vectors;
> +
> +        //++vp_dev->msix_used_vectors;
>  
>  	iowrite16(v, vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
>  	/* Verify we had enough resources to assign the vector */
> @@ -380,7 +381,7 @@ static int vp_request_intx(struct virtio_device *vdev)
>  {
>  	int err;
>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> -
> +	printk(KERN_INFO "share irq: %d\n", vp_dev->pci_dev->irq);
>  	err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
>  			  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
>  	if (!err)
> @@ -536,13 +537,13 @@ static int vp_try_to_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>  	} else {
>  		if (per_vq_vectors) {
>  			/* Best option: one for change interrupt, one per vq. */
> -			nvectors = 1;
> +			nvectors = 0;
>  			for (i = 0; i < nvqs; ++i)
>  				if (callbacks[i])
>  					++nvectors;
>  		} else {
>  			/* Second best: one for change, shared for all vqs. */
> -			nvectors = 2;
> +			nvectors = 1;
>  		}
>  
>  		err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);

> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 101db3f..5ae1844 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -62,6 +62,8 @@ struct virtio_pci_device
>  
>  	/* Whether we have vector per vq */
>  	bool per_vq_vectors;
> +
> +	char config_msix_name[256];
>  };
>  
>  /* Constants for MSI-X */
> @@ -302,6 +304,8 @@ static void vp_free_vectors(struct virtio_device *vdev)
>  	vp_dev->msix_affinity_masks = NULL;
>  }
>  
> +static struct msix_entry *config_msix_entry;
> +static char *config_msix_name;
>  static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
>  				   bool per_vq_vectors)
>  {
> @@ -309,9 +313,14 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
>  	const char *name = dev_name(&vp_dev->vdev.dev);
>  	unsigned i, v;
>  	int err = -ENOMEM;
> +        int has_config_msix = 1;
>  
> -	vp_dev->msix_vectors = nvectors;
> +        if (!config_msix_entry) {
> +		has_config_msix = 0;
> +		nvectors += 1;
> +        }
>  
> +	vp_dev->msix_vectors = nvectors;
>  	vp_dev->msix_entries = kmalloc(nvectors * sizeof *vp_dev->msix_entries,
>  				       GFP_KERNEL);
>  	if (!vp_dev->msix_entries)
> @@ -341,14 +350,24 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
>  
>  	/* Set the vector used for configuration */
>  	v = vp_dev->msix_used_vectors;
> -	snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
> +
> +        if (has_config_msix == 0) {
> +		config_msix_entry = &vp_dev->msix_entries[v];
> +		config_msix_name = vp_dev->msix_names[v];
> +        } else {
> +		config_msix_name = vp_dev->config_msix_name;
> +	}
> +
> +	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],
> -			  vp_dev);
> +	err = request_irq(config_msix_entry->vector,
> +                          vp_config_changed, IRQF_SHARED,
> +                          vp_dev->config_msix_name, vp_dev);
>  	if (err)
>  		goto error;
> -	++vp_dev->msix_used_vectors;
> +
> +        if (has_config_msix == 0)
> +             ++vp_dev->msix_used_vectors;
>  
>  	iowrite16(v, vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
>  	/* Verify we had enough resources to assign the vector */
> @@ -536,13 +555,13 @@ static int vp_try_to_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>  	} else {
>  		if (per_vq_vectors) {
>  			/* Best option: one for change interrupt, one per vq. */
> -			nvectors = 1;
> +			nvectors = 0;
>  			for (i = 0; i < nvqs; ++i)
>  				if (callbacks[i])
>  					++nvectors;
>  		} else {
>  			/* Second best: one for change, shared for all vqs. */
> -			nvectors = 2;
> +			nvectors = 1;
>  		}
>  
>  		err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);

On Sun, Apr 27, 2014 at 10:35:41PM +0800, Amos Kong wrote:
> On Sat, Apr 19, 2014 at 12:08:15PM +0800, Amos Kong wrote:
> > 
> > Hi all,
> > 
> > I'm working on this item in Upstream Networking todolist:
> > 
> > | - Sharing config interrupts
> > |    Support more devices by sharing a single msi vector
> > |    between multiple virtio devices.
> > |    (Applies to virtio-blk too).
> > 
> > I have this solution here, and only have draft patches of Solution
> > 1 & 2, let's discuss if solution 3 is feasible.
> > 
> > * We should not introduce perf regression in this change
> > * little effect is acceptable because we are _sharing_ interrupt
> > 
> > Solution 1:
> > ==========
> > share one LSI interrupt for configuration change of all virtio devices.
> > Draft patch: share_lsi_for_all_config.patch
> > 
> > Solution 2:
> > ==========
> > share one MSI interrupt for configuration change of all virtio devices.
> > Draft patch: share_msix_for_all_config.patch
> > 
> > Solution 3:
> > ==========
> > dynamic adjust interrupt policy when device is added or removed
> > Current:
> > -------
> > - try to allocate a MSI to device's config
> > - try to allocate a MSI for each vq
> > - share one MSI for all vqs of one device
> > - share one LSI for config & vqs of one device
> > 
> > additional dynamic policies:
> > ---------------------------
> > - if there is no enough MSI, adjust interrupt allocation for returning
> >   some MSI
> >    - try to find a device that allocated one MSI for each vq, change
> >      it to share one MSI for saving the MSI
> >    - try to share one MSI for config of all virtio_pci devices
> >    - try to share one LSI for config of all devices
> > 
> > - if device is removed, try to re-allocate freed MSI to existed
> >   devices, if they aren't in best status (one MSI for config, one
> >   MSI for each vq)
> >    - if config of all devices is sharing one LSI, try to upgrade it to MSI
> >    - if config of all devices is sharing one MSI, try to allocate one
> >      MSI for configuration change of each device
> >    - try to find a device that is sharing one MSI for all vqs, try to
> >      allocate one MSI for each vq
> 
> Hi Michael, Alex
> 
> Any thoughts about this ? :)
> 
> 
> Thanks, Amos


I don't think we need to worry about dynamic.

Questions: just by using IRQF_SHARED, do we always end
up with a shared interrupt? Or only if system is running
out of vectors?
Did you test that interrupt is actually shared and all
handlers are called? We don't stop after the 1st handler that
returned OK?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sun, Apr 27, 2014 at 06:03:04PM +0300, Michael S. Tsirkin wrote:
> On Sun, Apr 27, 2014 at 10:35:41PM +0800, Amos Kong wrote:
> > On Sat, Apr 19, 2014 at 12:08:15PM +0800, Amos Kong wrote:
> > > 
> > > Hi all,
> > > 
> > > I'm working on this item in Upstream Networking todolist:
> > > 
> > > | - Sharing config interrupts
> > > |    Support more devices by sharing a single msi vector
> > > |    between multiple virtio devices.
> > > |    (Applies to virtio-blk too).
> > > 
> > > I have this solution here, and only have draft patches of Solution
> > > 1 & 2, let's discuss if solution 3 is feasible.
> > > 
> > > * We should not introduce perf regression in this change
> > > * little effect is acceptable because we are _sharing_ interrupt
> > > 
> > > Solution 1:
> > > ==========
> > > share one LSI interrupt for configuration change of all virtio devices.
> > > Draft patch: share_lsi_for_all_config.patch
> > > 
> > > Solution 2:
> > > ==========
> > > share one MSI interrupt for configuration change of all virtio devices.
> > > Draft patch: share_msix_for_all_config.patch
> > > 
> > > Solution 3:
> > > ==========
> > > dynamic adjust interrupt policy when device is added or removed
> > > Current:
> > > -------
> > > - try to allocate a MSI to device's config
> > > - try to allocate a MSI for each vq
> > > - share one MSI for all vqs of one device
> > > - share one LSI for config & vqs of one device
> > > 
> > > additional dynamic policies:
> > > ---------------------------
> > > - if there is no enough MSI, adjust interrupt allocation for returning
> > >   some MSI
> > >    - try to find a device that allocated one MSI for each vq, change
> > >      it to share one MSI for saving the MSI
> > >    - try to share one MSI for config of all virtio_pci devices
> > >    - try to share one LSI for config of all devices
> > > 
> > > - if device is removed, try to re-allocate freed MSI to existed
> > >   devices, if they aren't in best status (one MSI for config, one
> > >   MSI for each vq)
> > >    - if config of all devices is sharing one LSI, try to upgrade it to MSI
> > >    - if config of all devices is sharing one MSI, try to allocate one
> > >      MSI for configuration change of each device
> > >    - try to find a device that is sharing one MSI for all vqs, try to
> > >      allocate one MSI for each vq
> > 
> > Hi Michael, Alex
> > 
> > Any thoughts about this ? :)
> > 
> > 
> > Thanks, Amos
> 
> 
> I don't think we need to worry about dynamic.
 
Dynamic policy can always get good balance. If we use a fixed policy,
devices might share IRQ with some unused msix, or sometimes it's not
fair enough for all devices.

> Questions: just by using IRQF_SHARED, do we always end
> up with a shared interrupt? Or only if system is running
> out of vectors?

In solution1: always share one LSI for config of all virtio devices
In solution2: always share one MSI for config of all virtio devices

> Did you test that interrupt is actually shared and all
> handlers are called?

Yes. I can find many devices (virtio.0, virtio.1,...) share same
interrupt in guest's /proc/interrupts. And nics work well.

> We don't stop after the 1st handler that
> returned OK?

When we set IRQF_SHARED flag, dev_id is necessary. In
irq_wake_thread() it's used to identify the device for which
the irq_thread should be woke.
So when we share one IRQ, one single interrupt will only wake one
handler once.

On Sun, Apr 27, 2014 at 11:22:14PM +0800, Amos Kong wrote:
> On Sun, Apr 27, 2014 at 06:03:04PM +0300, Michael S. Tsirkin wrote:
> > On Sun, Apr 27, 2014 at 10:35:41PM +0800, Amos Kong wrote:
> > > On Sat, Apr 19, 2014 at 12:08:15PM +0800, Amos Kong wrote:
> > > > 
> > > > Hi all,
> > > > 
> > > > I'm working on this item in Upstream Networking todolist:
> > > > 
> > > > | - Sharing config interrupts
> > > > |    Support more devices by sharing a single msi vector
> > > > |    between multiple virtio devices.
> > > > |    (Applies to virtio-blk too).
> > > > 
> > > > I have this solution here, and only have draft patches of Solution
> > > > 1 & 2, let's discuss if solution 3 is feasible.
> > > > 
> > > > * We should not introduce perf regression in this change
> > > > * little effect is acceptable because we are _sharing_ interrupt
> > > > 
> > > > Solution 1:
> > > > ==========
> > > > share one LSI interrupt for configuration change of all virtio devices.
> > > > Draft patch: share_lsi_for_all_config.patch
> > > > 
> > > > Solution 2:
> > > > ==========
> > > > share one MSI interrupt for configuration change of all virtio devices.
> > > > Draft patch: share_msix_for_all_config.patch
> > > > 
> > > > Solution 3:
> > > > ==========
> > > > dynamic adjust interrupt policy when device is added or removed
> > > > Current:
> > > > -------
> > > > - try to allocate a MSI to device's config
> > > > - try to allocate a MSI for each vq
> > > > - share one MSI for all vqs of one device
> > > > - share one LSI for config & vqs of one device
> > > > 
> > > > additional dynamic policies:
> > > > ---------------------------
> > > > - if there is no enough MSI, adjust interrupt allocation for returning
> > > >   some MSI
> > > >    - try to find a device that allocated one MSI for each vq, change
> > > >      it to share one MSI for saving the MSI
> > > >    - try to share one MSI for config of all virtio_pci devices
> > > >    - try to share one LSI for config of all devices
> > > > 
> > > > - if device is removed, try to re-allocate freed MSI to existed
> > > >   devices, if they aren't in best status (one MSI for config, one
> > > >   MSI for each vq)
> > > >    - if config of all devices is sharing one LSI, try to upgrade it to MSI
> > > >    - if config of all devices is sharing one MSI, try to allocate one
> > > >      MSI for configuration change of each device
> > > >    - try to find a device that is sharing one MSI for all vqs, try to
> > > >      allocate one MSI for each vq
> > > 
> > > Hi Michael, Alex
> > > 
> > > Any thoughts about this ? :)
> > > 
> > > 
> > > Thanks, Amos
> > 
> > 
> > I don't think we need to worry about dynamic.
>  
> Dynamic policy can always get good balance. If we use a fixed policy,
> devices might share IRQ with some unused msix, or sometimes it's not
> fair enough for all devices.


Yes but can we not make this up to the OS as a whole?
Why would we want to make this virtio specific?


> > Questions: just by using IRQF_SHARED, do we always end
> > up with a shared interrupt? Or only if system is running
> > out of vectors?
> 
> In solution1: always share one LSI for config of all virtio devices

Don't get this one. LSI is already shared unconditionally.

> In solution2: always share one MSI for config of all virtio devices
>
> > Did you test that interrupt is actually shared and all
> > handlers are called?
> 
> Yes. I can find many devices (virtio.0, virtio.1,...) share same
> interrupt in guest's /proc/interrupts. And nics work well.

I haven't looked into this, I would imagine that we mark
the config interrupt as shared, then linux would
start by allocating dedicated interrupts but as we start
running out of these, consolidate interrupts together.
This isn't what's happening?

> > We don't stop after the 1st handler that
> > returned OK?
> 
> When we set IRQF_SHARED flag, dev_id is necessary. In
> irq_wake_thread() it's used to identify the device for which
> the irq_thread should be woke.
> So when we share one IRQ, one single interrupt will only wake one
> handler once.
> 
> -- 
> 			Amos.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html