* [PATCH v2] vp_vdpa: fix the method of calculating vectors
@ 2024-03-18 13:00 gavin.liu
2024-04-08 8:02 ` Michael S. Tsirkin
0 siblings, 1 reply; 15+ messages in thread
From: gavin.liu @ 2024-03-18 13:00 UTC (permalink / raw)
To: jasowang, mst; +Cc: xuanzhuo, virtualization, angus.chen, yuxue.liu
From: Yuxue Liu <yuxue.liu@jaguarmicro.com>
When there is a ctlq and it doesn't require interrupt
callbacks,the original method of calculating vectors
wastes hardware MSI or MSI-X resources as well as system
IRQ resources. Referencing the per_vq_vectors mode in the
vp_find_vqs_msix function, calculate the required number
of vectors based on whether the callback is set.
Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>
---
V1 -> V2: fix when allocating IRQs, scan all queues.
V1: https://lore.kernel.org/all/20240318030121.1873-1-gavin.liu@jaguarmicro.com/
---
drivers/vdpa/virtio_pci/vp_vdpa.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
index 281287fae89f..87329d4358ce 100644
--- a/drivers/vdpa/virtio_pci/vp_vdpa.c
+++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
@@ -160,8 +160,15 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
struct pci_dev *pdev = mdev->pci_dev;
int i, ret, irq;
int queues = vp_vdpa->queues;
- int vectors = queues + 1;
+ int allocated_vectors, vectors = 0;
+ u16 msix_vec;
+ for (i = 0; i < queues; i++) {
+ if (vp_vdpa->vring[i].cb.callback != NULL)
+ vectors++;
+ }
+ /*By default, config interrupts request a single vector*/
+ vectors = vectors + 1;
ret = pci_alloc_irq_vectors(pdev, vectors, vectors, PCI_IRQ_MSIX);
if (ret != vectors) {
dev_err(&pdev->dev,
@@ -169,13 +176,17 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
vectors, ret);
return ret;
}
-
vp_vdpa->vectors = vectors;
for (i = 0; i < queues; i++) {
+ if (vp_vdpa->vring[i].cb.callback == NULL)
+ continue;
+ else
+ msix_vec = allocated_vectors++;
+
snprintf(vp_vdpa->vring[i].msix_name, VP_VDPA_NAME_SIZE,
"vp-vdpa[%s]-%d\n", pci_name(pdev), i);
- irq = pci_irq_vector(pdev, i);
+ irq = pci_irq_vector(pdev, msix_vec);
ret = devm_request_irq(&pdev->dev, irq,
vp_vdpa_vq_handler,
0, vp_vdpa->vring[i].msix_name,
@@ -185,13 +196,14 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
"vp_vdpa: fail to request irq for vq %d\n", i);
goto err;
}
- vp_modern_queue_vector(mdev, i, i);
+ vp_modern_queue_vector(mdev, i, msix_vec);
vp_vdpa->vring[i].irq = irq;
}
+ msix_vec = allocated_vectors;
snprintf(vp_vdpa->msix_name, VP_VDPA_NAME_SIZE, "vp-vdpa[%s]-config\n",
pci_name(pdev));
- irq = pci_irq_vector(pdev, queues);
+ irq = pci_irq_vector(pdev, msix_vec);
ret = devm_request_irq(&pdev->dev, irq, vp_vdpa_config_handler, 0,
vp_vdpa->msix_name, vp_vdpa);
if (ret) {
@@ -199,7 +211,7 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
"vp_vdpa: fail to request irq for vq %d\n", i);
goto err;
}
- vp_modern_config_vector(mdev, queues);
+ vp_modern_config_vector(mdev, msix_vec);
vp_vdpa->config_irq = irq;
return 0;
--
2.43.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2] vp_vdpa: fix the method of calculating vectors
2024-03-18 13:00 [PATCH v2] vp_vdpa: fix the method of calculating vectors gavin.liu
@ 2024-04-08 8:02 ` Michael S. Tsirkin
2024-04-09 1:49 ` [PATCH v3] " lyx634449800
0 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2024-04-08 8:02 UTC (permalink / raw)
To: gavin.liu; +Cc: jasowang, xuanzhuo, virtualization, angus.chen, yuxue.liu
On Mon, Mar 18, 2024 at 09:00:08PM +0800, gavin.liu wrote:
> From: Yuxue Liu <yuxue.liu@jaguarmicro.com>
>
> When there is a ctlq and it doesn't require interrupt
> callbacks,the original method of calculating vectors
> wastes hardware MSI or MSI-X resources as well as system
> IRQ resources. Referencing the per_vq_vectors mode in the
> vp_find_vqs_msix function, calculate the required number
> of vectors based on whether the callback is set.
>
> Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>
Overall, the patch makes sense. But you need to clean it up.
Also, given previous versions were broken,
pls document which configurations you tested, and how.
> ---
>
> V1 -> V2: fix when allocating IRQs, scan all queues.
> V1: https://lore.kernel.org/all/20240318030121.1873-1-gavin.liu@jaguarmicro.com/
> ---
> drivers/vdpa/virtio_pci/vp_vdpa.c | 24 ++++++++++++++++++------
> 1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
> index 281287fae89f..87329d4358ce 100644
> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> @@ -160,8 +160,15 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> struct pci_dev *pdev = mdev->pci_dev;
> int i, ret, irq;
> int queues = vp_vdpa->queues;
> - int vectors = queues + 1;
> + int allocated_vectors, vectors = 0;
> + u16 msix_vec;
The names are messed up.
What we allocate should be called allocated_vectors.
So rename vectors -> allocated_vectors.
The currect vector used for each vq can be called e.g. msix_vec.
And it is pointless to make it u16 here IMHO - just int will do.
>
> + for (i = 0; i < queues; i++) {
> + if (vp_vdpa->vring[i].cb.callback != NULL)
I don't like != NULL style - just
if (vp_vdpa->vring[i].cb.callback)
will do.
> + vectors++;
> + }
> + /*By default, config interrupts request a single vector*/
bad coding style
and what does "by default" mean here?
> + vectors = vectors + 1;
> ret = pci_alloc_irq_vectors(pdev, vectors, vectors, PCI_IRQ_MSIX);
> if (ret != vectors) {
> dev_err(&pdev->dev,
> @@ -169,13 +176,17 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> vectors, ret);
> return ret;
> }
> -
why?
> vp_vdpa->vectors = vectors;
>
> for (i = 0; i < queues; i++) {
> + if (vp_vdpa->vring[i].cb.callback == NULL)
> + continue;
> + else
> + msix_vec = allocated_vectors++;
So just put replace allocated_vectors with msix_vec
incrementing it at the end of the loop,
and you will not need the allocated_vectors variable.
> +
Same here.
if (!vp_vdpa->vring[i].cb.callback)
Also there's no need for else after continue.
> snprintf(vp_vdpa->vring[i].msix_name, VP_VDPA_NAME_SIZE,
> "vp-vdpa[%s]-%d\n", pci_name(pdev), i);
> - irq = pci_irq_vector(pdev, i);
> + irq = pci_irq_vector(pdev, msix_vec);
> ret = devm_request_irq(&pdev->dev, irq,
> vp_vdpa_vq_handler,
> 0, vp_vdpa->vring[i].msix_name,
> @@ -185,13 +196,14 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> "vp_vdpa: fail to request irq for vq %d\n", i);
> goto err;
> }
> - vp_modern_queue_vector(mdev, i, i);
> + vp_modern_queue_vector(mdev, i, msix_vec);
> vp_vdpa->vring[i].irq = irq;
> }
>
> + msix_vec = allocated_vectors;
> snprintf(vp_vdpa->msix_name, VP_VDPA_NAME_SIZE, "vp-vdpa[%s]-config\n",
> pci_name(pdev));
> - irq = pci_irq_vector(pdev, queues);
> + irq = pci_irq_vector(pdev, msix_vec);
> ret = devm_request_irq(&pdev->dev, irq, vp_vdpa_config_handler, 0,
> vp_vdpa->msix_name, vp_vdpa);
> if (ret) {
> @@ -199,7 +211,7 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> "vp_vdpa: fail to request irq for vq %d\n", i);
> goto err;
> }
> - vp_modern_config_vector(mdev, queues);
> + vp_modern_config_vector(mdev, msix_vec);
> vp_vdpa->config_irq = irq;
>
> return 0;
> --
> 2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] vp_vdpa: fix the method of calculating vectors
2024-04-08 8:02 ` Michael S. Tsirkin
@ 2024-04-09 1:49 ` lyx634449800
2024-04-09 3:53 ` Jason Wang
2024-04-09 5:40 ` Michael S. Tsirkin
0 siblings, 2 replies; 15+ messages in thread
From: lyx634449800 @ 2024-04-09 1:49 UTC (permalink / raw)
To: mst, jasowang
Cc: angus.chen, virtualization, xuanzhuo, yuxue.liu, linux-kernel
When there is a ctlq and it doesn't require interrupt
callbacks,the original method of calculating vectors
wastes hardware msi or msix resources as well as system
IRQ resources.
When conducting performance testing using testpmd in the
guest os, it was found that the performance was lower compared
to directly using vfio-pci to passthrough the device
In scenarios where the virtio device in the guest os does
not utilize interrupts, the vdpa driver still configures
the hardware's msix vector. Therefore, the hardware still
sends interrupts to the host os. Because of this unnecessary
action by the hardware, hardware performance decreases, and
it also affects the performance of the host os.
Before modification:(interrupt mode)
32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
After modification:(interrupt mode)
32: 0 0 1 7 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
33: 36 0 3 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-config
Before modification:(virtio pmd mode for guest os)
32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
After modification:(virtio pmd mode for guest os)
32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-config
To verify the use of the virtio PMD mode in the guest operating
system, the following patch needs to be applied to QEMU:
https://lore.kernel.org/all/20240408073311.2049-1-yuxue.liu@jaguarmicro.com
Signed-off-by: lyx634449800 <yuxue.liu@jaguarmicro.com>
---
V3: delete unused variables and add validation records
V2: fix when allocating IRQs, scan all queues
drivers/vdpa/virtio_pci/vp_vdpa.c | 35 +++++++++++++++++++------------
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
index df5f4a3bccb5..cd3aeb3b8f21 100644
--- a/drivers/vdpa/virtio_pci/vp_vdpa.c
+++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
@@ -160,22 +160,31 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
struct pci_dev *pdev = mdev->pci_dev;
int i, ret, irq;
int queues = vp_vdpa->queues;
- int vectors = queues + 1;
+ int msix_vec, allocated_vectors = 0;
- ret = pci_alloc_irq_vectors(pdev, vectors, vectors, PCI_IRQ_MSIX);
- if (ret != vectors) {
+ for (i = 0; i < queues; i++) {
+ if (vp_vdpa->vring[i].cb.callback)
+ allocated_vectors++;
+ }
+ allocated_vectors = allocated_vectors + 1;
+
+ ret = pci_alloc_irq_vectors(pdev, allocated_vectors, allocated_vectors,
+ PCI_IRQ_MSIX);
+ if (ret != allocated_vectors) {
dev_err(&pdev->dev,
"vp_vdpa: fail to allocate irq vectors want %d but %d\n",
- vectors, ret);
+ allocated_vectors, ret);
return ret;
}
-
- vp_vdpa->vectors = vectors;
+ vp_vdpa->vectors = allocated_vectors;
for (i = 0; i < queues; i++) {
+ if (!vp_vdpa->vring[i].cb.callback)
+ continue;
+
snprintf(vp_vdpa->vring[i].msix_name, VP_VDPA_NAME_SIZE,
"vp-vdpa[%s]-%d\n", pci_name(pdev), i);
- irq = pci_irq_vector(pdev, i);
+ irq = pci_irq_vector(pdev, msix_vec);
ret = devm_request_irq(&pdev->dev, irq,
vp_vdpa_vq_handler,
0, vp_vdpa->vring[i].msix_name,
@@ -185,23 +194,23 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
"vp_vdpa: fail to request irq for vq %d\n", i);
goto err;
}
- vp_modern_queue_vector(mdev, i, i);
+ vp_modern_queue_vector(mdev, i, msix_vec);
vp_vdpa->vring[i].irq = irq;
+ msix_vec++;
}
snprintf(vp_vdpa->msix_name, VP_VDPA_NAME_SIZE, "vp-vdpa[%s]-config\n",
- pci_name(pdev));
- irq = pci_irq_vector(pdev, queues);
+ pci_name(pdev));
+ irq = pci_irq_vector(pdev, msix_vec);
ret = devm_request_irq(&pdev->dev, irq, vp_vdpa_config_handler, 0,
vp_vdpa->msix_name, vp_vdpa);
if (ret) {
dev_err(&pdev->dev,
- "vp_vdpa: fail to request irq for vq %d\n", i);
+ "vp_vdpa: fail to request irq for config\n");
goto err;
}
- vp_modern_config_vector(mdev, queues);
+ vp_modern_config_vector(mdev, msix_vec);
vp_vdpa->config_irq = irq;
-
return 0;
err:
vp_vdpa_free_irq(vp_vdpa);
--
2.43.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3] vp_vdpa: fix the method of calculating vectors
2024-04-09 1:49 ` [PATCH v3] " lyx634449800
@ 2024-04-09 3:53 ` Jason Wang
2024-04-09 5:40 ` Michael S. Tsirkin
1 sibling, 0 replies; 15+ messages in thread
From: Jason Wang @ 2024-04-09 3:53 UTC (permalink / raw)
To: lyx634449800; +Cc: mst, angus.chen, virtualization, xuanzhuo, linux-kernel
On Tue, Apr 9, 2024 at 9:49 AM lyx634449800 <yuxue.liu@jaguarmicro.com> wrote:
>
> When there is a ctlq and it doesn't require interrupt
> callbacks,the original method of calculating vectors
> wastes hardware msi or msix resources as well as system
> IRQ resources.
>
> When conducting performance testing using testpmd in the
> guest os, it was found that the performance was lower compared
> to directly using vfio-pci to passthrough the device
>
> In scenarios where the virtio device in the guest os does
> not utilize interrupts, the vdpa driver still configures
> the hardware's msix vector. Therefore, the hardware still
> sends interrupts to the host os. Because of this unnecessary
> action by the hardware, hardware performance decreases, and
> it also affects the performance of the host os.
>
> Before modification:(interrupt mode)
> 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
>
> After modification:(interrupt mode)
> 32: 0 0 1 7 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> 33: 36 0 3 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-config
>
> Before modification:(virtio pmd mode for guest os)
> 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
>
> After modification:(virtio pmd mode for guest os)
> 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-config
>
> To verify the use of the virtio PMD mode in the guest operating
> system, the following patch needs to be applied to QEMU:
> https://lore.kernel.org/all/20240408073311.2049-1-yuxue.liu@jaguarmicro.com
>
> Signed-off-by: lyx634449800 <yuxue.liu@jaguarmicro.com>
> ---
>
> V3: delete unused variables and add validation records
> V2: fix when allocating IRQs, scan all queues
>
> drivers/vdpa/virtio_pci/vp_vdpa.c | 35 +++++++++++++++++++------------
> 1 file changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
> index df5f4a3bccb5..cd3aeb3b8f21 100644
> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> @@ -160,22 +160,31 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> struct pci_dev *pdev = mdev->pci_dev;
> int i, ret, irq;
> int queues = vp_vdpa->queues;
> - int vectors = queues + 1;
> + int msix_vec, allocated_vectors = 0;
>
> - ret = pci_alloc_irq_vectors(pdev, vectors, vectors, PCI_IRQ_MSIX);
> - if (ret != vectors) {
> + for (i = 0; i < queues; i++) {
> + if (vp_vdpa->vring[i].cb.callback)
> + allocated_vectors++;
> + }
> + allocated_vectors = allocated_vectors + 1;
> +
> + ret = pci_alloc_irq_vectors(pdev, allocated_vectors, allocated_vectors,
> + PCI_IRQ_MSIX);
> + if (ret != allocated_vectors) {
> dev_err(&pdev->dev,
> "vp_vdpa: fail to allocate irq vectors want %d but %d\n",
> - vectors, ret);
> + allocated_vectors, ret);
> return ret;
> }
> -
> - vp_vdpa->vectors = vectors;
> + vp_vdpa->vectors = allocated_vectors;
>
> for (i = 0; i < queues; i++) {
> + if (!vp_vdpa->vring[i].cb.callback)
> + continue;
> +
> snprintf(vp_vdpa->vring[i].msix_name, VP_VDPA_NAME_SIZE,
> "vp-vdpa[%s]-%d\n", pci_name(pdev), i);
> - irq = pci_irq_vector(pdev, i);
> + irq = pci_irq_vector(pdev, msix_vec);
> ret = devm_request_irq(&pdev->dev, irq,
> vp_vdpa_vq_handler,
> 0, vp_vdpa->vring[i].msix_name,
> @@ -185,23 +194,23 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> "vp_vdpa: fail to request irq for vq %d\n", i);
> goto err;
> }
> - vp_modern_queue_vector(mdev, i, i);
> + vp_modern_queue_vector(mdev, i, msix_vec);
> vp_vdpa->vring[i].irq = irq;
> + msix_vec++;
> }
>
> snprintf(vp_vdpa->msix_name, VP_VDPA_NAME_SIZE, "vp-vdpa[%s]-config\n",
> - pci_name(pdev));
> - irq = pci_irq_vector(pdev, queues);
> + pci_name(pdev));
> + irq = pci_irq_vector(pdev, msix_vec);
> ret = devm_request_irq(&pdev->dev, irq, vp_vdpa_config_handler, 0,
> vp_vdpa->msix_name, vp_vdpa);
> if (ret) {
> dev_err(&pdev->dev,
> - "vp_vdpa: fail to request irq for vq %d\n", i);
> + "vp_vdpa: fail to request irq for config\n");
> goto err;
> }
> - vp_modern_config_vector(mdev, queues);
> + vp_modern_config_vector(mdev, msix_vec);
> vp_vdpa->config_irq = irq;
> -
Unnecessary changes.
Others look good.
Acked-by: Jason Wang <jasowang@redhat.com>
Thanks
> return 0;
> err:
> vp_vdpa_free_irq(vp_vdpa);
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] vp_vdpa: fix the method of calculating vectors
2024-04-09 1:49 ` [PATCH v3] " lyx634449800
2024-04-09 3:53 ` Jason Wang
@ 2024-04-09 5:40 ` Michael S. Tsirkin
2024-04-09 8:58 ` [PATCH v4] vp_vdpa: don't allocate unused msix vectors lyx634449800
1 sibling, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2024-04-09 5:40 UTC (permalink / raw)
To: lyx634449800; +Cc: jasowang, angus.chen, virtualization, xuanzhuo, linux-kernel
better subject:
vp_vdpa: don't allocate unused msix vectors
to make it clear it's not a bugfix.
more comments below, but most importantly this
looks like it adds a bug.
On Tue, Apr 09, 2024 at 09:49:35AM +0800, lyx634449800 wrote:
> When there is a ctlq and it doesn't require interrupt
> callbacks,the original method of calculating vectors
> wastes hardware msi or msix resources as well as system
> IRQ resources.
>
> When conducting performance testing using testpmd in the
> guest os, it was found that the performance was lower compared
> to directly using vfio-pci to passthrough the device
>
> In scenarios where the virtio device in the guest os does
> not utilize interrupts, the vdpa driver still configures
> the hardware's msix vector. Therefore, the hardware still
> sends interrupts to the host os. Because of this unnecessary
> action by the hardware, hardware performance decreases, and
> it also affects the performance of the host os.
>
> Before modification:(interrupt mode)
> 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
>
> After modification:(interrupt mode)
> 32: 0 0 1 7 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> 33: 36 0 3 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-config
>
> Before modification:(virtio pmd mode for guest os)
> 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
>
> After modification:(virtio pmd mode for guest os)
> 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-config
>
> To verify the use of the virtio PMD mode in the guest operating
> system, the following patch needs to be applied to QEMU:
> https://lore.kernel.org/all/20240408073311.2049-1-yuxue.liu@jaguarmicro.com
>
> Signed-off-by: lyx634449800 <yuxue.liu@jaguarmicro.com>
Bad S.O.B format. Should be
Signed-off-by: Real Name <email>
> ---
>
> V3: delete unused variables and add validation records
> V2: fix when allocating IRQs, scan all queues
>
> drivers/vdpa/virtio_pci/vp_vdpa.c | 35 +++++++++++++++++++------------
> 1 file changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
> index df5f4a3bccb5..cd3aeb3b8f21 100644
> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> @@ -160,22 +160,31 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> struct pci_dev *pdev = mdev->pci_dev;
> int i, ret, irq;
> int queues = vp_vdpa->queues;
> - int vectors = queues + 1;
> + int msix_vec, allocated_vectors = 0;
I would actually call allocated_vectors -> vectors, make the patch
smaller.
>
> - ret = pci_alloc_irq_vectors(pdev, vectors, vectors, PCI_IRQ_MSIX);
> - if (ret != vectors) {
> + for (i = 0; i < queues; i++) {
> + if (vp_vdpa->vring[i].cb.callback)
> + allocated_vectors++;
> + }
> + allocated_vectors = allocated_vectors + 1;
better:
allocated_vectors++; /* extra one for config */
> +
> + ret = pci_alloc_irq_vectors(pdev, allocated_vectors, allocated_vectors,
> + PCI_IRQ_MSIX);
> + if (ret != allocated_vectors) {
> dev_err(&pdev->dev,
> "vp_vdpa: fail to allocate irq vectors want %d but %d\n",
> - vectors, ret);
> + allocated_vectors, ret);
> return ret;
> }
> -
> - vp_vdpa->vectors = vectors;
> + vp_vdpa->vectors = allocated_vectors;
>
> for (i = 0; i < queues; i++) {
> + if (!vp_vdpa->vring[i].cb.callback)
> + continue;
> +
> snprintf(vp_vdpa->vring[i].msix_name, VP_VDPA_NAME_SIZE,
> "vp-vdpa[%s]-%d\n", pci_name(pdev), i);
> - irq = pci_irq_vector(pdev, i);
> + irq = pci_irq_vector(pdev, msix_vec);
using uninitialized msix_vec here?
I would expect compiler to warn about it.
pay attention to compiler warnings pls.
> ret = devm_request_irq(&pdev->dev, irq,
> vp_vdpa_vq_handler,
> 0, vp_vdpa->vring[i].msix_name,
> @@ -185,23 +194,23 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> "vp_vdpa: fail to request irq for vq %d\n", i);
> goto err;
> }
> - vp_modern_queue_vector(mdev, i, i);
> + vp_modern_queue_vector(mdev, i, msix_vec);
> vp_vdpa->vring[i].irq = irq;
> + msix_vec++;
> }
>
> snprintf(vp_vdpa->msix_name, VP_VDPA_NAME_SIZE, "vp-vdpa[%s]-config\n",
> - pci_name(pdev));
> - irq = pci_irq_vector(pdev, queues);
> + pci_name(pdev));
don't move pci_name - don't make unrelated code changes.
> + irq = pci_irq_vector(pdev, msix_vec);
> ret = devm_request_irq(&pdev->dev, irq, vp_vdpa_config_handler, 0,
> vp_vdpa->msix_name, vp_vdpa);
> if (ret) {
> dev_err(&pdev->dev,
> - "vp_vdpa: fail to request irq for vq %d\n", i);
> + "vp_vdpa: fail to request irq for config\n");
I would report ret here too.
> goto err;
> }
> - vp_modern_config_vector(mdev, queues);
> + vp_modern_config_vector(mdev, msix_vec);
> vp_vdpa->config_irq = irq;
> -
don't make unrelated code changes.
> return 0;
> err:
> vp_vdpa_free_irq(vp_vdpa);
> --
> 2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4] vp_vdpa: don't allocate unused msix vectors
2024-04-09 5:40 ` Michael S. Tsirkin
@ 2024-04-09 8:58 ` lyx634449800
2024-04-09 9:26 ` Michael S. Tsirkin
2024-04-09 9:56 ` Heng Qi
0 siblings, 2 replies; 15+ messages in thread
From: lyx634449800 @ 2024-04-09 8:58 UTC (permalink / raw)
To: mst, jasowang
Cc: angus.chen, virtualization, xuanzhuo, yuxue.liu, linux-kernel
From: Yuxue Liu <yuxue.liu@jaguarmicro.com>
When there is a ctlq and it doesn't require interrupt
callbacks,the original method of calculating vectors
wastes hardware msi or msix resources as well as system
IRQ resources.
When conducting performance testing using testpmd in the
guest os, it was found that the performance was lower compared
to directly using vfio-pci to passthrough the device
In scenarios where the virtio device in the guest os does
not utilize interrupts, the vdpa driver still configures
the hardware's msix vector. Therefore, the hardware still
sends interrupts to the host os. Because of this unnecessary
action by the hardware, hardware performance decreases, and
it also affects the performance of the host os.
Before modification:(interrupt mode)
32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
After modification:(interrupt mode)
32: 0 0 1 7 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
33: 36 0 3 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-config
Before modification:(virtio pmd mode for guest os)
32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
After modification:(virtio pmd mode for guest os)
32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-config
To verify the use of the virtio PMD mode in the guest operating
system, the following patch needs to be applied to QEMU:
https://lore.kernel.org/all/20240408073311.2049-1-yuxue.liu@jaguarmicro.com
Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
V4: Update the title and assign values to uninitialized variables
V3: delete unused variables and add validation records
V2: fix when allocating IRQs, scan all queues
drivers/vdpa/virtio_pci/vp_vdpa.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
index df5f4a3bccb5..74bc8adfc7e8 100644
--- a/drivers/vdpa/virtio_pci/vp_vdpa.c
+++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
@@ -160,7 +160,14 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
struct pci_dev *pdev = mdev->pci_dev;
int i, ret, irq;
int queues = vp_vdpa->queues;
- int vectors = queues + 1;
+ int vectors = 0;
+ int msix_vec = 0;
+
+ for (i = 0; i < queues; i++) {
+ if (vp_vdpa->vring[i].cb.callback)
+ vectors++;
+ }
+ vectors++;
ret = pci_alloc_irq_vectors(pdev, vectors, vectors, PCI_IRQ_MSIX);
if (ret != vectors) {
@@ -173,9 +180,12 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
vp_vdpa->vectors = vectors;
for (i = 0; i < queues; i++) {
+ if (!vp_vdpa->vring[i].cb.callback)
+ continue;
+
snprintf(vp_vdpa->vring[i].msix_name, VP_VDPA_NAME_SIZE,
"vp-vdpa[%s]-%d\n", pci_name(pdev), i);
- irq = pci_irq_vector(pdev, i);
+ irq = pci_irq_vector(pdev, msix_vec);
ret = devm_request_irq(&pdev->dev, irq,
vp_vdpa_vq_handler,
0, vp_vdpa->vring[i].msix_name,
@@ -185,21 +195,22 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
"vp_vdpa: fail to request irq for vq %d\n", i);
goto err;
}
- vp_modern_queue_vector(mdev, i, i);
+ vp_modern_queue_vector(mdev, i, msix_vec);
vp_vdpa->vring[i].irq = irq;
+ msix_vec++;
}
snprintf(vp_vdpa->msix_name, VP_VDPA_NAME_SIZE, "vp-vdpa[%s]-config\n",
pci_name(pdev));
- irq = pci_irq_vector(pdev, queues);
+ irq = pci_irq_vector(pdev, msix_vec);
ret = devm_request_irq(&pdev->dev, irq, vp_vdpa_config_handler, 0,
vp_vdpa->msix_name, vp_vdpa);
if (ret) {
dev_err(&pdev->dev,
- "vp_vdpa: fail to request irq for vq %d\n", i);
+ "vp_vdpa: fail to request irq for config, ret %d\n", ret);
goto err;
}
- vp_modern_config_vector(mdev, queues);
+ vp_modern_config_vector(mdev, msix_vec);
vp_vdpa->config_irq = irq;
return 0;
--
2.43.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4] vp_vdpa: don't allocate unused msix vectors
2024-04-09 8:58 ` [PATCH v4] vp_vdpa: don't allocate unused msix vectors lyx634449800
@ 2024-04-09 9:26 ` Michael S. Tsirkin
2024-04-09 9:56 ` Heng Qi
1 sibling, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2024-04-09 9:26 UTC (permalink / raw)
To: lyx634449800; +Cc: jasowang, angus.chen, virtualization, xuanzhuo, linux-kernel
Good and clear subject, I like it.
On Tue, Apr 09, 2024 at 04:58:18PM +0800, lyx634449800 wrote:
> From: Yuxue Liu <yuxue.liu@jaguarmicro.com>
>
> When there is a ctlq and it doesn't require interrupt
> callbacks,the original method of calculating vectors
> wastes hardware msi or msix resources as well as system
> IRQ resources.
>
> When conducting performance testing using testpmd in the
> guest os, it was found that the performance was lower compared
> to directly using vfio-pci to passthrough the device
>
> In scenarios where the virtio device in the guest os does
> not utilize interrupts, the vdpa driver still configures
> the hardware's msix vector. Therefore, the hardware still
> sends interrupts to the host os. Because of this unnecessary
> action by the hardware, hardware performance decreases, and
> it also affects the performance of the host os.
>
> Before modification:(interrupt mode)
> 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
>
> After modification:(interrupt mode)
> 32: 0 0 1 7 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> 33: 36 0 3 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-config
>
> Before modification:(virtio pmd mode for guest os)
> 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
>
> After modification:(virtio pmd mode for guest os)
> 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-config
>
> To verify the use of the virtio PMD mode in the guest operating
> system, the following patch needs to be applied to QEMU:
> https://lore.kernel.org/all/20240408073311.2049-1-yuxue.liu@jaguarmicro.com
>
> Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>
> Acked-by: Jason Wang <jasowang@redhat.com>
Much better, thanks!
A couple of small tweaks to polish it up and it'll be ready.
> ---
> V4: Update the title and assign values to uninitialized variables
> V3: delete unused variables and add validation records
> V2: fix when allocating IRQs, scan all queues
>
> drivers/vdpa/virtio_pci/vp_vdpa.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
> index df5f4a3bccb5..74bc8adfc7e8 100644
> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> @@ -160,7 +160,14 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> struct pci_dev *pdev = mdev->pci_dev;
> int i, ret, irq;
> int queues = vp_vdpa->queues;
> - int vectors = queues + 1;
> + int vectors = 0;
> + int msix_vec = 0;
> +
> + for (i = 0; i < queues; i++) {
> + if (vp_vdpa->vring[i].cb.callback)
> + vectors++;
> + }
> + vectors++;
Actually even easier: int vectors = 1; and then we do not need
this last line.
Sorry I only noticed now.
>
> ret = pci_alloc_irq_vectors(pdev, vectors, vectors, PCI_IRQ_MSIX);
> if (ret != vectors) {
> @@ -173,9 +180,12 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> vp_vdpa->vectors = vectors;
>
> for (i = 0; i < queues; i++) {
> + if (!vp_vdpa->vring[i].cb.callback)
> + continue;
> +
> snprintf(vp_vdpa->vring[i].msix_name, VP_VDPA_NAME_SIZE,
> "vp-vdpa[%s]-%d\n", pci_name(pdev), i);
> - irq = pci_irq_vector(pdev, i);
> + irq = pci_irq_vector(pdev, msix_vec);
> ret = devm_request_irq(&pdev->dev, irq,
> vp_vdpa_vq_handler,
> 0, vp_vdpa->vring[i].msix_name,
> @@ -185,21 +195,22 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> "vp_vdpa: fail to request irq for vq %d\n", i);
> goto err;
> }
> - vp_modern_queue_vector(mdev, i, i);
> + vp_modern_queue_vector(mdev, i, msix_vec);
> vp_vdpa->vring[i].irq = irq;
> + msix_vec++;
> }
>
> snprintf(vp_vdpa->msix_name, VP_VDPA_NAME_SIZE, "vp-vdpa[%s]-config\n",
> pci_name(pdev));
> - irq = pci_irq_vector(pdev, queues);
> + irq = pci_irq_vector(pdev, msix_vec);
> ret = devm_request_irq(&pdev->dev, irq, vp_vdpa_config_handler, 0,
> vp_vdpa->msix_name, vp_vdpa);
> if (ret) {
> dev_err(&pdev->dev,
> - "vp_vdpa: fail to request irq for vq %d\n", i);
> + "vp_vdpa: fail to request irq for config, ret %d\n", ret);
As long as we are here let's fix the grammar, and there's no need to
say "ret":
"vp_vdpa: failed to request irq for config: %d\n", ret);
> goto err;
> }
> - vp_modern_config_vector(mdev, queues);
> + vp_modern_config_vector(mdev, msix_vec);
> vp_vdpa->config_irq = irq;
>
> return 0;
> --
> 2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] vp_vdpa: don't allocate unused msix vectors
2024-04-09 8:58 ` [PATCH v4] vp_vdpa: don't allocate unused msix vectors lyx634449800
2024-04-09 9:26 ` Michael S. Tsirkin
@ 2024-04-09 9:56 ` Heng Qi
2024-04-10 3:30 ` [PATCH v5] " lyx634449800
1 sibling, 1 reply; 15+ messages in thread
From: Heng Qi @ 2024-04-09 9:56 UTC (permalink / raw)
To: lyx634449800
Cc: angus.chen, virtualization, xuanzhuo, linux-kernel,
Michael S. Tsirkin, Jason Wang
在 2024/4/9 下午4:58, lyx634449800 写道:
> From: Yuxue Liu <yuxue.liu@jaguarmicro.com>
>
> When there is a ctlq and it doesn't require interrupt
> callbacks,the original method of calculating vectors
> wastes hardware msi or msix resources as well as system
> IRQ resources.
>
> When conducting performance testing using testpmd in the
> guest os, it was found that the performance was lower compared
> to directly using vfio-pci to passthrough the device
>
> In scenarios where the virtio device in the guest os does
> not utilize interrupts, the vdpa driver still configures
> the hardware's msix vector. Therefore, the hardware still
> sends interrupts to the host os. Because of this unnecessary
> action by the hardware, hardware performance decreases, and
> it also affects the performance of the host os.
>
> Before modification:(interrupt mode)
> 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
>
> After modification:(interrupt mode)
> 32: 0 0 1 7 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> 33: 36 0 3 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-config
>
> Before modification:(virtio pmd mode for guest os)
> 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
>
> After modification:(virtio pmd mode for guest os)
> 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-config
>
> To verify the use of the virtio PMD mode in the guest operating
> system, the following patch needs to be applied to QEMU:
> https://lore.kernel.org/all/20240408073311.2049-1-yuxue.liu@jaguarmicro.com
>
> Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>
> Acked-by: Jason Wang <jasowang@redhat.com>
> ---
> V4: Update the title and assign values to uninitialized variables
> V3: delete unused variables and add validation records
> V2: fix when allocating IRQs, scan all queues
>
> drivers/vdpa/virtio_pci/vp_vdpa.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
> index df5f4a3bccb5..74bc8adfc7e8 100644
> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> @@ -160,7 +160,14 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> struct pci_dev *pdev = mdev->pci_dev;
> int i, ret, irq;
> int queues = vp_vdpa->queues;
> - int vectors = queues + 1;
> + int vectors = 0;
> + int msix_vec = 0;
> +
> + for (i = 0; i < queues; i++) {
> + if (vp_vdpa->vring[i].cb.callback)
> + vectors++;
> + }
> + vectors++;
>
> ret = pci_alloc_irq_vectors(pdev, vectors, vectors, PCI_IRQ_MSIX);
> if (ret != vectors) {
> @@ -173,9 +180,12 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> vp_vdpa->vectors = vectors;
>
> for (i = 0; i < queues; i++) {
> + if (!vp_vdpa->vring[i].cb.callback)
> + continue;
> +
> snprintf(vp_vdpa->vring[i].msix_name, VP_VDPA_NAME_SIZE,
> "vp-vdpa[%s]-%d\n", pci_name(pdev), i);
> - irq = pci_irq_vector(pdev, i);
> + irq = pci_irq_vector(pdev, msix_vec);
> ret = devm_request_irq(&pdev->dev, irq,
> vp_vdpa_vq_handler,
> 0, vp_vdpa->vring[i].msix_name,
> @@ -185,21 +195,22 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> "vp_vdpa: fail to request irq for vq %d\n", i);
> goto err;
> }
> - vp_modern_queue_vector(mdev, i, i);
> + vp_modern_queue_vector(mdev, i, msix_vec);
> vp_vdpa->vring[i].irq = irq;
> + msix_vec++;
> }
>
> snprintf(vp_vdpa->msix_name, VP_VDPA_NAME_SIZE, "vp-vdpa[%s]-config\n",
> pci_name(pdev));
> - irq = pci_irq_vector(pdev, queues);
> + irq = pci_irq_vector(pdev, msix_vec);
> ret = devm_request_irq(&pdev->dev, irq, vp_vdpa_config_handler, 0,
> vp_vdpa->msix_name, vp_vdpa);
> if (ret) {
> dev_err(&pdev->dev,
> - "vp_vdpa: fail to request irq for vq %d\n", i);
> + "vp_vdpa: fail to request irq for config, ret %d\n", ret);
> goto err;
> }
> - vp_modern_config_vector(mdev, queues);
> + vp_modern_config_vector(mdev, msix_vec);
> vp_vdpa->config_irq = irq;
>
> return 0;
Reviewed-by: Heng Qi <hengqi@linux.alibaba.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v5] vp_vdpa: don't allocate unused msix vectors
2024-04-09 9:56 ` Heng Qi
@ 2024-04-10 3:30 ` lyx634449800
2024-04-22 12:08 ` Michael S. Tsirkin
0 siblings, 1 reply; 15+ messages in thread
From: lyx634449800 @ 2024-04-10 3:30 UTC (permalink / raw)
To: mst, jasowang
Cc: angus.chen, virtualization, xuanzhuo, yuxue.liu, linux-kernel, Heng Qi
From: Yuxue Liu <yuxue.liu@jaguarmicro.com>
When there is a ctlq and it doesn't require interrupt
callbacks,the original method of calculating vectors
wastes hardware msi or msix resources as well as system
IRQ resources.
When conducting performance testing using testpmd in the
guest os, it was found that the performance was lower compared
to directly using vfio-pci to passthrough the device
In scenarios where the virtio device in the guest os does
not utilize interrupts, the vdpa driver still configures
the hardware's msix vector. Therefore, the hardware still
sends interrupts to the host os. Because of this unnecessary
action by the hardware, hardware performance decreases, and
it also affects the performance of the host os.
Before modification:(interrupt mode)
32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
After modification:(interrupt mode)
32: 0 0 1 7 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
33: 36 0 3 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-config
Before modification:(virtio pmd mode for guest os)
32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
After modification:(virtio pmd mode for guest os)
32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-config
To verify the use of the virtio PMD mode in the guest operating
system, the following patch needs to be applied to QEMU:
https://lore.kernel.org/all/20240408073311.2049-1-yuxue.liu@jaguarmicro.com
Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Reviewed-by: Heng Qi <hengqi@linux.alibaba.com>
---
V5: modify the description of the printout when an exception occurs
V4: update the title and assign values to uninitialized variables
V3: delete unused variables and add validation records
V2: fix when allocating IRQs, scan all queues
drivers/vdpa/virtio_pci/vp_vdpa.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
index df5f4a3bccb5..8de0224e9ec2 100644
--- a/drivers/vdpa/virtio_pci/vp_vdpa.c
+++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
@@ -160,7 +160,13 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
struct pci_dev *pdev = mdev->pci_dev;
int i, ret, irq;
int queues = vp_vdpa->queues;
- int vectors = queues + 1;
+ int vectors = 1;
+ int msix_vec = 0;
+
+ for (i = 0; i < queues; i++) {
+ if (vp_vdpa->vring[i].cb.callback)
+ vectors++;
+ }
ret = pci_alloc_irq_vectors(pdev, vectors, vectors, PCI_IRQ_MSIX);
if (ret != vectors) {
@@ -173,9 +179,12 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
vp_vdpa->vectors = vectors;
for (i = 0; i < queues; i++) {
+ if (!vp_vdpa->vring[i].cb.callback)
+ continue;
+
snprintf(vp_vdpa->vring[i].msix_name, VP_VDPA_NAME_SIZE,
"vp-vdpa[%s]-%d\n", pci_name(pdev), i);
- irq = pci_irq_vector(pdev, i);
+ irq = pci_irq_vector(pdev, msix_vec);
ret = devm_request_irq(&pdev->dev, irq,
vp_vdpa_vq_handler,
0, vp_vdpa->vring[i].msix_name,
@@ -185,21 +194,22 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
"vp_vdpa: fail to request irq for vq %d\n", i);
goto err;
}
- vp_modern_queue_vector(mdev, i, i);
+ vp_modern_queue_vector(mdev, i, msix_vec);
vp_vdpa->vring[i].irq = irq;
+ msix_vec++;
}
snprintf(vp_vdpa->msix_name, VP_VDPA_NAME_SIZE, "vp-vdpa[%s]-config\n",
pci_name(pdev));
- irq = pci_irq_vector(pdev, queues);
+ irq = pci_irq_vector(pdev, msix_vec);
ret = devm_request_irq(&pdev->dev, irq, vp_vdpa_config_handler, 0,
vp_vdpa->msix_name, vp_vdpa);
if (ret) {
dev_err(&pdev->dev,
- "vp_vdpa: fail to request irq for vq %d\n", i);
+ "vp_vdpa: fail to request irq for config: %d\n", ret);
goto err;
}
- vp_modern_config_vector(mdev, queues);
+ vp_modern_config_vector(mdev, msix_vec);
vp_vdpa->config_irq = irq;
return 0;
--
2.43.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v5] vp_vdpa: don't allocate unused msix vectors
2024-04-10 3:30 ` [PATCH v5] " lyx634449800
@ 2024-04-22 12:08 ` Michael S. Tsirkin
2024-04-23 1:39 ` 回复: " Gavin Liu
0 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2024-04-22 12:08 UTC (permalink / raw)
To: lyx634449800
Cc: jasowang, angus.chen, virtualization, xuanzhuo, linux-kernel, Heng Qi
On Wed, Apr 10, 2024 at 11:30:20AM +0800, lyx634449800 wrote:
> From: Yuxue Liu <yuxue.liu@jaguarmicro.com>
>
> When there is a ctlq and it doesn't require interrupt
> callbacks,the original method of calculating vectors
> wastes hardware msi or msix resources as well as system
> IRQ resources.
>
> When conducting performance testing using testpmd in the
> guest os, it was found that the performance was lower compared
> to directly using vfio-pci to passthrough the device
>
> In scenarios where the virtio device in the guest os does
> not utilize interrupts, the vdpa driver still configures
> the hardware's msix vector. Therefore, the hardware still
> sends interrupts to the host os.
I just have a question on this part. How come hardware
sends interrupts does not guest driver disable them?
> Because of this unnecessary
> action by the hardware, hardware performance decreases, and
> it also affects the performance of the host os.
>
> Before modification:(interrupt mode)
> 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
>
> After modification:(interrupt mode)
> 32: 0 0 1 7 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> 33: 36 0 3 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-config
>
> Before modification:(virtio pmd mode for guest os)
> 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
>
> After modification:(virtio pmd mode for guest os)
> 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-config
>
> To verify the use of the virtio PMD mode in the guest operating
> system, the following patch needs to be applied to QEMU:
> https://lore.kernel.org/all/20240408073311.2049-1-yuxue.liu@jaguarmicro.com
>
> Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>
> Acked-by: Jason Wang <jasowang@redhat.com>
> Reviewed-by: Heng Qi <hengqi@linux.alibaba.com>
> ---
> V5: modify the description of the printout when an exception occurs
> V4: update the title and assign values to uninitialized variables
> V3: delete unused variables and add validation records
> V2: fix when allocating IRQs, scan all queues
>
> drivers/vdpa/virtio_pci/vp_vdpa.c | 22 ++++++++++++++++------
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
> index df5f4a3bccb5..8de0224e9ec2 100644
> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> @@ -160,7 +160,13 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> struct pci_dev *pdev = mdev->pci_dev;
> int i, ret, irq;
> int queues = vp_vdpa->queues;
> - int vectors = queues + 1;
> + int vectors = 1;
> + int msix_vec = 0;
> +
> + for (i = 0; i < queues; i++) {
> + if (vp_vdpa->vring[i].cb.callback)
> + vectors++;
> + }
>
> ret = pci_alloc_irq_vectors(pdev, vectors, vectors, PCI_IRQ_MSIX);
> if (ret != vectors) {
> @@ -173,9 +179,12 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> vp_vdpa->vectors = vectors;
>
> for (i = 0; i < queues; i++) {
> + if (!vp_vdpa->vring[i].cb.callback)
> + continue;
> +
> snprintf(vp_vdpa->vring[i].msix_name, VP_VDPA_NAME_SIZE,
> "vp-vdpa[%s]-%d\n", pci_name(pdev), i);
> - irq = pci_irq_vector(pdev, i);
> + irq = pci_irq_vector(pdev, msix_vec);
> ret = devm_request_irq(&pdev->dev, irq,
> vp_vdpa_vq_handler,
> 0, vp_vdpa->vring[i].msix_name,
> @@ -185,21 +194,22 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> "vp_vdpa: fail to request irq for vq %d\n", i);
> goto err;
> }
> - vp_modern_queue_vector(mdev, i, i);
> + vp_modern_queue_vector(mdev, i, msix_vec);
> vp_vdpa->vring[i].irq = irq;
> + msix_vec++;
> }
>
> snprintf(vp_vdpa->msix_name, VP_VDPA_NAME_SIZE, "vp-vdpa[%s]-config\n",
> pci_name(pdev));
> - irq = pci_irq_vector(pdev, queues);
> + irq = pci_irq_vector(pdev, msix_vec);
> ret = devm_request_irq(&pdev->dev, irq, vp_vdpa_config_handler, 0,
> vp_vdpa->msix_name, vp_vdpa);
> if (ret) {
> dev_err(&pdev->dev,
> - "vp_vdpa: fail to request irq for vq %d\n", i);
> + "vp_vdpa: fail to request irq for config: %d\n", ret);
> goto err;
> }
> - vp_modern_config_vector(mdev, queues);
> + vp_modern_config_vector(mdev, msix_vec);
> vp_vdpa->config_irq = irq;
>
> return 0;
> --
> 2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* 回复: [PATCH v5] vp_vdpa: don't allocate unused msix vectors
2024-04-22 12:08 ` Michael S. Tsirkin
@ 2024-04-23 1:39 ` Gavin Liu
2024-04-23 8:35 ` Michael S. Tsirkin
0 siblings, 1 reply; 15+ messages in thread
From: Gavin Liu @ 2024-04-23 1:39 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: jasowang, Angus Chen, virtualization, xuanzhuo, linux-kernel, Heng Qi
On Wed, Apr 10, 2024 at 11:30:20AM +0800, lyx634449800 wrote:
> From: Yuxue Liu <yuxue.liu@jaguarmicro.com>
>
> When there is a ctlq and it doesn't require interrupt callbacks,the
> original method of calculating vectors wastes hardware msi or msix
> resources as well as system IRQ resources.
>
> When conducting performance testing using testpmd in the guest os, it
> was found that the performance was lower compared to directly using
> vfio-pci to passthrough the device
>
> In scenarios where the virtio device in the guest os does not utilize
> interrupts, the vdpa driver still configures the hardware's msix
> vector. Therefore, the hardware still sends interrupts to the host os.
>I just have a question on this part. How come hardware sends interrupts does not guest driver disable them?
1：Assuming the guest OS's Virtio device is using PMD mode, QEMU sets the call fd to -1
2：On the host side, the vhost_vdpa program will set vp_vdpa->vring[i].cb.callback to invalid
3：Before the modification, the vp_vdpa_request_irq function does not check whether
vp_vdpa->vring[i].cb.callback is valid. Instead, it enables the hardware's MSIX
interrupts based on the number of queues of the device
----- Original Message -----
From: Michael S. Tsirkin mst@redhat.com
Sent: April 22, 2024 20:09
To: Gavin Liu gavin.liu@jaguarmicro.com
Cc: jasowang@redhat.com; Angus Chen angus.chen@jaguarmicro.com; virtualization@lists.linux.dev; xuanzhuo@linux.alibaba.com; linux-kernel@vger.kernel.org; Heng Qi hengqi@linux.alibaba.com
Subject: Re: [PATCH v5] vp_vdpa: don't allocate unused msix vectors
External Mail: This email originated from OUTSIDE of the organization!
Do not click links, open attachments or provide ANY information unless you recognize the sender and know the content is safe.
On Wed, Apr 10, 2024 at 11:30:20AM +0800, lyx634449800 wrote:
> From: Yuxue Liu <yuxue.liu@jaguarmicro.com>
>
> When there is a ctlq and it doesn't require interrupt callbacks,the
> original method of calculating vectors wastes hardware msi or msix
> resources as well as system IRQ resources.
>
> When conducting performance testing using testpmd in the guest os, it
> was found that the performance was lower compared to directly using
> vfio-pci to passthrough the device
>
> In scenarios where the virtio device in the guest os does not utilize
> interrupts, the vdpa driver still configures the hardware's msix
> vector. Therefore, the hardware still sends interrupts to the host os.
I just have a question on this part. How come hardware sends interrupts does not guest driver disable them?
> Because of this unnecessary
> action by the hardware, hardware performance decreases, and it also
> affects the performance of the host os.
>
> Before modification:(interrupt mode)
> 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
>
> After modification:(interrupt mode)
> 32: 0 0 1 7 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> 33: 36 0 3 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-config
>
> Before modification:(virtio pmd mode for guest os)
> 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
>
> After modification:(virtio pmd mode for guest os)
> 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-config
>
> To verify the use of the virtio PMD mode in the guest operating
> system, the following patch needs to be applied to QEMU:
> https://lore.kernel.org/all/20240408073311.2049-1-yuxue.liu@jaguarmicr
> o.com
>
> Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>
> Acked-by: Jason Wang <jasowang@redhat.com>
> Reviewed-by: Heng Qi <hengqi@linux.alibaba.com>
> ---
> V5: modify the description of the printout when an exception occurs
> V4: update the title and assign values to uninitialized variables
> V3: delete unused variables and add validation records
> V2: fix when allocating IRQs, scan all queues
>
> drivers/vdpa/virtio_pci/vp_vdpa.c | 22 ++++++++++++++++------
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c
> b/drivers/vdpa/virtio_pci/vp_vdpa.c
> index df5f4a3bccb5..8de0224e9ec2 100644
> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> @@ -160,7 +160,13 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> struct pci_dev *pdev = mdev->pci_dev;
> int i, ret, irq;
> int queues = vp_vdpa->queues;
> - int vectors = queues + 1;
> + int vectors = 1;
> + int msix_vec = 0;
> +
> + for (i = 0; i < queues; i++) {
> + if (vp_vdpa->vring[i].cb.callback)
> + vectors++;
> + }
>
> ret = pci_alloc_irq_vectors(pdev, vectors, vectors, PCI_IRQ_MSIX);
> if (ret != vectors) {
> @@ -173,9 +179,12 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> vp_vdpa->vectors = vectors;
>
> for (i = 0; i < queues; i++) {
> + if (!vp_vdpa->vring[i].cb.callback)
> + continue;
> +
> snprintf(vp_vdpa->vring[i].msix_name, VP_VDPA_NAME_SIZE,
> "vp-vdpa[%s]-%d\n", pci_name(pdev), i);
> - irq = pci_irq_vector(pdev, i);
> + irq = pci_irq_vector(pdev, msix_vec);
> ret = devm_request_irq(&pdev->dev, irq,
> vp_vdpa_vq_handler,
> 0, vp_vdpa->vring[i].msix_name,
> @@ -185,21 +194,22 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> "vp_vdpa: fail to request irq for vq %d\n", i);
> goto err;
> }
> - vp_modern_queue_vector(mdev, i, i);
> + vp_modern_queue_vector(mdev, i, msix_vec);
> vp_vdpa->vring[i].irq = irq;
> + msix_vec++;
> }
>
> snprintf(vp_vdpa->msix_name, VP_VDPA_NAME_SIZE, "vp-vdpa[%s]-config\n",
> pci_name(pdev));
> - irq = pci_irq_vector(pdev, queues);
> + irq = pci_irq_vector(pdev, msix_vec);
> ret = devm_request_irq(&pdev->dev, irq, vp_vdpa_config_handler, 0,
> vp_vdpa->msix_name, vp_vdpa);
> if (ret) {
> dev_err(&pdev->dev,
> - "vp_vdpa: fail to request irq for vq %d\n", i);
> + "vp_vdpa: fail to request irq for config: %d\n",
> + ret);
> goto err;
> }
> - vp_modern_config_vector(mdev, queues);
> + vp_modern_config_vector(mdev, msix_vec);
> vp_vdpa->config_irq = irq;
>
> return 0;
> --
> 2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: 回复: [PATCH v5] vp_vdpa: don't allocate unused msix vectors
2024-04-23 1:39 ` 回复: " Gavin Liu
@ 2024-04-23 8:35 ` Michael S. Tsirkin
2024-04-23 8:42 ` Angus Chen
0 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2024-04-23 8:35 UTC (permalink / raw)
To: Gavin Liu
Cc: jasowang, Angus Chen, virtualization, xuanzhuo, linux-kernel, Heng Qi
On Tue, Apr 23, 2024 at 01:39:17AM +0000, Gavin Liu wrote:
> On Wed, Apr 10, 2024 at 11:30:20AM +0800, lyx634449800 wrote:
> > From: Yuxue Liu <yuxue.liu@jaguarmicro.com>
> >
> > When there is a ctlq and it doesn't require interrupt callbacks,the
> > original method of calculating vectors wastes hardware msi or msix
> > resources as well as system IRQ resources.
> >
> > When conducting performance testing using testpmd in the guest os, it
> > was found that the performance was lower compared to directly using
> > vfio-pci to passthrough the device
> >
> > In scenarios where the virtio device in the guest os does not utilize
> > interrupts, the vdpa driver still configures the hardware's msix
> > vector. Therefore, the hardware still sends interrupts to the host os.
>
> >I just have a question on this part. How come hardware sends interrupts does not guest driver disable them?
>
> 1：Assuming the guest OS's Virtio device is using PMD mode, QEMU sets the call fd to -1
> 2：On the host side, the vhost_vdpa program will set vp_vdpa->vring[i].cb.callback to invalid
> 3：Before the modification, the vp_vdpa_request_irq function does not check whether
> vp_vdpa->vring[i].cb.callback is valid. Instead, it enables the hardware's MSIX
> interrupts based on the number of queues of the device
>
So MSIX is enabled but why would it trigger? virtio PMD in poll mode
presumably suppresses interrupts after all.
>
>
> ----- Original Message -----
> From: Michael S. Tsirkin mst@redhat.com
> Sent: April 22, 2024 20:09
> To: Gavin Liu gavin.liu@jaguarmicro.com
> Cc: jasowang@redhat.com; Angus Chen angus.chen@jaguarmicro.com; virtualization@lists.linux.dev; xuanzhuo@linux.alibaba.com; linux-kernel@vger.kernel.org; Heng Qi hengqi@linux.alibaba.com
> Subject: Re: [PATCH v5] vp_vdpa: don't allocate unused msix vectors
>
>
>
> External Mail: This email originated from OUTSIDE of the organization!
> Do not click links, open attachments or provide ANY information unless you recognize the sender and know the content is safe.
>
>
> On Wed, Apr 10, 2024 at 11:30:20AM +0800, lyx634449800 wrote:
> > From: Yuxue Liu <yuxue.liu@jaguarmicro.com>
> >
> > When there is a ctlq and it doesn't require interrupt callbacks,the
> > original method of calculating vectors wastes hardware msi or msix
> > resources as well as system IRQ resources.
> >
> > When conducting performance testing using testpmd in the guest os, it
> > was found that the performance was lower compared to directly using
> > vfio-pci to passthrough the device
> >
> > In scenarios where the virtio device in the guest os does not utilize
> > interrupts, the vdpa driver still configures the hardware's msix
> > vector. Therefore, the hardware still sends interrupts to the host os.
>
> I just have a question on this part. How come hardware sends interrupts does not guest driver disable them?
>
> > Because of this unnecessary
> > action by the hardware, hardware performance decreases, and it also
> > affects the performance of the host os.
> >
> > Before modification:(interrupt mode)
> > 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> > 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> > 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> > 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
> >
> > After modification:(interrupt mode)
> > 32: 0 0 1 7 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> > 33: 36 0 3 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> > 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-config
> >
> > Before modification:(virtio pmd mode for guest os)
> > 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> > 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> > 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> > 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
> >
> > After modification:(virtio pmd mode for guest os)
> > 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-config
> >
> > To verify the use of the virtio PMD mode in the guest operating
> > system, the following patch needs to be applied to QEMU:
> > https://lore.kernel.org/all/20240408073311.2049-1-yuxue.liu@jaguarmicr
> > o.com
> >
> > Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>
> > Acked-by: Jason Wang <jasowang@redhat.com>
> > Reviewed-by: Heng Qi <hengqi@linux.alibaba.com>
> > ---
> > V5: modify the description of the printout when an exception occurs
> > V4: update the title and assign values to uninitialized variables
> > V3: delete unused variables and add validation records
> > V2: fix when allocating IRQs, scan all queues
> >
> > drivers/vdpa/virtio_pci/vp_vdpa.c | 22 ++++++++++++++++------
> > 1 file changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c
> > b/drivers/vdpa/virtio_pci/vp_vdpa.c
> > index df5f4a3bccb5..8de0224e9ec2 100644
> > --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> > +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> > @@ -160,7 +160,13 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> > struct pci_dev *pdev = mdev->pci_dev;
> > int i, ret, irq;
> > int queues = vp_vdpa->queues;
> > - int vectors = queues + 1;
> > + int vectors = 1;
> > + int msix_vec = 0;
> > +
> > + for (i = 0; i < queues; i++) {
> > + if (vp_vdpa->vring[i].cb.callback)
> > + vectors++;
> > + }
> >
> > ret = pci_alloc_irq_vectors(pdev, vectors, vectors, PCI_IRQ_MSIX);
> > if (ret != vectors) {
> > @@ -173,9 +179,12 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> > vp_vdpa->vectors = vectors;
> >
> > for (i = 0; i < queues; i++) {
> > + if (!vp_vdpa->vring[i].cb.callback)
> > + continue;
> > +
> > snprintf(vp_vdpa->vring[i].msix_name, VP_VDPA_NAME_SIZE,
> > "vp-vdpa[%s]-%d\n", pci_name(pdev), i);
> > - irq = pci_irq_vector(pdev, i);
> > + irq = pci_irq_vector(pdev, msix_vec);
> > ret = devm_request_irq(&pdev->dev, irq,
> > vp_vdpa_vq_handler,
> > 0, vp_vdpa->vring[i].msix_name,
> > @@ -185,21 +194,22 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
> > "vp_vdpa: fail to request irq for vq %d\n", i);
> > goto err;
> > }
> > - vp_modern_queue_vector(mdev, i, i);
> > + vp_modern_queue_vector(mdev, i, msix_vec);
> > vp_vdpa->vring[i].irq = irq;
> > + msix_vec++;
> > }
> >
> > snprintf(vp_vdpa->msix_name, VP_VDPA_NAME_SIZE, "vp-vdpa[%s]-config\n",
> > pci_name(pdev));
> > - irq = pci_irq_vector(pdev, queues);
> > + irq = pci_irq_vector(pdev, msix_vec);
> > ret = devm_request_irq(&pdev->dev, irq, vp_vdpa_config_handler, 0,
> > vp_vdpa->msix_name, vp_vdpa);
> > if (ret) {
> > dev_err(&pdev->dev,
> > - "vp_vdpa: fail to request irq for vq %d\n", i);
> > + "vp_vdpa: fail to request irq for config: %d\n",
> > + ret);
> > goto err;
> > }
> > - vp_modern_config_vector(mdev, queues);
> > + vp_modern_config_vector(mdev, msix_vec);
> > vp_vdpa->config_irq = irq;
> >
> > return 0;
> > --
> > 2.43.0
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: 回复: [PATCH v5] vp_vdpa: don't allocate unused msix vectors
2024-04-23 8:35 ` Michael S. Tsirkin
@ 2024-04-23 8:42 ` Angus Chen
2024-04-23 9:25 ` 回复: " Gavin Liu
2024-04-25 22:09 ` Michael S. Tsirkin
0 siblings, 2 replies; 15+ messages in thread
From: Angus Chen @ 2024-04-23 8:42 UTC (permalink / raw)
To: Michael S. Tsirkin, Gavin Liu
Cc: jasowang, virtualization, xuanzhuo, linux-kernel, Heng Qi
Hi mst.
> -----Original Message-----
> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, April 23, 2024 4:35 PM
> To: Gavin Liu <gavin.liu@jaguarmicro.com>
> Cc: jasowang@redhat.com; Angus Chen <angus.chen@jaguarmicro.com>;
> virtualization@lists.linux.dev; xuanzhuo@linux.alibaba.com;
> linux-kernel@vger.kernel.org; Heng Qi <hengqi@linux.alibaba.com>
> Subject: Re: 回复: [PATCH v5] vp_vdpa: don't allocate unused msix vectors
>
> On Tue, Apr 23, 2024 at 01:39:17AM +0000, Gavin Liu wrote:
> > On Wed, Apr 10, 2024 at 11:30:20AM +0800, lyx634449800 wrote:
> > > From: Yuxue Liu <yuxue.liu@jaguarmicro.com>
> > >
> > > When there is a ctlq and it doesn't require interrupt callbacks,the
> > > original method of calculating vectors wastes hardware msi or msix
> > > resources as well as system IRQ resources.
> > >
> > > When conducting performance testing using testpmd in the guest os, it
> > > was found that the performance was lower compared to directly using
> > > vfio-pci to passthrough the device
> > >
> > > In scenarios where the virtio device in the guest os does not utilize
> > > interrupts, the vdpa driver still configures the hardware's msix
> > > vector. Therefore, the hardware still sends interrupts to the host os.
> >
> > >I just have a question on this part. How come hardware sends interrupts does
> not guest driver disable them?
> >
> > 1：Assuming the guest OS's Virtio device is using PMD mode, QEMU sets
> the call fd to -1
> > 2：On the host side, the vhost_vdpa program will set
> vp_vdpa->vring[i].cb.callback to invalid
> > 3：Before the modification, the vp_vdpa_request_irq function does not
> check whether
> > vp_vdpa->vring[i].cb.callback is valid. Instead, it enables the
> hardware's MSIX
> > interrupts based on the number of queues of the device
> >
>
> So MSIX is enabled but why would it trigger? virtio PMD in poll mode
> presumably suppresses interrupts after all.
Virtio pmd is in the guest,but in host side,the msix is enabled,then the device will triger
Interrupt normally. I analysed this bug before,and I think gavin is right.
Did I make it clear?
>
> >
> >
> > ----- Original Message -----
> > From: Michael S. Tsirkin mst@redhat.com
> > Sent: April 22, 2024 20:09
> > To: Gavin Liu gavin.liu@jaguarmicro.com
> > Cc: jasowang@redhat.com; Angus Chen angus.chen@jaguarmicro.com;
> virtualization@lists.linux.dev; xuanzhuo@linux.alibaba.com;
> linux-kernel@vger.kernel.org; Heng Qi hengqi@linux.alibaba.com
> > Subject: Re: [PATCH v5] vp_vdpa: don't allocate unused msix vectors
> >
> >
> >
> > External Mail: This email originated from OUTSIDE of the organization!
> > Do not click links, open attachments or provide ANY information unless you
> recognize the sender and know the content is safe.
> >
> >
> > On Wed, Apr 10, 2024 at 11:30:20AM +0800, lyx634449800 wrote:
> > > From: Yuxue Liu <yuxue.liu@jaguarmicro.com>
> > >
> > > When there is a ctlq and it doesn't require interrupt callbacks,the
> > > original method of calculating vectors wastes hardware msi or msix
> > > resources as well as system IRQ resources.
> > >
> > > When conducting performance testing using testpmd in the guest os, it
> > > was found that the performance was lower compared to directly using
> > > vfio-pci to passthrough the device
> > >
> > > In scenarios where the virtio device in the guest os does not utilize
> > > interrupts, the vdpa driver still configures the hardware's msix
> > > vector. Therefore, the hardware still sends interrupts to the host os.
> >
> > I just have a question on this part. How come hardware sends interrupts does
> not guest driver disable them?
> >
> > > Because of this unnecessary
> > > action by the hardware, hardware performance decreases, and it also
> > > affects the performance of the host os.
> > >
> > > Before modification:(interrupt mode)
> > > 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> > > 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> > > 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> > > 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
> > >
> > > After modification:(interrupt mode)
> > > 32: 0 0 1 7 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> > > 33: 36 0 3 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> > > 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-config
> > >
> > > Before modification:(virtio pmd mode for guest os)
> > > 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> > > 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> > > 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> > > 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
> > >
> > > After modification:(virtio pmd mode for guest os)
> > > 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-config
> > >
> > > To verify the use of the virtio PMD mode in the guest operating
> > > system, the following patch needs to be applied to QEMU:
> > > https://lore.kernel.org/all/20240408073311.2049-1-yuxue.liu@jaguarmicr
> > > o.com
> > >
> > > Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>
> > > Acked-by: Jason Wang <jasowang@redhat.com>
> > > Reviewed-by: Heng Qi <hengqi@linux.alibaba.com>
> > > ---
> > > V5: modify the description of the printout when an exception occurs
> > > V4: update the title and assign values to uninitialized variables
> > > V3: delete unused variables and add validation records
> > > V2: fix when allocating IRQs, scan all queues
> > >
> > > drivers/vdpa/virtio_pci/vp_vdpa.c | 22 ++++++++++++++++------
> > > 1 file changed, 16 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c
> > > b/drivers/vdpa/virtio_pci/vp_vdpa.c
> > > index df5f4a3bccb5..8de0224e9ec2 100644
> > > --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> > > +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> > > @@ -160,7 +160,13 @@ static int vp_vdpa_request_irq(struct vp_vdpa
> *vp_vdpa)
> > > struct pci_dev *pdev = mdev->pci_dev;
> > > int i, ret, irq;
> > > int queues = vp_vdpa->queues;
> > > - int vectors = queues + 1;
> > > + int vectors = 1;
> > > + int msix_vec = 0;
> > > +
> > > + for (i = 0; i < queues; i++) {
> > > + if (vp_vdpa->vring[i].cb.callback)
> > > + vectors++;
> > > + }
> > >
> > > ret = pci_alloc_irq_vectors(pdev, vectors, vectors, PCI_IRQ_MSIX);
> > > if (ret != vectors) {
> > > @@ -173,9 +179,12 @@ static int vp_vdpa_request_irq(struct vp_vdpa
> *vp_vdpa)
> > > vp_vdpa->vectors = vectors;
> > >
> > > for (i = 0; i < queues; i++) {
> > > + if (!vp_vdpa->vring[i].cb.callback)
> > > + continue;
> > > +
> > > snprintf(vp_vdpa->vring[i].msix_name,
> VP_VDPA_NAME_SIZE,
> > > "vp-vdpa[%s]-%d\n", pci_name(pdev), i);
> > > - irq = pci_irq_vector(pdev, i);
> > > + irq = pci_irq_vector(pdev, msix_vec);
> > > ret = devm_request_irq(&pdev->dev, irq,
> > > vp_vdpa_vq_handler,
> > > 0,
> vp_vdpa->vring[i].msix_name,
> > > @@ -185,21 +194,22 @@ static int vp_vdpa_request_irq(struct vp_vdpa
> *vp_vdpa)
> > > "vp_vdpa: fail to request irq for
> vq %d\n", i);
> > > goto err;
> > > }
> > > - vp_modern_queue_vector(mdev, i, i);
> > > + vp_modern_queue_vector(mdev, i, msix_vec);
> > > vp_vdpa->vring[i].irq = irq;
> > > + msix_vec++;
> > > }
> > >
> > > snprintf(vp_vdpa->msix_name, VP_VDPA_NAME_SIZE,
> "vp-vdpa[%s]-config\n",
> > > pci_name(pdev));
> > > - irq = pci_irq_vector(pdev, queues);
> > > + irq = pci_irq_vector(pdev, msix_vec);
> > > ret = devm_request_irq(&pdev->dev, irq, vp_vdpa_config_handler,
> 0,
> > > vp_vdpa->msix_name, vp_vdpa);
> > > if (ret) {
> > > dev_err(&pdev->dev,
> > > - "vp_vdpa: fail to request irq for vq %d\n", i);
> > > + "vp_vdpa: fail to request irq for config: %d\n",
> > > + ret);
> > > goto err;
> > > }
> > > - vp_modern_config_vector(mdev, queues);
> > > + vp_modern_config_vector(mdev, msix_vec);
> > > vp_vdpa->config_irq = irq;
> > >
> > > return 0;
> > > --
> > > 2.43.0
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* 回复: 回复: [PATCH v5] vp_vdpa: don't allocate unused msix vectors
2024-04-23 8:42 ` Angus Chen
@ 2024-04-23 9:25 ` Gavin Liu
2024-04-25 22:09 ` Michael S. Tsirkin
1 sibling, 0 replies; 15+ messages in thread
From: Gavin Liu @ 2024-04-23 9:25 UTC (permalink / raw)
To: Angus Chen, Michael S. Tsirkin
Cc: jasowang, virtualization, xuanzhuo, linux-kernel, Heng Qi
> > When there is a ctlq and it doesn't require interrupt callbacks,the
> > original method of calculating vectors wastes hardware msi or msix
> > resources as well as system IRQ resources.
> >
> > When conducting performance testing using testpmd in the guest os,
> > it was found that the performance was lower compared to directly
> > using vfio-pci to passthrough the device
> >
> > In scenarios where the virtio device in the guest os does not
> > utilize interrupts, the vdpa driver still configures the hardware's
> > msix vector. Therefore, the hardware still sends interrupts to the host os.
>
> >I just have a question on this part. How come hardware sends interrupts does not guest driver disable them?
>
> 1：Assuming the guest OS's Virtio device is using PMD mode, QEMU sets the call fd to -1
> 2：On the host side, the vhost_vdpa program will set vp_vdpa->vring[i].cb.callback to invalid
> 3：Before the modification, the vp_vdpa_request_irq function does not check whether
> vp_vdpa->vring[i].cb.callback is valid. Instead, it enables the hardware's MSIX
> interrupts based on the number of queues of the device
>
> So MSIX is enabled but why would it trigger? virtio PMD in poll mode presumably suppresses interrupts after all.
I didn't express the test model clearly. The testing model is as follows:
----testpmd---- ----testpmd---
^ ^
| |
| |
| |
v v
----vfio pci--- ---vfio pci---
----pci device --- --pci device--
----guest os ----- ----guest os ------
---virtio device-- ---vfio device--
------qemu------- ------qemu-------
^ ^
| |
| |
| |
v v
----vhost_vdpa---- ----vfio pci----
------------------------host os--------------------------
----------------------hw----------------------------
VF1 VF2
After the hardware completes DMA, it will still send an interrupt to the host OS.
-----Original Message-----
From: Angus Chen angus.chen@jaguarmicro.com
Sent: April 23, 2024 16:43
To: Michael S. Tsirkin mst@redhat.com; Gavin Liu gavin.liu@jaguarmicro.com
Cc: jasowang@redhat.com; virtualization@lists.linux.dev; xuanzhuo@linux.alibaba.com; linux-kernel@vger.kernel.org; Heng Qi hengqi@linux.alibaba.com
Subject: RE: Reply: [PATCH v5] vp_vdpa: don't allocate unused msix vectors
Hi mst.
> -----Original Message-----
> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, April 23, 2024 4:35 PM
> To: Gavin Liu <gavin.liu@jaguarmicro.com>
> Cc: jasowang@redhat.com; Angus Chen <angus.chen@jaguarmicro.com>;
> virtualization@lists.linux.dev; xuanzhuo@linux.alibaba.com;
> linux-kernel@vger.kernel.org; Heng Qi <hengqi@linux.alibaba.com>
> Subject: Re: 回复: [PATCH v5] vp_vdpa: don't allocate unused msix
> vectors
>
> On Tue, Apr 23, 2024 at 01:39:17AM +0000, Gavin Liu wrote:
> > On Wed, Apr 10, 2024 at 11:30:20AM +0800, lyx634449800 wrote:
> > > From: Yuxue Liu <yuxue.liu@jaguarmicro.com>
> > >
> > > When there is a ctlq and it doesn't require interrupt
> > > callbacks,the original method of calculating vectors wastes
> > > hardware msi or msix resources as well as system IRQ resources.
> > >
> > > When conducting performance testing using testpmd in the guest os,
> > > it was found that the performance was lower compared to directly
> > > using vfio-pci to passthrough the device
> > >
> > > In scenarios where the virtio device in the guest os does not
> > > utilize interrupts, the vdpa driver still configures the
> > > hardware's msix vector. Therefore, the hardware still sends interrupts to the host os.
> >
> > >I just have a question on this part. How come hardware sends
> > >interrupts does
> not guest driver disable them?
> >
> > 1：Assuming the guest OS's Virtio device is using PMD mode, QEMU
> > sets
> the call fd to -1
> > 2：On the host side, the vhost_vdpa program will set
> vp_vdpa->vring[i].cb.callback to invalid
> > 3：Before the modification, the vp_vdpa_request_irq function does
> > not
> check whether
> > vp_vdpa->vring[i].cb.callback is valid. Instead, it enables
> > the
> hardware's MSIX
> > interrupts based on the number of queues of the device
> >
>
> So MSIX is enabled but why would it trigger? virtio PMD in poll mode
> presumably suppresses interrupts after all.
Virtio pmd is in the guest,but in host side,the msix is enabled,then the device will triger Interrupt normally. I analysed this bug before,and I think gavin is right.
Did I make it clear?
>
> >
> >
> > ----- Original Message -----
> > From: Michael S. Tsirkin mst@redhat.com
> > Sent: April 22, 2024 20:09
> > To: Gavin Liu gavin.liu@jaguarmicro.com
> > Cc: jasowang@redhat.com; Angus Chen angus.chen@jaguarmicro.com;
> virtualization@lists.linux.dev; xuanzhuo@linux.alibaba.com;
> linux-kernel@vger.kernel.org; Heng Qi hengqi@linux.alibaba.com
> > Subject: Re: [PATCH v5] vp_vdpa: don't allocate unused msix vectors
> >
> >
> >
> > External Mail: This email originated from OUTSIDE of the organization!
> > Do not click links, open attachments or provide ANY information
> > unless you
> recognize the sender and know the content is safe.
> >
> >
> > On Wed, Apr 10, 2024 at 11:30:20AM +0800, lyx634449800 wrote:
> > > From: Yuxue Liu <yuxue.liu@jaguarmicro.com>
> > >
> > > When there is a ctlq and it doesn't require interrupt
> > > callbacks,the original method of calculating vectors wastes
> > > hardware msi or msix resources as well as system IRQ resources.
> > >
> > > When conducting performance testing using testpmd in the guest os,
> > > it was found that the performance was lower compared to directly
> > > using vfio-pci to passthrough the device
> > >
> > > In scenarios where the virtio device in the guest os does not
> > > utilize interrupts, the vdpa driver still configures the
> > > hardware's msix vector. Therefore, the hardware still sends interrupts to the host os.
> >
> > I just have a question on this part. How come hardware sends
> > interrupts does
> not guest driver disable them?
> >
> > > Because of this unnecessary
> > > action by the hardware, hardware performance decreases, and it
> > > also affects the performance of the host os.
> > >
> > > Before modification:(interrupt mode)
> > > 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> > > 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> > > 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> > > 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
> > >
> > > After modification:(interrupt mode)
> > > 32: 0 0 1 7 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> > > 33: 36 0 3 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> > > 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-config
> > >
> > > Before modification:(virtio pmd mode for guest os)
> > > 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> > > 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> > > 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> > > 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
> > >
> > > After modification:(virtio pmd mode for guest os)
> > > 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-config
> > >
> > > To verify the use of the virtio PMD mode in the guest operating
> > > system, the following patch needs to be applied to QEMU:
> > > https://lore.kernel.org/all/20240408073311.2049-1-yuxue.liu@jaguar
> > > micr
> > > o.com
> > >
> > > Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>
> > > Acked-by: Jason Wang <jasowang@redhat.com>
> > > Reviewed-by: Heng Qi <hengqi@linux.alibaba.com>
> > > ---
> > > V5: modify the description of the printout when an exception
> > > occurs
> > > V4: update the title and assign values to uninitialized variables
> > > V3: delete unused variables and add validation records
> > > V2: fix when allocating IRQs, scan all queues
> > >
> > > drivers/vdpa/virtio_pci/vp_vdpa.c | 22 ++++++++++++++++------
> > > 1 file changed, 16 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c
> > > b/drivers/vdpa/virtio_pci/vp_vdpa.c
> > > index df5f4a3bccb5..8de0224e9ec2 100644
> > > --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> > > +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> > > @@ -160,7 +160,13 @@ static int vp_vdpa_request_irq(struct vp_vdpa
> *vp_vdpa)
> > > struct pci_dev *pdev = mdev->pci_dev;
> > > int i, ret, irq;
> > > int queues = vp_vdpa->queues;
> > > - int vectors = queues + 1;
> > > + int vectors = 1;
> > > + int msix_vec = 0;
> > > +
> > > + for (i = 0; i < queues; i++) {
> > > + if (vp_vdpa->vring[i].cb.callback)
> > > + vectors++;
> > > + }
> > >
> > > ret = pci_alloc_irq_vectors(pdev, vectors, vectors, PCI_IRQ_MSIX);
> > > if (ret != vectors) {
> > > @@ -173,9 +179,12 @@ static int vp_vdpa_request_irq(struct vp_vdpa
> *vp_vdpa)
> > > vp_vdpa->vectors = vectors;
> > >
> > > for (i = 0; i < queues; i++) {
> > > + if (!vp_vdpa->vring[i].cb.callback)
> > > + continue;
> > > +
> > > snprintf(vp_vdpa->vring[i].msix_name,
> VP_VDPA_NAME_SIZE,
> > > "vp-vdpa[%s]-%d\n", pci_name(pdev), i);
> > > - irq = pci_irq_vector(pdev, i);
> > > + irq = pci_irq_vector(pdev, msix_vec);
> > > ret = devm_request_irq(&pdev->dev, irq,
> > > vp_vdpa_vq_handler,
> > > 0,
> vp_vdpa->vring[i].msix_name,
> > > @@ -185,21 +194,22 @@ static int vp_vdpa_request_irq(struct
> > > vp_vdpa
> *vp_vdpa)
> > > "vp_vdpa: fail to request irq for
> vq %d\n", i);
> > > goto err;
> > > }
> > > - vp_modern_queue_vector(mdev, i, i);
> > > + vp_modern_queue_vector(mdev, i, msix_vec);
> > > vp_vdpa->vring[i].irq = irq;
> > > + msix_vec++;
> > > }
> > >
> > > snprintf(vp_vdpa->msix_name, VP_VDPA_NAME_SIZE,
> "vp-vdpa[%s]-config\n",
> > > pci_name(pdev));
> > > - irq = pci_irq_vector(pdev, queues);
> > > + irq = pci_irq_vector(pdev, msix_vec);
> > > ret = devm_request_irq(&pdev->dev, irq,
> > > vp_vdpa_config_handler,
> 0,
> > > vp_vdpa->msix_name, vp_vdpa);
> > > if (ret) {
> > > dev_err(&pdev->dev,
> > > - "vp_vdpa: fail to request irq for vq %d\n", i);
> > > + "vp_vdpa: fail to request irq for config:
> > > + %d\n", ret);
> > > goto err;
> > > }
> > > - vp_modern_config_vector(mdev, queues);
> > > + vp_modern_config_vector(mdev, msix_vec);
> > > vp_vdpa->config_irq = irq;
> > >
> > > return 0;
> > > --
> > > 2.43.0
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: 回复: [PATCH v5] vp_vdpa: don't allocate unused msix vectors
2024-04-23 8:42 ` Angus Chen
2024-04-23 9:25 ` 回复: " Gavin Liu
@ 2024-04-25 22:09 ` Michael S. Tsirkin
1 sibling, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2024-04-25 22:09 UTC (permalink / raw)
To: Angus Chen
Cc: Gavin Liu, jasowang, virtualization, xuanzhuo, linux-kernel, Heng Qi
On Tue, Apr 23, 2024 at 08:42:57AM +0000, Angus Chen wrote:
> Hi mst.
>
> > -----Original Message-----
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Tuesday, April 23, 2024 4:35 PM
> > To: Gavin Liu <gavin.liu@jaguarmicro.com>
> > Cc: jasowang@redhat.com; Angus Chen <angus.chen@jaguarmicro.com>;
> > virtualization@lists.linux.dev; xuanzhuo@linux.alibaba.com;
> > linux-kernel@vger.kernel.org; Heng Qi <hengqi@linux.alibaba.com>
> > Subject: Re: 回复: [PATCH v5] vp_vdpa: don't allocate unused msix vectors
> >
> > On Tue, Apr 23, 2024 at 01:39:17AM +0000, Gavin Liu wrote:
> > > On Wed, Apr 10, 2024 at 11:30:20AM +0800, lyx634449800 wrote:
> > > > From: Yuxue Liu <yuxue.liu@jaguarmicro.com>
> > > >
> > > > When there is a ctlq and it doesn't require interrupt callbacks,the
> > > > original method of calculating vectors wastes hardware msi or msix
> > > > resources as well as system IRQ resources.
> > > >
> > > > When conducting performance testing using testpmd in the guest os, it
> > > > was found that the performance was lower compared to directly using
> > > > vfio-pci to passthrough the device
> > > >
> > > > In scenarios where the virtio device in the guest os does not utilize
> > > > interrupts, the vdpa driver still configures the hardware's msix
> > > > vector. Therefore, the hardware still sends interrupts to the host os.
> > >
> > > >I just have a question on this part. How come hardware sends interrupts does
> > not guest driver disable them?
> > >
> > > 1：Assuming the guest OS's Virtio device is using PMD mode, QEMU sets
> > the call fd to -1
> > > 2：On the host side, the vhost_vdpa program will set
> > vp_vdpa->vring[i].cb.callback to invalid
> > > 3：Before the modification, the vp_vdpa_request_irq function does not
> > check whether
> > > vp_vdpa->vring[i].cb.callback is valid. Instead, it enables the
> > hardware's MSIX
> > > interrupts based on the number of queues of the device
> > >
> >
> > So MSIX is enabled but why would it trigger? virtio PMD in poll mode
> > presumably suppresses interrupts after all.
> Virtio pmd is in the guest,but in host side,the msix is enabled,then the device will triger
> Interrupt normally. I analysed this bug before,and I think gavin is right.
> Did I make it clear?
Not really. Guest disables interrupts presumably (it's polling)
why does device still send them?
> >
> > >
> > >
> > > ----- Original Message -----
> > > From: Michael S. Tsirkin mst@redhat.com
> > > Sent: April 22, 2024 20:09
> > > To: Gavin Liu gavin.liu@jaguarmicro.com
> > > Cc: jasowang@redhat.com; Angus Chen angus.chen@jaguarmicro.com;
> > virtualization@lists.linux.dev; xuanzhuo@linux.alibaba.com;
> > linux-kernel@vger.kernel.org; Heng Qi hengqi@linux.alibaba.com
> > > Subject: Re: [PATCH v5] vp_vdpa: don't allocate unused msix vectors
> > >
> > >
> > >
> > > External Mail: This email originated from OUTSIDE of the organization!
> > > Do not click links, open attachments or provide ANY information unless you
> > recognize the sender and know the content is safe.
> > >
> > >
> > > On Wed, Apr 10, 2024 at 11:30:20AM +0800, lyx634449800 wrote:
> > > > From: Yuxue Liu <yuxue.liu@jaguarmicro.com>
> > > >
> > > > When there is a ctlq and it doesn't require interrupt callbacks,the
> > > > original method of calculating vectors wastes hardware msi or msix
> > > > resources as well as system IRQ resources.
> > > >
> > > > When conducting performance testing using testpmd in the guest os, it
> > > > was found that the performance was lower compared to directly using
> > > > vfio-pci to passthrough the device
> > > >
> > > > In scenarios where the virtio device in the guest os does not utilize
> > > > interrupts, the vdpa driver still configures the hardware's msix
> > > > vector. Therefore, the hardware still sends interrupts to the host os.
> > >
> > > I just have a question on this part. How come hardware sends interrupts does
> > not guest driver disable them?
> > >
> > > > Because of this unnecessary
> > > > action by the hardware, hardware performance decreases, and it also
> > > > affects the performance of the host os.
> > > >
> > > > Before modification:(interrupt mode)
> > > > 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> > > > 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> > > > 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> > > > 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
> > > >
> > > > After modification:(interrupt mode)
> > > > 32: 0 0 1 7 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> > > > 33: 36 0 3 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> > > > 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-config
> > > >
> > > > Before modification:(virtio pmd mode for guest os)
> > > > 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-0
> > > > 33: 0 0 0 0 PCI-MSI 32769-edge vp-vdpa[0000:00:02.0]-1
> > > > 34: 0 0 0 0 PCI-MSI 32770-edge vp-vdpa[0000:00:02.0]-2
> > > > 35: 0 0 0 0 PCI-MSI 32771-edge vp-vdpa[0000:00:02.0]-config
> > > >
> > > > After modification:(virtio pmd mode for guest os)
> > > > 32: 0 0 0 0 PCI-MSI 32768-edge vp-vdpa[0000:00:02.0]-config
> > > >
> > > > To verify the use of the virtio PMD mode in the guest operating
> > > > system, the following patch needs to be applied to QEMU:
> > > > https://lore.kernel.org/all/20240408073311.2049-1-yuxue.liu@jaguarmicr
> > > > o.com
> > > >
> > > > Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>
> > > > Acked-by: Jason Wang <jasowang@redhat.com>
> > > > Reviewed-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > ---
> > > > V5: modify the description of the printout when an exception occurs
> > > > V4: update the title and assign values to uninitialized variables
> > > > V3: delete unused variables and add validation records
> > > > V2: fix when allocating IRQs, scan all queues
> > > >
> > > > drivers/vdpa/virtio_pci/vp_vdpa.c | 22 ++++++++++++++++------
> > > > 1 file changed, 16 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c
> > > > b/drivers/vdpa/virtio_pci/vp_vdpa.c
> > > > index df5f4a3bccb5..8de0224e9ec2 100644
> > > > --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> > > > +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> > > > @@ -160,7 +160,13 @@ static int vp_vdpa_request_irq(struct vp_vdpa
> > *vp_vdpa)
> > > > struct pci_dev *pdev = mdev->pci_dev;
> > > > int i, ret, irq;
> > > > int queues = vp_vdpa->queues;
> > > > - int vectors = queues + 1;
> > > > + int vectors = 1;
> > > > + int msix_vec = 0;
> > > > +
> > > > + for (i = 0; i < queues; i++) {
> > > > + if (vp_vdpa->vring[i].cb.callback)
> > > > + vectors++;
> > > > + }
> > > >
> > > > ret = pci_alloc_irq_vectors(pdev, vectors, vectors, PCI_IRQ_MSIX);
> > > > if (ret != vectors) {
> > > > @@ -173,9 +179,12 @@ static int vp_vdpa_request_irq(struct vp_vdpa
> > *vp_vdpa)
> > > > vp_vdpa->vectors = vectors;
> > > >
> > > > for (i = 0; i < queues; i++) {
> > > > + if (!vp_vdpa->vring[i].cb.callback)
> > > > + continue;
> > > > +
> > > > snprintf(vp_vdpa->vring[i].msix_name,
> > VP_VDPA_NAME_SIZE,
> > > > "vp-vdpa[%s]-%d\n", pci_name(pdev), i);
> > > > - irq = pci_irq_vector(pdev, i);
> > > > + irq = pci_irq_vector(pdev, msix_vec);
> > > > ret = devm_request_irq(&pdev->dev, irq,
> > > > vp_vdpa_vq_handler,
> > > > 0,
> > vp_vdpa->vring[i].msix_name,
> > > > @@ -185,21 +194,22 @@ static int vp_vdpa_request_irq(struct vp_vdpa
> > *vp_vdpa)
> > > > "vp_vdpa: fail to request irq for
> > vq %d\n", i);
> > > > goto err;
> > > > }
> > > > - vp_modern_queue_vector(mdev, i, i);
> > > > + vp_modern_queue_vector(mdev, i, msix_vec);
> > > > vp_vdpa->vring[i].irq = irq;
> > > > + msix_vec++;
> > > > }
> > > >
> > > > snprintf(vp_vdpa->msix_name, VP_VDPA_NAME_SIZE,
> > "vp-vdpa[%s]-config\n",
> > > > pci_name(pdev));
> > > > - irq = pci_irq_vector(pdev, queues);
> > > > + irq = pci_irq_vector(pdev, msix_vec);
> > > > ret = devm_request_irq(&pdev->dev, irq, vp_vdpa_config_handler,
> > 0,
> > > > vp_vdpa->msix_name, vp_vdpa);
> > > > if (ret) {
> > > > dev_err(&pdev->dev,
> > > > - "vp_vdpa: fail to request irq for vq %d\n", i);
> > > > + "vp_vdpa: fail to request irq for config: %d\n",
> > > > + ret);
> > > > goto err;
> > > > }
> > > > - vp_modern_config_vector(mdev, queues);
> > > > + vp_modern_config_vector(mdev, msix_vec);
> > > > vp_vdpa->config_irq = irq;
> > > >
> > > > return 0;
> > > > --
> > > > 2.43.0
> > >
>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2024-04-25 22:09 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-18 13:00 [PATCH v2] vp_vdpa: fix the method of calculating vectors gavin.liu
2024-04-08 8:02 ` Michael S. Tsirkin
2024-04-09 1:49 ` [PATCH v3] " lyx634449800
2024-04-09 3:53 ` Jason Wang
2024-04-09 5:40 ` Michael S. Tsirkin
2024-04-09 8:58 ` [PATCH v4] vp_vdpa: don't allocate unused msix vectors lyx634449800
2024-04-09 9:26 ` Michael S. Tsirkin
2024-04-09 9:56 ` Heng Qi
2024-04-10 3:30 ` [PATCH v5] " lyx634449800
2024-04-22 12:08 ` Michael S. Tsirkin
2024-04-23 1:39 ` 回复: " Gavin Liu
2024-04-23 8:35 ` Michael S. Tsirkin
2024-04-23 8:42 ` Angus Chen
2024-04-23 9:25 ` 回复: " Gavin Liu
2024-04-25 22:09 ` Michael S. Tsirkin
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).