Re: [PATCH 2/9] virtio_pci: use shared interrupts for virtqueues

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 2017年02月06日 01:15, Christoph Hellwig wrote:
This lets IRQ layer handle dispatching IRQs to separate handlers for the
case where we don't have per-VQ MSI-X vectors, and allows us to greatly
simplify the code based on the assumption that we always have interrupt
vector 0 (legacy INTx or config interrupt for MSI-X) available, and
any other interrupt is request/freed throught the VQ, even if the
actual interrupt line might be shared in some cases.

This allows removing a great deal of variables keeping track of the
interrupt state in struct virtio_pci_device, as we can now simply walk the
list of VQs and deal with per-VQ interrupt handlers there, and only treat
vector 0 special.

Additionally clean up the VQ allocation code to properly unwind on error
instead of having a single global cleanup label, which is error prone,
and in this case also leads to more code.

Signed-off-by: Christoph Hellwig <hch@xxxxxx>

Reviewed-by: Jason Wang <jasowang@xxxxxxxxxx>

---
  drivers/virtio/virtio_pci_common.c | 235 ++++++++++++++++---------------------
  drivers/virtio/virtio_pci_common.h |  16 +--
  2 files changed, 106 insertions(+), 145 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index a33767318cbf..274dc1ff09c0 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -33,10 +33,8 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
  	int i;
- if (vp_dev->intx_enabled)
-		synchronize_irq(vp_dev->pci_dev->irq);
-
-	for (i = 0; i < vp_dev->msix_vectors; ++i)
+	synchronize_irq(pci_irq_vector(vp_dev->pci_dev, 0));
+	for (i = 1; i < vp_dev->msix_vectors; i++)
  		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
  }
@@ -99,77 +97,10 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
  	return vp_vring_interrupt(irq, opaque);
  }
-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;
-
-	vp_dev->msix_vectors = nvectors;
-
-	vp_dev->msix_names = kmalloc(nvectors * sizeof *vp_dev->msix_names,
-				     GFP_KERNEL);
-	if (!vp_dev->msix_names)
-		goto error;
-	vp_dev->msix_affinity_masks
-		= kzalloc(nvectors * sizeof *vp_dev->msix_affinity_masks,
-			  GFP_KERNEL);
-	if (!vp_dev->msix_affinity_masks)
-		goto error;
-	for (i = 0; i < nvectors; ++i)
-		if (!alloc_cpumask_var(&vp_dev->msix_affinity_masks[i],
-					GFP_KERNEL))
-			goto error;
-
-	err = pci_alloc_irq_vectors(vp_dev->pci_dev, nvectors, nvectors,
-			PCI_IRQ_MSIX);
-	if (err < 0)
-		goto error;
-	vp_dev->msix_enabled = 1;
-
-	/* Set the vector used for configuration */
-	v = vp_dev->msix_used_vectors;
-	snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
-		 "%s-config", name);
-	err = request_irq(pci_irq_vector(vp_dev->pci_dev, v),
-			  vp_config_changed, 0, vp_dev->msix_names[v],
-			  vp_dev);
-	if (err)
-		goto error;
-	++vp_dev->msix_used_vectors;
-
-	v = vp_dev->config_vector(vp_dev, v);
-	/* Verify we had enough resources to assign the vector */
-	if (v == VIRTIO_MSI_NO_VECTOR) {
-		err = -EBUSY;
-		goto error;
-	}
-
-	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,
-			 "%s-virtqueues", name);
-		err = request_irq(pci_irq_vector(vp_dev->pci_dev, v),
-				  vp_vring_interrupt, 0, vp_dev->msix_names[v],
-				  vp_dev);
-		if (err)
-			goto error;
-		++vp_dev->msix_used_vectors;
-	}
-	return 0;
-error:
-	return err;
-}
-
-/* the config->del_vqs() implementation */
-void vp_del_vqs(struct virtio_device *vdev)
+static void vp_remove_vqs(struct virtio_device *vdev)
  {
  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
  	struct virtqueue *vq, *n;
-	int i;
list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
  		if (vp_dev->msix_vector_map) {
@@ -181,35 +112,33 @@ void vp_del_vqs(struct virtio_device *vdev)
  		}
  		vp_dev->del_vq(vq);
  	}
