linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/2] virtio-mmio: support multiple interrupt vectors
@ 2019-07-19 13:31 Fei Li
  2019-07-19 13:31 ` [PATCH 1/2] virtio-mmio: Process vrings more proactively Fei Li
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Fei Li @ 2019-07-19 13:31 UTC (permalink / raw)
  To: linux-kernel
  Cc: Jason Wang, Pawel Moll, Michael S . Tsirkin, Suzuki K Poulose,
	Fam Zheng, Fei Li

Hi,

This patch series implements multiple interrupt vectors support for
virtio-mmio device. This is especially useful for multiqueue vhost-net
device when using firecracker micro-vms as the guest.

Test result:
With 8 vcpus & 8 net queues set, one vhost-net device with 8 irqs can
receive 9 times more pps comparing with only one irq:
- 564830.38 rxpck/s for 8 irqs on
- 67665.06 rxpck/s for 1 irq on

Please help to review, thanks!

Have a nice day
Fei


Fam Zheng (1):
  virtio-mmio: Process vrings more proactively

Fei Li (1):
  virtio-mmio: support multiple interrupt vectors

 drivers/virtio/virtio_mmio.c | 238 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 196 insertions(+), 42 deletions(-)

-- 
2.11.0


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

* [PATCH 1/2] virtio-mmio: Process vrings more proactively
  2019-07-19 13:31 [PATCH v1 0/2] virtio-mmio: support multiple interrupt vectors Fei Li
@ 2019-07-19 13:31 ` Fei Li
  2019-07-19 15:17   ` Michael S. Tsirkin
  2019-07-19 13:31 ` [PATCH 2/2] virtio-mmio: support multiple interrupt vectors Fei Li
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Fei Li @ 2019-07-19 13:31 UTC (permalink / raw)
  To: linux-kernel
  Cc: Jason Wang, Pawel Moll, Michael S . Tsirkin, Suzuki K Poulose, Fam Zheng

From: Fam Zheng <zhengfeiran@bytedance.com>

This allows the backend to _not_ trap mmio read of the status register
after injecting IRQ in the data path, which can improve the performance
significantly by avoiding a vmexit for each interrupt.

More importantly it also makes it possible for Firecracker to hook up
virtio-mmio with vhost-net, in which case there isn't a way to implement
proper status register handling.

For a complete backend that does set either INT_CONFIG bit or INT_VRING
bit upon generating irq, what happens hasn't changed.

Signed-off-by: Fam Zheng <zhengfeiran@bytedance.com>
---
 drivers/virtio/virtio_mmio.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index e09edb5c5e06..9b42502b2204 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -295,9 +295,7 @@ static irqreturn_t vm_interrupt(int irq, void *opaque)
 	if (unlikely(status & VIRTIO_MMIO_INT_CONFIG)) {
 		virtio_config_changed(&vm_dev->vdev);
 		ret = IRQ_HANDLED;
-	}
-
-	if (likely(status & VIRTIO_MMIO_INT_VRING)) {
+	} else {
 		spin_lock_irqsave(&vm_dev->lock, flags);
 		list_for_each_entry(info, &vm_dev->virtqueues, node)
 			ret |= vring_interrupt(irq, info->vq);
-- 
2.11.0


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

* [PATCH 2/2] virtio-mmio: support multiple interrupt vectors
  2019-07-19 13:31 [PATCH v1 0/2] virtio-mmio: support multiple interrupt vectors Fei Li
  2019-07-19 13:31 ` [PATCH 1/2] virtio-mmio: Process vrings more proactively Fei Li
@ 2019-07-19 13:31 ` Fei Li
  2019-07-19 15:14 ` [PATCH v1 0/2] " Michael S. Tsirkin
  2019-07-31  4:10 ` 李菲
  3 siblings, 0 replies; 14+ messages in thread
From: Fei Li @ 2019-07-19 13:31 UTC (permalink / raw)
  To: linux-kernel
  Cc: Jason Wang, Pawel Moll, Michael S . Tsirkin, Suzuki K Poulose,
	Fam Zheng, Fei Li

Rework vm_find_vqs() to support multiple interrupt vectors for
virtio-mmio device. Considering without msi/msi-x only limited irq
routing entries (only 24) are allocated, to support more interrupts
for device, esp. virtio-net device with multi rx/tx queue pairs, this
patch requests one vector for the config change and one vector for
two continuous vqs (e.g. each rx/tx queue pair).

If failing to request multiple interrupt vectors, fall back to the
old style: request only one irq for both the config and all vqs.

Add irq_first & irq_last to store the irq information when
processing the device command line.

Signed-off-by: Fei Li <lifei.shirley@bytedance.com>
---
 drivers/virtio/virtio_mmio.c | 237 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 194 insertions(+), 43 deletions(-)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 9b42502b2204..92d16c86ea8f 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -75,7 +75,7 @@
  * Currently hardcoded to the page size. */
 #define VIRTIO_MMIO_VRING_ALIGN		PAGE_SIZE
 
-
+#define VQ_NAME_LEN     20
 
 #define to_virtio_mmio_device(_plat_dev) \
 	container_of(_plat_dev, struct virtio_mmio_device, vdev)
@@ -90,6 +90,9 @@ struct virtio_mmio_device {
 	/* a list of queues so we can dispatch IRQs */
 	spinlock_t lock;
 	struct list_head virtqueues;
+
+	/* Add name for each virtqueue, can be used for the callback. */
+	char *vq_names;
 };
 
 struct virtio_mmio_vq_info {
@@ -279,7 +282,16 @@ static bool vm_notify(struct virtqueue *vq)
 	return true;
 }
 
-/* Notify all virtqueues on an interrupt. */
+/* Only handle the config change, then return. */
+static irqreturn_t vm_config_changed(int irq, void *opaque)
+{
+	struct virtio_mmio_device *vm_dev = opaque;
+
+	virtio_config_changed(&vm_dev->vdev);
+	return IRQ_HANDLED;
+}
+
+/* For old style: only one interrupt for both the config and all virtqueues. */
 static irqreturn_t vm_interrupt(int irq, void *opaque)
 {
 	struct virtio_mmio_device *vm_dev = opaque;
@@ -336,11 +348,31 @@ static void vm_del_vqs(struct virtio_device *vdev)
 {
 	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
 	struct virtqueue *vq, *n;
+	unsigned int start;
+	int i = 0, shared = 0;
+	struct resource *res = platform_get_resource(vm_dev->pdev,
+						     IORESOURCE_IRQ, 0);
 
-	list_for_each_entry_safe(vq, n, &vdev->vqs, list)
-		vm_del_vq(vq);
+	if (res == NULL)
+		return;
+	start = res->start;
+	if (res->end == start) {
+		free_irq(start, vm_dev);
+	} else {
+		/* Try to free_irq for vq[i] */
+		list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
+			free_irq(start + shared + 1, vq);
+			if (i % 2 != 0)
+				shared++;
+			vm_del_vq(vq);
+			i++;
+		}
+		/* Try to free_irq for config */
+		free_irq(start, vdev);
+	}
 
-	free_irq(platform_get_irq(vm_dev->pdev, 0), vm_dev);
+	kfree(vm_dev->vq_names);
+	vm_dev->vq_names = NULL;
 }
 
 static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
@@ -453,26 +485,66 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
 	return ERR_PTR(err);
 }
 
