[PATCHv4 1/2] virtio: make del_vq delete vq from list

[PATCHv4 1/2] virtio: make del_vq delete vq from list

[Michael S. Tsirkin]

This makes delete vq the reverse of find vq.
This is required to make it possible to retry find_vqs
after a failure, otherwise the list gets corrupted.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/virtio/virtio_pci.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 7e21389..4c74c72 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -464,7 +464,11 @@ static void vp_del_vq(struct virtqueue *vq)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
 	struct virtio_pci_vq_info *info = vq->priv;
-	unsigned long size;
+	unsigned long flags, size;
+
+	spin_lock_irqsave(&vp_dev->lock, flags);
+	list_del(&info->node);
+	spin_unlock_irqrestore(&vp_dev->lock, flags);
 
 	iowrite16(info->queue_index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
 
-- 
1.6.2.5

[PATCHv4 2/2] virtio: refactor find_vqs

[Michael S. Tsirkin]

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.

Reported-by: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/virtio/virtio_pci.c |  212 ++++++++++++++++++++++++-------------------
 1 files changed, 119 insertions(+), 93 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 4c74c72..c17b830 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -52,8 +52,10 @@ struct virtio_pci_device
 	char (*msix_names)[256];
 	/* Number of available vectors */
 	unsigned msix_vectors;
-	/* Vectors allocated */
+	/* Vectors allocated, excluding per-vq vectors if any */
 	unsigned msix_used_vectors;
+	/* Whether we have vector per vq */
+	bool per_vq_vectors;
 };
 
 /* Constants for MSI-X */
@@ -278,27 +280,24 @@ static void vp_free_vectors(struct virtio_device *vdev)
 	vp_dev->msix_entries = NULL;
 }
 
-static int vp_enable_msix(struct pci_dev *dev, struct msix_entry *entries,
-			  int *options, int noptions)
-{
-	int i;
-	for (i = 0; i < noptions; ++i)
-		if (!pci_enable_msix(dev, entries, options[i]))
-			return options[i];
-	return -EBUSY;
-}
-
-static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
+static int vp_request_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;
-	/* We want at most one vector per queue and one for config changes.
-	 * Fallback to separate vectors for config and a shared for queues.
-	 * Finally fall back to regular interrupts. */
-	int options[] = { max_vqs + 1, 2 };
-	int nvectors = max(options[0], options[1]);
+
+	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);
@@ -312,41 +311,34 @@ static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
 	for (i = 0; i < nvectors; ++i)
 		vp_dev->msix_entries[i].entry = i;
 
-	err = vp_enable_msix(vp_dev->pci_dev, vp_dev->msix_entries,
-			     options, ARRAY_SIZE(options));
-	if (err < 0) {
-		/* Can't allocate enough 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)
-			goto error;
-		vp_dev->intx_enabled = 1;
-	} else {
-		vp_dev->msix_vectors = err;
-		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,
-			 "%s-config", name);
-		err = request_irq(vp_dev->msix_entries[v].vector,
-				  vp_config_changed, 0, vp_dev->msix_names[v],
-				  vp_dev);
-		if (err)
-			goto error;
-		++vp_dev->msix_used_vectors;
+	err = pci_enable_msix(vp_dev->pci_dev, vp_dev->msix_entries, nvectors);
+	if (err > 0)
+		err = -ENOSPC;
+	if (err)
+		goto error;
+	vp_dev->msix_vectors = nvectors;
+	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,
+		 "%s-config", name);
+	err = request_irq(vp_dev->msix_entries[v].vector,
+			  vp_config_changed, 0, vp_dev->msix_names[v],
+			  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;
-			goto error;
-		}
+	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;
+		goto error;
 	}
 
-	if (vp_dev->msix_vectors && vp_dev->msix_vectors != max_vqs + 1) {
+	if (!per_vq_vectors) {
 		/* Shared vector for all VQs */
 		v = vp_dev->msix_used_vectors;
 		snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
@@ -366,13 +358,14 @@ error:
 
 static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
 				    void (*callback)(struct virtqueue *vq),
-				    const char *name)
+				    const char *name,
+				    u16 vector)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtio_pci_vq_info *info;
 	struct virtqueue *vq;
 	unsigned long flags, size;
-	u16 num, vector;
+	u16 num;
 	int err;
 
 	/* Select the queue we're interested in */
@@ -391,7 +384,7 @@ static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
 
 	info->queue_index = index;
 	info->num = num;
-	info->vector = VIRTIO_MSI_NO_VECTOR;
+	info->vector = vector;
 
 	size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
 	info->queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);
