Hi Jason Wang Thank you for your review. Yes, you're correct. I will update the patch accordingly to address the issues you pointed out. V2：https://lore.kernel.org/all/20240318130008.1928-1-gavin.liu@xxxxxxxxxxxxxxx/ Here is the content of the V2 version: From: Yuxue Liu <yuxue.liu@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> --- V1 -> V2: fix when allocating IRQs, scan all queues. V1: https://lore.kernel.org/all/20240318030121.1873-1-gavin.liu@xxxxxxxxxxxxxxx/ --- 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; ------- —————————————————————————————————————— Best regards, Gavin ----- Original Message ----- From: Jason Wang jasowang@xxxxxxxxxx Sent: March 18, 2024, 12:06 To: Gavin Liu gavin.liu@xxxxxxxxxxxxxxx Cc: mst@xxxxxxxxxx; xuanzhuo@xxxxxxxxxxxxxxxxx; virtualization@xxxxxxxxxxxxxxx; Angus Chen angus.chen@xxxxxxxxxxxxxxx; Gavin Liu gavin.liu@xxxxxxxxxxxxxxx Subject: Re: [PATCH] vp_vdpa: fix the method of calculating 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 Mon, Mar 18, 2024 at 11:01 AM gavin.liu <gavin.liu@xxxxxxxxxxxxxxx> wrote: > > From: Yuxue Liu <yuxue.liu@xxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxx> > --- > drivers/vdpa/virtio_pci/vp_vdpa.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c > b/drivers/vdpa/virtio_pci/vp_vdpa.c > index 281287fae89f..5066970b2570 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 = 0; > + > + for (i = 0; i < queues; i++) { > + if (vp_vdpa->vring[i].cb.callback != NULL) > + vectors++; > + } > + vectors = vectors + 1; > > ret = pci_alloc_irq_vectors(pdev, vectors, vectors, PCI_IRQ_MSIX); > if (ret != vectors) { > @@ -172,7 +178,7 @@ static int vp_vdpa_request_irq(struct vp_vdpa > *vp_vdpa) > > vp_vdpa->vectors = vectors; > > - for (i = 0; i < queues; i++) { > + for (i = 0; i < vectors - 1; i++) { This seems to be buggy. You didn't scan all queues so you will miss the IRQ for the last queue. Thanks > 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); @@ -191,7 +197,7 @@ > static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa) > > 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, vectors - 1); > ret = devm_request_irq(&pdev->dev, irq, vp_vdpa_config_handler, 0, > vp_vdpa->msix_name, vp_vdpa); > if (ret) { > @@ -199,7 +205,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, vectors - 1); > vp_vdpa->config_irq = irq; > > return 0; > -- > 2.43.0 >
