Re: [PATCH net V2 1/4] {RDMA,net}/mlx5: cache and use TX-enabled aggregate speed for vports

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net/mlx5: lag: missing agg_speed_mbps reset on MPESW and mode-change teardown
--

commit ce16ef57f040ab430add649db26c3f383cbab840
Author: Or Har-Toov <ohartoov@nvidia.com>

{RDMA,net}/mlx5: cache and use TX-enabled aggregate speed for vports

This commit caches the calculated operational TX-enabled aggregate speed in
struct mlx5_lag and exposes mlx5_lag_query_aggregated_speed() to query it
instead of the bond speed. It also resets the cached speed to SPEED_UNKNOWN
via mlx5_lag_reset_agg_speed() when resetting vports speed.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index 4e173b08cb37a..57755ceab8a75 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c

[ ... ]

> @@ -1594,6 +1625,7 @@ void mlx5_lag_reset_vports_speed(struct mlx5_lag *ldev)
>  	int pf_idx;
>  	int ret;
>  
> +	mlx5_lag_reset_agg_speed(ldev);
>  	mlx5_ldev_for_each(pf_idx, 0, ldev) {
>  		pf = mlx5_lag_pf(ldev, pf_idx);
>  		if (!pf)

[Severity: Medium]
Does calling mlx5_lag_reset_agg_speed() only from
mlx5_lag_reset_vports_speed() miss resetting ldev->agg_speed_mbps (and vport
speeds) when LAG is torn down via MPESW disable or mode changes?

Currently, mlx5_lag_reset_vports_speed() is only called from mlx5_do_bond()
when mlx5_lag_should_disable_lag() returns true:

drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c:mlx5_do_bond() {
	...
	} else if (mlx5_lag_should_disable_lag(ldev, do_bond)) {
		mlx5_lag_reset_vports_speed(ldev);
		mlx5_disable_lag(ldev);
	}
}

where mlx5_lag_should_disable_lag() checks:

	return !do_bond && __mlx5_lag_is_active(ldev) &&
	       ldev->mode != MLX5_LAG_MODE_MPESW;

When MPESW LAG is enabled via mlx5_lag_enable_mpesw() ->
mlx5_lag_shared_fdb_create(), mlx5_lag_set_vports_agg_speed() populates
ldev->agg_speed_mbps (and mlx5_mpesw_speed_update_work() updates it).
However, when MPESW is torn down via mlx5_lag_disable_mpesw():

drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.c:mlx5_lag_disable_mpesw() {
	if (ldev->mode != MLX5_LAG_MODE_MPESW)
		return;

	mlx5_mpesw_metadata_cleanup(ldev);
	mlx5_lag_shared_fdb_destroy(ldev, MLX5_LAG_FILTER_ALL);
	if (mlx5_lag_has_sd_group(ldev))
		mlx5_mpesw_restore_sd_fdb(ldev);
}

neither mlx5_lag_reset_vports_speed() nor mlx5_lag_reset_agg_speed() is
called, and mlx5_lag_should_disable_lag() explicitly excludes
MLX5_LAG_MODE_MPESW.

Similarly, during eswitch mode or SR-IOV changes, mlx5_lag_disable_change()
disables an active LAG directly without calling
mlx5_lag_reset_vports_speed() or mlx5_lag_reset_agg_speed():

drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c:mlx5_lag_disable_change() {
	...
	ldev->mode_changes_in_progress++;
	if (__mlx5_lag_is_active(ldev)) {
		if (ldev->mode == MLX5_LAG_MODE_MPESW)
			mlx5_lag_disable_mpesw(ldev);
		else
			mlx5_disable_lag(ldev);
	}
}

Because mlx5_disable_lag() -> mlx5_deactivate_lag() clears ldev->mode to
MLX5_LAG_MODE_NONE, when mlx5_lag_enable_change() later runs mlx5_do_bond(),
__mlx5_lag_is_active(ldev) is already false. If do_bond is false (for
example, if mlx5_lag_check_prereq(ldev) is now false after a mode change or
if the bond was removed during the transition), mlx5_lag_should_disable_lag()
evaluates to false and the reset never runs.

Wouldn't this leave a stale speed in ldev->agg_speed_mbps (and stale vport
rate limits) after teardown, causing mlx5_lag_query_aggregated_speed() to
return the old speed instead of -ENODATA, and in the subsequent patch
("{RDMA,net}/mlx5: notify IB uplink on LAG aggregate speed change") causing
mlx5_lag_update_agg_speed() to compare against the stale old_speed and
suppress IB_EVENT_DEVICE_SPEED_CHANGE if LAG is re-activated at the same
aggregate speed?

[ ... ]

>
>
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022193053.3815866-1-tariqt@nvidia.com?part=1