[RFC PATCH 0/3] Sharing MSIX irq for tx/rx queue pairs

[RFC PATCH 0/3] Sharing MSIX irq for tx/rx queue pairs

[Jason Wang]

Hi all:

This series try to share MSIX irq for each tx/rx queue pair. This is
done through:

- introducing virtio pci channel which are group of virtqueues that
  sharing a single MSIX irq (Patch 1)
- expose channel setting to virtio core api (Patch 2)
- try to use channel setting in virtio-net (Patch 3)

For the transport that does not support channel, channel paramters
were simply ignored. For devices that does not use channel, it can
simply pass NULL or zero to virito core.

With the patch, 1 MSIX irq were saved for each TX/RX queue pair.

Please review.

Thanks

Jason Wang (3):
  virtio-pci: introduce channels
  virtio: let vp_find_vqs accept channel setting paramters
  virtio-net: using single MSIX irq for each TX/RX queue pair

 drivers/block/virtio_blk.c             |   3 +-
 drivers/char/virtio_console.c          |   3 +-
 drivers/lguest/lguest_device.c         |   5 +-
 drivers/misc/mic/card/mic_virtio.c     |   5 +-
 drivers/net/caif/caif_virtio.c         |   3 +-
 drivers/net/virtio_net.c               |  26 ++++-
 drivers/remoteproc/remoteproc_virtio.c |   5 +-
 drivers/rpmsg/virtio_rpmsg_bus.c       |   3 +-
 drivers/s390/kvm/kvm_virtio.c          |   5 +-
 drivers/s390/kvm/virtio_ccw.c          |   5 +-
 drivers/scsi/virtio_scsi.c             |   3 +-
 drivers/virtio/virtio_balloon.c        |   3 +-
 drivers/virtio/virtio_mmio.c           |   5 +-
 drivers/virtio/virtio_pci_common.c     | 207 ++++++++++++++++++++-------------
 drivers/virtio/virtio_pci_common.h     |  19 ++-
 include/linux/virtio_config.h          |   8 +-
 16 files changed, 208 insertions(+), 100 deletions(-)

-- 
1.8.3.1

[RFC PATCH 1/3] virtio-pci: introduce channels

[Jason Wang]

This patch introduce 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.

Virtio-net will be the first user.

Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio_pci_common.c | 193 ++++++++++++++++++++++---------------
 drivers/virtio/virtio_pci_common.h |  14 ++-
 2 files changed, 124 insertions(+), 83 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 2ef9529..36db4ac 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -68,6 +68,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
@@ -104,8 +121,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])
@@ -119,6 +140,7 @@ static void vp_free_vectors(struct virtio_device *vdev)
 		vp_dev->msix_enabled = 0;
 	}
 
+
 	vp_dev->msix_vectors = 0;
 	vp_dev->msix_used_vectors = 0;
 	kfree(vp_dev->msix_names);
@@ -129,8 +151,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);
@@ -167,8 +188,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],
@@ -184,18 +205,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);
@@ -220,6 +229,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;
@@ -234,9 +244,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);
 	}
@@ -249,15 +266,16 @@ 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);
@@ -267,94 +285,98 @@ 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);
+	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;
+	}
 
-		err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
+	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]);
 		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;
 
