On Mon, Mar 18, 2024 at 09:00:08PM +0800, gavin.liu 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> 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@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; 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
