All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 1/3] virtio-net: Using single MSIX IRQ for TX/RX Q pair
@ 2015-10-26 17:52 Ravi Kerur
  2015-10-26 17:52 ` [PATCH v1 2/3] virtio: vp_find_vqs accept channel setting params Ravi Kerur
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Ravi Kerur @ 2015-10-26 17:52 UTC (permalink / raw)
  To: netdev; +Cc: jasowang, mst, rusty, Ravi Kerur

Ported earlier patch from Jason Wang (dated 12/26/2014).

This patch tries to reduce the number of MSIX irqs required for
virtio-net by sharing a MSIX irq for each TX/RX queue pair through
channels. If transport support channel, about half of the MSIX irqs
were reduced.

Signed-off-by: Ravi Kerur <rkerur@gmail.com>
---
 drivers/net/virtio_net.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d8838ded..d705cce 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -72,6 +72,9 @@ struct send_queue {
 
 	/* Name of the send queue: output.$index */
 	char name[40];
+
+	/* Name of the channel, shared with irq. */
+	char channel_name[40];
 };
 
 /* Internal representation of a receive virtqueue */
@@ -1529,6 +1532,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
 	int ret = -ENOMEM;
 	int i, total_vqs;
 	const char **names;
+	const char **channel_names;
+	unsigned *channels;
 
 	/* We expect 1 RX virtqueue followed by 1 TX virtqueue, followed by
 	 * possible N-1 RX/TX queue pairs used in multiqueue mode, followed by
@@ -1548,6 +1553,17 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
 	if (!names)
 		goto err_names;
 
+	channel_names = kmalloc_array(vi->max_queue_pairs,
+				      sizeof(*channel_names),
+				      GFP_KERNEL);
+	if (!channel_names)
+		goto err_channel_names;
+
+	channels = kmalloc_array(total_vqs, sizeof(*channels),
+				 GFP_KERNEL);
+	if (!channels)
+		goto err_channels;
+
 	/* Parameters for control virtqueue, if any */
 	if (vi->has_cvq) {
 		callbacks[total_vqs - 1] = NULL;
@@ -1562,10 +1578,15 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
 		sprintf(vi->sq[i].name, "output.%d", i);
 		names[rxq2vq(i)] = vi->rq[i].name;
 		names[txq2vq(i)] = vi->sq[i].name;
+		sprintf(vi->sq[i].channel_name, "txrx.%d", i);
+		channel_names[i] = vi->sq[i].channel_name;
+		channels[rxq2vq(i)] = i;
+		channels[txq2vq(i)] = i;
 	}
 
 	ret = vi->vdev->config->find_vqs(vi->vdev, total_vqs, vqs, callbacks,
-					 names);
+					 names, channels, channel_names,
+					 vi->max_queue_pairs);
 	if (ret)
 		goto err_find;
 
@@ -1580,6 +1601,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
 		vi->sq[i].vq = vqs[txq2vq(i)];
 	}
 
+	kfree(channels);
+	kfree(channel_names);
 	kfree(names);
 	kfree(callbacks);
 	kfree(vqs);
@@ -1587,6 +1610,10 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
 	return 0;
 
 err_find:
+	kfree(channels);
+err_channels:
+	kfree(channel_names);
+err_channel_names:
 	kfree(names);
 err_names:
 	kfree(callbacks);
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v1 2/3] virtio: vp_find_vqs accept channel setting params
  2015-10-26 17:52 [PATCH v1 1/3] virtio-net: Using single MSIX IRQ for TX/RX Q pair Ravi Kerur
@ 2015-10-26 17:52 ` Ravi Kerur
  2015-10-26 18:12   ` kbuild test robot
  2015-10-26 18:14   ` kbuild test robot
  2015-10-26 17:52 ` [PATCH v1 3/3] virtio-pci: Introduce channels Ravi Kerur
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 12+ messages in thread
From: Ravi Kerur @ 2015-10-26 17:52 UTC (permalink / raw)
  To: netdev; +Cc: jasowang, mst, rusty, Ravi Kerur

Port earlier patch from Jason Wang (dated 12/26/2014).

This patch lets vp_find_vqs function accept channel parameters.
For the transports that do not support channel currently, all
the parameters are ignored. For the device that does not use channel,
it can simply pass NULL to transport.

Signed-off-by: Ravi Kerur <rkerur@gmail.com>
---
 drivers/block/virtio_blk.c             |  3 ++-
 drivers/char/virtio_console.c          |  3 ++-
 drivers/gpu/drm/virtio/virtgpu_kms.c   |  3 ++-
 drivers/misc/mic/card/mic_virtio.c     |  5 ++++-
 drivers/net/caif/caif_virtio.c         |  3 ++-
 drivers/remoteproc/remoteproc_virtio.c |  9 ++++++---
 drivers/rpmsg/virtio_rpmsg_bus.c       |  3 ++-
 drivers/s390/virtio/kvm_virtio.c       |  5 ++++-
 drivers/s390/virtio/virtio_ccw.c       |  5 ++++-
 drivers/scsi/virtio_scsi.c             |  3 ++-
 drivers/virtio/virtio_balloon.c        |  3 ++-
 drivers/virtio/virtio_input.c          |  3 ++-
 drivers/virtio/virtio_mmio.c           |  9 ++++++---
 drivers/virtio/virtio_pci_modern.c     |  8 ++++++--
 include/linux/virtio_config.h          | 11 +++++++++--
 15 files changed, 55 insertions(+), 21 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index e93899c..7fb70b3 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -418,7 +418,8 @@ static int init_vq(struct virtio_blk *vblk)
 	}
 
 	/* Discover virtqueues and write information to configuration.  */
-	err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
+	err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names,
+				     NULL, NULL, 0);
 	if (err)
 		goto err_find_vqs;
 
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index d2406fe..b316820 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1923,7 +1923,8 @@ static int init_vqs(struct ports_device *portdev)
 	/* Find the queues. */
 	err = portdev->vdev->config->find_vqs(portdev->vdev, nr_queues, vqs,
 					      io_callbacks,
-					      (const char **)io_names);
+					      (const char **)io_names,
+					      NULL, NULL, 0);
 	if (err)
 		goto free;
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 782766c..4e521c2f 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -100,7 +100,8 @@ int virtio_gpu_driver_load(struct drm_device *dev, unsigned long flags)
 		  virtio_gpu_config_changed_work_func);
 
 	ret = vgdev->vdev->config->find_vqs(vgdev->vdev, 2, vqs,
-					    callbacks, names);
+					    callbacks, names,
+					    NULL, NULL, 0);
 	if (ret) {
 		DRM_ERROR("failed to find virt queues\n");
 		goto err_vqs;
diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c
index e486a0c..09c3f85 100644
--- a/drivers/misc/mic/card/mic_virtio.c
+++ b/drivers/misc/mic/card/mic_virtio.c
@@ -311,7 +311,10 @@ unmap:
 static int mic_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 			struct virtqueue *vqs[],
 			vq_callback_t *callbacks[],
-			const char *names[])
+			const char *names[],
+			unsigned channels[],
+			const char *channel_names[],
+			unsigned nchannels)
 {
 	struct mic_vdev *mvdev = to_micvdev(vdev);
 	struct mic_device_ctrl __iomem *dc = mvdev->dc;
diff --git a/drivers/net/caif/caif_virtio.c b/drivers/net/caif/caif_virtio.c
index b306210..150809d 100644
--- a/drivers/net/caif/caif_virtio.c
+++ b/drivers/net/caif/caif_virtio.c
@@ -679,7 +679,8 @@ static int cfv_probe(struct virtio_device *vdev)
 		goto err;
 
 	/* Get the TX virtio ring. This is a "guest side vring". */
-	err = vdev->config->find_vqs(vdev, 1, &cfv->vq_tx, &vq_cbs, &names);
+	err = vdev->config->find_vqs(vdev, 1, &cfv->vq_tx, &vq_cbs, &names,
+				     NULL, NULL, 0);
 	if (err)
 		goto err;
 
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index e1a1023..16b3532 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -145,9 +145,12 @@ static void rproc_virtio_del_vqs(struct virtio_device *vdev)
 }
 
 static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned nvqs,
-		       struct virtqueue *vqs[],
-		       vq_callback_t *callbacks[],
-		       const char *names[])
+				 struct virtqueue *vqs[],
+				 vq_callback_t *callbacks[],
+				 const char *names[],
+				 unsigned channels[],
+				 const char *channel_names,
+				 unsigned nchannels)
 {
 	struct rproc *rproc = vdev_to_rproc(vdev);
 	int i, ret;
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 73354ee..8421666 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -965,7 +965,8 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	init_waitqueue_head(&vrp->sendq);
 
 	/* We expect two virtqueues, rx and tx (and in this order) */
-	err = vdev->config->find_vqs(vdev, 2, vqs, vq_cbs, names);
+	err = vdev->config->find_vqs(vdev, 2, vqs, vq_cbs, names,
+				     NULL, NULL, 0);
 	if (err)
 		goto free_vrp;
 
diff --git a/drivers/s390/virtio/kvm_virtio.c b/drivers/s390/virtio/kvm_virtio.c
index 53fb975..a60d5ae 100644
--- a/drivers/s390/virtio/kvm_virtio.c
+++ b/drivers/s390/virtio/kvm_virtio.c
@@ -255,7 +255,10 @@ static void kvm_del_vqs(struct virtio_device *vdev)
 static int kvm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 			struct virtqueue *vqs[],
 			vq_callback_t *callbacks[],
-			const char *names[])
+			const char *names[],
+			unsigned channels[],
+			const char *channel_names[],
+			unsigned nchannels)
 {
 	struct kvm_device *kdev = to_kvmdev(vdev);
 	int i;
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index e9fae30..4ed0463 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -626,7 +626,10 @@ out:
 static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 			       struct virtqueue *vqs[],
 			       vq_callback_t *callbacks[],
-			       const char *names[])
+			       const char *names[],
+			       unsigned channels[],
+			       const char *channel_names[],
+			       unsigned nchannels)
 {
 	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
 	unsigned long *indicatorp = NULL;
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 7dbbb29..06b54f6 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -919,7 +919,8 @@ static int virtscsi_init(struct virtio_device *vdev,
 	}
 
 	/* Discover virtqueues and write information to configuration.  */
-	err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
+	err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names,
+				     NULL, NULL, 0);
 	if (err)
 		goto out;
 
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 7efc329..a1a5dbc 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -396,7 +396,8 @@ static int init_vqs(struct virtio_balloon *vb)
 	 * optionally stat.
 	 */
 	nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2;
-	err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks, names);
+	err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks, names,
+					 NULL, NULL, 0);
 	if (err)
 		return err;
 
diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
index c96944b..5493235 100644
--- a/drivers/virtio/virtio_input.c
+++ b/drivers/virtio/virtio_input.c
@@ -173,7 +173,8 @@ static int virtinput_init_vqs(struct virtio_input *vi)
 	static const char *names[] = { "events", "status" };
 	int err;
 
-	err = vi->vdev->config->find_vqs(vi->vdev, 2, vqs, cbs, names);
+	err = vi->vdev->config->find_vqs(vi->vdev, 2, vqs, cbs, names,
+					 NULL, NULL, 0);
 	if (err)
 		return err;
 	vi->evt = vqs[0];
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index f499d9d..8c9be80 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -480,9 +480,12 @@ error_available:
 }
 
 static int vm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
-		       struct virtqueue *vqs[],
-		       vq_callback_t *callbacks[],
-		       const char *names[])
+			struct virtqueue *vqs[],
+			vq_callback_t *callbacks[],
+			const char *names[],
+			unsigned channels[],
+			const char *channel_names[],
+			unsigned nchannels)
 {
 	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
 	unsigned int irq = platform_get_irq(vm_dev->pdev, 0);
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 8e5cf19..c308983 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -418,11 +418,15 @@ err_new_queue:
 static int vp_modern_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 			      struct virtqueue *vqs[],
 			      vq_callback_t *callbacks[],
-			      const char *names[])
+			      const char *names[],
+			      unsigned channels[],
+			      const char *channel_names[],
+			      unsigned nchannels)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtqueue *vq;
-	int rc = vp_find_vqs(vdev, nvqs, vqs, callbacks, names);
+	int rc = vp_find_vqs(vdev, nvqs, vqs, callbacks, names,
+			     NULL, NULL, 0);
 
 	if (rc)
 		return rc;
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index e5ce8ab..9c94a2f 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -41,6 +41,9 @@
  *		include a NULL entry for vqs that do not need a callback
  *	names: array of virtqueue names (mainly for debugging)
  *		include a NULL entry for vqs unused by driver
+ *	channels: array of channels each vq belong to
+ *	channel_names: names of channels
+ *	nchannels: number of channels
  *	Returns 0 on success or error status
  * @del_vqs: free virtqueues found by find_vqs().
  * @get_features: get the array of feature bits for this device.
@@ -70,7 +73,10 @@ struct virtio_config_ops {
 	int (*find_vqs)(struct virtio_device *, unsigned nvqs,
 			struct virtqueue *vqs[],
 			vq_callback_t *callbacks[],
-			const char *names[]);
+			const char *names[],
+			unsigned int channels[],
+			const char *channel_names,
+			unsigned nchannels);
 	void (*del_vqs)(struct virtio_device *);
 	u64 (*get_features)(struct virtio_device *vdev);
 	int (*finalize_features)(struct virtio_device *vdev);
@@ -156,7 +162,8 @@ struct virtqueue *virtio_find_single_vq(struct virtio_device *vdev,
 	vq_callback_t *callbacks[] = { c };
 	const char *names[] = { n };
 	struct virtqueue *vq;
-	int err = vdev->config->find_vqs(vdev, 1, &vq, callbacks, names);
+	int err = vdev->config->find_vqs(vdev, 1, &vq, callbacks, names,
+					 NULL, NULL, 0);
 	if (err < 0)
 		return ERR_PTR(err);
 	return vq;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v1 3/3] virtio-pci: Introduce channels
  2015-10-26 17:52 [PATCH v1 1/3] virtio-net: Using single MSIX IRQ for TX/RX Q pair Ravi Kerur
  2015-10-26 17:52 ` [PATCH v1 2/3] virtio: vp_find_vqs accept channel setting params Ravi Kerur
