Re: [PATCH net 4/5] net/mlx5: Lag, limit aggregated TX speed by PCIe bandwidth

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/mlx5: Lag, limit aggregated TX speed by PCIe bandwidth

This adds a new helper mlx5_pcie_bandwidth() that derives a PCIe bandwidth
from PCI_EXP_LNKSTA, and uses it to clamp each uplink's contribution to the
aggregated LAG TX speed in mlx5_lag_get_devices_oper_speed(),
mlx5_lag_get_devices_max_speed() and mlx5_lag_reset_vports_speed().  A few
questions below about how the bandwidth is computed and where the clamp is
applied.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/dev.c b/drivers/net/ethernet/mellanox/mlx5/core/dev.c
> index df2e3ad01819f..81f3dc3b8034f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/dev.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/dev.c

[ ... ]

> @@ -40,6 +41,25 @@
>  
>  static DEFINE_IDA(mlx5_adev_ida);
>  
> +#define MLX5_PCIE_MIN_SPEED_MBPS	(2500)
> +
> +u32 mlx5_pcie_bandwidth(struct mlx5_core_dev *dev)
> +{
> +	u16 lnksta;
> +	int speed;
> +	u32 width;
> +
> +	if (pcie_capability_read_word(dev->pdev, PCI_EXP_LNKSTA, &lnksta))
> +		return 0;
> +
> +	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, lnksta);
> +	speed = pcie_link_speed_mbps(dev->pdev);
> +	if (speed < MLX5_PCIE_MIN_SPEED_MBPS)
> +		return 0;
> +
> +	return speed * width;
> +}

[Severity: Medium]
Is speed * width the same quantity the PCI core calls PCIe bandwidth?

pcie_link_speed_mbps() returns the raw per-lane transfer rate, while
pcie_bandwidth_available() in drivers/pci/pci.c applies the line encoding
correction and walks upstream taking the minimum:

  drivers/pci/pci.h:
  #define PCIE_SPEED2MBS_ENC(speed) \
	((speed) == PCIE_SPEED_64_0GT ? 64000*1/1 : \
	 (speed) == PCIE_SPEED_32_0GT ? 32000*128/130 : \
	 ...
	 (speed) == PCIE_SPEED_5_0GT  ?  5000*8/10 : \
	 (speed) == PCIE_SPEED_2_5GT  ?  2500*8/10 : \

  drivers/pci/pci.c:pcie_bandwidth_available() {
	...
		next_bw = next_width * PCIE_SPEED2MBS_ENC(next_speed);
		/* Check if current device limits the total bandwidth */
		if (!bw || next_bw <= bw) {
	...
		dev = pci_upstream_bridge(dev);
  }

For a Gen2 x8 link the helper here yields 5000 * 8 = 40000 Mb/s, whereas the
core helper yields 4000 * 8 = 32000 Mb/s.  Does that make
min(speed, pci_bw) a no-op for a 40GbE uplink on a bus that cannot sustain
it?  On Gen3 and later the difference is about 1.5%, which is still larger
than the MLX5_MAX_TX_SPEED_UNIT (100 Mb/s) granularity the value is
programmed at.

The same driver already computes this quantity for the same pdev with the
core helper:

  drivers/net/ethernet/mellanox/mlx5/core/en/params.c:slow_pci_heuristic() {
	...
	pci_bw = pcie_bandwidth_available(mdev->pdev, NULL, NULL, NULL);
	mlx5_core_dbg(mdev, "Max link speed = %d, PCI BW = %d\n", link_speed,
		      pci_bw);
	...
  }

Should mlx5_pcie_bandwidth() use pcie_bandwidth_available() as well, so the
driver does not carry two different definitions of PCI bandwidth for one
device?

[Severity: Low]
Can the width and speed here come from two different link states?  The width
is taken from the local snapshot in lnksta, but the speed comes from a second
independent read of the very same register:

  drivers/pci/pci.c:pcie_link_speed_mbps() {
	err = pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnksta);
	if (err)
		return err;

	return pcie_dev_speed_mbps(to_pcie_link_speed(lnksta));
  }

PCI_EXP_LNKSTA_NLW and PCI_EXP_LNKSTA_CLS both live in the word already held
in lnksta, and the callers hold only ldev->lock, which does not serialise
link retraining, autonomous width/speed changes, bandwidth control speed
changes or a function reset during health recovery.  If the link changes
between the two reads, speed * width is a bandwidth the link never had.
Would deriving the speed from the existing snapshot with
pcie_dev_speed_mbps(to_pcie_link_speed(lnksta)) be both cheaper and
consistent?

> +
>  static bool is_eth_rep_supported(struct mlx5_core_dev *dev)
>  {
>  	if (!IS_ENABLED(CONFIG_MLX5_ESWITCH))
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index 76b574fa0d7a6..fd91becd6848f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c

[ ... ]

> @@ -1452,6 +1453,9 @@ static int mlx5_lag_get_devices_oper_speed(struct mlx5_lag *ldev,
>  			return ret;
>  		}
>  
> +		pci_bw = mlx5_pcie_bandwidth(pf_mdev);
> +		if (pci_bw)
> +			speed = min(speed, pci_bw);
>  		*sum_speed += speed;
>  	}

[Severity: Medium]
Does clamping per uplink and then summing actually bound the aggregate for
the two PFs of a dual-port adapter?  Both PFs sit on the same physical PCIe
link and read the same PCI_EXP_LNKSTA, so each contributes up to that single
link's bandwidth.

For a dual-port 200GbE adapter on Gen4 x16, mlx5_pcie_bandwidth() returns
16000 * 16 = 256000 for both PFs, each contributes min(200000, 256000) =
200000, and the aggregate becomes 400000 Mb/s, roughly 1.6x the shared link.

That aggregate is stored in ldev->agg_speed_mbps and then programmed into
every vport:

  mlx5_lag_set_vports_agg_speed() {
	...
	speed = speed / MLX5_MAX_TX_SPEED_UNIT;

	mlx5_ldev_for_each(pf_idx, 0, ldev) {
	...
		mlx5_lag_modify_device_vports_speed(mdev, speed);
  }

Should the PCIe bandwidth instead be applied to the summed value, i.e. after
aggregation, so PFs sharing one link are not counted twice?  Only the
take_max (active-backup) path stays bounded today.

>  
> @@ -1488,6 +1493,9 @@ static int mlx5_lag_get_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
>  			return ret;
>  		}
>  
> +		pci_bw = mlx5_pcie_bandwidth(pf_mdev);
> +		if (pci_bw)
> +			speed = min(speed, pci_bw);
>  		*max_speed = take_max ?
>  			max(*max_speed, speed) : *max_speed + speed;
>  	}

