Re: [PATCH net-next v3 9/9] bnxt_en: Support dynamic MSIX

[Michal Swiatkowski <michal.swiatkowski@linux.intel.com>]

On Fri, Aug 23, 2024 at 12:56:57PM -0700, Michael Chan wrote:
> A range of MSIX vectors are allocated at initialization for the number
> needed for RocE and L2.  During run-time, if the user increases or
> decreases the number of L2 rings, all the MSIX vectors have to be
> freed and a new range has to be allocated.  This is not optimal and
> causes disruptions to RoCE traffic every time there is a change in L2
> MSIX.
> 
> If the system supports dynamic MSIX allocations, use dynamic
> allocation to add new L2 MSIX vectors or free unneeded L2 MSIX
> vectors.  RoCE traffic is not affected using this scheme.
> 
> Reviewed-by: Hongguang Gao <hongguang.gao@broadcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
> v2: Fix typo in changelog
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 57 +++++++++++++++++++++--
>  1 file changed, 54 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index fa4115f6dafe..39dc67dbe9b2 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -10622,6 +10622,43 @@ static void bnxt_setup_msix(struct bnxt *bp)
>  
>  static int bnxt_init_int_mode(struct bnxt *bp);
>  
> +static int bnxt_add_msix(struct bnxt *bp, int total)
> +{
> +	int i;
> +
> +	if (bp->total_irqs >= total)
> +		return total;
> +
> +	for (i = bp->total_irqs; i < total; i++) {
> +		struct msi_map map;
> +
> +		map = pci_msix_alloc_irq_at(bp->pdev, i, NULL);
> +		if (map.index < 0)
> +			break;
> +		bp->irq_tbl[i].vector = map.virq;
> +		bp->total_irqs++;
> +	}
> +	return bp->total_irqs;
> +}
> +
> +static int bnxt_trim_msix(struct bnxt *bp, int total)
> +{
> +	int i;
> +
> +	if (bp->total_irqs <= total)
> +		return total;
> +
> +	for (i = bp->total_irqs; i > total; i--) {
> +		struct msi_map map;
> +
> +		map.index = i - 1;
> +		map.virq = bp->irq_tbl[i - 1].vector;
> +		pci_msix_free_irq(bp->pdev, map);
> +		bp->total_irqs--;
> +	}
> +	return bp->total_irqs;
> +}

Patch looks fine, treat it only as suggestion:

You can save some lines of code by merging this two function.

static int bnxt_change_msix(struct bnxt *bp, int total)
{
	int i;

	/* add MSI-Xs if needed */
	for (i = bp->total_irqs; i < total; i++) {
		...
	}

	/* remove MSI-Xs if needed */
	for (i = bp->total_irqs; i > total; i--) {
		...
	}

	return bp->total_irqs;
}
> +
>  static int bnxt_setup_int_mode(struct bnxt *bp)
>  {
>  	int rc;
> @@ -10788,6 +10825,7 @@ static void bnxt_clear_int_mode(struct bnxt *bp)
>  int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
>  {
>  	bool irq_cleared = false;
> +	bool irq_change = false;
>  	int tcs = bp->num_tc;
>  	int irqs_required;
>  	int rc;
> @@ -10806,15 +10844,28 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
>  	}
>  
>  	if (irq_re_init && BNXT_NEW_RM(bp) && irqs_required != bp->total_irqs) {
> -		bnxt_ulp_irq_stop(bp);
> -		bnxt_clear_int_mode(bp);
> -		irq_cleared = true;
> +		irq_change = true;
> +		if (!pci_msix_can_alloc_dyn(bp->pdev)) {
> +			bnxt_ulp_irq_stop(bp);
> +			bnxt_clear_int_mode(bp);
> +			irq_cleared = true;
> +		}
>  	}
>  	rc = __bnxt_reserve_rings(bp);
>  	if (irq_cleared) {
>  		if (!rc)
>  			rc = bnxt_init_int_mode(bp);
>  		bnxt_ulp_irq_restart(bp, rc);
> +	} else if (irq_change && !rc) {
> +		int total;
> +
> +		if (irqs_required > bp->total_irqs)
> +			total = bnxt_add_msix(bp, irqs_required);
> +		else
> +			total = bnxt_trim_msix(bp, irqs_required);
> +
> +		if (total != irqs_required)
> +			rc = -ENOSPC;

and here

if (bnxt_change_msix(bp, irqs_required) != irqs_required)
	rc = -ENOSPC;

Thanks,
Michal
>  	}
>  	if (rc) {
>  		netdev_err(bp->dev, "ring reservation/IRQ init failure rc: %d\n", rc);
> -- 
> 2.30.1
>