-static int vm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
-		       struct virtqueue *vqs[],
-		       vq_callback_t *callbacks[],
-		       const char * const names[],
-		       const bool *ctx,
-		       struct irq_affinity *desc)
+static int vm_request_multi_vectors(struct virtio_device *vdev,
+				    unsigned int start, int nvectors)
 {
 	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
-	int irq = platform_get_irq(vm_dev->pdev, 0);
-	int i, err, queue_idx = 0;
+	int err = -ENOMEM;
 
-	if (irq < 0) {
-		dev_err(&vdev->dev, "Cannot get IRQ resource\n");
-		return irq;
-	}
+	vm_dev->vq_names = kmalloc(nvectors * VQ_NAME_LEN, GFP_KERNEL);
+	if (!vm_dev->vq_names)
+		goto error;
 
-	err = request_irq(irq, vm_interrupt, IRQF_SHARED,
-			dev_name(&vdev->dev), vm_dev);
+	/* Firstly, request one irq vector for config */
+	snprintf(vm_dev->vq_names, VQ_NAME_LEN,
+		"%s-config", dev_name(&vdev->dev));
+	err = request_irq(start, vm_config_changed, 0,
+			vm_dev->vq_names, vm_dev);
 	if (err)
-		return err;
+		goto error;
+
+	return 0;
+error:
+	vm_del_vqs(vdev);
+	return err;
+}
+
+static int vm_try_to_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
+			      struct virtqueue *vqs[],
+			      vq_callback_t *callbacks[],
+			      const char * const names[],
+			      const bool *ctx,
+			      struct irq_affinity *desc,
+			      bool multi_vectors)
+{
+	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+	unsigned int start, shared = 0;
+	int i, err = 0, nvectors = 0, queue_idx = 0;
+	struct resource *res = platform_get_resource(vm_dev->pdev,
+						     IORESOURCE_IRQ, 0);
+
+	if (res == NULL)
+		return -EINVAL;
+
+	start = res->start;
+	if (!multi_vectors) {
+		/* Old style: only one interrupt for config and all vqs. */
+		err = request_irq(start, vm_interrupt, IRQF_SHARED,
+				  dev_name(&vdev->dev), vm_dev);
+		if (err)
+			goto error_request;
+		res->end = start;
+	} else {
+		/* Optimizing: one for config change, one per vq pair. */
+		nvectors = 1;
+		for (i = 0; i < nvqs; i++)
+			if (callbacks[i])
+				++nvectors;
+
+		err = vm_request_multi_vectors(vdev, start, nvectors);
+		if (err)
+			goto error_request;
+	}
 
 	for (i = 0; i < nvqs; ++i) {
 		if (!names[i]) {
@@ -483,14 +555,65 @@ static int vm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 		vqs[i] = vm_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
 				     ctx ? ctx[i] : false);
 		if (IS_ERR(vqs[i])) {
-			vm_del_vqs(vdev);
-			return PTR_ERR(vqs[i]);
+			err = PTR_ERR(vqs[i]);
+			goto error_vq;
 		}
+
+		/* Do not request_irq for vq without a callback, e.i. control */
+		if (!callbacks[i])
+			continue;
+
+		/* If multi-vectors not supported: don't request more vectors */
+		if (start == res->end)
+			break;
+
+		/* For multi-vectors: choose vq as the dev_id for request_irq */
+		snprintf(vm_dev->vq_names + (i + 1) * VQ_NAME_LEN, VQ_NAME_LEN,
+			 "%s-%s", dev_name(&vdev->dev), names[i]);
+		err = request_irq(start + shared + 1, vring_interrupt,
+				  IRQF_SHARED,
+				  vm_dev->vq_names + (i + 1) * VQ_NAME_LEN,
+				  vqs[i]);
+		if (err)
+			goto error_vq;
+
+		if (i % 2 != 0)
+			shared += 1;
 	}
+	return err;
+error_vq:
+	vm_del_vqs(vdev);
+error_request:
+	return err;
+}
 
-	return 0;
+static int vm_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
+		       struct virtqueue *vqs[],
+		       vq_callback_t *callbacks[],
+		       const char * const names[],
+		       const bool *ctx,
+		       struct irq_affinity *desc)
+{
+	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+	struct resource *res = platform_get_resource(vm_dev->pdev,
+						     IORESOURCE_IRQ, 0);
+
+	if (res == NULL)
+		return -EINVAL;
+	/* If supports multi-vectors: request more vectors for config and vqs */
+	if (res->start < res->end)
+		if (!vm_try_to_find_vqs(vdev, nvqs, vqs, callbacks,
+					names, ctx, desc, true))
+			return 0;
+	/* Only request one vector for both the config and all vqs:
+	 * 1. Handle for devices not supporting multi vectors;
+	 * 2. Fallback to the old style in case requesting multi-vectors failed
+	 */
+	return vm_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
+				  ctx, desc, false);
 }
 