@@ -369,20 +391,33 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 		vq_callback_t *callbacks[],
 		const char *names[])
 {
-	int err;
+	int err, i;
+	unsigned *channels = kmalloc_array(nvqs, sizeof(unsigned), GFP_KERNEL);
+	const char *cnames[] = {"virtqueues"};
+
+	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 adddb64..ffe8f7a 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;
@@ -66,6 +71,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;
@@ -76,12 +85,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,
-- 
1.8.3.1

[RFC PATCH 2/3] virtio: let vp_find_vqs accept channel setting paramters

[Jason Wang]

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

Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/block/virtio_blk.c             |  3 ++-
 drivers/char/virtio_console.c          |  3 ++-
 drivers/lguest/lguest_device.c         |  5 ++++-
 drivers/misc/mic/card/mic_virtio.c     |  5 ++++-
 drivers/net/caif/caif_virtio.c         |  3 ++-
 drivers/net/virtio_net.c               |  2 +-
 drivers/remoteproc/remoteproc_virtio.c |  5 ++++-
 drivers/rpmsg/virtio_rpmsg_bus.c       |  3 ++-
 drivers/s390/kvm/kvm_virtio.c          |  5 ++++-
 drivers/s390/kvm/virtio_ccw.c          |  5 ++++-
 drivers/scsi/virtio_scsi.c             |  3 ++-
 drivers/virtio/virtio_balloon.c        |  3 ++-
 drivers/virtio/virtio_mmio.c           |  5 ++++-
 drivers/virtio/virtio_pci_common.c     | 16 ++++++++++++++--
 drivers/virtio/virtio_pci_common.h     |  5 ++++-
 include/linux/virtio_config.h          |  8 ++++++--
 16 files changed, 61 insertions(+), 18 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 7ef7c09..7b2a15f 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -419,7 +419,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 de03df9..68ed755 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1911,7 +1911,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/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index 89088d6..f0866dc 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -375,7 +375,10 @@ static void lg_del_vqs(struct virtio_device *vdev)
 static int lg_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 lguest_device *ldev = to_lgdev(vdev);
 	int i;
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/net/virtio_net.c b/drivers/net/virtio_net.c
index 5ca9771..3ba3499 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1558,7 +1558,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
 	}
 
 	ret = vi->vdev->config->find_vqs(vi->vdev, total_vqs, vqs, callbacks,
-					 names);
+					 names, NULL, NULL, 0);
 	if (ret)
 		goto err_find;
 
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index e1a1023..f193b24 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -147,7 +147,10 @@ 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[])
+		       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 92f6af6..3f35ab5 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -964,7 +964,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/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index dd65c8b..96991af 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/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/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 71d7802..9369520 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -617,7 +617,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 c52bb5d..5889a57 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -914,7 +914,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 50c5f42..f3169ed 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -375,7 +375,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_mmio.c b/drivers/virtio/virtio_mmio.c
index 00d115b..a0d40f0 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -393,7 +393,10 @@ error_available:
 static int vm_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_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_common.c b/drivers/virtio/virtio_pci_common.c
index 36db4ac..5b88ba0 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -389,12 +389,23 @@ 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, i;
-	unsigned *channels = kmalloc_array(nvqs, sizeof(unsigned), GFP_KERNEL);
 	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;
 
@@ -405,6 +416,7 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 				 names, nvqs);
 	if (!err)
 		goto out;
+
 	/* Fallback: MSI-X with one vector for config, one shared for queues. */
 	for (i = 0; i < nvqs; i++)
 		channels[i] = 0;
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index ffe8f7a..c16e46e 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -123,7 +123,10 @@ void vp_del_vqs(struct virtio_device *vdev);
 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);
 const char *vp_bus_name(struct virtio_device *vdev);
 
 /* Setup the affinity for a virtqueue:
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index ca3ed78..33fd210 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -70,7 +70,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 +159,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.8.3.1

[RFC PATCH 3/3] virtio-net: using single MSIX irq for each TX/RX queue pair

[Jason Wang]

This patch try 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.

Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/net/virtio_net.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 3ba3499..03a0c28 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, share with rxq */
+	char channel_name[40];
 };
 
 /* Internal representation of a receive virtqueue */
@@ -1522,6 +1525,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
@@ -1540,6 +1545,14 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
 	names = kmalloc(total_vqs * sizeof(*names), GFP_KERNEL);
 	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) {
@@ -1555,10 +1568,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, NULL, NULL, 0);
+					 names, channels, channel_names,
+					 vi->max_queue_pairs);
 	if (ret)
 		goto err_find;
 
@@ -1573,6 +1591,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);
@@ -1580,6 +1600,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.8.3.1

Re: [RFC PATCH 3/3] virtio-net: using single MSIX irq for each TX/RX queue pair

[Jason Wang]

On 12/26/2014 10:53 AM, Jason Wang wrote:
> This patch try 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.
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/net/virtio_net.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
>

Note: an issue with this patch is it may trigger tx irq handler if at
least one more used. We could solve this by checking the used idx and
bypass the tx irq if host does not signal us. (Or just does the old tx
skbs reclaiming before the checking.)

Re: [RFC PATCH 0/3] Sharing MSIX irq for tx/rx queue pairs

[Michael S. Tsirkin]

On Fri, Dec 26, 2014 at 10:53:42AM +0800, Jason Wang wrote:
> Hi all:
> 
> This series try to share MSIX irq for each tx/rx queue pair. This is
> done through:
> 
> - introducing virtio pci channel which are group of virtqueues that
>   sharing a single MSIX irq (Patch 1)
> - expose channel setting to virtio core api (Patch 2)
> - try to use channel setting in virtio-net (Patch 3)
> 
> For the transport that does not support channel, channel paramters
> were simply ignored. For devices that does not use channel, it can
> simply pass NULL or zero to virito core.
> 
> With the patch, 1 MSIX irq were saved for each TX/RX queue pair.
> 
> Please review.