@ 2015-10-26 17:52 ` Ravi Kerur
  2015-10-27  5:11 ` [PATCH v1 1/3] virtio-net: Using single MSIX IRQ for TX/RX Q pair Jason Wang
  2015-10-27  8:38 ` Michael S. Tsirkin
  3 siblings, 0 replies; 12+ messages in thread
From: Ravi Kerur @ 2015-10-26 17:52 UTC (permalink / raw)
  To: netdev; +Cc: jasowang, mst, rusty, Ravi Kerur

Port earlier patch from Jason Wang (dated 12/26/2014).

This patch introduces virtio pci channel which are virtqueue groups
that sharing a single MSIX irq. This can be used to reduce the irqs
needed by virtio device.

The channel are in fact a list of virtqueues, and vp_channel_interrupt()
was introduced to traverse the list. The current strategy was kept but
is converted to channel internally:

- per vq vectors was implemented through per vq channel
- sharing interrupts was implemented through a single channel for all
  virtqueues

This is done by letting vp_try_to_find_vqs() to accept the array of
channel names and the channels that each vq belongs to.

Signed-off-by: Ravi Kerur <rkerur@gmail.com>
---
 drivers/virtio/virtio_pci_common.c | 208 +++++++++++++++++++++++--------------
 drivers/virtio/virtio_pci_common.h |  23 ++--
 2 files changed, 146 insertions(+), 85 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 78f804a..5c0594e 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -76,6 +76,23 @@ static irqreturn_t vp_vring_interrupt(int irq, void *opaque)
 	return ret;
 }
 
+static irqreturn_t vp_channel_interrupt(int irq, void *opaque)
+{
+	struct virtio_pci_channel *vp_channel = opaque;
+	struct virtio_pci_vq_info *info;
+	irqreturn_t ret = IRQ_NONE;
+	unsigned long flags;
+
+	spin_lock_irqsave(&vp_channel->lock, flags);
+	list_for_each_entry(info, &vp_channel->virtqueues, node) {
+		if (vring_interrupt(irq, info->vq) == IRQ_HANDLED)
+			ret = IRQ_HANDLED;
+	}
+	spin_unlock_irqrestore(&vp_channel->lock, flags);
+
+	return ret;
+}
+
 /* A small wrapper to also acknowledge the interrupt when it's handled.
  * I really need an EIO hook for the vring so I can ack the interrupt once we
  * know that we'll be handling the IRQ but before we invoke the callback since
@@ -112,8 +129,12 @@ static void vp_free_vectors(struct virtio_device *vdev)
 		vp_dev->intx_enabled = 0;
 	}
 
-	for (i = 0; i < vp_dev->msix_used_vectors; ++i)
-		free_irq(vp_dev->msix_entries[i].vector, vp_dev);
+	if (vp_dev->msix_used_vectors)
+		free_irq(vp_dev->msix_entries[0].vector, vp_dev);
+
+	for (i = 1; i < vp_dev->msix_used_vectors; ++i)
+		free_irq(vp_dev->msix_entries[i].vector,
+			 &vp_dev->channels[i - 1]);
 
 	for (i = 0; i < vp_dev->msix_vectors; i++)
 		if (vp_dev->msix_affinity_masks[i])
@@ -137,8 +158,7 @@ static void vp_free_vectors(struct virtio_device *vdev)
 	vp_dev->msix_affinity_masks = NULL;
 }
 
-static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
-				   bool per_vq_vectors)
+static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	const char *name = dev_name(&vp_dev->vdev.dev);
@@ -175,8 +195,8 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int 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,
+	v = 0;
+	snprintf(vp_dev->msix_names[0], 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],
@@ -192,18 +212,6 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
 		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(vp_dev->msix_entries[v].vector,
-				  vp_vring_interrupt, 0, vp_dev->msix_names[v],
-				  vp_dev);
-		if (err)
-			goto error;
-		++vp_dev->msix_used_vectors;
-	}
 	return 0;
 error:
 	vp_free_vectors(vdev);
@@ -228,6 +236,7 @@ static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
 				     u16 msix_vec)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+	struct virtio_pci_channel *vp_channel;
 	struct virtio_pci_vq_info *info = kmalloc(sizeof *info, GFP_KERNEL);
 	struct virtqueue *vq;
 	unsigned long flags;
@@ -242,9 +251,16 @@ static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
 
 	info->vq = vq;
 	if (callback) {
-		spin_lock_irqsave(&vp_dev->lock, flags);
-		list_add(&info->node, &vp_dev->virtqueues);
-		spin_unlock_irqrestore(&vp_dev->lock, flags);
+		if (msix_vec != VIRTIO_MSI_NO_VECTOR) {
+			vp_channel = &vp_dev->channels[msix_vec - 1];
+			spin_lock_irqsave(&vp_channel->lock, flags);
+			list_add(&info->node, &vp_channel->virtqueues);
+			spin_unlock_irqrestore(&vp_channel->lock, flags);
+		} else {
+			spin_lock_irqsave(&vp_dev->lock, flags);
+			list_add(&info->node, &vp_dev->virtqueues);
+			spin_unlock_irqrestore(&vp_dev->lock, flags);
+		}
 	} else {
 		INIT_LIST_HEAD(&info->node);
 	}
@@ -257,15 +273,17 @@ out_info:
 	return vq;
 }
 
-static void vp_del_vq(struct virtqueue *vq)
+static void vp_del_vq(struct virtqueue *vq,
+		      struct virtio_pci_channel *channel)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
 	struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
+	spinlock_t *lock = channel ? &channel->lock : &vp_dev->lock;
 	unsigned long flags;
 
-	spin_lock_irqsave(&vp_dev->lock, flags);
+	spin_lock_irqsave(lock, flags);
 	list_del(&info->node);
-	spin_unlock_irqrestore(&vp_dev->lock, flags);
+	spin_unlock_irqrestore(lock, flags);
 
 	vp_dev->del_vq(info);
 	kfree(info);
@@ -275,95 +293,103 @@ static void vp_del_vq(struct virtqueue *vq)
 void vp_del_vqs(struct virtio_device *vdev)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+	struct virtio_pci_vq_info *info, *ninfo;
+	struct virtio_pci_channel *channel;
 	struct virtqueue *vq, *n;
-	struct virtio_pci_vq_info *info;
+	int i;
 
-	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
-		info = vp_dev->vqs[vq->index];
-		if (vp_dev->per_vq_vectors &&
-			info->msix_vector != VIRTIO_MSI_NO_VECTOR)
-			free_irq(vp_dev->msix_entries[info->msix_vector].vector,
-				 vq);
-		vp_del_vq(vq);
+	if (vp_dev->nchannels) {
+		for (i = 0; i < vp_dev->nchannels; i++) {
+			channel = &vp_dev->channels[i];
+			list_for_each_entry_safe(info, ninfo,
+						&channel->virtqueues, node) {
+				vp_del_vq(info->vq, channel);
+			}
+		}
+	} else {
+		list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
+			vp_del_vq(vq, NULL);
+		}
 	}
-	vp_dev->per_vq_vectors = false;
+
+	vp_dev->nchannels = 0;
 
 	vp_free_vectors(vdev);
 	kfree(vp_dev->vqs);
 	vp_dev->vqs = NULL;
+	kfree(vp_dev->channels);
 }
 
 static int vp_try_to_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 			      struct virtqueue *vqs[],
 			      vq_callback_t *callbacks[],
 			      const char *names[],
-			      bool use_msix,
-			      bool per_vq_vectors)
+			      unsigned *channels,
+			      const char *channel_names[],
+			      unsigned nchannels)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	u16 msix_vec;
-	int i, err, nvectors, allocated_vectors;
+	int i, err = 0;
 
 	vp_dev->vqs = kmalloc(nvqs * sizeof *vp_dev->vqs, GFP_KERNEL);
 	if (!vp_dev->vqs)
 		return -ENOMEM;
 
-	if (!use_msix) {
+	if (nchannels) {
+		vp_dev->channels = kmalloc(nchannels *
+					   sizeof(*vp_dev->channels),
+					   GFP_KERNEL);
+		if (!vp_dev->channels)
+			goto error_find;
+	}
+	vp_dev->nchannels = nchannels;
+
+	for (i = 0; i < nchannels; i++) {
+		spin_lock_init(&vp_dev->channels[i].lock);
+		INIT_LIST_HEAD(&vp_dev->channels[i].virtqueues);
+	}
+
+	if (!nchannels) {
 		/* Old style: one normal interrupt for change and all vqs. */
 		err = vp_request_intx(vdev);
 		if (err)
 			goto error_find;
 	} 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, nchannels + 1);
+		if (err)
+			goto error_find;
+	}
+
+	for (i = 0; i < nchannels; i++) {
+		snprintf(vp_dev->msix_names[i + 1],
+			sizeof(*vp_dev->msix_names),
+			"%s-%s",
+			dev_name(&vp_dev->vdev.dev), channel_names[i]);
+		err = request_irq(vp_dev->msix_entries[i + 1].vector,
+				vp_channel_interrupt, 0,
+				vp_dev->msix_names[i + 1],
+				&vp_dev->channels[i]);
 
-		err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
 		if (err)
 			goto error_find;
+		++vp_dev->msix_used_vectors;
 	}
 
-	vp_dev->per_vq_vectors = per_vq_vectors;
-	allocated_vectors = vp_dev->msix_used_vectors;
 	for (i = 0; i < nvqs; ++i) {
 		if (!names[i]) {
 			vqs[i] = NULL;
 			continue;
 		} else if (!callbacks[i] || !vp_dev->msix_enabled)
 			msix_vec = VIRTIO_MSI_NO_VECTOR;
-		else if (vp_dev->per_vq_vectors)
-			msix_vec = allocated_vectors++;
 		else
-			msix_vec = VP_MSIX_VQ_VECTOR;
+			msix_vec = channels[i] + 1;
 		vqs[i] = vp_setup_vq(vdev, i, callbacks[i], names[i], msix_vec);
 		if (IS_ERR(vqs[i])) {
 			err = PTR_ERR(vqs[i]);
 			goto error_find;
 		}
 
-		if (!vp_dev->per_vq_vectors || msix_vec == 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",
-			 dev_name(&vp_dev->vdev.dev), names[i]);
-		err = request_irq(vp_dev->msix_entries[msix_vec].vector,
-				  vring_interrupt, 0,
-				  vp_dev->msix_names[msix_vec],
-				  vqs[i]);
-		if (err) {
-			vp_del_vq(vqs[i]);
-			goto error_find;
-		}
 	}
 	return 0;
 
@@ -376,22 +402,48 @@ error_find:
 int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 		struct virtqueue *vqs[],
 		vq_callback_t *callbacks[],
-		const char *names[])
+		const char *names[],
+		unsigned channels[],
+		const char *channel_names[],
+		unsigned nchannels)
 {
-	int err;
+	int err, i;
+	const char *cnames[] = {"virtqueues"};
+
+	if (nchannels) {
+		/* Try channel settings */
+		err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
+					 channels, channel_names, nchannels);
+		if (!err)
+			return 0;
+	}
+
+	channels = kmalloc_array(nvqs, sizeof(unsigned), GFP_KERNEL);
+	if (!channels)
+		return -ENOMEM;
 
 	/* Try MSI-X with one vector per queue. */
-	err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names, true, true);
+	for (i = 0; i < nvqs; i++)
+		channels[i] = i;
+	err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names, channels,
+				 names, nvqs);
 	if (!err)
-		return 0;
+		goto out;
+
 	/* Fallback: MSI-X with one vector for config, one shared for queues. */
-	err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
-				 true, false);
+	for (i = 0; i < nvqs; i++)
+		channels[i] = 0;
+	err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names, channels,
+				 cnames, 1);
 	if (!err)
-		return 0;
+		goto out;
+
 	/* Finally fall back to regular interrupts. */
-	return vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
-				  false, false);
+	err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
+				NULL, NULL, 0);
+out:
+	kfree(channels);
+	return err;
 }
 
 const char *vp_bus_name(struct virtio_device *vdev)
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index b976d96..e8a739d 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -48,6 +48,11 @@ struct virtio_pci_vq_info {
 	unsigned msix_vector;
 };
 
+struct virtio_pci_channel {
+	spinlock_t lock;
+	struct list_head virtqueues;
+};
+
 /* Our device structure */
 struct virtio_pci_device {
 	struct virtio_device vdev;
@@ -88,6 +93,10 @@ struct virtio_pci_device {
 	/* array of all queues for house-keeping */
 	struct virtio_pci_vq_info **vqs;
 
+	/* array of channels */
+	struct virtio_pci_channel *channels;
+	unsigned nchannels;
+
 	/* MSI-X support */
 	int msix_enabled;
 	int intx_enabled;
@@ -98,12 +107,9 @@ struct virtio_pci_device {
 	char (*msix_names)[256];
 	/* Number of available vectors */
 	unsigned msix_vectors;
-	/* Vectors allocated, excluding per-vq vectors if any */
+	/* Vectors allocated */
 	unsigned msix_used_vectors;
 
-	/* Whether we have vector per vq */
-	bool per_vq_vectors;
-
 	struct virtqueue *(*setup_vq)(struct virtio_pci_device *vp_dev,
 				      struct virtio_pci_vq_info *info,
 				      unsigned idx,
@@ -137,9 +143,12 @@ bool vp_notify(struct virtqueue *vq);
 void vp_del_vqs(struct virtio_device *vdev);
 /* the config->find_vqs() implementation */
 int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
-		       struct virtqueue *vqs[],
-		       vq_callback_t *callbacks[],
-		       const char *names[]);
+		struct virtqueue *vqs[],
+		vq_callback_t *callbacks[],
+		const char *names[],
+		unsigned channels[],
+		const char *channel_names[],
+		unsigned nchannels);
 const char *vp_bus_name(struct virtio_device *vdev);
 
 /* Setup the affinity for a virtqueue:
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 2/3] virtio: vp_find_vqs accept channel setting params
  2015-10-26 17:52 ` [PATCH v1 2/3] virtio: vp_find_vqs accept channel setting params Ravi Kerur