+}
- if (vp_dev->intx_enabled) {
-		free_irq(vp_dev->pci_dev->irq, vp_dev);
-		vp_dev->intx_enabled = 0;
-	}
+/* the config->del_vqs() implementation */
+void vp_del_vqs(struct virtio_device *vdev)
+{
+	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+	int i;
- for (i = 0; i < vp_dev->msix_used_vectors; ++i)
-		free_irq(pci_irq_vector(vp_dev->pci_dev, i), vp_dev);
+	if (WARN_ON_ONCE(list_empty_careful(&vdev->vqs)))
+		return;
- for (i = 0; i < vp_dev->msix_vectors; i++)
-		if (vp_dev->msix_affinity_masks[i])
-			free_cpumask_var(vp_dev->msix_affinity_masks[i]);
+	vp_remove_vqs(vdev);
if (vp_dev->msix_enabled) {
+		for (i = 0; i < vp_dev->msix_vectors; i++)
+			free_cpumask_var(vp_dev->msix_affinity_masks[i]);
+
  		/* Disable the vector used for configuration */
  		vp_dev->config_vector(vp_dev, VIRTIO_MSI_NO_VECTOR);
- pci_free_irq_vectors(vp_dev->pci_dev);
-		vp_dev->msix_enabled = 0;
+		kfree(vp_dev->msix_affinity_masks);
+		kfree(vp_dev->msix_names);
+		kfree(vp_dev->msix_vector_map);
  	}
- vp_dev->msix_vectors = 0;
-	vp_dev->msix_used_vectors = 0;
-	kfree(vp_dev->msix_names);
-	vp_dev->msix_names = NULL;
-	kfree(vp_dev->msix_affinity_masks);
-	vp_dev->msix_affinity_masks = NULL;
-	kfree(vp_dev->msix_vector_map);
-	vp_dev->msix_vector_map = NULL;
+	free_irq(pci_irq_vector(vp_dev->pci_dev, 0), vp_dev);
+	pci_free_irq_vectors(vp_dev->pci_dev);
  }
static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
@@ -219,79 +148,122 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
  			      bool per_vq_vectors)
  {
  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+	const char *name = dev_name(&vp_dev->vdev.dev);
+	int i, err = -ENOMEM, allocated_vectors, nvectors;
  	u16 msix_vec;
-	int i, err, nvectors, allocated_vectors;
+
+	nvectors = 1;
+	for (i = 0; i < nvqs; i++)
+		if (callbacks[i])
+			nvectors++;
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;
+		err = pci_alloc_irq_vectors(vp_dev->pci_dev, nvectors, nvectors,
+				PCI_IRQ_MSIX);
  	} else {
-		/* Second best: one for change, shared for all vqs. */
-		nvectors = 2;
+		err = pci_alloc_irq_vectors(vp_dev->pci_dev, 2, 2,
+				PCI_IRQ_MSIX);
  	}
+	if (err < 0)
+		return err;
- err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
+	vp_dev->msix_vectors = nvectors;
+	vp_dev->msix_names = kmalloc_array(nvectors,
+			sizeof(*vp_dev->msix_names), GFP_KERNEL);
+	if (!vp_dev->msix_names)
+		goto out_free_irq_vectors;
+
+	vp_dev->msix_affinity_masks = kcalloc(nvectors,
+			sizeof(*vp_dev->msix_affinity_masks), GFP_KERNEL);
+	if (!vp_dev->msix_affinity_masks)
+		goto out_free_msix_names;
+
+	for (i = 0; i < nvectors; ++i) {
+		if (!alloc_cpumask_var(&vp_dev->msix_affinity_masks[i],
+				GFP_KERNEL))
+			goto out_free_msix_affinity_masks;
+	}
+
+	/* Set the vector used for configuration */
+	snprintf(vp_dev->msix_names[0], sizeof(*vp_dev->msix_names),
+		 "%s-config", name);
+	err = request_irq(pci_irq_vector(vp_dev->pci_dev, 0), vp_config_changed,
+			0, vp_dev->msix_names[0], vp_dev);
  	if (err)
-		goto error_find;
+		goto out_free_irq_vectors;
- if (per_vq_vectors) {
-		vp_dev->msix_vector_map = kmalloc_array(nvqs,
-				sizeof(*vp_dev->msix_vector_map), GFP_KERNEL);
-		if (!vp_dev->msix_vector_map)
-			goto error_find;
+	/* Verify we had enough resources to assign the vector */
+	if (vp_dev->config_vector(vp_dev, 0) == VIRTIO_MSI_NO_VECTOR) {
+		err = -EBUSY;
+		goto out_free_config_irq;
  	}
- allocated_vectors = vp_dev->msix_used_vectors;
+	vp_dev->msix_vector_map = kmalloc_array(nvqs,
+			sizeof(*vp_dev->msix_vector_map), GFP_KERNEL);
+	if (!vp_dev->msix_vector_map)
+		goto out_disable_config_irq;
+
+	allocated_vectors = 1; /* vector 0 is the config interrupt */
  	for (i = 0; i < nvqs; ++i) {
  		if (!names[i]) {
  			vqs[i] = NULL;
  			continue;
  		}
- if (!callbacks[i])
-			msix_vec = VIRTIO_MSI_NO_VECTOR;
-		else if (per_vq_vectors)
-			msix_vec = allocated_vectors++;
+		if (callbacks[i])
+			msix_vec = allocated_vectors;
  		else
-			msix_vec = VP_MSIX_VQ_VECTOR;
+			msix_vec = VIRTIO_MSI_NO_VECTOR;
+
  		vqs[i] = vp_dev->setup_vq(vp_dev, i, callbacks[i], names[i],
  				msix_vec);
  		if (IS_ERR(vqs[i])) {
  			err = PTR_ERR(vqs[i]);
-			goto error_find;
+			goto out_remove_vqs;
  		}
- if (!per_vq_vectors)
-			continue;
-
  		if (msix_vec == VIRTIO_MSI_NO_VECTOR) {
  			vp_dev->msix_vector_map[i] = VIRTIO_MSI_NO_VECTOR;
  			continue;
  		}
- /* allocate per-vq irq if available and necessary */
-		snprintf(vp_dev->msix_names[msix_vec],
-			 sizeof *vp_dev->msix_names,
-			 "%s-%s",
+		snprintf(vp_dev->msix_names[i + 1],
+			 sizeof(*vp_dev->msix_names), "%s-%s",
  			 dev_name(&vp_dev->vdev.dev), names[i]);
  		err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec),
-				  vring_interrupt, 0,
-				  vp_dev->msix_names[msix_vec],
-				  vqs[i]);
+				  vring_interrupt, IRQF_SHARED,
+				  vp_dev->msix_names[i + 1], vqs[i]);
  		if (err) {
  			/* don't free this irq on error */
  			vp_dev->msix_vector_map[i] = VIRTIO_MSI_NO_VECTOR;
-			goto error_find;
+			goto out_remove_vqs;
  		}
  		vp_dev->msix_vector_map[i] = msix_vec;