How does this sharing affect performance?


> Thanks
> 
> Jason Wang (3):
>   virtio-pci: introduce channels
>   virtio: let vp_find_vqs accept channel setting paramters
>   virtio-net: using single MSIX irq for each TX/RX queue pair
> 
>  drivers/block/virtio_blk.c             |   3 +-
>  drivers/char/virtio_console.c          |   3 +-
>  drivers/lguest/lguest_device.c         |   5 +-
>  drivers/misc/mic/card/mic_virtio.c     |   5 +-
>  drivers/net/caif/caif_virtio.c         |   3 +-
>  drivers/net/virtio_net.c               |  26 ++++-
>  drivers/remoteproc/remoteproc_virtio.c |   5 +-
>  drivers/rpmsg/virtio_rpmsg_bus.c       |   3 +-
>  drivers/s390/kvm/kvm_virtio.c          |   5 +-
>  drivers/s390/kvm/virtio_ccw.c          |   5 +-
>  drivers/scsi/virtio_scsi.c             |   3 +-
>  drivers/virtio/virtio_balloon.c        |   3 +-
>  drivers/virtio/virtio_mmio.c           |   5 +-
>  drivers/virtio/virtio_pci_common.c     | 207 ++++++++++++++++++++-------------
>  drivers/virtio/virtio_pci_common.h     |  19 ++-
>  include/linux/virtio_config.h          |   8 +-
>  16 files changed, 208 insertions(+), 100 deletions(-)
> 
> -- 
> 1.8.3.1

Re: [RFC PATCH 0/3] Sharing MSIX irq for tx/rx queue pairs

[Jason Wang]

On 12/28/2014 03:52 PM, Michael S. Tsirkin wrote:
> On Fri, Dec 26, 2014 at 10:53:42AM +0800, Jason Wang wrote:
>> Hi all:
>>
>> This series try to share MSIX irq for each tx/rx queue pair. This is
>> done through:
>>
>> - introducing virtio pci channel which are group of virtqueues that
>>   sharing a single MSIX irq (Patch 1)
>> - expose channel setting to virtio core api (Patch 2)
>> - try to use channel setting in virtio-net (Patch 3)
>>
>> For the transport that does not support channel, channel paramters
>> were simply ignored. For devices that does not use channel, it can
>> simply pass NULL or zero to virito core.
>>
>> With the patch, 1 MSIX irq were saved for each TX/RX queue pair.
>>
>> Please review.
> How does this sharing affect performance?
>

Patch 3 only checks more_used() for tx ring which in fact reduces the
effect of event index and may introduce more tx interrupts. After fixing
this issue, tested with 1 vcpu and 1 queue. No obvious changes in
performance were noticed.

Thanks

Re: [RFC PATCH 0/3] Sharing MSIX irq for tx/rx queue pairs

[Michael S. Tsirkin]

On Sun, Jan 04, 2015 at 04:38:17PM +0800, Jason Wang wrote:
> 
> On 12/28/2014 03:52 PM, Michael S. Tsirkin wrote:
> > On Fri, Dec 26, 2014 at 10:53:42AM +0800, Jason Wang wrote:
> >> Hi all:
> >>
> >> This series try to share MSIX irq for each tx/rx queue pair. This is
> >> done through:
> >>
> >> - introducing virtio pci channel which are group of virtqueues that
> >>   sharing a single MSIX irq (Patch 1)
> >> - expose channel setting to virtio core api (Patch 2)
> >> - try to use channel setting in virtio-net (Patch 3)
> >>
> >> For the transport that does not support channel, channel paramters
> >> were simply ignored. For devices that does not use channel, it can
> >> simply pass NULL or zero to virito core.
> >>
> >> With the patch, 1 MSIX irq were saved for each TX/RX queue pair.
> >>
> >> Please review.
> > How does this sharing affect performance?
> >
> 
> Patch 3 only checks more_used() for tx ring which in fact reduces the
> effect of event index and may introduce more tx interrupts. After fixing
> this issue, tested with 1 vcpu and 1 queue. No obvious changes in
> performance were noticed.
> 
> Thanks

