| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240409052233-mutt-send-email-mst@kernel.org>
Date: Tue, 9 Apr 2024 05:26:29 -0400
From: "Michael S. Tsirkin" <mst@...hat.com>
To: lyx634449800 <yuxue.liu@...uarmicro.com>
Cc: jasowang@...hat.com, angus.chen@...uarmicro.com,
virtualization@...ts.linux.dev, xuanzhuo@...ux.alibaba.com,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] vp_vdpa: don't allocate unused msix vectors
Good and clear subject, I like it.
On Tue, Apr 09, 2024 at 04:58:18PM +0800, lyx634449800 wrote:
> From: Yuxue Liu <yuxue.liu@...uarmicro.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@...uarmicro.com>
> Acked-by: Jason Wang <jasowang@...hat.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
Powered by blists - more mailing lists