Re: [PATCHv4 2/2] virtio: refactor find_vqs

[Rusty Russell <rusty@rustcorp.com.au>]

On Mon, 27 Jul 2009 01:17:09 am Michael S. Tsirkin wrote:
> This refactors find_vqs, making it more readable and robust, and fixing
> two regressions from 2.6.30:
> - double free_irq causing BUG_ON on device removal
> - probe failure when vq can't be assigned to msi-x vector
>   (reported on old host kernels)
> 
> An older version of this patch was tested by Amit Shah.

OK, I've applied both of these; I'd like to see a new test by Amit to
make sure tho.

I really like this cleanup!  I looked harder at this code, and my best
attempts to untangle it further came to very little.  This is what I
ended up with, but it's all cosmetic and can wait until next merge window.
See what you think.

Thanks!
Rusty.

virtio_pci: minor MSI-X cleanups

1) Rename vp_request_vectors to vp_request_msix_vectors, and take
   non-MSI-X case out to caller.
2) Comment weird pci_enable_msix API
3) Rename vp_find_vq to setup_vq.
4) Fix spaces to tabs
5) Make nvectors calc internal to vp_try_to_find_vqs()

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/virtio/virtio_pci.c |   84 +++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 39 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -280,25 +280,14 @@ static void vp_free_vectors(struct virti
 	vp_dev->msix_entries = NULL;
 }
 
-static int vp_request_vectors(struct virtio_device *vdev, int nvectors,
-			      bool per_vq_vectors)
+static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
+				   bool per_vq_vectors)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	const char *name = dev_name(&vp_dev->vdev.dev);
 	unsigned i, v;
 	int err = -ENOMEM;
 
-	if (!nvectors) {
-		/* Can't allocate MSI-X vectors, use regular interrupt */
-		vp_dev->msix_vectors = 0;
-		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
-				  IRQF_SHARED, name, vp_dev);
-		if (err)
-			return err;
-		vp_dev->intx_enabled = 1;
-		return 0;
-	}
-
 	vp_dev->msix_entries = kmalloc(nvectors * sizeof *vp_dev->msix_entries,
 				       GFP_KERNEL);
 	if (!vp_dev->msix_entries)
@@ -311,6 +300,7 @@ static int vp_request_vectors(struct vir
 	for (i = 0; i < nvectors; ++i)
 		vp_dev->msix_entries[i].entry = i;
 
+	/* pci_enable_msix returns positive if we can't get this many. */
 	err = pci_enable_msix(vp_dev->pci_dev, vp_dev->msix_entries, nvectors);
 	if (err > 0)
 		err = -ENOSPC;
@@ -356,10 +346,10 @@ error:
 	return err;
 }
 
-static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
-				    void (*callback)(struct virtqueue *vq),
-				    const char *name,
-				    u16 vector)
+static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
+				  void (*callback)(struct virtqueue *vq),
+				  const char *name,
+				  u16 vector)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtio_pci_vq_info *info;
@@ -408,7 +398,7 @@ static struct virtqueue *vp_find_vq(stru
 	vq->priv = info;
 	info->vq = vq;
 
-	 if (vector != VIRTIO_MSI_NO_VECTOR) {
+	if (vector != VIRTIO_MSI_NO_VECTOR) {
 		iowrite16(vector, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
 		vector = ioread16(vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
 		if (vector == VIRTIO_MSI_NO_VECTOR) {
@@ -484,14 +474,36 @@ static int vp_try_to_find_vqs(struct vir
 			      struct virtqueue *vqs[],
 			      vq_callback_t *callbacks[],
 			      const char *names[],
-			      int nvectors,
+			      bool use_msix,
 			      bool per_vq_vectors)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	u16 vector;
-	int i, err, allocated_vectors;
+	int i, err, nvectors, allocated_vectors;
 
-	err = vp_request_vectors(vdev, nvectors, per_vq_vectors);
+	if (!use_msix) {
+		/* Old style: one normal interrupt for change and all vqs. */
+		vp_dev->msix_vectors = 0;
+		vp_dev->per_vq_vectors = false;
+		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
+				  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
+		if (!err)
+			vp_dev->intx_enabled = 1;
+		return err;
+	}
+
+	if (per_vq_vectors) {
+		/* Best option: one for change interrupt, one per vq. */
+		nvectors = 1;
+		for (i = 0; i < nvqs; ++i)
+			if (callbacks[i])
+				++nvectors;
+	} else {
+		/* Second best: one for change, shared one for all vqs. */
+		nvectors = 2;
+	}
+
+	err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
 	if (err)
 		goto error_request;
 
@@ -504,15 +516,17 @@ static int vp_try_to_find_vqs(struct vir
 			vector = allocated_vectors++;
 		else
 			vector = VP_MSIX_VQ_VECTOR;
-		vqs[i] = vp_find_vq(vdev, i, callbacks[i], names[i], vector);
+		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], vector);
 		if (IS_ERR(vqs[i])) {
 			err = PTR_ERR(vqs[i]);
 			goto error_find;
 		}
 		/* allocate per-vq irq if available and necessary */
 		if (vp_dev->per_vq_vectors && vector != VIRTIO_MSI_NO_VECTOR) {
-			snprintf(vp_dev->msix_names[vector], sizeof *vp_dev->msix_names,
-				 "%s-%s", dev_name(&vp_dev->vdev.dev), names[i]);
+			snprintf(vp_dev->msix_names[vector],
+				 sizeof *vp_dev->msix_names,
+				 "%s-%s",
+				 dev_name(&vp_dev->vdev.dev), names[i]);
 			err = request_irq(vp_dev->msix_entries[vector].vector,
 					  vring_interrupt, 0,
 					  vp_dev->msix_names[vector], vqs[i]);
@@ -537,28 +551,20 @@ static int vp_find_vqs(struct virtio_dev
 		       vq_callback_t *callbacks[],
 		       const char *names[])
 {
-	int vectors = 0;
-	int i, uninitialized_var(err);
+	int err;
 
-	/* How many vectors would we like? */
-	for (i = 0; i < nvqs; ++i)
-		if (callbacks[i])
-			++vectors;
-
-	/* We want at most one vector per queue and one for config changes. */
-	err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
-				 vectors + 1, true);
+	/* Try MSI-X with one vector per queue. */
+	err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names, true, true);
 	if (!err)
 		return 0;
-	/* Fallback to separate vectors for config and a shared for queues. */
+	/* Fallback: MSI-X with one vector for config, one shared for queues. */
 	err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
-				 2, false);
+				 true, false);
 	if (!err)
 		return 0;
 	/* Finally fall back to regular interrupts. */
-	err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
-				 0, false);
-	return err;
+	return vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
+				  false, false);
 }
 
 static struct virtio_config_ops virtio_pci_config_ops = {