Is this with or without MQ?
With MQ, it seems easy to believe as interrupts are
distributed between CPUs.

Without MQ, it should be possible to create UDP workloads where
processing incoming and outgoing interrupts
on separate CPUs is a win.

-- 
MST

Re: [RFC PATCH 0/3] Sharing MSIX irq for tx/rx queue pairs

[Rusty Russell]

Jason Wang <jasowang@redhat.com> writes:
> Hi all:
>
> This series try to share MSIX irq for each tx/rx queue pair. This is
> done through:
>
> - introducing virtio pci channel which are group of virtqueues that
>   sharing a single MSIX irq (Patch 1)
> - expose channel setting to virtio core api (Patch 2)
> - try to use channel setting in virtio-net (Patch 3)

Hi Jason,

        Is "channel" a term you created yourself, or something I was
just unaware of?  irq_group would seem more obvious, if the former.

> For the transport that does not support channel, channel paramters
> were simply ignored. For devices that does not use channel, it can
> simply pass NULL or zero to virito core.
>
> With the patch, 1 MSIX irq were saved for each TX/RX queue pair.

It seems fairly straightforward.

Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Thanks,
Rusty.

Re: [RFC PATCH 0/3] Sharing MSIX irq for tx/rx queue pairs

[Jason Wang]

On 01/04/2015 07:36 PM, Michael S. Tsirkin wrote:
> On Sun, Jan 04, 2015 at 04:38:17PM +0800, Jason Wang wrote:
>> On 12/28/2014 03:52 PM, Michael S. Tsirkin wrote:
>>> On Fri, Dec 26, 2014 at 10:53:42AM +0800, Jason Wang wrote:
>>>> Hi all:
>>>>
>>>> This series try to share MSIX irq for each tx/rx queue pair. This is
>>>> done through:
>>>>
>>>> - introducing virtio pci channel which are group of virtqueues that
>>>>   sharing a single MSIX irq (Patch 1)
>>>> - expose channel setting to virtio core api (Patch 2)
>>>> - try to use channel setting in virtio-net (Patch 3)
>>>>
>>>> For the transport that does not support channel, channel paramters
>>>> were simply ignored. For devices that does not use channel, it can
>>>> simply pass NULL or zero to virito core.
>>>>
>>>> With the patch, 1 MSIX irq were saved for each TX/RX queue pair.
>>>>
>>>> Please review.
>>> How does this sharing affect performance?
>>>
>> Patch 3 only checks more_used() for tx ring which in fact reduces the
>> effect of event index and may introduce more tx interrupts. After fixing
>> this issue, tested with 1 vcpu and 1 queue. No obvious changes in
>> performance were noticed.
>>
>> Thanks
> Is this with or without MQ?

Without MQ. 1 vcpu and 1 queue were used.
> With MQ, it seems easy to believe as interrupts are
> distributed between CPUs.
>
> Without MQ, it should be possible to create UDP workloads where
> processing incoming and outgoing interrupts
> on separate CPUs is a win.

Not sure. Processing on separate CPUs may only win when the system is
not busy. But if we processing a single flow in two cpus, it may lead
extra lock contention and bad cache utilization.
And if we really want to distribute the load, RPS/RFS could be used.

Re: [RFC PATCH 0/3] Sharing MSIX irq for tx/rx queue pairs

[Jason Wang]

On 01/05/2015 09:39 AM, Rusty Russell wrote:
> Jason Wang <jasowang@redhat.com> writes:
>> Hi all:
>>
>> This series try to share MSIX irq for each tx/rx queue pair. This is
>> done through:
>>
>> - introducing virtio pci channel which are group of virtqueues that
>>   sharing a single MSIX irq (Patch 1)
>> - expose channel setting to virtio core api (Patch 2)
>> - try to use channel setting in virtio-net (Patch 3)
> Hi Jason,
>
>         Is "channel" a term you created yourself, or something I was
> just unaware of?  

By myself and probably not accurate.
> irq_group would seem more obvious, if the former.
>

Yes, will use this in next version. Thanks.
>> For the transport that does not support channel, channel paramters
>> were simply ignored. For devices that does not use channel, it can
>> simply pass NULL or zero to virito core.
>>
>> With the patch, 1 MSIX irq were saved for each TX/RX queue pair.
> It seems fairly straightforward.
>
> Acked-by: Rusty Russell <rusty@rustcorp.com.au>
>
> Thanks,
> Rusty.
> --