+
 static const char *vm_bus_name(struct virtio_device *vdev)
 {
 	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
@@ -637,32 +760,42 @@ static int vm_cmdline_set(const char *device,
 	struct resource resources[2] = {};
 	char *str;
 	long long int base, size;
-	unsigned int irq;
+	unsigned int irq_first, irq_last;
 	int processed, consumed = 0;
 	struct platform_device *pdev;
 
 	/* Consume "size" part of the command line parameter */
 	size = memparse(device, &str);
 
-	/* Get "@<base>:<irq>[:<id>]" chunks */
-	processed = sscanf(str, "@%lli:%u%n:%d%n",
-			&base, &irq, &consumed,
-			&vm_cmdline_id, &consumed);
-
-	/*
-	 * sscanf() must processes at least 2 chunks; also there
-	 * must be no extra characters after the last chunk, so
-	 * str[consumed] must be '\0'
-	 */
-	if (processed < 2 || str[consumed])
-		return -EINVAL;
+	if (strchr(str, '[') == NULL && strchr(str, ']') == NULL) {
+		/* For old style: parse as "@<base>:<irq>[:<id>]" chunks */
+		processed = sscanf(str, "@%lli:%u%n:%d%n",
+				&base, &irq_first, &consumed,
+				&vm_cmdline_id, &consumed);
+		/*
+		 * sscanf() must processes at least 2 chunks; also there
+		 * must be no extra characters after the last chunk, so
+		 * str[consumed] must be '\0'
+		 */
+		if (processed < 2 || str[consumed])
+			return -EINVAL;
+		irq_last = irq_first;
+	} else {
+		/* For multi-vectors: @<base>:[<irq_first>-<irq_last>][:<id>] */
+		processed = sscanf(str, "@%lli:[%u-%u]%n:%d%n",
+				&base, &irq_first, &irq_last, &consumed,
+				&vm_cmdline_id, &consumed);
+		if (processed < 3 || str[consumed])
+			return -EINVAL;
+	}
 
 	resources[0].flags = IORESOURCE_MEM;
 	resources[0].start = base;
 	resources[0].end = base + size - 1;
 
 	resources[1].flags = IORESOURCE_IRQ;
-	resources[1].start = resources[1].end = irq;
+	resources[1].start = irq_first;
+	resources[1].end = irq_last;
 
 	if (!vm_cmdline_parent_registered) {
 		err = device_register(&vm_cmdline_parent);
@@ -673,11 +806,19 @@ static int vm_cmdline_set(const char *device,
 		vm_cmdline_parent_registered = 1;
 	}
 
-	pr_info("Registering device virtio-mmio.%d at 0x%llx-0x%llx, IRQ %d.\n",
-		       vm_cmdline_id,
-		       (unsigned long long)resources[0].start,
-		       (unsigned long long)resources[0].end,
-		       (int)resources[1].start);
+	if (resources[1].end > resources[1].start)
+		pr_info("Registering device virtio-mmio.%d at 0x%llx-0x%llx, IRQs %d-%d.\n",
+			vm_cmdline_id,
+			(unsigned long long)resources[0].start,
+			(unsigned long long)resources[0].end,
+			(int)resources[1].start,
+			(int)resources[1].end);
+	else
+		pr_info("Registering device virtio-mmio.%d at 0x%llx-0x%llx, IRQ %d.\n",
+			vm_cmdline_id,
+			(unsigned long long)resources[0].start,
+			(unsigned long long)resources[0].end,
+			(int)resources[1].start);
 
 	pdev = platform_device_register_resndata(&vm_cmdline_parent,
 			"virtio-mmio", vm_cmdline_id++,
@@ -692,7 +833,17 @@ static int vm_cmdline_get_device(struct device *dev, void *data)
 	unsigned int len = strlen(buffer);
 	struct platform_device *pdev = to_platform_device(dev);
 
-	snprintf(buffer + len, PAGE_SIZE - len, "0x%llx@0x%llx:%llu:%d\n",
+	if (pdev->resource[1].end > pdev->resource[1].start)
+		snprintf(buffer + len, PAGE_SIZE - len,
+			"0x%llx@0x%llx:[%llu-%llu]:%d\n",
+			pdev->resource[0].end - pdev->resource[0].start + 1ULL,
+			(unsigned long long)pdev->resource[0].start,
+			(unsigned long long)pdev->resource[1].start,
+			(unsigned long long)pdev->resource[1].end,
+			pdev->id);
+	else
+		snprintf(buffer + len, PAGE_SIZE - len,
+			"0x%llx@0x%llx:%llu:%d\n",
 			pdev->resource[0].end - pdev->resource[0].start + 1ULL,
 			(unsigned long long)pdev->resource[0].start,
 			(unsigned long long)pdev->resource[1].start,
-- 
2.11.0


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

* Re: [PATCH v1 0/2] virtio-mmio: support multiple interrupt vectors
  2019-07-19 13:31 [PATCH v1 0/2] virtio-mmio: support multiple interrupt vectors Fei Li
  2019-07-19 13:31 ` [PATCH 1/2] virtio-mmio: Process vrings more proactively Fei Li
  2019-07-19 13:31 ` [PATCH 2/2] virtio-mmio: support multiple interrupt vectors Fei Li
@ 2019-07-19 15:14 ` Michael S. Tsirkin
  2019-07-22  3:22   ` [External Email] " 李菲
  2019-07-31  4:10 ` 李菲
  3 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2019-07-19 15:14 UTC (permalink / raw)
  To: Fei Li; +Cc: linux-kernel, Jason Wang, Pawel Moll, Suzuki K Poulose, Fam Zheng

On Fri, Jul 19, 2019 at 09:31:33PM +0800, Fei Li wrote:
> Hi,
> 
> This patch series implements multiple interrupt vectors support for
> virtio-mmio device. This is especially useful for multiqueue vhost-net
> device when using firecracker micro-vms as the guest.
> 
> Test result:
> With 8 vcpus & 8 net queues set, one vhost-net device with 8 irqs can
> receive 9 times more pps comparing with only one irq:
> - 564830.38 rxpck/s for 8 irqs on
> - 67665.06 rxpck/s for 1 irq on
> 
> Please help to review, thanks!
> 
> Have a nice day
> Fei


Interesting. The spec says though:

	4.2.3.4
	Notifications From The Device
	The memory mapped virtio device is using a single, dedicated interrupt signal, which is asserted when at
	least one of the bits described in the description of InterruptStatus is set. This is how the device sends a
	used buffer notification or a configuration change notification to the device.


So I'm guessing we need to change the host/guest interface?
If true pls cc virtio-dev.

Also, do we need to update dt bindings documentation?

> 
> Fam Zheng (1):
>   virtio-mmio: Process vrings more proactively
> 
> Fei Li (1):
>   virtio-mmio: support multiple interrupt vectors
> 
>  drivers/virtio/virtio_mmio.c | 238 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 196 insertions(+), 42 deletions(-)
> 
> -- 
> 2.11.0

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

* Re: [PATCH 1/2] virtio-mmio: Process vrings more proactively
  2019-07-19 13:31 ` [PATCH 1/2] virtio-mmio: Process vrings more proactively Fei Li
@ 2019-07-19 15:17   ` Michael S. Tsirkin
  2019-07-22  2:28     ` [External Email] " Fam Zheng
  0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2019-07-19 15:17 UTC (permalink / raw)
  To: Fei Li; +Cc: linux-kernel, Jason Wang, Pawel Moll, Suzuki K Poulose, Fam Zheng

On Fri, Jul 19, 2019 at 09:31:34PM +0800, Fei Li wrote:
> From: Fam Zheng <zhengfeiran@bytedance.com>
> 
> This allows the backend to _not_ trap mmio read of the status register
> after injecting IRQ in the data path, which can improve the performance
> significantly by avoiding a vmexit for each interrupt.
> 
> More importantly it also makes it possible for Firecracker to hook up
> virtio-mmio with vhost-net, in which case there isn't a way to implement
> proper status register handling.
> 
> For a complete backend that does set either INT_CONFIG bit or INT_VRING
> bit upon generating irq, what happens hasn't changed.
> 
> Signed-off-by: Fam Zheng <zhengfeiran@bytedance.com>

This has a side effect of skipping vring callbacks
if they trigger at the same time with a config
interrupt.
I don't see why this is safe.


> ---
>  drivers/virtio/virtio_mmio.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index e09edb5c5e06..9b42502b2204 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -295,9 +295,7 @@ static irqreturn_t vm_interrupt(int irq, void *opaque)
>  	if (unlikely(status & VIRTIO_MMIO_INT_CONFIG)) {
>  		virtio_config_changed(&vm_dev->vdev);
>  		ret = IRQ_HANDLED;
> -	}
> -
> -	if (likely(status & VIRTIO_MMIO_INT_VRING)) {
> +	} else {
>  		spin_lock_irqsave(&vm_dev->lock, flags);
>  		list_for_each_entry(info, &vm_dev->virtqueues, node)
>  			ret |= vring_interrupt(irq, info->vq);
> -- 
> 2.11.0

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

* Re: [External Email] Re: [PATCH 1/2] virtio-mmio: Process vrings more proactively
  2019-07-19 15:17   ` Michael S. Tsirkin
@ 2019-07-22  2:28     ` Fam Zheng
  2019-07-25 14:33       ` Michael S. Tsirkin
  0 siblings, 1 reply; 14+ messages in thread
From: Fam Zheng @ 2019-07-22  2:28 UTC (permalink / raw)
  To: Michael S. Tsirkin, Fei Li
  Cc: linux-kernel, Jason Wang, Pawel Moll, Suzuki K Poulose


On 7/19/19 11:17 PM, Michael S. Tsirkin wrote:
> On Fri, Jul 19, 2019 at 09:31:34PM +0800, Fei Li wrote:
>> From: Fam Zheng <zhengfeiran@bytedance.com>
>>
>> This allows the backend to _not_ trap mmio read of the status register
>> after injecting IRQ in the data path, which can improve the performance
>> significantly by avoiding a vmexit for each interrupt.
>>
>> More importantly it also makes it possible for Firecracker to hook up
>> virtio-mmio with vhost-net, in which case there isn't a way to implement
>> proper status register handling.
>>
>> For a complete backend that does set either INT_CONFIG bit or INT_VRING
>> bit upon generating irq, what happens hasn't changed.
>>
>> Signed-off-by: Fam Zheng <zhengfeiran@bytedance.com>
> This has a side effect of skipping vring callbacks
> if they trigger at the same time with a config
> interrupt.
> I don't see why this is safe.

Good point! I think the block can be moved out from the else block and 
run unconditionally then.

Fam


>
>
>> ---
>>   drivers/virtio/virtio_mmio.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
>> index e09edb5c5e06..9b42502b2204 100644
>> --- a/drivers/virtio/virtio_mmio.c
>> +++ b/drivers/virtio/virtio_mmio.c
>> @@ -295,9 +295,7 @@ static irqreturn_t vm_interrupt(int irq, void *opaque)
>>   	if (unlikely(status & VIRTIO_MMIO_INT_CONFIG)) {
>>   		virtio_config_changed(&vm_dev->vdev);
>>   		ret = IRQ_HANDLED;
>> -	}
>> -
>> -	if (likely(status & VIRTIO_MMIO_INT_VRING)) {
>> +	} else {
>>   		spin_lock_irqsave(&vm_dev->lock, flags);
>>   		list_for_each_entry(info, &vm_dev->virtqueues, node)
>>   			ret |= vring_interrupt(irq, info->vq);
>> -- 
>> 2.11.0

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

* Re: [External Email] Re: [PATCH v1 0/2] virtio-mmio: support multiple interrupt vectors
  2019-07-19 15:14 ` [PATCH v1 0/2] " Michael S. Tsirkin
@ 2019-07-22  3:22   ` 李菲
  2019-07-22  8:39     ` Michael S. Tsirkin
  0 siblings, 1 reply; 14+ messages in thread
From: 李菲 @ 2019-07-22  3:22 UTC (permalink / raw)
  To: Michael S. Tsirkin, virtio-dev
  Cc: linux-kernel, Jason Wang, Pawel Moll, Suzuki K Poulose, Fam Zheng

On Fri, Jul 19, 2019 at 11:14 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Jul 19, 2019 at 09:31:33PM +0800, Fei Li wrote:
> > Hi,
> >
> > This patch series implements multiple interrupt vectors support for
> > virtio-mmio device. This is especially useful for multiqueue vhost-net
> > device when using firecracker micro-vms as the guest.
> >
> > Test result:
> > With 8 vcpus & 8 net queues set, one vhost-net device with 8 irqs can
> > receive 9 times more pps comparing with only one irq:
> > - 564830.38 rxpck/s for 8 irqs on
> > - 67665.06 rxpck/s for 1 irq on
> >
> > Please help to review, thanks!
> >
> > Have a nice day
> > Fei
>
>
> Interesting. The spec says though:
>
>         4.2.3.4
>         Notifications From The Device
>         The memory mapped virtio device is using a single, dedicated interrupt signal, which is asserted when at
>         least one of the bits described in the description of InterruptStatus is set. This is how the device sends a
>         used buffer notification or a configuration change notification to the device.
>
Yes, the spec needs to be updated if we want to use mult-irqs.
>
> So I'm guessing we need to change the host/guest interface?
Just to confirm, does the "the host/guest interface" you mentioned mean how to
pass the irq information from the user space tool to guest kernel?
In this patch, we do this by passing the [irq_start, irq_end]
interface via setting guest
kernel command line, that is done in vm_cmdline_set().
Also there is another way to do this: add two new registers describing irq info
(irq_start & irq_end OR irq_start & irq_numbers) to the virtio config space.

Which one do you prefer?

> If true pls cc virtio-dev.
Sure.
>
> Also, do we need to update dt bindings documentation?
You mean the following doc? Sure. :)
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/virtio/mmio.txt

Thanks for the review!

Have a nice day
Fei


>
> >
> > Fam Zheng (1):
> >   virtio-mmio: Process vrings more proactively
> >
> > Fei Li (1):
> >   virtio-mmio: support multiple interrupt vectors
> >
> >  drivers/virtio/virtio_mmio.c | 238 +++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 196 insertions(+), 42 deletions(-)
> >
> > --
> > 2.11.0

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

* Re: [External Email] Re: [PATCH v1 0/2] virtio-mmio: support multiple interrupt vectors
  2019-07-22  3:22   ` [External Email] " 李菲
@ 2019-07-22  8:39     ` Michael S. Tsirkin
  2019-07-22 13:43       ` 李菲
  0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2019-07-22  8:39 UTC (permalink / raw)
  To: 李菲
  Cc: virtio-dev, linux-kernel, Jason Wang, Pawel Moll,
	Suzuki K Poulose, Fam Zheng

On Mon, Jul 22, 2019 at 11:22:02AM +0800, 李菲 wrote:
> On Fri, Jul 19, 2019 at 11:14 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Jul 19, 2019 at 09:31:33PM +0800, Fei Li wrote:
> > > Hi,
> > >
> > > This patch series implements multiple interrupt vectors support for
> > > virtio-mmio device. This is especially useful for multiqueue vhost-net
> > > device when using firecracker micro-vms as the guest.
> > >
> > > Test result:
> > > With 8 vcpus & 8 net queues set, one vhost-net device with 8 irqs can
> > > receive 9 times more pps comparing with only one irq:
> > > - 564830.38 rxpck/s for 8 irqs on
> > > - 67665.06 rxpck/s for 1 irq on
> > >
> > > Please help to review, thanks!
> > >
> > > Have a nice day
> > > Fei
> >
> >
> > Interesting. The spec says though:
> >
> >         4.2.3.4
> >         Notifications From The Device
> >         The memory mapped virtio device is using a single, dedicated interrupt signal, which is asserted when at
> >         least one of the bits described in the description of InterruptStatus is set. This is how the device sends a
> >         used buffer notification or a configuration change notification to the device.
> >
> Yes, the spec needs to be updated if we want to use mult-irqs.
> >
> > So I'm guessing we need to change the host/guest interface?
> Just to confirm, does the "the host/guest interface" you mentioned mean how to
> pass the irq information from the user space tool to guest kernel?
> In this patch, we do this by passing the [irq_start, irq_end]
> interface via setting guest
> kernel command line, that is done in vm_cmdline_set().
> Also there is another way to do this: add two new registers describing irq info
> (irq_start & irq_end OR irq_start & irq_numbers) to the virtio config space.
> 
> Which one do you prefer?

I'm not sure - so far irq was passed on the command line, right?

The first step in implementing any spec change would be to update qemu
code to virtio 1. Which is not a huge project but so far no one
bothered.


> > If true pls cc virtio-dev.
> Sure.
> >
> > Also, do we need to update dt bindings documentation?
> You mean the following doc? Sure. :)
> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/virtio/mmio.txt
> 
> Thanks for the review!
> 
> Have a nice day
> Fei
> 
> 
> >
> > >
> > > Fam Zheng (1):
> > >   virtio-mmio: Process vrings more proactively
> > >
> > > Fei Li (1):
> > >   virtio-mmio: support multiple interrupt vectors
> > >
> > >  drivers/virtio/virtio_mmio.c | 238 +++++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 196 insertions(+), 42 deletions(-)
> > >
> > > --
> > > 2.11.0

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

* Re: [External Email] Re: [PATCH v1 0/2] virtio-mmio: support multiple interrupt vectors
  2019-07-22  8:39     ` Michael S. Tsirkin
@ 2019-07-22 13:43       ` 李菲
  2019-07-30 20:26         ` Michael S. Tsirkin
  0 siblings, 1 reply; 14+ messages in thread
From: 李菲 @ 2019-07-22 13:43 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-dev, linux-kernel, Jason Wang, Pawel Moll,
	Suzuki K Poulose, Fam Zheng

On Mon, Jul 22, 2019 at 4:39 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Jul 22, 2019 at 11:22:02AM +0800, 李菲 wrote:
> > On Fri, Jul 19, 2019 at 11:14 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Fri, Jul 19, 2019 at 09:31:33PM +0800, Fei Li wrote:
> > > > Hi,
> > > >
> > > > This patch series implements multiple interrupt vectors support for
> > > > virtio-mmio device. This is especially useful for multiqueue vhost-net
> > > > device when using firecracker micro-vms as the guest.
> > > >
> > > > Test result:
> > > > With 8 vcpus & 8 net queues set, one vhost-net device with 8 irqs can
> > > > receive 9 times more pps comparing with only one irq:
> > > > - 564830.38 rxpck/s for 8 irqs on
> > > > - 67665.06 rxpck/s for 1 irq on
> > > >
> > > > Please help to review, thanks!
> > > >
> > > > Have a nice day
> > > > Fei
> > >
> > >
> > > Interesting. The spec says though:
> > >
> > >         4.2.3.4
> > >         Notifications From The Device
> > >         The memory mapped virtio device is using a single, dedicated interrupt signal, which is asserted when at
> > >         least one of the bits described in the description of InterruptStatus is set. This is how the device sends a
> > >         used buffer notification or a configuration change notification to the device.
> > >
> > Yes, the spec needs to be updated if we want to use mult-irqs.
> > >
> > > So I'm guessing we need to change the host/guest interface?
> > Just to confirm, does the "the host/guest interface" you mentioned mean how to
> > pass the irq information from the user space tool to guest kernel?
> > In this patch, we do this by passing the [irq_start, irq_end]
> > interface via setting guest
> > kernel command line, that is done in vm_cmdline_set().
> > Also there is another way to do this: add two new registers describing irq info
> > (irq_start & irq_end OR irq_start & irq_numbers) to the virtio config space.
> >
> > Which one do you prefer?
>
> I'm not sure - so far irq was passed on the command line, right?
Yes.
>
> The first step in implementing any spec change would be to update qemu
> code to virtio 1. Which is not a huge project but so far no one
> bothered.
Emm, actually I only did the test with using firecracker to start a
micro-vm, but without qemu.
To be honest, the reason why implementing multi-irq on virtio-mmio is
mainly because the
current firecracker using virtio-mmio device and it has no pci thing,
thus no msi/msix to
handle the interruptions.
On the other hand, considering pci is well supported in qemu, I am
wondering whether we
still need this. If needed, we would like to do this. :)

Have a nice day, thanks
Fei
>
>
> > > If true pls cc virtio-dev.
> > Sure.
> > >
> > > Also, do we need to update dt bindings documentation?
> > You mean the following doc? Sure. :)
> > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/virtio/mmio.txt
> >
> > Thanks for the review!
> >
> > Have a nice day
> > Fei
> >
> >
> > >
> > > >
> > > > Fam Zheng (1):
> > > >   virtio-mmio: Process vrings more proactively
> > > >
> > > > Fei Li (1):
> > > >   virtio-mmio: support multiple interrupt vectors
> > > >
> > > >  drivers/virtio/virtio_mmio.c | 238 +++++++++++++++++++++++++++++++++++--------
> > > >  1 file changed, 196 insertions(+), 42 deletions(-)
> > > >
> > > > --
> > > > 2.11.0

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

* Re: [External Email] Re: [PATCH 1/2] virtio-mmio: Process vrings more proactively
  2019-07-22  2:28     ` [External Email] " Fam Zheng
@ 2019-07-25 14:33       ` Michael S. Tsirkin
  0 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2019-07-25 14:33 UTC (permalink / raw)
  To: Fam Zheng; +Cc: Fei Li, linux-kernel, Jason Wang, Pawel Moll, Suzuki K Poulose

On Mon, Jul 22, 2019 at 10:28:30AM +0800, Fam Zheng wrote:
> 
> On 7/19/19 11:17 PM, Michael S. Tsirkin wrote:
> > On Fri, Jul 19, 2019 at 09:31:34PM +0800, Fei Li wrote:
> > > From: Fam Zheng <zhengfeiran@bytedance.com>
> > > 
> > > This allows the backend to _not_ trap mmio read of the status register
> > > after injecting IRQ in the data path, which can improve the performance
> > > significantly by avoiding a vmexit for each interrupt.
> > > 
> > > More importantly it also makes it possible for Firecracker to hook up
> > > virtio-mmio with vhost-net, in which case there isn't a way to implement
> > > proper status register handling.
> > > 
> > > For a complete backend that does set either INT_CONFIG bit or INT_VRING
> > > bit upon generating irq, what happens hasn't changed.
> > > 
> > > Signed-off-by: Fam Zheng <zhengfeiran@bytedance.com>
> > This has a side effect of skipping vring callbacks
> > if they trigger at the same time with a config
> > interrupt.
> > I don't see why this is safe.
> 
> Good point! I think the block can be moved out from the else block and run
> unconditionally then.
> 
> Fam


Won't same callback run from multiple irq handlers then?
Running multiple vring callbacks at the same time isn't
a good idea either.

> 
> > 
> > 
> > > ---
> > >   drivers/virtio/virtio_mmio.c | 4 +---
> > >   1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> > > index e09edb5c5e06..9b42502b2204 100644
> > > --- a/drivers/virtio/virtio_mmio.c
> > > +++ b/drivers/virtio/virtio_mmio.c
> > > @@ -295,9 +295,7 @@ static irqreturn_t vm_interrupt(int irq, void *opaque)
> > >   	if (unlikely(status & VIRTIO_MMIO_INT_CONFIG)) {
> > >   		virtio_config_changed(&vm_dev->vdev);
> > >   		ret = IRQ_HANDLED;
> > > -	}
> > > -
> > > -	if (likely(status & VIRTIO_MMIO_INT_VRING)) {
> > > +	} else {
> > >   		spin_lock_irqsave(&vm_dev->lock, flags);
> > >   		list_for_each_entry(info, &vm_dev->virtqueues, node)
> > >   			ret |= vring_interrupt(irq, info->vq);
> > > -- 
> > > 2.11.0

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

* Re: [External Email] Re: [PATCH v1 0/2] virtio-mmio: support multiple interrupt vectors
  2019-07-22 13:43       ` 李菲
@ 2019-07-30 20:26         ` Michael S. Tsirkin
  2019-07-31  4:04           ` 李菲
  0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2019-07-30 20:26 UTC (permalink / raw)
  To: 李菲
  Cc: virtio-dev, linux-kernel, Jason Wang, Pawel Moll,
	Suzuki K Poulose, Fam Zheng

On Mon, Jul 22, 2019 at 09:43:18PM +0800, 李菲 wrote:
> On Mon, Jul 22, 2019 at 4:39 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Jul 22, 2019 at 11:22:02AM +0800, 李菲 wrote:
> > > On Fri, Jul 19, 2019 at 11:14 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Fri, Jul 19, 2019 at 09:31:33PM +0800, Fei Li wrote:
> > > > > Hi,
> > > > >
> > > > > This patch series implements multiple interrupt vectors support for
> > > > > virtio-mmio device. This is especially useful for multiqueue vhost-net
> > > > > device when using firecracker micro-vms as the guest.
> > > > >
> > > > > Test result:
> > > > > With 8 vcpus & 8 net queues set, one vhost-net device with 8 irqs can
> > > > > receive 9 times more pps comparing with only one irq:
> > > > > - 564830.38 rxpck/s for 8 irqs on
> > > > > - 67665.06 rxpck/s for 1 irq on
> > > > >
> > > > > Please help to review, thanks!
> > > > >
> > > > > Have a nice day
> > > > > Fei
> > > >
> > > >
> > > > Interesting. The spec says though:
> > > >
> > > >         4.2.3.4
> > > >         Notifications From The Device
> > > >         The memory mapped virtio device is using a single, dedicated interrupt signal, which is asserted when at
> > > >         least one of the bits described in the description of InterruptStatus is set. This is how the device sends a
> > > >         used buffer notification or a configuration change notification to the device.
> > > >
> > > Yes, the spec needs to be updated if we want to use mult-irqs.
> > > >
> > > > So I'm guessing we need to change the host/guest interface?
> > > Just to confirm, does the "the host/guest interface" you mentioned mean how to
> > > pass the irq information from the user space tool to guest kernel?
> > > In this patch, we do this by passing the [irq_start, irq_end]
> > > interface via setting guest
> > > kernel command line, that is done in vm_cmdline_set().
> > > Also there is another way to do this: add two new registers describing irq info
> > > (irq_start & irq_end OR irq_start & irq_numbers) to the virtio config space.
> > >
> > > Which one do you prefer?
> >
> > I'm not sure - so far irq was passed on the command line, right?
> Yes.
> >
> > The first step in implementing any spec change would be to update qemu
> > code to virtio 1. Which is not a huge project but so far no one
> > bothered.
> Emm, actually I only did the test with using firecracker to start a
> micro-vm, but without qemu.
> To be honest, the reason why implementing multi-irq on virtio-mmio is
> mainly because the
> current firecracker using virtio-mmio device and it has no pci thing,
> thus no msi/msix to
> handle the interruptions.
> On the other hand, considering pci is well supported in qemu, I am
> wondering whether we
> still need this. If needed, we would like to do this. :)
> 
> Have a nice day, thanks
> Fei


Sergio Lopez is now working on updating mmio to v1 support in qemu.
Maybe get in touch with him on how he looks at this extension.

Not asking you to work on qemu, but it makes sense
to get an ack for guest bits from another popular hypervisor.



> >
> >
> > > > If true pls cc virtio-dev.
> > > Sure.
> > > >
> > > > Also, do we need to update dt bindings documentation?
> > > You mean the following doc? Sure. :)
> > > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/virtio/mmio.txt
> > >
> > > Thanks for the review!
> > >
> > > Have a nice day
> > > Fei
> > >
> > >
> > > >
> > > > >
> > > > > Fam Zheng (1):
> > > > >   virtio-mmio: Process vrings more proactively
> > > > >
> > > > > Fei Li (1):
> > > > >   virtio-mmio: support multiple interrupt vectors
> > > > >
> > > > >  drivers/virtio/virtio_mmio.c | 238 +++++++++++++++++++++++++++++++++++--------
> > > > >  1 file changed, 196 insertions(+), 42 deletions(-)
> > > > >
> > > > > --
> > > > > 2.11.0

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

* Re: [External Email] Re: [PATCH v1 0/2] virtio-mmio: support multiple interrupt vectors
  2019-07-30 20:26         ` Michael S. Tsirkin
@ 2019-07-31  4:04           ` 李菲
  2019-08-02 11:01             ` Sergio Lopez
  0 siblings, 1 reply; 14+ messages in thread
From: 李菲 @ 2019-07-31  4:04 UTC (permalink / raw)
  To: Michael S. Tsirkin, Sergio Lopez
  Cc: virtio-dev, linux-kernel, Jason Wang, Pawel Moll,
	Suzuki K Poulose, Fam Zheng, Xiongchun Duan

On Wed, Jul 31, 2019 at 4:26 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Jul 22, 2019 at 09:43:18PM +0800, 李菲 wrote:
> > On Mon, Jul 22, 2019 at 4:39 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Jul 22, 2019 at 11:22:02AM +0800, 李菲 wrote:
> > > > On Fri, Jul 19, 2019 at 11:14 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Fri, Jul 19, 2019 at 09:31:33PM +0800, Fei Li wrote:
> > > > > > Hi,
> > > > > >
> > > > > > This patch series implements multiple interrupt vectors support for
> > > > > > virtio-mmio device. This is especially useful for multiqueue vhost-net
> > > > > > device when using firecracker micro-vms as the guest.
> > > > > >
> > > > > > Test result:
> > > > > > With 8 vcpus & 8 net queues set, one vhost-net device with 8 irqs can
> > > > > > receive 9 times more pps comparing with only one irq:
> > > > > > - 564830.38 rxpck/s for 8 irqs on
> > > > > > - 67665.06 rxpck/s for 1 irq on
> > > > > >
> > > > > > Please help to review, thanks!
> > > > > >
> > > > > > Have a nice day
> > > > > > Fei
> > > > >
> > > > >
> > > > > Interesting. The spec says though:
> > > > >
> > > > >         4.2.3.4
> > > > >         Notifications From The Device
> > > > >         The memory mapped virtio device is using a single, dedicated interrupt signal, which is asserted when at
> > > > >         least one of the bits described in the description of InterruptStatus is set. This is how the device sends a
> > > > >         used buffer notification or a configuration change notification to the device.
> > > > >
> > > > Yes, the spec needs to be updated if we want to use mult-irqs.
> > > > >
> > > > > So I'm guessing we need to change the host/guest interface?
> > > > Just to confirm, does the "the host/guest interface" you mentioned mean how to
> > > > pass the irq information from the user space tool to guest kernel?
> > > > In this patch, we do this by passing the [irq_start, irq_end]
> > > > interface via setting guest
> > > > kernel command line, that is done in vm_cmdline_set().
> > > > Also there is another way to do this: add two new registers describing irq info
> > > > (irq_start & irq_end OR irq_start & irq_numbers) to the virtio config space.
> > > >
> > > > Which one do you prefer?
> > >
> > > I'm not sure - so far irq was passed on the command line, right?
> > Yes.
> > >
> > > The first step in implementing any spec change would be to update qemu
> > > code to virtio 1. Which is not a huge project but so far no one
> > > bothered.
> > Emm, actually I only did the test with using firecracker to start a
> > micro-vm, but without qemu.
> > To be honest, the reason why implementing multi-irq on virtio-mmio is
> > mainly because the
> > current firecracker using virtio-mmio device and it has no pci thing,
> > thus no msi/msix to
> > handle the interruptions.
> > On the other hand, considering pci is well supported in qemu, I am
> > wondering whether we
> > still need this. If needed, we would like to do this. :)
> >
> > Have a nice day, thanks
> > Fei
>
>
> Sergio Lopez is now working on updating mmio to v1 support in qemu.
> Maybe get in touch with him on how he looks at this extension.
Thanks for the info! :)

Hi Sergio Lopez,
I saw your [virtio-mmio: modern (v2)] patch series in Qemu mailing
list, thanks for moving this on.
And long Story Short, these two kernel patches is to add the multi-irq
support for virtio-mmio driver.
As this involves the spec change and you are now implementing
virtio-mmio v2, could you help to
give some suggestions on this extension?
I will cc you the original patch soon, thanks.

> Not asking you to work on qemu, but it makes sense
> to get an ack for guest bits from another popular hypervisor.
I agree, absolutely right. And I once work on Qemu development, hope
the combined
background could help to move this multi-irq feature forward. :)


Have a nice day, many thanks
Fei
>
>
> > >
> > >
> > > > > If true pls cc virtio-dev.
> > > > Sure.
> > > > >
> > > > > Also, do we need to update dt bindings documentation?
> > > > You mean the following doc? Sure. :)
> > > > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/virtio/mmio.txt
> > > >
> > > > Thanks for the review!
> > > >
> > > > Have a nice day
> > > > Fei
> > > >
> > > >
> > > > >
> > > > > >
> > > > > > Fam Zheng (1):
> > > > > >   virtio-mmio: Process vrings more proactively
> > > > > >
> > > > > > Fei Li (1):
> > > > > >   virtio-mmio: support multiple interrupt vectors
> > > > > >
> > > > > >  drivers/virtio/virtio_mmio.c | 238 +++++++++++++++++++++++++++++++++++--------
> > > > > >  1 file changed, 196 insertions(+), 42 deletions(-)
> > > > > >
> > > > > > --
> > > > > > 2.11.0

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

* Re: [PATCH v1 0/2] virtio-mmio: support multiple interrupt vectors
  2019-07-19 13:31 [PATCH v1 0/2] virtio-mmio: support multiple interrupt vectors Fei Li
                   ` (2 preceding siblings ...)
  2019-07-19 15:14 ` [PATCH v1 0/2] " Michael S. Tsirkin
@ 2019-07-31  4:10 ` 李菲
  3 siblings, 0 replies; 14+ messages in thread
From: 李菲 @ 2019-07-31  4:10 UTC (permalink / raw)
  To: Sergio Lopez
  Cc: Jason Wang, Pawel Moll, Michael S . Tsirkin, Suzuki K Poulose,
	Fam Zheng, linux-kernel, Xiongchun Duan

Hi Sergio,

Considering your implementing virtio-mmio v2 in Qemu, please help to give some
suggestions on this patch series. Thanks :)

For web, this link:
https://www.spinics.net/lists/kernel/msg3195667.html may help.

Have a nice day
Fei

On Fri, Jul 19, 2019 at 9:31 PM Fei Li <lifei.shirley@bytedance.com> wrote:
>
> Hi,
>
> This patch series implements multiple interrupt vectors support for
> virtio-mmio device. This is especially useful for multiqueue vhost-net
> device when using firecracker micro-vms as the guest.
>
> Test result:
> With 8 vcpus & 8 net queues set, one vhost-net device with 8 irqs can
> receive 9 times more pps comparing with only one irq:
> - 564830.38 rxpck/s for 8 irqs on
> - 67665.06 rxpck/s for 1 irq on
>
> Please help to review, thanks!
>
> Have a nice day
> Fei
>
>
> Fam Zheng (1):
>   virtio-mmio: Process vrings more proactively
>
> Fei Li (1):
>   virtio-mmio: support multiple interrupt vectors
>
>  drivers/virtio/virtio_mmio.c | 238 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 196 insertions(+), 42 deletions(-)
>
> --
> 2.11.0
>

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

* Re: [External Email] Re: [PATCH v1 0/2] virtio-mmio: support multiple interrupt vectors
  2019-07-31  4:04           ` 李菲
@ 2019-08-02 11:01             ` Sergio Lopez
  0 siblings, 0 replies; 14+ messages in thread
From: Sergio Lopez @ 2019-08-02 11:01 UTC (permalink / raw)
  To: 李菲
  Cc: Michael S. Tsirkin, virtio-dev, linux-kernel, Jason Wang,
	Pawel Moll, Suzuki K Poulose, Fam Zheng, Xiongchun Duan

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


李菲 <lifei.shirley@bytedance.com> writes:

> On Wed, Jul 31, 2019 at 4:26 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Mon, Jul 22, 2019 at 09:43:18PM +0800, 李菲 wrote:
>> > On Mon, Jul 22, 2019 at 4:39 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>> > >
>> > > On Mon, Jul 22, 2019 at 11:22:02AM +0800, 李菲 wrote:
>> > > > On Fri, Jul 19, 2019 at 11:14 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>> > > > >
>> > > > > On Fri, Jul 19, 2019 at 09:31:33PM +0800, Fei Li wrote:
>> > > > > > Hi,
>> > > > > >
>> > > > > > This patch series implements multiple interrupt vectors support for
>> > > > > > virtio-mmio device. This is especially useful for multiqueue vhost-net
>> > > > > > device when using firecracker micro-vms as the guest.
>> > > > > >
>> > > > > > Test result:
>> > > > > > With 8 vcpus & 8 net queues set, one vhost-net device with 8 irqs can
>> > > > > > receive 9 times more pps comparing with only one irq:
>> > > > > > - 564830.38 rxpck/s for 8 irqs on
>> > > > > > - 67665.06 rxpck/s for 1 irq on
>> > > > > >
>> > > > > > Please help to review, thanks!
>> > > > > >
>> > > > > > Have a nice day
>> > > > > > Fei
>> > > > >
>> > > > >
>> > > > > Interesting. The spec says though:
>> > > > >
>> > > > >         4.2.3.4
>> > > > >         Notifications From The Device
>> > > > >         The memory mapped virtio device is using a single, dedicated interrupt signal, which is asserted when at
>> > > > >         least one of the bits described in the description of InterruptStatus is set. This is how the device sends a
>> > > > >         used buffer notification or a configuration change notification to the device.
>> > > > >
>> > > > Yes, the spec needs to be updated if we want to use mult-irqs.
>> > > > >
>> > > > > So I'm guessing we need to change the host/guest interface?
>> > > > Just to confirm, does the "the host/guest interface" you mentioned mean how to
>> > > > pass the irq information from the user space tool to guest kernel?
>> > > > In this patch, we do this by passing the [irq_start, irq_end]
>> > > > interface via setting guest
>> > > > kernel command line, that is done in vm_cmdline_set().
>> > > > Also there is another way to do this: add two new registers describing irq info
>> > > > (irq_start & irq_end OR irq_start & irq_numbers) to the virtio config space.
>> > > >
>> > > > Which one do you prefer?
>> > >
>> > > I'm not sure - so far irq was passed on the command line, right?
>> > Yes.
>> > >
>> > > The first step in implementing any spec change would be to update qemu
>> > > code to virtio 1. Which is not a huge project but so far no one
>> > > bothered.
>> > Emm, actually I only did the test with using firecracker to start a
>> > micro-vm, but without qemu.
>> > To be honest, the reason why implementing multi-irq on virtio-mmio is
>> > mainly because the
>> > current firecracker using virtio-mmio device and it has no pci thing,
>> > thus no msi/msix to
>> > handle the interruptions.
>> > On the other hand, considering pci is well supported in qemu, I am
>> > wondering whether we
>> > still need this. If needed, we would like to do this. :)
>> >
>> > Have a nice day, thanks
>> > Fei
>>
>>
>> Sergio Lopez is now working on updating mmio to v1 support in qemu.
>> Maybe get in touch with him on how he looks at this extension.
> Thanks for the info! :)
>
> Hi Sergio Lopez,
> I saw your [virtio-mmio: modern (v2)] patch series in Qemu mailing
> list, thanks for moving this on.
> And long Story Short, these two kernel patches is to add the multi-irq
> support for virtio-mmio driver.
> As this involves the spec change and you are now implementing
> virtio-mmio v2, could you help to
> give some suggestions on this extension?
> I will cc you the original patch soon, thanks.

I like having the possibility of using multiple irq lines for a single
virtio-mmio, but I think having to specify an irq range for each device
is a bit inconvenient.

Usually, you want an irq line per each virtqueue. So, ideally, irq
assignment should be somehow linked to the virtqueue initialization. But
this isn't PCI, so the Guest can't simply request some MSI/MSI-X
vectors and tell the device about them, as virtio-pci does.

I think an option could be extending the specification with a new
QueueVector read-only register, which would return either the irq line
to be used for the virtqueue selected by QueueSel, or a value indicating
there isn't a dedicated line for this particular virtqueue.

This way, hypervisors could lazily allocate irq lines on demand when
the QueueVector is read for the first time (or after a reset), and
users (and fdt's) would still only need to specify a single interrupt
for each virtio-mmio definition.

>> Not asking you to work on qemu, but it makes sense
>> to get an ack for guest bits from another popular hypervisor.
> I agree, absolutely right. And I once work on Qemu development, hope
> the combined
> background could help to move this multi-irq feature forward. :)
>
>
> Have a nice day, many thanks
> Fei
>>
>>
>> > >
>> > >
>> > > > > If true pls cc virtio-dev.
>> > > > Sure.
>> > > > >
>> > > > > Also, do we need to update dt bindings documentation?
>> > > > You mean the following doc? Sure. :)
>> > > > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/virtio/mmio.txt
>> > > >
>> > > > Thanks for the review!
>> > > >
>> > > > Have a nice day
>> > > > Fei
>> > > >
>> > > >
>> > > > >
>> > > > > >
>> > > > > > Fam Zheng (1):
>> > > > > >   virtio-mmio: Process vrings more proactively
>> > > > > >
>> > > > > > Fei Li (1):
>> > > > > >   virtio-mmio: support multiple interrupt vectors
>> > > > > >
>> > > > > >  drivers/virtio/virtio_mmio.c | 238 +++++++++++++++++++++++++++++++++++--------
>> > > > > >  1 file changed, 196 insertions(+), 42 deletions(-)
>> > > > > >
>> > > > > > --
>> > > > > > 2.11.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

end of thread, other threads:[~2019-08-02 11:01 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-19 13:31 [PATCH v1 0/2] virtio-mmio: support multiple interrupt vectors Fei Li
2019-07-19 13:31 ` [PATCH 1/2] virtio-mmio: Process vrings more proactively Fei Li
2019-07-19 15:17   ` Michael S. Tsirkin
2019-07-22  2:28     ` [External Email] " Fam Zheng
2019-07-25 14:33       ` Michael S. Tsirkin
2019-07-19 13:31 ` [PATCH 2/2] virtio-mmio: support multiple interrupt vectors Fei Li
2019-07-19 15:14 ` [PATCH v1 0/2] " Michael S. Tsirkin
2019-07-22  3:22   ` [External Email] " 李菲
2019-07-22  8:39     ` Michael S. Tsirkin
2019-07-22 13:43       ` 李菲
2019-07-30 20:26         ` Michael S. Tsirkin
2019-07-31  4:04           ` 李菲
2019-08-02 11:01             ` Sergio Lopez
2019-07-31  4:10 ` 李菲

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).