@@ -415,22 +408,7 @@ static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
 	vq->priv = info;
 	info->vq = vq;
 
-	/* allocate per-vq vector if available and necessary */
-	if (callback && vp_dev->msix_used_vectors < vp_dev->msix_vectors) {
-		vector = vp_dev->msix_used_vectors;
-		snprintf(vp_dev->msix_names[vector], sizeof *vp_dev->msix_names,
-			 "%s-%s", dev_name(&vp_dev->vdev.dev), name);
-		err = request_irq(vp_dev->msix_entries[vector].vector,
-				  vring_interrupt, 0,
-				  vp_dev->msix_names[vector], vq);
-		if (err)
-			goto out_request_irq;
-		info->vector = vector;
-		++vp_dev->msix_used_vectors;
-	} else
-		vector = VP_MSIX_VQ_VECTOR;
-
-	 if (callback && vp_dev->msix_enabled) {
+	 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) {
@@ -446,11 +424,6 @@ static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
 	return vq;
 
 out_assign:
-	if (info->vector != VIRTIO_MSI_NO_VECTOR) {
-		free_irq(vp_dev->msix_entries[info->vector].vector, vq);
-		--vp_dev->msix_used_vectors;
-	}
-out_request_irq:
 	vring_del_virtqueue(vq);
 out_activate_queue:
 	iowrite32(0, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
@@ -472,9 +445,6 @@ static void vp_del_vq(struct virtqueue *vq)
 
 	iowrite16(info->queue_index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
 
-	if (info->vector != VIRTIO_MSI_NO_VECTOR)
-		free_irq(vp_dev->msix_entries[info->vector].vector, vq);
-
 	if (vp_dev->msix_enabled) {
 		iowrite16(VIRTIO_MSI_NO_VECTOR,
 			  vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
@@ -495,36 +465,62 @@ static void vp_del_vq(struct virtqueue *vq)
 /* the config->del_vqs() implementation */
 static void vp_del_vqs(struct virtio_device *vdev)
 {
+	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtqueue *vq, *n;
+	struct virtio_pci_vq_info *info;
 
-	list_for_each_entry_safe(vq, n, &vdev->vqs, list)
+	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
+		info = vq->priv;
+		if (vp_dev->per_vq_vectors)
+			free_irq(vp_dev->msix_entries[info->vector].vector, vq);
 		vp_del_vq(vq);
+	}
+	vp_dev->per_vq_vectors = false;
 
 	vp_free_vectors(vdev);
 }
 
-/* the config->find_vqs() implementation */
-static int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
-		       struct virtqueue *vqs[],
-		       vq_callback_t *callbacks[],
-		       const char *names[])
+static int vp_try_to_find_vqs(struct virtio_device *vdev, unsigned nvqs,
+			      struct virtqueue *vqs[],
+			      vq_callback_t *callbacks[],
+			      const char *names[],
+			      int nvectors,
+			      bool per_vq_vectors)
 {
-	int vectors = 0;
-	int i, err;
-
-	/* How many vectors would we like? */
-	for (i = 0; i < nvqs; ++i)
-		if (callbacks[i])
-			++vectors;
+	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+	u16 vector;
+	int i, err, allocated_vectors;
 
-	err = vp_request_vectors(vdev, vectors);
+	err = vp_request_vectors(vdev, nvectors, per_vq_vectors);
 	if (err)
 		goto error_request;
 
+	vp_dev->per_vq_vectors = per_vq_vectors;
+	allocated_vectors = vp_dev->msix_used_vectors;
 	for (i = 0; i < nvqs; ++i) {
-		vqs[i] = vp_find_vq(vdev, i, callbacks[i], names[i]);
-		if (IS_ERR(vqs[i]))
+		if (!callbacks[i] || !vp_dev->msix_enabled)
+			vector = VIRTIO_MSI_NO_VECTOR;
+		else if (vp_dev->per_vq_vectors)
+			vector = allocated_vectors++;
+		else
+			vector = VP_MSIX_VQ_VECTOR;
+		vqs[i] = vp_find_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]);
+			err = request_irq(vp_dev->msix_entries[vector].vector,
+					  vring_interrupt, 0,
+					  vp_dev->msix_names[vector], vqs[i]);
+			if (err) {
+				vp_del_vq(vqs[i]);
+				goto error_find;
+			}
+		}
 	}
 	return 0;
 
@@ -532,7 +528,37 @@ error_find:
 	vp_del_vqs(vdev);
 
 error_request:
-	return PTR_ERR(vqs[i]);
+	return err;
+}
+
+/* the config->find_vqs() implementation */
+static int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
+		       struct virtqueue *vqs[],
+		       vq_callback_t *callbacks[],
+		       const char *names[])
+{
+	int vectors = 0;
+	int i, uninitialized_var(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);
+	if (!err)
+		return 0;
+	/* Fallback to separate vectors for config and a shared for queues. */
+	err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
+				 2, 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;
 }
 
 static struct virtio_config_ops virtio_pci_config_ops = {
-- 
1.6.2.5

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

[Rusty Russell]

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 = {

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

[Michael S. Tsirkin]

On Tue, Jul 28, 2009 at 12:44:31PM +0930, Rusty Russell wrote:
> 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.

I'm not sure this change was for the best: we still have a separate code
path under if !use_msix, only in another place now.  See below.
And this seems to break the symmetry between request_ and free_vectors.

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

The other changes look good to me.

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

shorter as vp_dev->intx_enabled = !err

> +		return err;

Is that all? Don't we need to create the vqs?

> +	}
> +
> +	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 = {

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

[Rusty Russell]

On Tue, 28 Jul 2009 06:03:08 pm Michael S. Tsirkin wrote:
> On Tue, Jul 28, 2009 at 12:44:31PM +0930, Rusty Russell wrote:
> > 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.
> 
> I'm not sure this change was for the best: we still have a separate code
> path under if !use_msix, only in another place now.  See below.
> And this seems to break the symmetry between request_ and free_vectors.

Yes, but unfortunately request_vectors was never really symmetrical :(

That's because we didn't do the request_irq's for the per_vector case, because
we don't have the names.  This is what prevented me from doing a nice
encapsulation.

> > -	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;
> 
> shorter as vp_dev->intx_enabled = !err
> 
> > +		return err;
> 
> Is that all? Don't we need to create the vqs?

Oh, yeah :)

This patch applies on top.  Basically reverts that part, and
renames vector to msix_vector, which I think makes the code more
readable.

virtio: more PCI minor cleanups

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/virtio/virtio_pci.c |   73 +++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 37 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
@@ -84,7 +84,7 @@ struct virtio_pci_vq_info
 	struct list_head node;
 
 	/* MSI-X vector (or none) */
-	unsigned vector;
+	unsigned msix_vector;
 };
 
 /* Qumranet donated their vendor ID for devices 0x1000 thru 0x10FF. */
@@ -349,7 +349,7 @@ error:
 static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 				  void (*callback)(struct virtqueue *vq),
 				  const char *name,
-				  u16 vector)
+				  u16 msix_vector)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtio_pci_vq_info *info;
@@ -374,7 +374,7 @@ static struct virtqueue *setup_vq(struct
 
 	info->queue_index = index;
 	info->num = num;
-	info->vector = vector;
+	info->msix_vector = msix_vector;
 
 	size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
 	info->queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);
