Re: [PATCH net 1/5] net/mlx5: Lag, split aggregate speed into oper and max helpers

[Jakub Kicinski]

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, split aggregate speed into oper and max helpers

This splits the single mlx5_lag_sum_devices_speed() helper into
mlx5_lag_get_devices_oper_speed() and mlx5_lag_get_devices_max_speed().
The oper helper skips ports that are not forwarding traffic, while the max
helper takes the best single port for active-backup and sums the ports for
XOR, LACP and MPESW.

A few questions on the new helpers below.

> net/mlx5: Lag, split aggregate speed into oper and max helpers
>
> The oper function skips a port for two reasons:
> - the bonding driver set tx_enabled=false - LACP converging or
>   active-backup standby slave, where carrier may still be up but the
>   port is intentionally not forwarding traffic.
> - the link is down.

[Severity: Low]
The changelog lists two skip reasons, but the new oper helper also adds a
third, MPESW-only gate that queries the vNIC vport state per PF:

	if (mpesw) {
		if (mlx5_query_vport_state(pf_mdev, opmod, 0) !=
		    VPORT_STATE_UP)
			continue;
	}

Since the value returned by this helper ends up in every vport's
max_tx_speed, would it be worth spelling out the MPESW gate and its effect
on the programmed speed in the changelog too?

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index c655f6e32e9b0..5aa9d5c98fe16 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> @@ -1412,16 +1412,20 @@ static bool mlx5_lag_should_disable_lag(struct mlx5_lag *ldev, bool do_bond)
>  }
>  
>  #ifdef CONFIG_MLX5_ESWITCH
> -static int
> -mlx5_lag_sum_devices_speed(struct mlx5_lag *ldev, u32 *sum_speed,
> -			   int (*get_speed)(struct mlx5_core_dev *, u32 *))
> +static int mlx5_lag_get_devices_oper_speed(struct mlx5_lag *ldev,
> +					   u32 *sum_speed)
>  {
>  	struct mlx5_core_dev *pf_mdev;
>  	struct lag_func *pf;
>  	int pf_idx;
> +	bool mpesw;
>  	u32 speed;
> +	u8 opmod;
>  	int ret;
>  
> +	mpesw = ldev->mode == MLX5_LAG_MODE_MPESW;
> +	opmod = MLX5_VPORT_STATE_OP_MOD_VNIC_VPORT;
> +
>  	*sum_speed = 0;
>  	mlx5_ldev_for_each(pf_idx, 0, ldev) {
>  		pf = mlx5_lag_pf(ldev, pf_idx);
> @@ -1430,13 +1434,20 @@ mlx5_lag_sum_devices_speed(struct mlx5_lag *ldev, u32 *sum_speed,
>  		pf_mdev = pf->dev;
>  		if (!pf_mdev)
>  			continue;
> +		if (mpesw) {
> +			if (mlx5_query_vport_state(pf_mdev, opmod, 0) !=
> +			    VPORT_STATE_UP)
> +				continue;
> +		}

[Severity: Medium]
Can a failed firmware query be mistaken for a down vport here?
mlx5_query_vport_state() in core/vport.c returns 0 on command failure:

	err = mlx5_cmd_exec_inout(mdev, query_vport_state, in, out);
	if (err)
		return 0;

	return MLX5_GET(query_vport_state_out, out, state);

and include/linux/mlx5/device.h has VPORT_STATE_DOWN = 0x0, so a timeout,
-EIO on a device in error state, or -EBUSY is indistinguishable from a real
"vport down" reply.

The PF is then silently dropped from the sum and the helper still returns 0,
unlike the mlx5_port_oper_linkspeed() failure a few lines below which aborts
the whole computation.

The under-computed value reaches the hardware through
mlx5_lag_set_vports_agg_speed() -> mlx5_lag_modify_device_vports_speed(),
which stores it in vport->agg_max_tx_speed and calls
mlx5_modify_vport_max_tx_speed() for every enabled vport, so with one
failing PF in a two-PF MPESW LAG the rate limit is roughly halved until the
next port change event. If every query fails the sum stays 0 and the
capability-max fallback below is used instead.

This path is reachable from ordinary events via
mlx5_lag_mpesw_port_change_event() -> mlx5_mpesw_speed_update_work() ->
mlx5_lag_set_vports_agg_speed(). Should the query failure be distinguished
from VPORT_STATE_DOWN and propagated like the linkspeed error?

> +		} else if (!ldev->tracker.netdev_state[pf_idx].tx_enabled ||
> +			   !ldev->tracker.netdev_state[pf_idx].link_up) {
> +			continue;
> +		}

[Severity: Low]
Is this else-if arm reachable at this point of the series? The only caller
in this commit is mlx5_lag_set_vports_agg_speed(), which calls the helper
exclusively under:

	if (ldev->mode == MLX5_LAG_MODE_MPESW) {
		if (mlx5_lag_get_devices_oper_speed(ldev, &speed))
			return;

so mpesw is always true inside the helper and the tx_enabled/link_up skip
described in the changelog has no effect yet. The later patch
"{RDMA,net}/mlx5: cache and use TX-enabled aggregate speed for vports"
introduces mlx5_lag_update_agg_speed(), which calls the oper helper for all
LAG modes and makes this branch live, so this is only an intra-series
ordering observation.

[Severity: Low]
This is a pre-existing issue, not introduced by this patch, but these reads
of ldev->tracker.netdev_state[pf_idx].tx_enabled/.link_up (and the
tracker.tx_type read in mlx5_lag_get_devices_max_speed() below) take only
ldev->lock, while the writers use different conventions:

mlx5_lag_netdev_event() republishes the whole struct from RTNL context with
neither ldev->lock nor lag_lock held:

	ldev->tracker = tracker;

	if (changed)
		mlx5_queue_bond_work(ldev, 0);

and mlx5_ldev_add_netdev()/mlx5_ldev_remove_netdev() write the same fields
under lag_lock. The same pattern already exists in the baseline, where
mlx5_do_bond() snapshots ldev->tracker under ldev->lock and
mlx5_infer_tx_enabled() consumes these fields to program the port mapping.

Any mixed snapshot here looks self-correcting because the event path requeues
the bond work, which recomputes the aggregate speed. Is it worth settling on
one lock for the tracker fields while this code is being reworked?

>  
> -		ret = get_speed(pf_mdev, &speed);
> +		ret = mlx5_port_oper_linkspeed(pf_mdev, &speed);
>  		if (ret) {
>  			mlx5_core_dbg(pf_mdev,
> -				      "Failed to get device speed using %ps. Device %s speed is not available (err=%d)\n",
> -				      get_speed, dev_name(pf_mdev->device),
> -				      ret);
> +				      "Failed to get device %s oper speed (err=%d)\n",
> +				      dev_name(pf_mdev->device), ret);
>  			return ret;
>  		}
>  
> @@ -1446,17 +1457,41 @@ mlx5_lag_sum_devices_speed(struct mlx5_lag *ldev, u32 *sum_speed,
>  	return 0;
>  }
>  
> -static int mlx5_lag_sum_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
> +static int mlx5_lag_get_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
>  {
> -	return mlx5_lag_sum_devices_speed(ldev, max_speed,
> -					  mlx5_port_max_linkspeed);
> -}
> +	struct mlx5_core_dev *pf_mdev;
> +	struct lag_func *pf;
> +	bool take_max;
> +	int pf_idx;
> +	u32 speed;
> +	int ret;

[ ... ]

> +		ret = mlx5_port_max_linkspeed(pf_mdev, &speed);
> +		if (ret) {
> +			mlx5_core_dbg(pf_mdev,
> +				      "Failed to get device %s max speed (err=%d)\n",
> +				      dev_name(pf_mdev->device), ret);
> +			return ret;
> +		}
> +
> +		*max_speed = take_max ?
> +			max(*max_speed, speed) : *max_speed + speed;
> +	}

[Severity: Low]
Both new helpers sum per-port Ethernet link speeds from
mlx5_port_max_linkspeed()/mlx5_port_oper_linkspeed(), which derive their
value purely from the PTYS protocol capabilities, with no bound from the
device's PCIe link bandwidth. The changelog says the max helper "represents
the maximum achievable speed", yet the summed value can exceed what the host
DMA path can sustain, and that value is programmed as the vport rate limit.

The follow-up patch "net/mlx5: Lag, limit aggregated TX speed by PCIe
bandwidth" adds the missing clamp in both helpers:

	pci_bw = mlx5_pcie_bandwidth(pf_mdev);
	if (pci_bw)
		speed = min(speed, pci_bw);

so this is closed inside the series; was there a reason for keeping the
clamp in a separate patch rather than adding it here?

>  
>  static void mlx5_lag_modify_device_vports_speed(struct mlx5_core_dev *mdev,
> @@ -1505,7 +1540,7 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev)
>  	int pf_idx;
>  
>  	if (ldev->mode == MLX5_LAG_MODE_MPESW) {
> -		if (mlx5_lag_sum_devices_oper_speed(ldev, &speed))
> +		if (mlx5_lag_get_devices_oper_speed(ldev, &speed))
>  			return;
>  	} else {
>  		speed = ldev->tracker.bond_speed_mbps;
> @@ -1514,7 +1549,7 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev)
>  	}
>  
>  	/* If speed is not set, use the sum of max speeds of all PFs */
> -	if (!speed && mlx5_lag_sum_devices_max_speed(ldev, &speed))
> +	if (!speed && mlx5_lag_get_devices_max_speed(ldev, &speed))
>  		return;

[Severity: Low]
Should this comment be updated? mlx5_lag_get_devices_max_speed() no longer
always sums, since:

	take_max = ldev->tracker.tx_type == NETDEV_LAG_TX_TYPE_ACTIVEBACKUP;
	...
	*max_speed = take_max ?
		max(*max_speed, speed) : *max_speed + speed;

For an active-backup bond in a non-MPESW mode the fallback returns the best
single port speed, not "the sum of max speeds of all PFs", and that is the
value programmed into vport max_tx_speed.

The comment also survives unchanged in the follow-up patch that moves this
code into mlx5_lag_update_agg_speed(), so nothing later in the series
corrects it.

>  
>  	speed = speed / MLX5_MAX_TX_SPEED_UNIT;