From: Shay Drori <shayd@nvidia.com>
To: Tariq Toukan <tariqt@nvidia.com>,
Andrew Lunn <andrew+netdev@lunn.ch>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@kernel.org>,
Jakub Kicinski <kuba@kernel.org>, <netdev@vger.kernel.org>,
Paolo Abeni <pabeni@redhat.com>
Cc: Akiva Goldberger <agoldberger@nvidia.com>,
Carolina Jubran <cjubran@nvidia.com>,
Cosmin Ratiu <cratiu@nvidia.com>,
Edward Srouji <edwards@nvidia.com>, Gal Pressman <gal@nvidia.com>,
Leon Romanovsky <leon@kernel.org>,
open list <linux-kernel@vger.kernel.org>,
<linux-rdma@vger.kernel.org>, Mark Bloch <mbloch@nvidia.com>,
Moshe Shemesh <moshe@nvidia.com>,
Or Har-Toov <ohartoov@nvidia.com>,
Patrisious Haddad <phaddad@nvidia.com>,
Saeed Mahameed <saeedm@nvidia.com>
Subject: Re: [PATCH net-next 05/15] net/mlx5: LAG, decouple iteration end-of-walk from MLX5_MAX_PORTS
Date: Mon, 9 Nov 2026 20:21:04 +0200 [thread overview]
Message-ID: <fa8bdce6-6f89-4de0-b81b-149ac20802a4@nvidia.com> (raw)
In-Reply-To: <20261103171009.563034-6-tariqt@nvidia.com>
On 03/11/2026 19:09, Tariq Toukan wrote:
> From: Shay Drory <shayd@nvidia.com>
>
> 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@nvidia.com>
> Reviewed-by: Mark Bloch <mbloch@nvidia.com>
> Reviewed-by: Moshe Shemesh <moshe@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> ---
> .../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;
> }
next prev parent reply other threads:[~2026-11-09 18:21 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-11-03 17:09 [PATCH net-next 00/15] net/mlx5: LAG over VFs/SFs (virtual LAG) Tariq Toukan
2026-11-03 17:09 ` [PATCH net-next 01/15] net/mlx5: Distinguish VFs/SFs in the SW system image GUID for LAG grouping Tariq Toukan
2026-11-09 17:22 ` Shay Drori
2026-11-03 17:09 ` [PATCH net-next 02/15] net/mlx5: LAG, select the MPESW master from the live member snapshot Tariq Toukan
2026-11-03 17:09 ` [PATCH net-next 03/15] net/mlx5: LAG, skip phys LAG prerequisites for a virt LAG Tariq Toukan
2026-11-03 17:09 ` [PATCH net-next 04/15] net/mlx5: LAG, treat virt LAG as a software managed LAG Tariq Toukan
2026-11-03 17:09 ` [PATCH net-next 05/15] net/mlx5: LAG, decouple iteration end-of-walk from MLX5_MAX_PORTS Tariq Toukan
2026-11-09 18:21 ` Shay Drori [this message]
2026-11-03 17:10 ` [PATCH net-next 06/15] net/mlx5: LAG, rename PF naming to fn Tariq Toukan
2026-11-03 17:10 ` [PATCH net-next 07/15] net/mlx5: LAG, resolve affinity indices only in queue-affinity mode Tariq Toukan
2026-11-03 17:10 ` [PATCH net-next 08/15] net/mlx5: LAG: replace fixed MLX5_MAX_PORTS arrays with dynamic allocation Tariq Toukan
2026-11-09 18:26 ` Shay Drori
2026-11-03 17:10 ` [PATCH net-next 09/15] net/mlx5: LAG, drop MLX5_MAX_PORTS scratch array from device rescan Tariq Toukan
2026-11-03 17:10 ` [PATCH net-next 10/15] net/mlx5: LAG, do not add a device to an already-active LAG Tariq Toukan
2026-11-09 18:31 ` Shay Drori
2026-11-03 17:10 ` [PATCH net-next 11/15] net/mlx5: E-switch, add match-metadata allocator for nested VF/SF eswitches Tariq Toukan
2026-11-03 17:10 ` [PATCH net-next 12/15] net/mlx5: E-switch, match master egress rule by vport metadata Tariq Toukan
2026-11-03 17:10 ` [PATCH net-next 13/15] net/mlx5: E-switch, drop master egress rule under a SW vport LAG Tariq Toukan
2026-11-03 17:10 ` [PATCH net-next 14/15] net/mlx5e: Register eswitch-manager VF/SF self-port with switch-id Tariq Toukan
2026-11-03 17:10 ` [PATCH net-next 15/15] net/mlx5: LAG, enable LAG over functions that do not represent a physical port Tariq Toukan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fa8bdce6-6f89-4de0-b81b-149ac20802a4@nvidia.com \
--to=shayd@nvidia.com \
--cc=agoldberger@nvidia.com \
--cc=andrew+netdev@lunn.ch \
--cc=cjubran@nvidia.com \
--cc=cratiu@nvidia.com \
--cc=davem@davemloft.net \
--cc=edumazet@kernel.org \
--cc=edwards@nvidia.com \
--cc=gal@nvidia.com \
--cc=kuba@kernel.org \
--cc=leon@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=mbloch@nvidia.com \
--cc=moshe@nvidia.com \
--cc=netdev@vger.kernel.org \
--cc=ohartoov@nvidia.com \
--cc=pabeni@redhat.com \
--cc=phaddad@nvidia.com \
--cc=saeedm@nvidia.com \
--cc=tariqt@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®