+
+		if (per_vq_vectors)
+			allocated_vectors++;
  	}
+
+	vp_dev->msix_enabled = 1;
  	return 0;
-error_find:
-	vp_del_vqs(vdev);
+out_remove_vqs:
+	vp_remove_vqs(vdev);
+	kfree(vp_dev->msix_vector_map);
+out_disable_config_irq:
+	vp_dev->config_vector(vp_dev, VIRTIO_MSI_NO_VECTOR);
+out_free_config_irq:
+	free_irq(pci_irq_vector(vp_dev->pci_dev, 0), vp_dev);
+out_free_msix_affinity_masks:
+	for (i = 0; i < nvectors; i++) {
+		if (vp_dev->msix_affinity_masks[i])
+			free_cpumask_var(vp_dev->msix_affinity_masks[i]);
+	}
+	kfree(vp_dev->msix_affinity_masks);
+out_free_msix_names:
+	kfree(vp_dev->msix_names);
+out_free_irq_vectors:
+	pci_free_irq_vectors(vp_dev->pci_dev);
  	return err;
  }
@@ -305,9 +277,8 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned nvqs,
  	err = request_irq(vp_dev->pci_dev->irq, vp_interrupt, IRQF_SHARED,
  			dev_name(&vdev->dev), vp_dev);
  	if (err)
-		goto out_del_vqs;
+		return err;
- vp_dev->intx_enabled = 1;
  	for (i = 0; i < nvqs; ++i) {
  		if (!names[i]) {
  			vqs[i] = NULL;
@@ -317,13 +288,15 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned nvqs,
  				VIRTIO_MSI_NO_VECTOR);
  		if (IS_ERR(vqs[i])) {
  			err = PTR_ERR(vqs[i]);
-			goto out_del_vqs;
+			goto out_remove_vqs;
  		}
  	}
return 0;
-out_del_vqs:
-	vp_del_vqs(vdev);
+
+out_remove_vqs:
+	vp_remove_vqs(vdev);
+	free_irq(pci_irq_vector(vp_dev->pci_dev, 0), vp_dev);
  	return err;
  }
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 2038887bdf23..85593867e712 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -66,16 +66,12 @@ struct virtio_pci_device {
/* MSI-X support */
  	int msix_enabled;
-	int intx_enabled;
  	cpumask_var_t *msix_affinity_masks;
  	/* Name strings for interrupts. This size should be enough,
  	 * and I'm too lazy to allocate each name separately. */
  	char (*msix_names)[256];
-	/* Number of available vectors */
-	unsigned msix_vectors;
-	/* Vectors allocated, excluding per-vq vectors if any */
-	unsigned msix_used_vectors;
-
+	/* Total Number of MSI-X vectors (including per-VQ ones). */
+	int msix_vectors;
  	/* Map of per-VQ MSI-X vectors, may be NULL */
  	unsigned *msix_vector_map;
@@ -89,14 +85,6 @@ struct virtio_pci_device {
  	u16 (*config_vector)(struct virtio_pci_device *vp_dev, u16 vector);
  };
-/* Constants for MSI-X */
-/* Use first vector for configuration changes, second and the rest for
- * virtqueues Thus, we need at least 2 vectors for MSI. */
-enum {
-	VP_MSIX_CONFIG_VECTOR = 0,
-	VP_MSIX_VQ_VECTOR = 1,
-};
-
  /* Convert a generic virtio device to our structure */
  static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
  {

_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization




[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux