Re: [PATCH v2] vp_vdpa: fix the method of calculating vectors

["Michael S. Tsirkin" <mst@redhat.com>]

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