@ 2015-10-26 18:12   ` kbuild test robot
  2015-10-26 18:14   ` kbuild test robot
  1 sibling, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2015-10-26 18:12 UTC (permalink / raw)
  To: Ravi Kerur; +Cc: kbuild-all, netdev, jasowang, mst, rusty, Ravi Kerur

[-- Attachment #1: Type: text/plain, Size: 2879 bytes --]

Hi Ravi,

[auto build test WARNING on char-misc/char-misc-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Ravi-Kerur/virtio-net-Using-single-MSIX-IRQ-for-TX-RX-Q-pair/20151027-015503
config: x86_64-randconfig-x015-201543 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

>> drivers/virtio/virtio_mmio.c:524:14: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
     .find_vqs = vm_find_vqs,
                 ^
   drivers/virtio/virtio_mmio.c:524:14: note: (near initialization for 'virtio_mmio_config_ops.find_vqs')

vim +524 drivers/virtio/virtio_mmio.c

edfd52e6 Pawel Moll         2011-10-24  508  }
edfd52e6 Pawel Moll         2011-10-24  509  
66846048 Rick Jones         2011-11-14  510  static const char *vm_bus_name(struct virtio_device *vdev)
66846048 Rick Jones         2011-11-14  511  {
66846048 Rick Jones         2011-11-14  512  	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
edfd52e6 Pawel Moll         2011-10-24  513  
66846048 Rick Jones         2011-11-14  514  	return vm_dev->pdev->name;
66846048 Rick Jones         2011-11-14  515  }
edfd52e6 Pawel Moll         2011-10-24  516  
93503932 Stephen Hemminger  2013-02-10  517  static const struct virtio_config_ops virtio_mmio_config_ops = {
edfd52e6 Pawel Moll         2011-10-24  518  	.get		= vm_get,
edfd52e6 Pawel Moll         2011-10-24  519  	.set		= vm_set,
87e7bf14 Michael S. Tsirkin 2015-03-12  520  	.generation	= vm_generation,
edfd52e6 Pawel Moll         2011-10-24  521  	.get_status	= vm_get_status,
edfd52e6 Pawel Moll         2011-10-24  522  	.set_status	= vm_set_status,
edfd52e6 Pawel Moll         2011-10-24  523  	.reset		= vm_reset,
edfd52e6 Pawel Moll         2011-10-24 @524  	.find_vqs	= vm_find_vqs,
edfd52e6 Pawel Moll         2011-10-24  525  	.del_vqs	= vm_del_vqs,
edfd52e6 Pawel Moll         2011-10-24  526  	.get_features	= vm_get_features,
edfd52e6 Pawel Moll         2011-10-24  527  	.finalize_features = vm_finalize_features,
66846048 Rick Jones         2011-11-14  528  	.bus_name	= vm_bus_name,
edfd52e6 Pawel Moll         2011-10-24  529  };
edfd52e6 Pawel Moll         2011-10-24  530  
edfd52e6 Pawel Moll         2011-10-24  531  
edfd52e6 Pawel Moll         2011-10-24  532  

:::::: The code at line 524 was first introduced by commit
:::::: edfd52e6367270c90f3fd7cc302b375ffa89f91e virtio: Add platform bus driver for memory mapped virtio device

:::::: TO: Pawel Moll <pawel.moll@arm.com>
:::::: CC: Rusty Russell <rusty@rustcorp.com.au>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 17898 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 2/3] virtio: vp_find_vqs accept channel setting params
  2015-10-26 17:52 ` [PATCH v1 2/3] virtio: vp_find_vqs accept channel setting params Ravi Kerur
  2015-10-26 18:12   ` kbuild test robot
