virtio: delete vq from list

PATCH Superseded Michael S. Tsirkin 1 participant 4 messages Jul 23, 2009 1 message not in the archive

Subsystems (2)

KVM virtio core

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Michael S. Tsirkin

author maintainer

2009-07-23 15:45 UTC #1 lore raw

PATCH 1/2 virtio: delete vq from list

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 <[email protected]>

---
 drivers/virtio/virtio_pci.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

drivers/virtio/virtio_pci.c +5 -1

index 3aff895..4bff231 100644
@@ -460,7 +460,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);

Signature · 1 line

1.6.2.5

Michael S. Tsirkin

author maintainer

2009-07-23 15:47 UTC #2 lore raw

PATCH 1/2 virtio: delete vq from list

In reply to a message not in the archive

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 <[email protected]>

---
 drivers/virtio/virtio_pci.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

drivers/virtio/virtio_pci.c +5 -1

index 3aff895..4bff231 100644
@@ -460,7 +460,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);

Signature · 1 line

1.6.2.5

Michael S. Tsirkin

author maintainer

2009-07-23 15:48 UTC #3 lore raw

PATCH 2/2 virtio: retry on vector assignment failure

In reply to a message not in the archive

virtio currently fails to find any vqs if the device supports msi-x but fails to assign it to a queue. Turns out, this actually happens for old host kernels which can't allocate a gsi for the vector. As a result guest does not use virtio net. Fix this by retrying with less vectors and finally disabling msi on such failures and falling back on regular interrupts.

Reported-by Amit Shah <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---

A slightly bigger patch was tested by Amit Shah and reported working, this one has been made as small as possible for -rc inclusion. Let's pospone bigger refactorings for 2.6.32.

 drivers/virtio/virtio_pci.c |  130 +++++++++++++++++++++++-------------------
 1 files changed, 71 insertions(+), 59 deletions(-)

drivers/virtio/virtio_pci.c +71 ~~-59~~

index 4bff231..25c7995 100644
@@ -278,27 +278,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, unsigned max_vqs,
+			      int nvectors)
 {
 	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,38 +309,31 @@ 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) {

@@ -499,23 +489,17 @@ static void vp_del_vqs(struct virtio_device *vdev)
 	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 max_vqs, int nvectors)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	u16 vector, per_vq_vector;
-	int vectors = 0;
 	int i, err, allocated_vectors;
 
-	/* How many vectors would we like? */
-	for (i = 0; i < nvqs; ++i)
-		if (callbacks[i])
-			++vectors;
-
-	err = vp_request_vectors(vdev, vectors);
+	err = vp_request_vectors(vdev, max_vqs, nvectors);
 	if (err)
 		goto error_request;

@@ -543,6 +527,34 @@ error_request:
 	return PTR_ERR(vqs[i]);
 }
 
+/* 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[])
+{
+	/* 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[] = { 1, 2, 0 };
+	int vectors = 0;
+	int i, uninitialized_var(err);
+
+	/* How many vectors would we like? */
+	for (i = 0; i < nvqs; ++i)
+		if (callbacks[i])
+			++vectors;
+	options[0] = vectors + 1;
+
+	for (i = 0; i < ARRAY_SIZE(options); ++i) {
+		err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
+					 vectors, options[i]);
+		if (!err)
+			break;
+	}
+	return err;
+}
+
 static struct virtio_config_ops virtio_pci_config_ops = {
 	.get		= vp_get,
 	.set		= vp_set,

Signature · 1 line

1.6.2.5

Michael S. Tsirkin

author maintainer

2009-07-23 15:50 UTC #4 lore raw

PATCH 2/2 virtio: retry on vector assignment failure

In reply to a message not in the archive

Reported-by Amit Shah <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---

A slightly bigger patch was tested by Amit Shah and reported working, this one has been made as small as possible for -rc inclusion. Let's pospone bigger refactorings for 2.6.32.

 drivers/virtio/virtio_pci.c |  130 +++++++++++++++++++++++-------------------
 1 files changed, 71 insertions(+), 59 deletions(-)

drivers/virtio/virtio_pci.c +71 ~~-59~~

index 4bff231..25c7995 100644
@@ -278,27 +278,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, unsigned max_vqs,
+			      int nvectors)
 {
 	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,38 +309,31 @@ 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) {

@@ -499,23 +489,17 @@ static void vp_del_vqs(struct virtio_device *vdev)
 	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 max_vqs, int nvectors)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	u16 vector, per_vq_vector;
-	int vectors = 0;
 	int i, err, allocated_vectors;
 
-	/* How many vectors would we like? */
-	for (i = 0; i < nvqs; ++i)
-		if (callbacks[i])
-			++vectors;
-
-	err = vp_request_vectors(vdev, vectors);
+	err = vp_request_vectors(vdev, max_vqs, nvectors);
 	if (err)
 		goto error_request;

@@ -543,6 +527,34 @@ error_request:
 	return PTR_ERR(vqs[i]);
 }
 
+/* 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[])
+{
+	/* 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[] = { 1, 2, 0 };
+	int vectors = 0;
+	int i, uninitialized_var(err);
+
+	/* How many vectors would we like? */
+	for (i = 0; i < nvqs; ++i)
+		if (callbacks[i])
+			++vectors;
+	options[0] = vectors + 1;
+
+	for (i = 0; i < ARRAY_SIZE(options); ++i) {
+		err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
+					 vectors, options[i]);
+		if (!err)
+			break;
+	}
+	return err;
+}
+
 static struct virtio_config_ops virtio_pci_config_ops = {
 	.get		= vp_get,
 	.set		= vp_set,

Signature · 1 line

1.6.2.5