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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new ldev->agg_speed_mbps cache is invalidated on only one
  teardown path, and it is filled only after the IB device is exposed.
- [Medium] For regular (non-MPESW) RoCE/SRIOV LAG, the patch replaces the
  bond's ethtool-derived speed (tracker.bond_speed_mbps) with a sum of…
- [Low] In non-MPESW mode, mlx5_lag_get_devices_oper_speed() now reads
  the live ldev->tracker.netdev_state[pf_idx].tx_enabled/link_up for…

--

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

This patch caches the aggregate operational speed of the TX-enabled LAG
ports in ldev->agg_speed_mbps. VF/SF vport max_tx_speed is now programmed
from that cache instead of from the bond speed. The exported
mlx5_lag_query_bond_speed() is replaced by
mlx5_lag_query_aggregated_speed(), which the mlx5 IB driver uses for port
speed queries and which returns -ENODATA until a measurement succeeds.

> 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

[ ... ]

> @@ -1452,15 +1453,21 @@ static int mlx5_lag_get_devices_oper_speed(struct mlx5_lag *ldev,
>  		pf_mdev = pf->dev;
>  		if (!pf_mdev)
>  			continue;
> -		ret = mlx5_query_vport_max_tx_speed(pf_mdev, opmod, 0, 0,
> -						    &speed, &state);
> -		if (ret) {
> -			mlx5_core_dbg(pf_mdev, "State query failed (err=%d)\n",
> -				      ret);
> -			return ret;
> -		}
> -		if (state != VPORT_STATE_UP)
> +		if (mpesw) {
> +			ret = mlx5_query_vport_max_tx_speed(pf_mdev, opmod, 0,
> +							    0, &speed, &state);
> +			if (ret) {
> +				mlx5_core_dbg(pf_mdev,
> +					      "State query failed (err=%d)\n",
> +					      ret);
> +				return ret;
> +			}
> +			if (state != VPORT_STATE_UP)
> +				continue;
> +		} else if (!ldev->tracker.netdev_state[pf_idx].tx_enabled ||
> +			   !ldev->tracker.netdev_state[pf_idx].link_up) {
>  			continue;
> +		}

[Severity: Low]

Can these live reads of ldev->tracker race with mlx5_lag_netdev_event()?
The notifier publishes the whole tracker while holding only RTNL:

	ldev->tracker = tracker;

This code runs from mlx5_do_bond_work() under ldev->lock and the devcom
lock, but without RTNL. The lockdep_assert_held(&ldev->lock) in
mlx5_lag_update_agg_speed() therefore does not exclude the writer.

Between the tx_enabled/link_up reads for different ports,
mlx5_port_oper_linkspeed() issues a PTYS command that sleeps. During an
active-backup failover:

bond work                              netdev notifier (RTNL)
port 0: tx_enabled, count it
mlx5_port_oper_linkspeed() sleeps
                                       ldev->tracker = tracker
                                       (port 0 off, port 1 on)
port 1: tx_enabled, count it

Would the aggregate then cover both ports, even though only one of them
was TX-enabled at any given moment? That value goes to RDMA and into the
VF/SF vport max_tx_speed, and it stays there until the requeued bond work
recomputes it.

mlx5_do_bond() already takes a snapshot with tracker = ldev->tracker for
activate/modify. Could the speed calculation use that same snapshot?

>  
>  		ret = mlx5_port_oper_linkspeed(pf_mdev, &speed);

[Severity: Medium]

For non-MPESW RoCE/SRIOV LAG, this replaces tracker.bond_speed_mbps,
which came from the netdevs' ethtool link speed, with a sum of
mlx5_port_oper_linkspeed() results. Does that lose the fallbacks the old
source had?

mlx5_port_oper_linkspeed() picks the PTYS encoding only from
mlx5_ptys_ext_supported(). When no known bit is set, it returns success
with a speed of 0:

	ext = mlx5_ptys_ext_supported(mdev);
	err = mlx5_port_query_eth_proto(mdev, 1, ext, &eproto);
	...
	for (i = 0; i < max_size; ++i)
		if (eproto.oper & MLX5E_PROT_MASK(i))
			oper_speed = max(oper_speed, table[i].speed);

	*speed = oper_speed;
	return 0;

The old source had two fallbacks:

- mlx5e_ethtool_get_link_ksettings() falls back to the legacy
  eth_proto_admin/eth_proto_oper when the extended admin field is zero.
- get_link_properties() falls back to 100 * data_rate_oper when the
  bitmap cannot be decoded.

The legacy admin case looks reachable through
mlx5e_ethtool_set_link_ksettings(). On ext-capable hardware with autoneg
enabled, advertising only legacy modes writes the legacy admin field.

In that configuration, would an up, TX-enabled member contribute 0 Mbps,
with the result cached as a successful measurement? RDMA would then
under-report the port speed. If the whole aggregate is 0, the VF/SF
vports would get the max-capability fallback instead of the operational
speed.

This assumes firmware reports oper only in the legacy field in that
configuration. That assumption comes from the ethtool code's handling,
not from a firmware specification.

>  		if (ret) {

[ ... ]

> @@ -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);

[Severity: Medium]

Is this the only teardown path that invalidates agg_speed_mbps?
mlx5_lag_reset_vports_speed() is only called from the disable branch of
mlx5_do_bond():

	} else if (mlx5_lag_should_disable_lag(ldev, do_bond)) {
		mlx5_lag_reset_vports_speed(ldev);
		mlx5_disable_lag(ldev);
	}

Other teardown paths do not appear to reset the cache. One example is
mlx5_lag_disable_change(), reached on eswitch disable or mode change:

	if (__mlx5_lag_is_active(ldev)) {
		if (ldev->mode == MLX5_LAG_MODE_MPESW)
			mlx5_lag_disable_mpesw(ldev);
		else
			mlx5_disable_lag(ldev);
	}

The MPESW disable work also calls mlx5_lag_disable_mpesw(), and the
shared FDB path goes through
mlx5_lag_shared_fdb_destroy()->mlx5_deactivate_lag(). Neither path
touches agg_speed_mbps.

On activation, mlx5_do_bond() calls mlx5_activate_lag() and then
mlx5_lag_rescan_dev_locked(ldev, dev0, true) before
mlx5_lag_set_vports_agg_speed() fills the cache.
mlx5_lag_shared_fdb_create() also rescans and reloads the IB reps before
it refreshes the speed.

mlx5_lag_query_aggregated_speed() reads agg_speed_mbps without a lock,
and its IB caller only checks the LAG mode. Could a uverbs
QUERY_PORT_SPEED in that window return the previous LAG instance's
aggregate as valid data? On a first activation, it would get -ENODATA
instead.

This seems to contradict the commit message:

  "Until the aggregate has been measured, or after a failed measurement,
  mlx5_lag_query_aggregated_speed() returns -ENODATA."

It also contradicts the v2 changelog note about resetting the cache to
SPEED_UNKNOWN on teardown.

Later patches in the series do not appear to change this.
mlx5_lag_reset_agg_speed() is still reachable only through the disable
branch of mlx5_do_bond(). In "notify IB uplink on LAG aggregate speed
change", the stale value also feeds the speed != old_speed check.

>  	mlx5_ldev_for_each(pf_idx, 0, ldev) {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022193053.3815866-1-tariqt%40nvidia.com