@ 2015-10-26 18:14   ` kbuild test robot
  1 sibling, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2015-10-26 18:14 UTC (permalink / raw)
  To: Ravi Kerur; +Cc: kbuild-all, netdev, jasowang, mst, rusty, Ravi Kerur

[-- Attachment #1: Type: text/plain, Size: 4205 bytes --]

Hi Ravi,

[auto build test ERROR on char-misc/char-misc-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Ravi-Kerur/virtio-net-Using-single-MSIX-IRQ-for-TX-RX-Q-pair/20151027-015503
config: x86_64-randconfig-x016-201543 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: the linux-review/Ravi-Kerur/virtio-net-Using-single-MSIX-IRQ-for-TX-RX-Q-pair/20151027-015503 HEAD 69781953042f14dfe510f90e63b4366d729daf9e builds fine.
      It only hurts bisectibility.

All error/warnings (new ones prefixed by >>):

>> drivers/misc/mic/card/mic_virtio.c:372:14: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
     .find_vqs = mic_find_vqs,
                 ^
   drivers/misc/mic/card/mic_virtio.c:372:14: note: (near initialization for 'mic_vq_config_ops.find_vqs')
--
   drivers/virtio/virtio_pci_modern.c: In function 'vp_modern_find_vqs':
>> drivers/virtio/virtio_pci_modern.c:428:11: error: too many arguments to function 'vp_find_vqs'
     int rc = vp_find_vqs(vdev, nvqs, vqs, callbacks, names,
              ^
   In file included from drivers/virtio/virtio_pci_modern.c:21:0:
   drivers/virtio/virtio_pci_common.h:139:5: note: declared here
    int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
        ^
   drivers/virtio/virtio_pci_modern.c: At top level:
>> drivers/virtio/virtio_pci_modern.c:474:14: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
     .find_vqs = vp_modern_find_vqs,
                 ^
   drivers/virtio/virtio_pci_modern.c:474:14: note: (near initialization for 'virtio_pci_config_nodev_ops.find_vqs')
   drivers/virtio/virtio_pci_modern.c:489:14: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
     .find_vqs = vp_modern_find_vqs,
                 ^
   drivers/virtio/virtio_pci_modern.c:489:14: note: (near initialization for 'virtio_pci_config_ops.find_vqs')

vim +/vp_find_vqs +428 drivers/virtio/virtio_pci_modern.c

   422				      unsigned channels[],
   423				      const char *channel_names[],
   424				      unsigned nchannels)
   425	{
   426		struct virtio_pci_device *vp_dev = to_vp_device(vdev);
   427		struct virtqueue *vq;
 > 428		int rc = vp_find_vqs(vdev, nvqs, vqs, callbacks, names,
   429				     NULL, NULL, 0);
   430	
   431		if (rc)
   432			return rc;
   433	
   434		/* Select and activate all queues. Has to be done last: once we do
   435		 * this, there's no way to go back except reset.
   436		 */
   437		list_for_each_entry(vq, &vdev->vqs, list) {
   438			vp_iowrite16(vq->index, &vp_dev->common->queue_select);
   439			vp_iowrite16(1, &vp_dev->common->queue_enable);
   440		}
   441	
   442		return 0;
   443	}
   444	
   445	static void del_vq(struct virtio_pci_vq_info *info)
   446	{
   447		struct virtqueue *vq = info->vq;
   448		struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
   449	
   450		vp_iowrite16(vq->index, &vp_dev->common->queue_select);
   451	
   452		if (vp_dev->msix_enabled) {
   453			vp_iowrite16(VIRTIO_MSI_NO_VECTOR,
   454				     &vp_dev->common->queue_msix_vector);
   455			/* Flush the write out to device */
   456			vp_ioread16(&vp_dev->common->queue_msix_vector);
   457		}
   458	
   459		if (!vp_dev->notify_base)
   460			pci_iounmap(vp_dev->pci_dev, (void __force __iomem *)vq->priv);
   461	
   462		vring_del_virtqueue(vq);
   463	
   464		free_pages_exact(info->queue, vring_pci_size(info->num));
   465	}
   466	
   467	static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
   468		.get		= NULL,
   469		.set		= NULL,
   470		.generation	= vp_generation,
   471		.get_status	= vp_get_status,
   472		.set_status	= vp_set_status,
   473		.reset		= vp_reset,
 > 474		.find_vqs	= vp_modern_find_vqs,
   475		.del_vqs	= vp_del_vqs,
   476		.get_features	= vp_get_features,
   477		.finalize_features = vp_finalize_features,

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 22770 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 1/3] virtio-net: Using single MSIX IRQ for TX/RX Q pair
  2015-10-26 17:52 [PATCH v1 1/3] virtio-net: Using single MSIX IRQ for TX/RX Q pair Ravi Kerur
  2015-10-26 17:52 ` [PATCH v1 2/3] virtio: vp_find_vqs accept channel setting params Ravi Kerur
  2015-10-26 17:52 ` [PATCH v1 3/3] virtio-pci: Introduce channels Ravi Kerur
@ 2015-10-27  5:11 ` Jason Wang
  2015-10-27 22:13   ` Ravi Kerur
  2015-10-27  8:38 ` Michael S. Tsirkin
  3 siblings, 1 reply; 12+ messages in thread