@@ -398,10 +398,10 @@ static struct virtqueue *setup_vq(struct
 	vq->priv = info;
 	info->vq = vq;
 
-	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) {
+	if (msix_vector != VIRTIO_MSI_NO_VECTOR) {
+		iowrite16(msix_vector, vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
+		msix_vector = ioread16(vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
+		if (msix_vector == VIRTIO_MSI_NO_VECTOR) {
 			err = -EBUSY;
 			goto out_assign;
 		}
@@ -462,7 +462,8 @@ static void vp_del_vqs(struct virtio_dev
 	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
 		info = vq->priv;
 		if (vp_dev->per_vq_vectors)
-			free_irq(vp_dev->msix_entries[info->vector].vector, vq);
+			free_irq(vp_dev->msix_entries[info->msix_vector].vector,
+				 vq);
 		vp_del_vq(vq);
 	}
 	vp_dev->per_vq_vectors = false;
@@ -478,58 +479,56 @@ static int vp_try_to_find_vqs(struct vir
 			      bool per_vq_vectors)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	u16 vector;
+	u16 msix_vector;
 	int i, err, nvectors, allocated_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 (err)
+			return err;
+	} else {
+		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 for all vqs. */
+			nvectors = 2;
+		}
+
+		err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
+		if (err)
+			goto error_request;
 	}
 
-	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;
-
 	vp_dev->per_vq_vectors = per_vq_vectors;
 	allocated_vectors = vp_dev->msix_used_vectors;
 	for (i = 0; i < nvqs; ++i) {
 		if (!callbacks[i] || !vp_dev->msix_enabled)
-			vector = VIRTIO_MSI_NO_VECTOR;
+			msix_vector = VIRTIO_MSI_NO_VECTOR;
 		else if (vp_dev->per_vq_vectors)
-			vector = allocated_vectors++;
+			msix_vector = allocated_vectors++;
 		else
-			vector = VP_MSIX_VQ_VECTOR;
-		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], vector);
+			msix_vector = VP_MSIX_VQ_VECTOR;
+		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], msix_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],
+		if (vp_dev->per_vq_vectors) {
+			snprintf(vp_dev->msix_names[msix_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]);
+			err = request_irq(msix_vector, vring_interrupt, 0,
+					  vp_dev->msix_names[msix_vector],
+					  vqs[i]);
 			if (err) {
 				vp_del_vq(vqs[i]);
 				goto error_find;

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

[Michael S. Tsirkin]

Sorry it took a bit to respond to this.  I kept hoping I'll have time to
test it but looks like I won't before I'm on vacation.  I agree it's
good cleanup, and maybe we can go even further, see below.

On Mon, Aug 10, 2009 at 09:07:52AM +0930, Rusty Russell wrote:
> On Tue, 28 Jul 2009 06:03:08 pm Michael S. Tsirkin wrote:
> > On Tue, Jul 28, 2009 at 12:44:31PM +0930, Rusty Russell wrote:
> > > 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.
> > 
> > I'm not sure this change was for the best: we still have a separate code
> > path under if !use_msix, only in another place now.  See below.
> > And this seems to break the symmetry between request_ and free_vectors.
> 
> Yes, but unfortunately request_vectors was never really symmetrical :(
> 
> That's because we didn't do the request_irq's for the per_vector case, because
> we don't have the names.  This is what prevented me from doing a nice
> encapsulation.

Yes. But let's split free_vectors out into free_msix_vectors and
free_intx as well?

> > > -	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;
> > 
> > shorter as vp_dev->intx_enabled = !err
> > 
> > > +		return err;
> > 
> > Is that all? Don't we need to create the vqs?
> 
> Oh, yeah :)
> 
> This patch applies on top.  Basically reverts that part, and
> renames vector to msix_vector, which I think makes the code more
> readable.

Yes, I agree, this is good cleanup, structure field names should be
desriptive.  Are you sure we want to make all local variables named
vector renamed to msix_vector though? CodingStyle says local var names
should be short ...  A couple of ideas below.

> virtio: more PCI minor cleanups
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  drivers/virtio/virtio_pci.c |   73 +++++++++++++++++++++-----------------------
>  1 file changed, 36 insertions(+), 37 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
> @@ -84,7 +84,7 @@ struct virtio_pci_vq_info
>  	struct list_head node;
>  
>  	/* MSI-X vector (or none) */
> -	unsigned vector;
> +	unsigned msix_vector;

Good. And now we don't need the comment.

>  };
>  
>  /* Qumranet donated their vendor ID for devices 0x1000 thru 0x10FF. */
> @@ -349,7 +349,7 @@ error:
>  static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
>  				  void (*callback)(struct virtqueue *vq),
>  				  const char *name,
> -				  u16 vector)
> +				  u16 msix_vector)
>  {
>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>  	struct virtio_pci_vq_info *info;
> @@ -374,7 +374,7 @@ static struct virtqueue *setup_vq(struct
>  
>  	info->queue_index = index;
>  	info->num = num;
> -	info->vector = vector;
> +	info->msix_vector = msix_vector;
>  
>  	size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
>  	info->queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);
> @@ -398,10 +398,10 @@ static struct virtqueue *setup_vq(struct
>  	vq->priv = info;
>  	info->vq = vq;
>  
> -	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) {
> +	if (msix_vector != VIRTIO_MSI_NO_VECTOR) {
> +		iowrite16(msix_vector, vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
> +		msix_vector = ioread16(vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);

checkpatch will complain that space is lacking around "+".
We won't have a problem if we keep it named vector.

> +		if (msix_vector == VIRTIO_MSI_NO_VECTOR) {
>  			err = -EBUSY;
>  			goto out_assign;
>  		}
> @@ -462,7 +462,8 @@ static void vp_del_vqs(struct virtio_dev
>  	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
>  		info = vq->priv;
>  		if (vp_dev->per_vq_vectors)
> -			free_irq(vp_dev->msix_entries[info->vector].vector, vq);
> +			free_irq(vp_dev->msix_entries[info->msix_vector].vector,
> +				 vq);
>  		vp_del_vq(vq);
>  	}
>  	vp_dev->per_vq_vectors = false;
> @@ -478,58 +479,56 @@ static int vp_try_to_find_vqs(struct vir
>  			      bool per_vq_vectors)
>  {
>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> -	u16 vector;
> +	u16 msix_vector;
>  	int i, err, nvectors, allocated_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;

I know it's enough to look at msix_vectors, but isn't
it cleaner to have per_vq_vectors consistent as well?
E.g. del_vqs seems to only look at per_vq_vectors.

>  		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 (err)
> +			return err;

Maybe move this part out into a separate function?  This way
vp_try_to_find_vqs does high-level processing.


> +	} else {
> +		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 for all vqs. */
> +			nvectors = 2;

Out of curiosity: why do you put {} here? They aren't
strictly necessary ...

> +		}
> +
> +		err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
> +		if (err)
> +			goto error_request;
>  	}
>  
> -	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;
> -
>  	vp_dev->per_vq_vectors = per_vq_vectors;
>  	allocated_vectors = vp_dev->msix_used_vectors;
>  	for (i = 0; i < nvqs; ++i) {
>  		if (!callbacks[i] || !vp_dev->msix_enabled)
> -			vector = VIRTIO_MSI_NO_VECTOR;
> +			msix_vector = VIRTIO_MSI_NO_VECTOR;
>  		else if (vp_dev->per_vq_vectors)
> -			vector = allocated_vectors++;
> +			msix_vector = allocated_vectors++;
>  		else
> -			vector = VP_MSIX_VQ_VECTOR;
> -		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], vector);
> +			msix_vector = VP_MSIX_VQ_VECTOR;
> +		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], msix_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],
> +		if (vp_dev->per_vq_vectors) {
> +			snprintf(vp_dev->msix_names[msix_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]);
> +			err = request_irq(msix_vector, vring_interrupt, 0,
> +					  vp_dev->msix_names[msix_vector],
> +					  vqs[i]);
>  			if (err) {
>  				vp_del_vq(vqs[i]);
>  				goto error_find;

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

[Rusty Russell]

On Tue, 25 Aug 2009 09:34:34 pm Michael S. Tsirkin wrote:
> > That's because we didn't do the request_irq's for the per_vector case, because
> > we don't have the names.  This is what prevented me from doing a nice
> > encapsulation.
> 
> Yes. But let's split free_vectors out into free_msix_vectors and
> free_intx as well?

Perhaps.  Patch welcome :)

> Yes, I agree, this is good cleanup, structure field names should be
> desriptive.  Are you sure we want to make all local variables named
> vector renamed to msix_vector though? CodingStyle says local var names
> should be short ...  A couple of ideas below.

msix_vec would work.  But vector for something which isn't always the vector
is misleading, IMHO.

> > -	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) {
> > +	if (msix_vector != VIRTIO_MSI_NO_VECTOR) {
> > +		iowrite16(msix_vector, vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
> > +		msix_vector = ioread16(vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
> 
> checkpatch will complain that space is lacking around "+".
> We won't have a problem if we keep it named vector.

Yeah, but OTOH ignoring checkpatch warnings is good for the soul.

> >  	if (!use_msix) {
> >  		/* Old style: one normal interrupt for change and all vqs. */
> >  		vp_dev->msix_vectors = 0;
> > -		vp_dev->per_vq_vectors = false;
> 
> I know it's enough to look at msix_vectors, but isn't
> it cleaner to have per_vq_vectors consistent as well?
> E.g. del_vqs seems to only look at per_vq_vectors.

This should in fact be:

 	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 (err)
+			return err;
+		vp_dev->intx_enabled = 1;


The msix fields should all be ignored if msix_enabled is false.  Think about
running valgrind (or equiv) over the code: if you initialize something which
shouldn't be accessed, valgrind won't spot it.

For similar reasons, I dislike memseting structs to 0, or kzallocing them.

> >  		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 (err)
> > +			return err;
> 
> Maybe move this part out into a separate function?  This way
> vp_try_to_find_vqs does high-level processing.

Nice.  Moved this to vp_request_intx().

> > +	} else {
> > +		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 for all vqs. */
> > +			nvectors = 2;
> 
> Out of curiosity: why do you put {} here? They aren't
> strictly necessary ...

Comment made it multiline, looked a bit neater?

Here's the diff result:

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
@@ -346,10 +346,22 @@ error:
 	return err;
 }
 
+static int vp_request_intx(struct virtio_device *vdev)
+{
+	int err;
+	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+
+	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;
+}
+
 static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 				  void (*callback)(struct virtqueue *vq),
 				  const char *name,
-				  u16 msix_vector)
+				  u16 msix_vec)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtio_pci_vq_info *info;
@@ -374,7 +386,7 @@ static struct virtqueue *setup_vq(struct
 
 	info->queue_index = index;
 	info->num = num;
-	info->msix_vector = msix_vector;
+	info->msix_vector = msix_vec;
 
 	size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
 	info->queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);
@@ -398,10 +410,10 @@ static struct virtqueue *setup_vq(struct
 	vq->priv = info;
 	info->vq = vq;
 
-	if (msix_vector != VIRTIO_MSI_NO_VECTOR) {
-		iowrite16(msix_vector, vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
-		msix_vector = ioread16(vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
-		if (msix_vector == VIRTIO_MSI_NO_VECTOR) {
+	if (msix_vec != VIRTIO_MSI_NO_VECTOR) {
+		iowrite16(msix_vec, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
+		msix_vec = ioread16(vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
+		if (msix_vec == VIRTIO_MSI_NO_VECTOR) {
 			err = -EBUSY;
 			goto out_assign;
 		}
@@ -479,16 +491,14 @@ static int vp_try_to_find_vqs(struct vir
 			      bool per_vq_vectors)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	u16 msix_vector;
+	u16 msix_vec;
 	int i, err, nvectors, allocated_vectors;
 
 	if (!use_msix) {
 		/* Old style: one normal interrupt for change and all vqs. */
-		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
-				  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
+		err = vp_request_intx(vdev);
 		if (err)
-			return err;
-		vp_dev->intx_enabled = 1;
+			goto error_request;
 	} else {
 		if (per_vq_vectors) {
 			/* Best option: one for change interrupt, one per vq. */
@@ -510,24 +520,24 @@ static int vp_try_to_find_vqs(struct vir
 	allocated_vectors = vp_dev->msix_used_vectors;
 	for (i = 0; i < nvqs; ++i) {
 		if (!callbacks[i] || !vp_dev->msix_enabled)
-			msix_vector = VIRTIO_MSI_NO_VECTOR;
+			msix_vec = VIRTIO_MSI_NO_VECTOR;
 		else if (vp_dev->per_vq_vectors)
-			msix_vector = allocated_vectors++;
+			msix_vec = allocated_vectors++;
 		else
-			msix_vector = VP_MSIX_VQ_VECTOR;
-		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], msix_vector);
+			msix_vec = VP_MSIX_VQ_VECTOR;
+		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], msix_vec);
 		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) {
-			snprintf(vp_dev->msix_names[msix_vector],
+			snprintf(vp_dev->msix_names[msix_vec],
 				 sizeof *vp_dev->msix_names,
 				 "%s-%s",
 				 dev_name(&vp_dev->vdev.dev), names[i]);
-			err = request_irq(msix_vector, vring_interrupt, 0,
-					  vp_dev->msix_names[msix_vector],
+			err = request_irq(msix_vec, vring_interrupt, 0,
+					  vp_dev->msix_names[msix_vec],
 					  vqs[i]);
 			if (err) {
 				vp_del_vq(vqs[i]);

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

[Michael S. Tsirkin]

On Thu, Aug 27, 2009 at 07:00:34PM +0930, Rusty Russell wrote:
> On Tue, 25 Aug 2009 09:34:34 pm Michael S. Tsirkin wrote:
> > > That's because we didn't do the request_irq's for the per_vector case, because
> > > we don't have the names.  This is what prevented me from doing a nice
> > > encapsulation.
> > 
> > Yes. But let's split free_vectors out into free_msix_vectors and
> > free_intx as well?
> 
> Perhaps.  Patch welcome :)

Could you put the end result somewhere so I can work on top of it?

> > Yes, I agree, this is good cleanup, structure field names should be
> > desriptive.  Are you sure we want to make all local variables named
> > vector renamed to msix_vector though? CodingStyle says local var names
> > should be short ...  A couple of ideas below.
> 
> msix_vec would work.

Yes, ok.

> But vector for something which isn't always the vector
> is misleading, IMHO.

I think you mean it's isn't always used? It's always a 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) {
> > > +	if (msix_vector != VIRTIO_MSI_NO_VECTOR) {
> > > +		iowrite16(msix_vector, vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
> > > +		msix_vector = ioread16(vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
> > 
> > checkpatch will complain that space is lacking around "+".
> > We won't have a problem if we keep it named vector.
> 
> Yeah, but OTOH ignoring checkpatch warnings is good for the soul.
> 
> > >  	if (!use_msix) {
> > >  		/* Old style: one normal interrupt for change and all vqs. */
> > >  		vp_dev->msix_vectors = 0;
> > > -		vp_dev->per_vq_vectors = false;
> > 
> > I know it's enough to look at msix_vectors, but isn't
> > it cleaner to have per_vq_vectors consistent as well?
> > E.g. del_vqs seems to only look at per_vq_vectors.
> 
> This should in fact be:
> 
>  	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 (err)
> +			return err;
> +		vp_dev->intx_enabled = 1;
> 
> 
> The msix fields should all be ignored if msix_enabled is false.

Need to audit code to make sure it's so then.  I actually think
per_vq_vectors is kind of not directly tied to msix.
Don't you think it's nicer like del_vqs does:
if (vp_dev->per_vq_vectors) {
}

Than

if (vp_dev->msix_enabled && vp_dev->per_vq_vectors) {
}

BTW, let's get rid of msix_enabled completely?
We can always use msix_vectors ...

> Think about
> running valgrind (or equiv) over the code: if you initialize something which
> shouldn't be accessed, valgrind won't spot it.
> 
> For similar reasons, I dislike memseting structs to 0, or kzallocing them.

Yes, I agree generally.

> > >  		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 (err)
> > > +			return err;
> > 
> > Maybe move this part out into a separate function?  This way
> > vp_try_to_find_vqs does high-level processing.
> 
> Nice.  Moved this to vp_request_intx().
> 
> > > +	} else {
> > > +		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 for all vqs. */
> > > +			nvectors = 2;
> > 
> > Out of curiosity: why do you put {} here? They aren't
> > strictly necessary ...
> 
> Comment made it multiline, looked a bit neater?
> 
> Here's the diff result:

Looks good to me. When we are done with cleanups, need to remember to
test this with all the possible combinations: no msix, 2 vectors, 1
vector.

> 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
> @@ -346,10 +346,22 @@ error:
>  	return err;
>  }
>  
> +static int vp_request_intx(struct virtio_device *vdev)
> +{
> +	int err;
> +	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> +
> +	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;
> +}
> +
>  static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
>  				  void (*callback)(struct virtqueue *vq),
>  				  const char *name,
> -				  u16 msix_vector)
> +				  u16 msix_vec)
>  {
>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>  	struct virtio_pci_vq_info *info;
> @@ -374,7 +386,7 @@ static struct virtqueue *setup_vq(struct
>  
>  	info->queue_index = index;
>  	info->num = num;
> -	info->msix_vector = msix_vector;
> +	info->msix_vector = msix_vec;
>  
>  	size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
>  	info->queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);
> @@ -398,10 +410,10 @@ static struct virtqueue *setup_vq(struct
>  	vq->priv = info;
>  	info->vq = vq;
>  
> -	if (msix_vector != VIRTIO_MSI_NO_VECTOR) {
> -		iowrite16(msix_vector, vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
> -		msix_vector = ioread16(vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
> -		if (msix_vector == VIRTIO_MSI_NO_VECTOR) {
> +	if (msix_vec != VIRTIO_MSI_NO_VECTOR) {
> +		iowrite16(msix_vec, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
> +		msix_vec = ioread16(vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
> +		if (msix_vec == VIRTIO_MSI_NO_VECTOR) {
>  			err = -EBUSY;
>  			goto out_assign;
>  		}
> @@ -479,16 +491,14 @@ static int vp_try_to_find_vqs(struct vir
>  			      bool per_vq_vectors)
>  {
>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> -	u16 msix_vector;
> +	u16 msix_vec;
>  	int i, err, nvectors, allocated_vectors;
>  
>  	if (!use_msix) {
>  		/* Old style: one normal interrupt for change and all vqs. */
> -		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
> -				  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
> +		err = vp_request_intx(vdev);
>  		if (err)
> -			return err;
> -		vp_dev->intx_enabled = 1;
> +			goto error_request;
>  	} else {
>  		if (per_vq_vectors) {
>  			/* Best option: one for change interrupt, one per vq. */
> @@ -510,24 +520,24 @@ static int vp_try_to_find_vqs(struct vir
>  	allocated_vectors = vp_dev->msix_used_vectors;
>  	for (i = 0; i < nvqs; ++i) {
>  		if (!callbacks[i] || !vp_dev->msix_enabled)
> -			msix_vector = VIRTIO_MSI_NO_VECTOR;
> +			msix_vec = VIRTIO_MSI_NO_VECTOR;
>  		else if (vp_dev->per_vq_vectors)
> -			msix_vector = allocated_vectors++;
> +			msix_vec = allocated_vectors++;
>  		else
> -			msix_vector = VP_MSIX_VQ_VECTOR;
> -		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], msix_vector);
> +			msix_vec = VP_MSIX_VQ_VECTOR;
> +		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], msix_vec);
>  		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) {
> -			snprintf(vp_dev->msix_names[msix_vector],
> +			snprintf(vp_dev->msix_names[msix_vec],
>  				 sizeof *vp_dev->msix_names,
>  				 "%s-%s",
>  				 dev_name(&vp_dev->vdev.dev), names[i]);
> -			err = request_irq(msix_vector, vring_interrupt, 0,
> -					  vp_dev->msix_names[msix_vector],
> +			err = request_irq(msix_vec, vring_interrupt, 0,
> +					  vp_dev->msix_names[msix_vec],
>  					  vqs[i]);
>  			if (err) {
>  				vp_del_vq(vqs[i]);

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

[Rusty Russell]

On Thu, 27 Aug 2009 07:19:26 pm Michael S. Tsirkin wrote:
> On Thu, Aug 27, 2009 at 07:00:34PM +0930, Rusty Russell wrote:
> > On Tue, 25 Aug 2009 09:34:34 pm Michael S. Tsirkin wrote:
> > > > That's because we didn't do the request_irq's for the per_vector case, because
> > > > we don't have the names.  This is what prevented me from doing a nice
> > > > encapsulation.
> > > 
> > > Yes. But let's split free_vectors out into free_msix_vectors and
> > > free_intx as well?
> > 
> > Perhaps.  Patch welcome :)
> 
> Could you put the end result somewhere so I can work on top of it?

Sure, it'll hit linux-next tomorrow, otherwise you can steal from
http://ozlabs.org/~rusty/kernel/rr-latest (virtio:pci-minor-cleanups.patch
and virtio:pci-minor-cleanups-fix.patch).

> > But vector for something which isn't always the vector
> > is misleading, IMHO.
> 
> I think you mean it's isn't always used? It's always a vector ...

The non-MSI case, it's set to VIRTIO_MSI_NO_VECTOR, and we use a normal
interrupt vector.

> BTW, let's get rid of msix_enabled completely?
> We can always use msix_vectors ...

That would be nice.  But yes, requiring more audit.

Ideally, if msix_vectors == 0, implies intx_enabled.

Thanks,
Rusty.

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

[Michael S. Tsirkin]

On Thu, Aug 27, 2009 at 08:32:24PM +0930, Rusty Russell wrote:
> On Thu, 27 Aug 2009 07:19:26 pm Michael S. Tsirkin wrote:
> > On Thu, Aug 27, 2009 at 07:00:34PM +0930, Rusty Russell wrote:
> > > On Tue, 25 Aug 2009 09:34:34 pm Michael S. Tsirkin wrote:
> > > > > That's because we didn't do the request_irq's for the per_vector case, because
> > > > > we don't have the names.  This is what prevented me from doing a nice
> > > > > encapsulation.
> > > > 
> > > > Yes. But let's split free_vectors out into free_msix_vectors and
> > > > free_intx as well?
> > > 
> > > Perhaps.  Patch welcome :)
> > 
> > Could you put the end result somewhere so I can work on top of it?
> 
> Sure, it'll hit linux-next tomorrow, otherwise you can steal from
> http://ozlabs.org/~rusty/kernel/rr-latest (virtio:pci-minor-cleanups.patch
> and virtio:pci-minor-cleanups-fix.patch).
> 
> > > But vector for something which isn't always the vector
> > > is misleading, IMHO.
> > 
> > I think you mean it's isn't always used? It's always a vector ...
> 
> The non-MSI case, it's set to VIRTIO_MSI_NO_VECTOR, and we use a normal
> interrupt vector.
> 
> > BTW, let's get rid of msix_enabled completely?
> > We can always use msix_vectors ...
> 
> That would be nice.  But yes, requiring more audit.
> 
> Ideally, if msix_vectors == 0, implies intx_enabled.

It seems that since we *can* request both an intx
and msix vectors, it's better to have them independent even if we
currently don't do that. No?

> Thanks,
> Rusty.

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

[Amit Shah]

On (Tue) Jul 28 2009 [12:44:31], Rusty Russell wrote:
> 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.

Tested these patches as well. They work fine.

Tested-by: Amit Shah <amit.shah@redhat.com>

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

[Michael S. Tsirkin]

On Tue, Jul 28, 2009 at 08:00:52PM +0530, Amit Shah wrote:
> On (Tue) Jul 28 2009 [12:44:31], Rusty Russell wrote:
> > 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.
> 
> Tested these patches as well. They work fine.
> 
> Tested-by: Amit Shah <amit.shah@redhat.com>

Amit, thanks very much for the testing.

-- 
MST