[Severity: Medium]
The same double counting applies in this fallback when take_max is false,
which is the case for LACP and XOR and is forced for MPESW.

[Severity: Low]
Unrelated to the clamp itself, but the caller's comment no longer matches
this helper:

  mlx5_lag_update_agg_speed() {
	...
	/* If speed is not set, use the sum of max speeds of all PFs */
	if (!speed && mlx5_lag_get_devices_max_speed(ldev, &speed))
		return;
  }

With take_max = ldev->tracker.tx_type == NETDEV_LAG_TX_TYPE_ACTIVEBACKUP,
the fallback returns max(*max_speed, speed) rather than a sum in
active-backup mode.  The comment was accurate in the baseline, where
mlx5_lag_sum_devices_max_speed() unconditionally did *sum_speed += speed;
should it be updated to describe the policy dependent behaviour?

>  
> @@ -1609,6 +1618,9 @@ void mlx5_lag_reset_vports_speed(struct mlx5_lag *ldev)
>  			continue;
>  		}
>  
> +		pci_bw = mlx5_pcie_bandwidth(mdev);
> +		if (pci_bw)
> +			speed = min(speed, pci_bw);
>  		speed = speed / MLX5_MAX_TX_SPEED_UNIT;
>  		mlx5_lag_modify_device_vports_speed(mdev, speed);
>  	}

[Severity: Medium]
This is the value restored into vport max_tx_speed on LAG teardown, so if
mlx5_pcie_bandwidth() over-estimates the bus bandwidth (raw rate, no
encoding correction, endpoint link only), the restored value is the
over-estimated one rather than what FW originally programmed.  Does that
still match the commit message claim that FW initializes max_tx_speed to the
minimum of the port speed and the PCI bandwidth?