From: Jason Wang @ 2015-10-27  5:11 UTC (permalink / raw)
  To: Ravi Kerur, netdev; +Cc: mst, rusty



On 10/27/2015 01:52 AM, Ravi Kerur wrote:
> Ported earlier patch from Jason Wang (dated 12/26/2014).
>
> This patch tries to reduce the number of MSIX irqs required for
> virtio-net by sharing a MSIX irq for each TX/RX queue pair through
> channels. If transport support channel, about half of the MSIX irqs
> were reduced.
>
> Signed-off-by: Ravi Kerur <rkerur@gmail.com>
> ---
>  drivers/net/virtio_net.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)

Thanks for the patches. Some minor comments:

- If there's no big changes of the code, better keep my sign-offs :)
- Rusty does not like the name "channels", so better rename it to
"virtqueue groups"
- Build bot reports some compiling issues, this need to be fixed in next
version.
- The order of patches in this series is reversed, pach 1/3 should be
3/3. And better to have a cover letter to describe the motivation and
changes since last series. (You can do this through git format-patch
--cover)
- Michale's comment about unnecessary wakeup of tx queue needs to be
addressed, otherwise, we may get unnecessary tx interrupts.
- Some benchmarks is needed to make sure there's no performance regression.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index d8838ded..d705cce 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -72,6 +72,9 @@ struct send_queue {
>  
>  	/* Name of the send queue: output.$index */
>  	char name[40];
> +
> +	/* Name of the channel, shared with irq. */
> +	char channel_name[40];
>  };
>  
>  /* Internal representation of a receive virtqueue */
> @@ -1529,6 +1532,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>  	int ret = -ENOMEM;
>  	int i, total_vqs;
>  	const char **names;
> +	const char **channel_names;
> +	unsigned *channels;
>  
>  	/* We expect 1 RX virtqueue followed by 1 TX virtqueue, followed by
>  	 * possible N-1 RX/TX queue pairs used in multiqueue mode, followed by
> @@ -1548,6 +1553,17 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>  	if (!names)
>  		goto err_names;
>  
> +	channel_names = kmalloc_array(vi->max_queue_pairs,
> +				      sizeof(*channel_names),
> +				      GFP_KERNEL);
> +	if (!channel_names)
> +		goto err_channel_names;
> +
> +	channels = kmalloc_array(total_vqs, sizeof(*channels),
> +				 GFP_KERNEL);
> +	if (!channels)
> +		goto err_channels;
> +
>  	/* Parameters for control virtqueue, if any */
>  	if (vi->has_cvq) {
>  		callbacks[total_vqs - 1] = NULL;
> @@ -1562,10 +1578,15 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>  		sprintf(vi->sq[i].name, "output.%d", i);
>  		names[rxq2vq(i)] = vi->rq[i].name;
>  		names[txq2vq(i)] = vi->sq[i].name;
> +		sprintf(vi->sq[i].channel_name, "txrx.%d", i);
> +		channel_names[i] = vi->sq[i].channel_name;
> +		channels[rxq2vq(i)] = i;
> +		channels[txq2vq(i)] = i;
>  	}
>  
>  	ret = vi->vdev->config->find_vqs(vi->vdev, total_vqs, vqs, callbacks,
> -					 names);
> +					 names, channels, channel_names,
> +					 vi->max_queue_pairs);
>  	if (ret)
>  		goto err_find;
>  
> @@ -1580,6 +1601,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>  		vi->sq[i].vq = vqs[txq2vq(i)];
>  	}
>  
> +	kfree(channels);
> +	kfree(channel_names);
>  	kfree(names);
>  	kfree(callbacks);
>  	kfree(vqs);
> @@ -1587,6 +1610,10 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>  	return 0;
>  
>  err_find:
> +	kfree(channels);
> +err_channels:
> +	kfree(channel_names);
> +err_channel_names:
>  	kfree(names);
>  err_names:
>  	kfree(callbacks);

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 1/3] virtio-net: Using single MSIX IRQ for TX/RX Q pair
  2015-10-26 17:52 [PATCH v1 1/3] virtio-net: Using single MSIX IRQ for TX/RX Q pair Ravi Kerur
                   ` (2 preceding siblings ...)
  2015-10-27  5:11 ` [PATCH v1 1/3] virtio-net: Using single MSIX IRQ for TX/RX Q pair Jason Wang
@ 2015-10-27  8:38 ` Michael S. Tsirkin
  2015-10-27 22:17   ` Ravi Kerur
  2015-10-28  3:13   ` Jason Wang
  3 siblings, 2 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2015-10-27  8:38 UTC (permalink / raw)
  To: Ravi Kerur; +Cc: netdev, jasowang, rusty

On Mon, Oct 26, 2015 at 10:52:47AM -0700, Ravi Kerur wrote:
> Ported earlier patch from Jason Wang (dated 12/26/2014).
> 
> This patch tries to reduce the number of MSIX irqs required for
> virtio-net by sharing a MSIX irq for each TX/RX queue pair through
> channels. If transport support channel, about half of the MSIX irqs
> were reduced.
> 
> Signed-off-by: Ravi Kerur <rkerur@gmail.com>

Why bother BTW? Looks like this is adding a bunch of overhead
on data path - to what end?

Maybe you have a huge number of these devices ... but in that case, how
about sharing the config interrupt instead?
That's only possible if host supports VIRTIO_1
(so we can detect config interrupt by reading the ISR).



> ---
>  drivers/net/virtio_net.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index d8838ded..d705cce 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -72,6 +72,9 @@ struct send_queue {
>  
>  	/* Name of the send queue: output.$index */
>  	char name[40];
> +
> +	/* Name of the channel, shared with irq. */
> +	char channel_name[40];
>  };
>  
>  /* Internal representation of a receive virtqueue */
> @@ -1529,6 +1532,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>  	int ret = -ENOMEM;
>  	int i, total_vqs;
>  	const char **names;
> +	const char **channel_names;
> +	unsigned *channels;
>  
>  	/* We expect 1 RX virtqueue followed by 1 TX virtqueue, followed by
>  	 * possible N-1 RX/TX queue pairs used in multiqueue mode, followed by
> @@ -1548,6 +1553,17 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>  	if (!names)
>  		goto err_names;
>  
> +	channel_names = kmalloc_array(vi->max_queue_pairs,
> +				      sizeof(*channel_names),
> +				      GFP_KERNEL);
> +	if (!channel_names)
> +		goto err_channel_names;
> +
> +	channels = kmalloc_array(total_vqs, sizeof(*channels),
> +				 GFP_KERNEL);
> +	if (!channels)
> +		goto err_channels;
> +
>  	/* Parameters for control virtqueue, if any */
>  	if (vi->has_cvq) {
>  		callbacks[total_vqs - 1] = NULL;
> @@ -1562,10 +1578,15 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>  		sprintf(vi->sq[i].name, "output.%d", i);
>  		names[rxq2vq(i)] = vi->rq[i].name;
>  		names[txq2vq(i)] = vi->sq[i].name;
> +		sprintf(vi->sq[i].channel_name, "txrx.%d", i);
> +		channel_names[i] = vi->sq[i].channel_name;
> +		channels[rxq2vq(i)] = i;
> +		channels[txq2vq(i)] = i;
>  	}
>  
>  	ret = vi->vdev->config->find_vqs(vi->vdev, total_vqs, vqs, callbacks,
> -					 names);
> +					 names, channels, channel_names,
> +					 vi->max_queue_pairs);
>  	if (ret)
>  		goto err_find;
>  
> @@ -1580,6 +1601,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>  		vi->sq[i].vq = vqs[txq2vq(i)];
>  	}
>  
> +	kfree(channels);
> +	kfree(channel_names);
>  	kfree(names);
>  	kfree(callbacks);
>  	kfree(vqs);
> @@ -1587,6 +1610,10 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>  	return 0;
>  
>  err_find:
> +	kfree(channels);
> +err_channels:
> +	kfree(channel_names);
> +err_channel_names:
>  	kfree(names);
>  err_names:
>  	kfree(callbacks);
> -- 
> 1.9.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 1/3] virtio-net: Using single MSIX IRQ for TX/RX Q pair
  2015-10-27  5:11 ` [PATCH v1 1/3] virtio-net: Using single MSIX IRQ for TX/RX Q pair Jason Wang
