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

[Or Har-Toov <ohartoov@nvidia.com>]

On 26/10/2026 22:32, netdev-bot+sashiko@kernel.org wrote:
> 
> 
> 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.
> 

will fix in V3
>>                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.
> 

already replied in another mail

>>        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