Re: [PATCH net-next 05/15] net/mlx5: LAG, decouple iteration end-of-walk from MLX5_MAX_PORTS
From: Shay Drori
Date: Mon Nov 09 2026 - 13:22:48 EST
On 03/11/2026 19:09, Tariq Toukan wrote:
From: Shay Drory <shayd@xxxxxxxxxx>
The LAG xarray iterators returned MLX5_MAX_PORTS as their end-of-walk
sentinel, and the open-coded callers compared the current index against
MLX5_MAX_PORTS to detect the end of iteration. This ties iteration to a
compile-time maximum rather than to the actual number of LAG members.
Add ldev->max_funcs, and use it as the loop bound. Update the open-coded
callers accordingly. When a member is added at an xarray index >=
ldev->max_funcs, increase max_funcs.
In addition, remove the xa_alloc() limit of MLX5_MAX_PORTS.
No functional change; this only removes the dependency on
MLX5_MAX_PORTS.
Signed-off-by: Shay Drory <shayd@xxxxxxxxxx>
Reviewed-by: Mark Bloch <mbloch@xxxxxxxxxx>
Reviewed-by: Moshe Shemesh <moshe@xxxxxxxxxx>
Signed-off-by: Tariq Toukan <tariqt@xxxxxxxxxx>
---
.../net/ethernet/mellanox/mlx5/core/lag/lag.c | 22 +++++++++++--------
.../net/ethernet/mellanox/mlx5/core/lag/lag.h | 3 ++-
.../ethernet/mellanox/mlx5/core/lag/mpesw.c | 2 +-
.../mellanox/mlx5/core/lag/shared_fdb.c | 2 +-
4 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
index b2ac5c671ae6..73bce918cddb 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
@@ -395,6 +395,7 @@ static struct mlx5_lag *mlx5_lag_dev_alloc(struct mlx5_core_dev *dev)
mlx5_core_err(dev, "Failed to init multipath lag err=%d\n",
err);
+ ldev->max_funcs = ldev->ports;
return ldev;
err_wq:
@@ -1284,7 +1285,7 @@ static void mlx5_lag_assert_locked_transition(struct mlx5_lag *ldev, u32 filter)
lockdep_assert_held(&ldev->lock);
i = mlx5_get_next_lag_func(ldev, 0, filter);
- if (i < MLX5_MAX_PORTS) {
+ if (i < ldev->max_funcs) {
pf = mlx5_lag_pf(ldev, i);
if (filter == MLX5_LAG_FILTER_PORTS ||
filter == MLX5_LAG_FILTER_ALL)
@@ -1831,7 +1832,7 @@ struct mlx5_devcom_comp_dev *mlx5_lag_get_devcom_comp(struct mlx5_lag *ldev)
filter = ldev->virt_lag ? MLX5_LAG_FILTER_ALL :
MLX5_LAG_FILTER_PORTS;
i = mlx5_get_next_lag_func(ldev, 0, filter);
- if (i < MLX5_MAX_PORTS) {
+ if (i < ldev->max_funcs) {
pf = mlx5_lag_pf(ldev, i);
devcom = pf->dev->priv.hca_devcom_comp;
}
@@ -2095,7 +2096,7 @@ static int mlx5_handle_changeupper_event(struct mlx5_lag *ldev,
break;
}
}
- if (i < MLX5_MAX_PORTS) {
+ if (i < ldev->ports) {
if (netif_is_bond_master(upper)) {
slave = bond_slave_get_rcu(ndev_tmp);
if (slave)
@@ -2360,13 +2361,16 @@ int mlx5_ldev_add_mdev(struct mlx5_lag *ldev,
if (!pf)
return -ENOMEM;
- err = xa_alloc(&ldev->pfs, &idx, pf, XA_LIMIT(0, MLX5_MAX_PORTS - 1),
+ err = xa_alloc(&ldev->pfs, &idx, pf, XA_LIMIT(0, U8_MAX - 1),
GFP_KERNEL);
if (err) {
kfree(pf);
return err;
}
+ /* max_funcs is the iteration ceiling; grow it, never shrink. */
+ if (idx >= ldev->max_funcs)
+ ldev->max_funcs = idx + 1;
pf->idx = idx;
pf->dev = dev;
answers to net-dev sashiko
Sashiko (Medium):
Is this store safe against readers that only hold lag_lock?
max_funcs is written here with a plain store, under ldev->lock only. The
store happens after xa_alloc() has already published pf, but before
pf->dev is set. mlx5_lag_get_next_peer_mdev(), mlx5_lag_get_slave_port()
and mlx5_lag_query_cong_counters() read it while holding only lag_lock.
Each walk reads max_funcs more than once:
- in mlx5_get_next_lag_func(), to produce the sentinel
- again in the mlx5_lag_for_each() condition
- and once more in mlx5_lag_get_next_peer_mdev(), after the loop
Suppose an add grows max_funcs from M to M + 1 between those reads. The
stale sentinel M then passes the tmp < (ldev)->max_funcs check. The body
runs on slot M, which holds the new pf, and pf->dev is not set yet.
Could mlx5_lag_get_next_peer_mdev() return a NULL peer here?
On weakly ordered CPUs, can the max_funcs store also become visible before
the xarray slot store? A reader could then see max_funcs == M + 1 while
mlx5_lag_pf(ldev, M) returns NULL. In that case this code in
mlx5_lag_get_next_peer_mdev() would dereference a NULL pf:
pf = mlx5_lag_pf(ldev, idx);
if (pf->dev != dev)
Before this patch, the sentinel and the comparison used the same constant,
so they could not disagree.
The later commit "net/mlx5: LAG, do not add a device to an already-active
LAG" narrows the window for callers that only walk while LAG is active.
These readers do not check for an active LAG themselves, though.
[SD] all the readers above running only while LAG is active, and
mlx5_lag_query_cong_counters() and mlx5_lag_get_slave_port() check
whether lag is active explicitly before walking over the lag
pf->group_id = group_id;
@@ -2395,7 +2399,7 @@ void mlx5_ldev_remove_mdev(struct mlx5_lag *ldev,
if (pf->dev == dev)
break;
}
- if (i >= MLX5_MAX_PORTS)
+ if (i >= ldev->max_funcs)
return;
if (pf->port_change_nb.nb.notifier_call)
@@ -2608,7 +2612,7 @@ int mlx5_get_next_lag_func(struct mlx5_lag *ldev, int start_idx, u32 filter)
MLX5_LAG_XA_MARK_PORT, start_idx)
if (pf->dev)
return idx;
- return MLX5_MAX_PORTS;
+ return ldev->max_funcs;
}
xa_for_each_start(&ldev->pfs, idx, pf, start_idx) {
@@ -2618,7 +2622,7 @@ int mlx5_get_next_lag_func(struct mlx5_lag *ldev, int start_idx, u32 filter)
filter == pf->group_id)
return idx;
}
- return MLX5_MAX_PORTS;
+ return ldev->max_funcs;
}
bool mlx5_lag_is_roce(struct mlx5_core_dev *dev)
@@ -2868,7 +2872,7 @@ struct mlx5_core_dev *mlx5_lag_get_next_peer_mdev(struct mlx5_core_dev *dev, int
if (!ldev)
goto unlock;
- if (*i == MLX5_MAX_PORTS)
+ if (*i == ldev->max_funcs)
goto unlock;
mlx5_lag_for_each(idx, *i, ldev, mlx5_lag_get_filter(ldev, dev)) {
pf = mlx5_lag_pf(ldev, idx);
@@ -2876,7 +2880,7 @@ struct mlx5_core_dev *mlx5_lag_get_next_peer_mdev(struct mlx5_core_dev *dev, int
break;
}
- if (idx == MLX5_MAX_PORTS) {
+ if (idx == ldev->max_funcs) {
*i = idx;
goto unlock;
}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
index 872cc769297c..479d9f42ff3f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
@@ -83,6 +83,7 @@ struct mlx5_lag {
unsigned long mode_flags;
unsigned long state_flags;
u8 ports;
+ u8 max_funcs;
u8 buckets;
bool virt_lag;
int mode_changes_in_progress;
@@ -298,7 +299,7 @@ static inline bool mlx5_lag_is_supported(struct mlx5_core_dev *dev)
#define mlx5_lag_for_each(i, start_index, ldev, filter) \
for (int tmp = start_index; \
tmp = mlx5_get_next_lag_func(ldev, tmp, filter), \
- i = tmp, tmp < MLX5_MAX_PORTS; tmp++)
+ i = tmp, tmp < (ldev)->max_funcs; tmp++)
#define mlx5_lag_for_each_reverse(i, start_index, end_index, ldev, filter) \
for (int tmp = start_index, tmp1 = end_index; \
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.c
index b65ad9a26f71..56faa1d498c7 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.c
@@ -223,7 +223,7 @@ void mlx5_mpesw_sd_devcoms_unlock(struct mlx5_lag *ldev)
struct mlx5_devcom_comp_dev *sd_devcom;
int i;
- mlx5_ldev_for_each_reverse(i, MLX5_MAX_PORTS, 0, ldev) {
+ mlx5_ldev_for_each_reverse(i, ldev->max_funcs, 0, ldev) {
sd_devcom = mlx5_sd_get_devcom(mlx5_lag_pf(ldev, i)->dev);
if (sd_devcom)
mlx5_devcom_comp_unlock(sd_devcom);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c
index 7c7e6c832a93..29048b0c2797 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c
@@ -71,7 +71,7 @@ static int mlx5_lag_create_single_fdb_filter(struct mlx5_lag *ldev, u32 filter)
err = mlx5_eswitch_offloads_single_fdb_add_one(master_esw,
slave_esw,
- ldev->ports);
+ ldev->max_funcs);
Sashiko (Medium):
The commit message says:
No functional change; this only removes the dependency on
MLX5_MAX_PORTS.
Is that accurate? This hunk changes the max_slaves argument from
ldev->ports to ldev->max_funcs, and that argument sizes the master's
egress ACL. A later commit in the series, "net/mlx5: E-switch, drop
master egress rule under a SW vport LAG", calls this value the ACL size.
So the size of the hardware flow table changes whenever max_funcs !=
ports.
[SD] Only the ACL size changes, and only for SD, where more PFs than
ports join the ldev and max_funcs > ports. The rules installed are the
same as before, so there is no functional change.
The patch also changes behaviour in two other places:
- The xa_alloc() limit in mlx5_ldev_add_mdev() now accepts a 9th member.
- The found check in mlx5_handle_changeupper_event() now uses
i < ldev->ports instead of the max_funcs sentinel.
Could the commit message describe these changes?
[SD] The xa_alloc() change is already in the commit message ("remove the
xa_alloc() limit of MLX5_MAX_PORTS"), and the cover letter explains why
it can't let in a 9th member.
mlx5_handle_changeupper_event() runs only for a PF LAG without SD: the
netdev notifier is registered only when !mlx5_sd_is_supported(). There
each LAG port is one PF at an xarray index below ldev->ports
(num_lag_ports), so "i < ldev->ports" is true in exactly the same cases
as the old end-of-walk check.
if (err)
goto err;
}