@ 2015-10-27 22:13   ` Ravi Kerur
  0 siblings, 0 replies; 12+ messages in thread
From: Ravi Kerur @ 2015-10-27 22:13 UTC (permalink / raw)
  To: Jason Wang, netdev; +Cc: mst, rusty



On 10/26/2015 10:11 PM, Jason Wang wrote:
> 
> 
> On 10/27/2015 01:52 AM, Ravi Kerur wrote:
>> Ported earlier patch from Jason Wang (dated 12/26/2014).
>>
>> This patch tries to reduce the number of MSIX irqs required for
>> virtio-net by sharing a MSIX irq for each TX/RX queue pair through
>> channels. If transport support channel, about half of the MSIX irqs
>> were reduced.
>>
>> Signed-off-by: Ravi Kerur <rkerur@gmail.com>
>> ---
>>  drivers/net/virtio_net.c | 29 ++++++++++++++++++++++++++++-
>>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> Thanks for the patches. Some minor comments:
> 
> - If there's no big changes of the code, better keep my sign-offs :)

Sorry for that. Will fix it in 'v2'

> - Rusty does not like the name "channels", so better rename it to
> "virtqueue groups"
> - Build bot reports some compiling issues, this need to be fixed in next
> version.

I saw build failure email, it was reported against 2nd patch. All 3 patches need to be applied for successful build. Will look into it and fix any issues.

> - The order of patches in this series is reversed, pach 1/3 should be
> 3/3. And better to have a cover letter to describe the motivation and
> changes since last series. (You can do this through git format-patch
> --cover)
> - Michale's comment about unnecessary wakeup of tx queue needs to be
> addressed, otherwise, we may get unnecessary tx interrupts.

I am working on it, will take care of above comments as well in 'v2'

> - Some benchmarks is needed to make sure there's no performance regression.
> 
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index d8838ded..d705cce 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -72,6 +72,9 @@ struct send_queue {
>>  
>>  	/* Name of the send queue: output.$index */
>>  	char name[40];
>> +
>> +	/* Name of the channel, shared with irq. */
>> +	char channel_name[40];
>>  };
>>  
>>  /* Internal representation of a receive virtqueue */
>> @@ -1529,6 +1532,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>  	int ret = -ENOMEM;
>>  	int i, total_vqs;
>>  	const char **names;
>> +	const char **channel_names;
>> +	unsigned *channels;
>>  
>>  	/* We expect 1 RX virtqueue followed by 1 TX virtqueue, followed by
>>  	 * possible N-1 RX/TX queue pairs used in multiqueue mode, followed by
>> @@ -1548,6 +1553,17 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>  	if (!names)
>>  		goto err_names;
>>  
>> +	channel_names = kmalloc_array(vi->max_queue_pairs,
>> +				      sizeof(*channel_names),
>> +				      GFP_KERNEL);
>> +	if (!channel_names)
>> +		goto err_channel_names;
>> +
>> +	channels = kmalloc_array(total_vqs, sizeof(*channels),
>> +				 GFP_KERNEL);
>> +	if (!channels)
>> +		goto err_channels;
>> +
>>  	/* Parameters for control virtqueue, if any */
>>  	if (vi->has_cvq) {
>>  		callbacks[total_vqs - 1] = NULL;
>> @@ -1562,10 +1578,15 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>  		sprintf(vi->sq[i].name, "output.%d", i);
>>  		names[rxq2vq(i)] = vi->rq[i].name;
>>  		names[txq2vq(i)] = vi->sq[i].name;
>> +		sprintf(vi->sq[i].channel_name, "txrx.%d", i);
>> +		channel_names[i] = vi->sq[i].channel_name;
>> +		channels[rxq2vq(i)] = i;
>> +		channels[txq2vq(i)] = i;
>>  	}
>>  
>>  	ret = vi->vdev->config->find_vqs(vi->vdev, total_vqs, vqs, callbacks,
>> -					 names);
>> +					 names, channels, channel_names,
>> +					 vi->max_queue_pairs);
>>  	if (ret)
>>  		goto err_find;
>>  
>> @@ -1580,6 +1601,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>  		vi->sq[i].vq = vqs[txq2vq(i)];
>>  	}
>>  
>> +	kfree(channels);
>> +	kfree(channel_names);
>>  	kfree(names);
>>  	kfree(callbacks);
>>  	kfree(vqs);
>> @@ -1587,6 +1610,10 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>  	return 0;
>>  
>>  err_find:
>> +	kfree(channels);
>> +err_channels:
>> +	kfree(channel_names);
>> +err_channel_names:
>>  	kfree(names);
>>  err_names:
>>  	kfree(callbacks);
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 1/3] virtio-net: Using single MSIX IRQ for TX/RX Q pair
  2015-10-27  8:38 ` Michael S. Tsirkin
@ 2015-10-27 22:17   ` Ravi Kerur
  2015-10-28  3:13   ` Jason Wang
  1 sibling, 0 replies; 12+ messages in thread
From: Ravi Kerur @ 2015-10-27 22:17 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: netdev, jasowang, rusty



On 10/27/2015 1:38 AM, Michael S. Tsirkin wrote:
> On Mon, Oct 26, 2015 at 10:52:47AM -0700, Ravi Kerur wrote:
>> Ported earlier patch from Jason Wang (dated 12/26/2014).
>>
>> This patch tries to reduce the number of MSIX irqs required for
>> virtio-net by sharing a MSIX irq for each TX/RX queue pair through
>> channels. If transport support channel, about half of the MSIX irqs
>> were reduced.
>>
>> Signed-off-by: Ravi Kerur <rkerur@gmail.com>
> 
> Why bother BTW? Looks like this is adding a bunch of overhead
> on data path - to what end?
> 
> Maybe you have a huge number of these devices ... but in that case, how
> about sharing the config interrupt instead?
> That's only possible if host supports VIRTIO_1
> (so we can detect config interrupt by reading the ISR).

For my clarification, are you suggesting this as an additional changes for config interrupts or rework existing patch?
> 
> 
> 
>> ---
>>  drivers/net/virtio_net.c | 29 ++++++++++++++++++++++++++++-
>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index d8838ded..d705cce 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -72,6 +72,9 @@ struct send_queue {
>>  
>>  	/* Name of the send queue: output.$index */
>>  	char name[40];
>> +
>> +	/* Name of the channel, shared with irq. */
>> +	char channel_name[40];
>>  };
>>  
>>  /* Internal representation of a receive virtqueue */
>> @@ -1529,6 +1532,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>  	int ret = -ENOMEM;
>>  	int i, total_vqs;
>>  	const char **names;
>> +	const char **channel_names;
>> +	unsigned *channels;
>>  
>>  	/* We expect 1 RX virtqueue followed by 1 TX virtqueue, followed by
>>  	 * possible N-1 RX/TX queue pairs used in multiqueue mode, followed by
>> @@ -1548,6 +1553,17 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>  	if (!names)
>>  		goto err_names;
>>  
>> +	channel_names = kmalloc_array(vi->max_queue_pairs,
>> +				      sizeof(*channel_names),
>> +				      GFP_KERNEL);
>> +	if (!channel_names)
>> +		goto err_channel_names;
>> +
>> +	channels = kmalloc_array(total_vqs, sizeof(*channels),
>> +				 GFP_KERNEL);
>> +	if (!channels)
>> +		goto err_channels;
>> +
>>  	/* Parameters for control virtqueue, if any */
>>  	if (vi->has_cvq) {
>>  		callbacks[total_vqs - 1] = NULL;
>> @@ -1562,10 +1578,15 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>  		sprintf(vi->sq[i].name, "output.%d", i);
>>  		names[rxq2vq(i)] = vi->rq[i].name;
>>  		names[txq2vq(i)] = vi->sq[i].name;
>> +		sprintf(vi->sq[i].channel_name, "txrx.%d", i);
>> +		channel_names[i] = vi->sq[i].channel_name;
>> +		channels[rxq2vq(i)] = i;
>> +		channels[txq2vq(i)] = i;
>>  	}
>>  
>>  	ret = vi->vdev->config->find_vqs(vi->vdev, total_vqs, vqs, callbacks,
>> -					 names);
>> +					 names, channels, channel_names,
>> +					 vi->max_queue_pairs);
>>  	if (ret)
>>  		goto err_find;
>>  
>> @@ -1580,6 +1601,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>  		vi->sq[i].vq = vqs[txq2vq(i)];
>>  	}
>>  
>> +	kfree(channels);
>> +	kfree(channel_names);
>>  	kfree(names);
>>  	kfree(callbacks);
>>  	kfree(vqs);
>> @@ -1587,6 +1610,10 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>  	return 0;
>>  
>>  err_find:
>> +	kfree(channels);
>> +err_channels:
>> +	kfree(channel_names);
>> +err_channel_names:
>>  	kfree(names);
>>  err_names:
>>  	kfree(callbacks);
>> -- 
>> 1.9.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 1/3] virtio-net: Using single MSIX IRQ for TX/RX Q pair
  2015-10-27  8:38 ` Michael S. Tsirkin
  2015-10-27 22:17   ` Ravi Kerur
@ 2015-10-28  3:13   ` Jason Wang
  2015-10-28  7:21     ` Michael S. Tsirkin
  1 sibling, 1 reply; 12+ messages in thread
From: Jason Wang @ 2015-10-28  3:13 UTC (permalink / raw)
  To: Michael S. Tsirkin, Ravi Kerur; +Cc: netdev, rusty



On 10/27/2015 04:38 PM, Michael S. Tsirkin wrote:
> On Mon, Oct 26, 2015 at 10:52:47AM -0700, Ravi Kerur wrote:
>> Ported earlier patch from Jason Wang (dated 12/26/2014).
>>
>> This patch tries to reduce the number of MSIX irqs required for
>> virtio-net by sharing a MSIX irq for each TX/RX queue pair through
>> channels. If transport support channel, about half of the MSIX irqs
>> were reduced.
>>
>> Signed-off-by: Ravi Kerur <rkerur@gmail.com>
> Why bother BTW? 

The reason is we want to save the number of interrupt vectors used.
Booting a guest with 256 queues with current driver will result all
tx/rx queues shares a single vector. This is suboptimal. With this
series, half could be saved. And more complex policy could be applied on
top (e.g limit the number of vectors used by driver).

> Looks like this is adding a bunch of overhead
> on data path - to what end?

I agree some benchmark is needed for this.

> Maybe you have a huge number of these devices ... but in that case, how
> about sharing the config interrupt instead?
> That's only possible if host supports VIRTIO_1
> (so we can detect config interrupt by reading the ISR).
>
>
>
>> ---
>>  drivers/net/virtio_net.c | 29 ++++++++++++++++++++++++++++-
>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index d8838ded..d705cce 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -72,6 +72,9 @@ struct send_queue {
>>  
>>  	/* Name of the send queue: output.$index */
>>  	char name[40];
>> +
>> +	/* Name of the channel, shared with irq. */
>> +	char channel_name[40];
>>  };
>>  
>>  /* Internal representation of a receive virtqueue */
>> @@ -1529,6 +1532,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>  	int ret = -ENOMEM;
>>  	int i, total_vqs;
>>  	const char **names;
>> +	const char **channel_names;
>> +	unsigned *channels;
>>  
>>  	/* We expect 1 RX virtqueue followed by 1 TX virtqueue, followed by
>>  	 * possible N-1 RX/TX queue pairs used in multiqueue mode, followed by
>> @@ -1548,6 +1553,17 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>  	if (!names)
>>  		goto err_names;
>>  
>> +	channel_names = kmalloc_array(vi->max_queue_pairs,
>> +				      sizeof(*channel_names),
>> +				      GFP_KERNEL);
>> +	if (!channel_names)
>> +		goto err_channel_names;
>> +
>> +	channels = kmalloc_array(total_vqs, sizeof(*channels),
>> +				 GFP_KERNEL);
>> +	if (!channels)
>> +		goto err_channels;
>> +
>>  	/* Parameters for control virtqueue, if any */
>>  	if (vi->has_cvq) {
>>  		callbacks[total_vqs - 1] = NULL;
>> @@ -1562,10 +1578,15 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>  		sprintf(vi->sq[i].name, "output.%d", i);
>>  		names[rxq2vq(i)] = vi->rq[i].name;
>>  		names[txq2vq(i)] = vi->sq[i].name;
>> +		sprintf(vi->sq[i].channel_name, "txrx.%d", i);
>> +		channel_names[i] = vi->sq[i].channel_name;
>> +		channels[rxq2vq(i)] = i;
>> +		channels[txq2vq(i)] = i;
>>  	}
>>  
>>  	ret = vi->vdev->config->find_vqs(vi->vdev, total_vqs, vqs, callbacks,
>> -					 names);
>> +					 names, channels, channel_names,
>> +					 vi->max_queue_pairs);
>>  	if (ret)
>>  		goto err_find;
>>  
>> @@ -1580,6 +1601,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>  		vi->sq[i].vq = vqs[txq2vq(i)];
>>  	}
>>  
>> +	kfree(channels);
>> +	kfree(channel_names);
>>  	kfree(names);
>>  	kfree(callbacks);
>>  	kfree(vqs);
>> @@ -1587,6 +1610,10 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>  	return 0;
>>  
>>  err_find:
>> +	kfree(channels);
>> +err_channels:
>> +	kfree(channel_names);
>> +err_channel_names:
>>  	kfree(names);
>>  err_names:
>>  	kfree(callbacks);
>> -- 
>> 1.9.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 1/3] virtio-net: Using single MSIX IRQ for TX/RX Q pair
  2015-10-28  3:13   ` Jason Wang
@ 2015-10-28  7:21     ` Michael S. Tsirkin
  2015-10-28  7:54       ` Jason Wang
  0 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2015-10-28  7:21 UTC (permalink / raw)
  To: Jason Wang; +Cc: Ravi Kerur, netdev, rusty

On Wed, Oct 28, 2015 at 11:13:39AM +0800, Jason Wang wrote:
> 
> 
> On 10/27/2015 04:38 PM, Michael S. Tsirkin wrote:
> > On Mon, Oct 26, 2015 at 10:52:47AM -0700, Ravi Kerur wrote:
> >> Ported earlier patch from Jason Wang (dated 12/26/2014).
> >>
> >> This patch tries to reduce the number of MSIX irqs required for
> >> virtio-net by sharing a MSIX irq for each TX/RX queue pair through
> >> channels. If transport support channel, about half of the MSIX irqs
> >> were reduced.
> >>
> >> Signed-off-by: Ravi Kerur <rkerur@gmail.com>
> > Why bother BTW? 
> 
> The reason is we want to save the number of interrupt vectors used.
> Booting a guest with 256 queues with current driver will result all
> tx/rx queues shares a single vector. This is suboptimal.

With a single CPU? But what configures so many queues? Why do it?

> With this
> series, half could be saved.

At cost of e.g. inability to balance the interrupts.

> And more complex policy could be applied on
> top (e.g limit the number of vectors used by driver).

If that's the motivation, I'd like to see a draft of that more complex
policy first.

> > Looks like this is adding a bunch of overhead
> > on data path - to what end?
> 
> I agree some benchmark is needed for this.
> 
> > Maybe you have a huge number of these devices ... but in that case, how
> > about sharing the config interrupt instead?
> > That's only possible if host supports VIRTIO_1
> > (so we can detect config interrupt by reading the ISR).
> >
> >
> >
> >> ---
> >>  drivers/net/virtio_net.c | 29 ++++++++++++++++++++++++++++-
> >>  1 file changed, 28 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >> index d8838ded..d705cce 100644
> >> --- a/drivers/net/virtio_net.c
> >> +++ b/drivers/net/virtio_net.c
> >> @@ -72,6 +72,9 @@ struct send_queue {
> >>  
> >>  	/* Name of the send queue: output.$index */
> >>  	char name[40];
> >> +
> >> +	/* Name of the channel, shared with irq. */
> >> +	char channel_name[40];
> >>  };
> >>  
> >>  /* Internal representation of a receive virtqueue */
> >> @@ -1529,6 +1532,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> >>  	int ret = -ENOMEM;
> >>  	int i, total_vqs;
> >>  	const char **names;
> >> +	const char **channel_names;
> >> +	unsigned *channels;
> >>  
> >>  	/* We expect 1 RX virtqueue followed by 1 TX virtqueue, followed by
> >>  	 * possible N-1 RX/TX queue pairs used in multiqueue mode, followed by
> >> @@ -1548,6 +1553,17 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> >>  	if (!names)
> >>  		goto err_names;
> >>  
> >> +	channel_names = kmalloc_array(vi->max_queue_pairs,
> >> +				      sizeof(*channel_names),
> >> +				      GFP_KERNEL);
> >> +	if (!channel_names)
> >> +		goto err_channel_names;
> >> +
> >> +	channels = kmalloc_array(total_vqs, sizeof(*channels),
> >> +				 GFP_KERNEL);
> >> +	if (!channels)
> >> +		goto err_channels;
> >> +
> >>  	/* Parameters for control virtqueue, if any */
> >>  	if (vi->has_cvq) {
> >>  		callbacks[total_vqs - 1] = NULL;
> >> @@ -1562,10 +1578,15 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> >>  		sprintf(vi->sq[i].name, "output.%d", i);
> >>  		names[rxq2vq(i)] = vi->rq[i].name;
> >>  		names[txq2vq(i)] = vi->sq[i].name;
> >> +		sprintf(vi->sq[i].channel_name, "txrx.%d", i);
> >> +		channel_names[i] = vi->sq[i].channel_name;
> >> +		channels[rxq2vq(i)] = i;
> >> +		channels[txq2vq(i)] = i;
> >>  	}
> >>  
> >>  	ret = vi->vdev->config->find_vqs(vi->vdev, total_vqs, vqs, callbacks,
> >> -					 names);
> >> +					 names, channels, channel_names,
> >> +					 vi->max_queue_pairs);
> >>  	if (ret)
> >>  		goto err_find;
> >>  
> >> @@ -1580,6 +1601,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> >>  		vi->sq[i].vq = vqs[txq2vq(i)];
> >>  	}
> >>  
> >> +	kfree(channels);
> >> +	kfree(channel_names);
> >>  	kfree(names);
> >>  	kfree(callbacks);
> >>  	kfree(vqs);
> >> @@ -1587,6 +1610,10 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> >>  	return 0;
> >>  
> >>  err_find:
> >> +	kfree(channels);
> >> +err_channels:
> >> +	kfree(channel_names);
> >> +err_channel_names:
> >>  	kfree(names);
> >>  err_names:
> >>  	kfree(callbacks);
> >> -- 
> >> 1.9.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 1/3] virtio-net: Using single MSIX IRQ for TX/RX Q pair
  2015-10-28  7:21     ` Michael S. Tsirkin
@ 2015-10-28  7:54       ` Jason Wang
  0 siblings, 0 replies; 12+ messages in thread
From: Jason Wang @ 2015-10-28  7:54 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Ravi Kerur, netdev, rusty



On 10/28/2015 03:21 PM, Michael S. Tsirkin wrote:
> On Wed, Oct 28, 2015 at 11:13:39AM +0800, Jason Wang wrote:
>>
>> On 10/27/2015 04:38 PM, Michael S. Tsirkin wrote:
>>> On Mon, Oct 26, 2015 at 10:52:47AM -0700, Ravi Kerur wrote:
>>>> Ported earlier patch from Jason Wang (dated 12/26/2014).
>>>>
>>>> This patch tries to reduce the number of MSIX irqs required for
>>>> virtio-net by sharing a MSIX irq for each TX/RX queue pair through
>>>> channels. If transport support channel, about half of the MSIX irqs
>>>> were reduced.
>>>>
>>>> Signed-off-by: Ravi Kerur <rkerur@gmail.com>
>>> Why bother BTW? 
>> The reason is we want to save the number of interrupt vectors used.
>> Booting a guest with 256 queues with current driver will result all
>> tx/rx queues shares a single vector. This is suboptimal.
> With a single CPU? 

Even for smp guests. Or you want a per-cpu interrupt?

> But what configures so many queues? Why do it?

Something like cpu hot add.

>
>> With this
>> series, half could be saved.
> At cost of e.g. inability to balance the interrupts.

Didn't follow. Btw, most psychical cards shares irq with tx/rx queue pair.

>
>> And more complex policy could be applied on
>> top (e.g limit the number of vectors used by driver).
> If that's the motivation, I'd like to see a draft of that more complex
> policy first.

How about something like:

1) Driver provides a min and max number of vectors it needs.
2) Virtio pci can then use pci_enable_msix_range() and return the actual
number of vectors to driver.
3) Then driver can divide the virtqueues into different groups

>
>>> Looks like this is adding a bunch of overhead
>>> on data path - to what end?
>> I agree some benchmark is needed for this.
>>
>>> Maybe you have a huge number of these devices ... but in that case, how
>>> about sharing the config interrupt instead?
>>> That's only possible if host supports VIRTIO_1
>>> (so we can detect config interrupt by reading the ISR).
>>>
>>>
>>>
>>>> ---
>>>>  drivers/net/virtio_net.c | 29 ++++++++++++++++++++++++++++-
>>>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index d8838ded..d705cce 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -72,6 +72,9 @@ struct send_queue {
>>>>  
>>>>  	/* Name of the send queue: output.$index */
>>>>  	char name[40];
>>>> +
>>>> +	/* Name of the channel, shared with irq. */
>>>> +	char channel_name[40];
>>>>  };
>>>>  
>>>>  /* Internal representation of a receive virtqueue */
>>>> @@ -1529,6 +1532,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>>>  	int ret = -ENOMEM;
>>>>  	int i, total_vqs;
>>>>  	const char **names;
>>>> +	const char **channel_names;
>>>> +	unsigned *channels;
>>>>  
>>>>  	/* We expect 1 RX virtqueue followed by 1 TX virtqueue, followed by
>>>>  	 * possible N-1 RX/TX queue pairs used in multiqueue mode, followed by
>>>> @@ -1548,6 +1553,17 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>>>  	if (!names)
>>>>  		goto err_names;
>>>>  
>>>> +	channel_names = kmalloc_array(vi->max_queue_pairs,
>>>> +				      sizeof(*channel_names),
>>>> +				      GFP_KERNEL);
>>>> +	if (!channel_names)
>>>> +		goto err_channel_names;
>>>> +
>>>> +	channels = kmalloc_array(total_vqs, sizeof(*channels),
>>>> +				 GFP_KERNEL);
>>>> +	if (!channels)
>>>> +		goto err_channels;
>>>> +
>>>>  	/* Parameters for control virtqueue, if any */
>>>>  	if (vi->has_cvq) {
>>>>  		callbacks[total_vqs - 1] = NULL;
>>>> @@ -1562,10 +1578,15 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>>>  		sprintf(vi->sq[i].name, "output.%d", i);
>>>>  		names[rxq2vq(i)] = vi->rq[i].name;
>>>>  		names[txq2vq(i)] = vi->sq[i].name;
>>>> +		sprintf(vi->sq[i].channel_name, "txrx.%d", i);
>>>> +		channel_names[i] = vi->sq[i].channel_name;
>>>> +		channels[rxq2vq(i)] = i;
>>>> +		channels[txq2vq(i)] = i;
>>>>  	}
>>>>  
>>>>  	ret = vi->vdev->config->find_vqs(vi->vdev, total_vqs, vqs, callbacks,
>>>> -					 names);
>>>> +					 names, channels, channel_names,
>>>> +					 vi->max_queue_pairs);
>>>>  	if (ret)
>>>>  		goto err_find;
>>>>  
>>>> @@ -1580,6 +1601,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>>>  		vi->sq[i].vq = vqs[txq2vq(i)];
>>>>  	}
>>>>  
>>>> +	kfree(channels);
>>>> +	kfree(channel_names);
>>>>  	kfree(names);
>>>>  	kfree(callbacks);
>>>>  	kfree(vqs);
>>>> @@ -1587,6 +1610,10 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>>>  	return 0;
>>>>  
>>>>  err_find:
>>>> +	kfree(channels);
>>>> +err_channels:
>>>> +	kfree(channel_names);
>>>> +err_channel_names:
>>>>  	kfree(names);
>>>>  err_names:
>>>>  	kfree(callbacks);
>>>> -- 
>>>> 1.9.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2015-10-28  7:54 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-26 17:52 [PATCH v1 1/3] virtio-net: Using single MSIX IRQ for TX/RX Q pair Ravi Kerur
2015-10-26 17:52 ` [PATCH v1 2/3] virtio: vp_find_vqs accept channel setting params Ravi Kerur
2015-10-26 18:12   ` kbuild test robot
2015-10-26 18:14   ` kbuild test robot
2015-10-26 17:52 ` [PATCH v1 3/3] virtio-pci: Introduce channels Ravi Kerur
2015-10-27  5:11 ` [PATCH v1 1/3] virtio-net: Using single MSIX IRQ for TX/RX Q pair Jason Wang
2015-10-27 22:13   ` Ravi Kerur
2015-10-27  8:38 ` Michael S. Tsirkin
2015-10-27 22:17   ` Ravi Kerur
2015-10-28  3:13   ` Jason Wang
2015-10-28  7:21     ` Michael S. Tsirkin
2015-10-28  7:54       ` Jason Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.