* [PATCH net V2 1/4] {RDMA,net}/mlx5: cache and use TX-enabled aggregate speed for vports
2026-10-22 19:30 [PATCH net V2 0/4] net/mlx5: fixes for LAG max_tx_speed update flow Tariq Toukan
@ 2026-10-22 19:30 ` Tariq Toukan
2026-10-26 20:32 ` netdev-bot+sashiko
2026-10-22 19:30 ` [PATCH net V2 2/4] net/mlx5: Lag, limit aggregated TX speed by PCIe bandwidth Tariq Toukan
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Tariq Toukan @ 2026-10-22 19:30 UTC (permalink / raw)
To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
netdev, Paolo Abeni
Cc: Bjorn Helgaas, Carolina Jubran, Cosmin Ratiu, Edward Srouji,
Gal Pressman, Jason Gunthorpe, Kees Cook, Leon Romanovsky,
open list, linux-pci, linux-rdma, Maher Sanalla, Mark Bloch,
Or Har-Toov, Saeed Mahameed, Shay Drori, Tariq Toukan
From: Or Har-Toov <ohartoov@nvidia.com>
Cache aggregate TX-enabled speed we calculated, and use it instead of
querying the bond speed so we can, in the following patch, limit it by
the PCI bandwidth.
The bond speed takes the calculation from PTYS, and PTYS is not bounded
by the PCI bandwidth of the NIC.
The max-speed fallback in mlx5_lag_set_vports_agg_speed() is local to
VF/SF vport rate-limit programming; agg_speed_mbps stores operational
speed only, and mlx5_lag_query_aggregated_speed() correctly returns zero
when all member links are down.
Until the aggregate has been measured, or after a failed measurement,
mlx5_lag_query_aggregated_speed() returns -ENODATA.
Fixes: 50f1d188c580 ("net/mlx5: Propagate LAG effective max_tx_speed to vports")
Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Reviewed-by: Shay Drori <shayd@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
drivers/infiniband/hw/mlx5/main.c | 2 +-
.../net/ethernet/mellanox/mlx5/core/esw/qos.c | 1 +
.../net/ethernet/mellanox/mlx5/core/lag/lag.c | 86 +++++++++++++++----
.../net/ethernet/mellanox/mlx5/core/lag/lag.h | 16 +++-
include/linux/mlx5/driver.h | 2 +-
5 files changed, 84 insertions(+), 23 deletions(-)
v1:
https://lore.kernel.org/all/20260910102432.3845360-4-tariqt@nvidia.com/
Changes in v2:
- Count only TX-enabled, link-up ports from the LAG tracker for
non-MPESW modes; MPESW keeps the vport state check
- mlx5_lag_update_agg_speed() returns an error, and
mlx5_lag_set_vports_agg_speed() bails out on failure instead of
using the previous cached speed; the #else stub checks the return too
- Stop exporting mlx5_lag_query_bond_speed(), which has no users
outside mlx5_core
- Describe in the changelog what mlx5_lag_query_aggregated_speed()
returns when all member links are down
- Start agg_speed_mbps as SPEED_UNKNOWN and reset it to SPEED_UNKNOWN on
teardown; mlx5_lag_query_aggregated_speed() returns -ENODATA instead
of a zero speed until the first successful measurement
- A failed measurement invalidates the cached speed
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index a457647edacd..6594d38cf199 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -1654,7 +1654,7 @@ static int mlx5_ib_query_port_speed_from_bond(struct mlx5_ib_dev *dev,
u32 bond_speed;
int err;
- err = mlx5_lag_query_bond_speed(mdev, &bond_speed);
+ err = mlx5_lag_query_aggregated_speed(mdev, &bond_speed);
if (err)
return err;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/esw/qos.c b/drivers/net/ethernet/mellanox/mlx5/core/esw/qos.c
index 0d20f51b9702..0148306421ad 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/esw/qos.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/esw/qos.c
@@ -5,6 +5,7 @@
#include "lib/mlx5.h"
#include "esw/qos.h"
#include "en/port.h"
+#include "lag/lag.h"
#define CREATE_TRACE_POINTS
#include "diag/qos_tracepoint.h"
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
index 4e173b08cb37..57755ceab8a7 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
@@ -360,6 +360,7 @@ static struct mlx5_lag *mlx5_lag_dev_alloc(struct mlx5_core_dev *dev)
ldev->ports = MLX5_CAP_GEN(dev, num_lag_ports);
ldev->buckets = 1;
+ ldev->agg_speed_mbps = SPEED_UNKNOWN;
return ldev;
}
@@ -1431,10 +1432,10 @@ static bool mlx5_lag_should_disable_lag(struct mlx5_lag *ldev, bool do_bond)
ldev->mode != MLX5_LAG_MODE_MPESW;
}
-#ifdef CONFIG_MLX5_ESWITCH
static int mlx5_lag_get_devices_oper_speed(struct mlx5_lag *ldev,
u32 *sum_speed)
{
+ bool mpesw = ldev->mode == MLX5_LAG_MODE_MPESW;
int pf_idx;
*sum_speed = 0;
@@ -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;
+ }
ret = mlx5_port_oper_linkspeed(pf_mdev, &speed);
if (ret) {
@@ -1476,6 +1483,31 @@ static int mlx5_lag_get_devices_oper_speed(struct mlx5_lag *ldev,
return 0;
}
+int mlx5_lag_update_agg_speed(struct mlx5_lag *ldev)
+{
+ u32 speed;
+ int err;
+
+ lockdep_assert_held(&ldev->lock);
+
+ err = mlx5_lag_get_devices_oper_speed(ldev, &speed);
+ if (err) {
+ WRITE_ONCE(ldev->agg_speed_mbps, SPEED_UNKNOWN);
+ return err;
+ }
+
+ WRITE_ONCE(ldev->agg_speed_mbps, speed);
+ return 0;
+}
+
+void mlx5_lag_reset_agg_speed(struct mlx5_lag *ldev)
+{
+ lockdep_assert_held(&ldev->lock);
+
+ WRITE_ONCE(ldev->agg_speed_mbps, SPEED_UNKNOWN);
+}
+
+#ifdef CONFIG_MLX5_ESWITCH
static int mlx5_lag_get_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
{
bool take_max;
@@ -1559,19 +1591,18 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev)
u32 speed;
int pf_idx;
- if (ldev->mode == MLX5_LAG_MODE_MPESW) {
- if (mlx5_lag_get_devices_oper_speed(ldev, &speed))
- return;
- } else {
- speed = ldev->tracker.bond_speed_mbps;
- if (speed == SPEED_UNKNOWN)
- return;
- }
+ if (mlx5_lag_update_agg_speed(ldev))
+ return;
+
+ speed = ldev->agg_speed_mbps;
- /* If speed is not set, fall back to the max achievable speed */
+ /* If speed is 0, fallback to the max speed */
if (!speed && mlx5_lag_get_devices_max_speed(ldev, &speed))
return;
+ if (!speed)
+ return;
+
speed = speed / MLX5_MAX_TX_SPEED_UNIT;
mlx5_ldev_for_each(pf_idx, 0, ldev) {
@@ -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)
@@ -2089,6 +2121,23 @@ static int mlx5_handle_changeinfodata_event(struct mlx5_lag *ldev,
return 1;
}
+/* Returns speed in Mbps. */
+int mlx5_lag_query_aggregated_speed(struct mlx5_core_dev *mdev, u32 *speed)
+{
+ struct mlx5_lag *ldev;
+
+ ldev = mlx5_lag_dev(mdev);
+ if (!ldev)
+ return -ENODEV;
+
+ *speed = READ_ONCE(ldev->agg_speed_mbps);
+ if (*speed == SPEED_UNKNOWN)
+ return -ENODATA;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(mlx5_lag_query_aggregated_speed);
+
static void mlx5_lag_update_tracker_speed(struct lag_tracker *tracker,
struct net_device *ndev)
{
@@ -2146,7 +2195,6 @@ int mlx5_lag_query_bond_speed(struct mlx5_core_dev *mdev, u32 *speed)
spin_unlock_irqrestore(&lag_lock, flags);
return ret;
}
-EXPORT_SYMBOL_GPL(mlx5_lag_query_bond_speed);
/* this handler is always registered to netdev events */
static int mlx5_lag_netdev_event(struct notifier_block *this,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
index e9f0ef83ce1d..45b0e8fd8f99 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
@@ -99,6 +99,7 @@ struct mlx5_lag {
/* Protect lag fields/state changes */
struct mutex lock;
struct lag_mpesw lag_mpesw;
+ u32 agg_speed_mbps;
};
static inline struct mlx5_lag *
@@ -258,12 +259,23 @@ void mlx5_lag_rescan_dev_locked(struct mlx5_lag *ldev,
void mlx5_lag_add_devices_filter(struct mlx5_lag *ldev, u32 filter);
struct mlx5_devcom_comp_dev *mlx5_lag_get_devcom_comp(struct mlx5_lag *ldev);
+int mlx5_lag_update_agg_speed(struct mlx5_lag *ldev);
+void mlx5_lag_reset_agg_speed(struct mlx5_lag *ldev);
+int mlx5_lag_query_bond_speed(struct mlx5_core_dev *dev, u32 *speed);
+
#ifdef CONFIG_MLX5_ESWITCH
void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev);
void mlx5_lag_reset_vports_speed(struct mlx5_lag *ldev);
#else
-static inline void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev) {}
-static inline void mlx5_lag_reset_vports_speed(struct mlx5_lag *ldev) {}
+static inline void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev)
+{
+ mlx5_lag_update_agg_speed(ldev);
+}
+
+static inline void mlx5_lag_reset_vports_speed(struct mlx5_lag *ldev)
+{
+ mlx5_lag_reset_agg_speed(ldev);
+}
#endif
static inline bool mlx5_lag_is_supported(struct mlx5_core_dev *dev)
diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
index 83d0a83bbfbc..ff38086c6c56 100644
--- a/include/linux/mlx5/driver.h
+++ b/include/linux/mlx5/driver.h
@@ -1154,7 +1154,7 @@ int mlx5_cmd_destroy_vport_lag(struct mlx5_core_dev *dev);
bool mlx5_lag_is_roce(struct mlx5_core_dev *dev);
bool mlx5_lag_is_sriov(struct mlx5_core_dev *dev);
bool mlx5_lag_is_active(struct mlx5_core_dev *dev);
-int mlx5_lag_query_bond_speed(struct mlx5_core_dev *dev, u32 *speed);
+int mlx5_lag_query_aggregated_speed(struct mlx5_core_dev *dev, u32 *speed);
bool mlx5_lag_mode_is_hash(struct mlx5_core_dev *dev);
bool mlx5_lag_is_master(struct mlx5_core_dev *dev);
bool mlx5_lag_is_shared_fdb(struct mlx5_core_dev *dev);
--
2.44.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH net V2 1/4] {RDMA,net}/mlx5: cache and use TX-enabled aggregate speed for vports
2026-10-22 19:30 ` [PATCH net V2 1/4] {RDMA,net}/mlx5: cache and use TX-enabled aggregate speed for vports Tariq Toukan
@ 2026-10-26 20:32 ` netdev-bot+sashiko
0 siblings, 0 replies; 12+ messages in thread
From: netdev-bot+sashiko @ 2026-10-26 20:32 UTC (permalink / raw)
To: tariqt
Cc: andrew+netdev, davem, edumazet, kuba, netdev, pabeni, bhelgaas,
cjubran, cratiu, edwards, gal, jgg, kees, leon, linux-kernel,
linux-pci, linux-rdma, msanalla, mbloch, ohartoov, saeedm, shayd
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net V2 2/4] net/mlx5: Lag, limit aggregated TX speed by PCIe bandwidth
2026-10-22 19:30 [PATCH net V2 0/4] net/mlx5: fixes for LAG max_tx_speed update flow Tariq Toukan
2026-10-22 19:30 ` [PATCH net V2 1/4] {RDMA,net}/mlx5: cache and use TX-enabled aggregate speed for vports Tariq Toukan
@ 2026-10-22 19:30 ` Tariq Toukan
2026-10-23 20:06 ` Bjorn Helgaas
2026-10-26 20:32 ` netdev-bot+sashiko
2026-10-22 19:30 ` [PATCH net V2 3/4] {RDMA,net}/mlx5: notify IB uplink on LAG aggregate speed change Tariq Toukan
` (2 subsequent siblings)
4 siblings, 2 replies; 12+ messages in thread
From: Tariq Toukan @ 2026-10-22 19:30 UTC (permalink / raw)
To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
netdev, Paolo Abeni
Cc: Bjorn Helgaas, Carolina Jubran, Cosmin Ratiu, Edward Srouji,
Gal Pressman, Jason Gunthorpe, Kees Cook, Leon Romanovsky,
open list, linux-pci, linux-rdma, Maher Sanalla, Mark Bloch,
Or Har-Toov, Saeed Mahameed, Shay Drori, Tariq Toukan
From: Or Har-Toov <ohartoov@nvidia.com>
FW initializes max_tx_speed to the minimum of the port speed and the
PCI bandwidth. Modifying it with a value computed by different rules
changes the meaning of the field rather than updating it.
Limit the aggregated LAG TX speed by the PCIe bandwidth. Clamping each
uplink separately and then summing could let the aggregate reach a
multiple of the link's real bandwidth, so clamp the sum once instead,
counting only the ports that contribute to it.
This assumes the bonded PFs share a single PCIe link, which holds when
they are functions of the same device. In Socket Direct, each port spans
multiple mdevs on separate PCIe links, so the PCIe budget for that port
is the sum of its mdevs' PCIe bandwidths. Where Socket Direct ports do
not have equal PCIe budgets, the aggregate is clamped to the narrowest
port, which under-reports rather than over-reports the available
bandwidth.
pcie_bandwidth_mbps() reads the endpoint link only, without traversing
upstream bridges, and uses the nominal per-lane transfer rate times the
link width, without encoding overhead. FW initializes max_tx_speed from
the same endpoint-only link status, so using the same calculation keeps
the clamp consistent with the value FW programs before LAG. Ethernet
speeds and max_tx_speed are likewise nominal rates, so the comparison is
like for like. A more accurate bandwidth estimate would make the
aggregate differ from the per-vport value FW sets up for the same link.
If the PCIe bandwidth of a contributing port cannot be read, the speed
calculation fails instead of returning an unclamped or partial result.
A device that has no Link Status register, an RC integrated endpoint,
has no PCIe budget to apply and is not clamped.
Fixes: 50f1d188c580 ("net/mlx5: Propagate LAG effective max_tx_speed to vports")
Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Reviewed-by: Shay Drori <shayd@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
.../net/ethernet/mellanox/mlx5/core/lag/lag.c | 79 ++++++++++++++++++-
drivers/pci/pci.c | 16 ++++
include/linux/pci.h | 2 +
3 files changed, 95 insertions(+), 2 deletions(-)
v1: https://lore.kernel.org/all/20260910102432.3845360-5-tariqt@nvidia.com/
Changes in v2:
- Clamp the aggregate sum once by the narrowest PCIe link instead of
clamping each uplink before summing, which could let the aggregate
exceed the real bandwidth
- Document the shared PCIe link assumption in the changelog
- Add pcie_bandwidth_mbps() to drivers/pci/pci.c, which reads
PCI_EXP_LNKSTA once and decodes both speed and width from that
value, avoiding a race between separate reads of speed and width
- In Socket Direct, sum the PCIe bandwidths of all mdevs in the same
port group instead of taking the minimum, since those links are
independent
- Fail the aggregate and max speed calculations when a PCIe bandwidth
cannot be read, instead of skipping the clamp; an RC integrated
endpoint has no Link Status and is not clamped
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
index 57755ceab8a7..05e6d0aa31fc 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
@@ -1432,10 +1432,55 @@ static bool mlx5_lag_should_disable_lag(struct mlx5_lag *ldev, bool do_bond)
ldev->mode != MLX5_LAG_MODE_MPESW;
}
+static int mlx5_lag_dev_pcie_bw(struct mlx5_core_dev *dev, u32 *bw)
+{
+ if (pci_pcie_type(dev->pdev) == PCI_EXP_TYPE_RC_END) {
+ *bw = U32_MAX;
+ return 0;
+ }
+
+ *bw = pcie_bandwidth_mbps(dev->pdev);
+ return *bw ? 0 : -EIO;
+}
+
+static int mlx5_lag_port_pcie_bw(struct mlx5_lag *ldev, struct lag_func *port,
+ u32 *bw)
+{
+ struct lag_func *member;
+ bool unlimited = false;
+ u32 member_bw;
+ int idx;
+ int err;
+
+ if (!port->group_id)
+ return mlx5_lag_dev_pcie_bw(port->dev, bw);
+
+ *bw = 0;
+ mlx5_lag_for_each(idx, 0, ldev, port->group_id) {
+ member = mlx5_lag_pf(ldev, idx);
+ if (!member)
+ continue;
+
+ err = mlx5_lag_dev_pcie_bw(member->dev, &member_bw);
+ if (err)
+ return err;
+
+ if (member_bw == U32_MAX)
+ unlimited = true;
+ else
+ *bw += member_bw;
+ }
+
+ if (unlimited)
+ *bw = U32_MAX;
+ return 0;
+}
+
static int mlx5_lag_get_devices_oper_speed(struct mlx5_lag *ldev,
u32 *sum_speed)
{
bool mpesw = ldev->mode == MLX5_LAG_MODE_MPESW;
+ u32 pci_bw = U32_MAX;
int pf_idx;
*sum_speed = 0;
@@ -1443,6 +1488,7 @@ static int mlx5_lag_get_devices_oper_speed(struct mlx5_lag *ldev,
u8 opmod = MLX5_VPORT_STATE_OP_MOD_VNIC_VPORT;
struct mlx5_core_dev *pf_mdev;
struct lag_func *pf;
+ u32 dev_pci_bw;
u32 speed;
u8 state;
int ret;
@@ -1477,9 +1523,20 @@ static int mlx5_lag_get_devices_oper_speed(struct mlx5_lag *ldev,
return ret;
}
+ ret = mlx5_lag_port_pcie_bw(ldev, pf, &dev_pci_bw);
+ if (ret) {
+ mlx5_core_dbg(pf_mdev,
+ "Failed to get PCIe bandwidth (err=%d)\n",
+ ret);
+ return ret;
+ }
+
+ pci_bw = min(pci_bw, dev_pci_bw);
*sum_speed += speed;
}
+ *sum_speed = min(*sum_speed, pci_bw);
+
return 0;
}
@@ -1510,6 +1567,7 @@ void mlx5_lag_reset_agg_speed(struct mlx5_lag *ldev)
#ifdef CONFIG_MLX5_ESWITCH
static int mlx5_lag_get_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
{
+ u32 pci_bw = U32_MAX;
bool take_max;
int pf_idx;
@@ -1521,6 +1579,7 @@ static int mlx5_lag_get_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
mlx5_ldev_for_each(pf_idx, 0, ldev) {
struct mlx5_core_dev *pf_mdev;
struct lag_func *pf;
+ u32 dev_pci_bw;
u32 speed;
int ret;
@@ -1539,10 +1598,26 @@ static int mlx5_lag_get_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
return ret;
}
- *max_speed = take_max ?
- max(*max_speed, speed) : *max_speed + speed;
+ ret = mlx5_lag_port_pcie_bw(ldev, pf, &dev_pci_bw);
+ if (ret) {
+ mlx5_core_dbg(pf_mdev,
+ "Failed to get PCIe bandwidth (err=%d)\n",
+ ret);
+ return ret;
+ }
+
+ if (take_max) {
+ speed = min(speed, dev_pci_bw);
+ *max_speed = max(*max_speed, speed);
+ } else {
+ pci_bw = min(pci_bw, dev_pci_bw);
+ *max_speed += speed;
+ }
}
+ if (!take_max)
+ *max_speed = min(*max_speed, pci_bw);
+
return 0;
}
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 8cc6a89130b5..064eef237d6f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6006,6 +6006,22 @@ int pcie_link_speed_mbps(struct pci_dev *pdev)
}
EXPORT_SYMBOL(pcie_link_speed_mbps);
+u32 pcie_bandwidth_mbps(struct pci_dev *pdev)
+{
+ u16 lnksta;
+ u32 width;
+ int speed;
+
+ if (pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnksta))
+ return 0;
+ speed = pcie_dev_speed_mbps(to_pcie_link_speed(lnksta));
+ if (speed <= 0)
+ return 0;
+ width = FIELD_GET(PCI_EXP_LNKSTA_NLW, lnksta);
+ return speed * width;
+}
+EXPORT_SYMBOL_GPL(pcie_bandwidth_mbps);
+
/**
* pcie_bandwidth_available - determine minimum link settings of a PCIe
* device and its bandwidth limitation
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5ffcf5ce8c96..202bc8680601 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1479,6 +1479,8 @@ u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev,
enum pci_bus_speed *speed,
enum pcie_link_width *width);
int pcie_link_speed_mbps(struct pci_dev *pdev);
+u32 pcie_bandwidth_mbps(struct pci_dev *pdev);
+
void pcie_print_link_status(struct pci_dev *dev);
int pcie_reset_flr(struct pci_dev *dev, bool probe);
int pcie_flr(struct pci_dev *dev);
--
2.44.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH net V2 2/4] net/mlx5: Lag, limit aggregated TX speed by PCIe bandwidth
2026-10-22 19:30 ` [PATCH net V2 2/4] net/mlx5: Lag, limit aggregated TX speed by PCIe bandwidth Tariq Toukan
@ 2026-10-23 20:06 ` Bjorn Helgaas
2026-10-26 20:32 ` netdev-bot+sashiko
1 sibling, 0 replies; 12+ messages in thread
From: Bjorn Helgaas @ 2026-10-23 20:06 UTC (permalink / raw)
To: Tariq Toukan
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
netdev, Paolo Abeni, Bjorn Helgaas, Carolina Jubran, Cosmin Ratiu,
Edward Srouji, Gal Pressman, Jason Gunthorpe, Kees Cook,
Leon Romanovsky, open list, linux-pci, linux-rdma, Maher Sanalla,
Mark Bloch, Or Har-Toov, Saeed Mahameed, Shay Drori
On Thu, Oct 22, 2026 at 10:30:51PM +0300, Tariq Toukan wrote:
> From: Or Har-Toov <ohartoov@nvidia.com>
>
> FW initializes max_tx_speed to the minimum of the port speed and the
> PCI bandwidth. Modifying it with a value computed by different rules
> changes the meaning of the field rather than updating it.
>
> Limit the aggregated LAG TX speed by the PCIe bandwidth. Clamping each
> uplink separately and then summing could let the aggregate reach a
> multiple of the link's real bandwidth, so clamp the sum once instead,
> counting only the ports that contribute to it.
>
> This assumes the bonded PFs share a single PCIe link, which holds when
> they are functions of the same device. In Socket Direct, each port spans
> multiple mdevs on separate PCIe links, so the PCIe budget for that port
> is the sum of its mdevs' PCIe bandwidths. Where Socket Direct ports do
> not have equal PCIe budgets, the aggregate is clamped to the narrowest
> port, which under-reports rather than over-reports the available
> bandwidth.
>
> pcie_bandwidth_mbps() reads the endpoint link only, without traversing
> upstream bridges, and uses the nominal per-lane transfer rate times the
> link width, without encoding overhead. FW initializes max_tx_speed from
> the same endpoint-only link status, so using the same calculation keeps
> the clamp consistent with the value FW programs before LAG. Ethernet
> speeds and max_tx_speed are likewise nominal rates, so the comparison is
> like for like. A more accurate bandwidth estimate would make the
> aggregate differ from the per-vport value FW sets up for the same link.
>
> If the PCIe bandwidth of a contributing port cannot be read, the speed
> calculation fails instead of returning an unclamped or partial result.
> A device that has no Link Status register, an RC integrated endpoint,
> has no PCIe budget to apply and is not clamped.
>
> Fixes: 50f1d188c580 ("net/mlx5: Propagate LAG effective max_tx_speed to vports")
> Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
> Reviewed-by: Mark Bloch <mbloch@nvidia.com>
> Reviewed-by: Shay Drori <shayd@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> ...
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8cc6a89130b5..064eef237d6f 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6006,6 +6006,22 @@ int pcie_link_speed_mbps(struct pci_dev *pdev)
> }
> EXPORT_SYMBOL(pcie_link_speed_mbps);
>
> +u32 pcie_bandwidth_mbps(struct pci_dev *pdev)
> +{
> + u16 lnksta;
> + u32 width;
> + int speed;
> +
> + if (pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnksta))
> + return 0;
> + speed = pcie_dev_speed_mbps(to_pcie_link_speed(lnksta));
> + if (speed <= 0)
> + return 0;
> + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, lnksta);
> + return speed * width;
> +}
> +EXPORT_SYMBOL_GPL(pcie_bandwidth_mbps);
Can you split this into its own patch?
It looks like cxl_pci_get_bandwidth() and maybe even
pcie_bandwidth_available() could be refactored to use this.
> /**
> * pcie_bandwidth_available - determine minimum link settings of a PCIe
> * device and its bandwidth limitation
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 5ffcf5ce8c96..202bc8680601 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1479,6 +1479,8 @@ u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev,
> enum pci_bus_speed *speed,
> enum pcie_link_width *width);
> int pcie_link_speed_mbps(struct pci_dev *pdev);
> +u32 pcie_bandwidth_mbps(struct pci_dev *pdev);
> +
> void pcie_print_link_status(struct pci_dev *dev);
> int pcie_reset_flr(struct pci_dev *dev, bool probe);
> int pcie_flr(struct pci_dev *dev);
> --
> 2.44.0
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net V2 2/4] net/mlx5: Lag, limit aggregated TX speed by PCIe bandwidth
2026-10-22 19:30 ` [PATCH net V2 2/4] net/mlx5: Lag, limit aggregated TX speed by PCIe bandwidth Tariq Toukan
2026-10-23 20:06 ` Bjorn Helgaas
@ 2026-10-26 20:32 ` netdev-bot+sashiko
1 sibling, 0 replies; 12+ messages in thread
From: netdev-bot+sashiko @ 2026-10-26 20:32 UTC (permalink / raw)
To: tariqt
Cc: andrew+netdev, davem, edumazet, kuba, netdev, pabeni, bhelgaas,
cjubran, cratiu, edwards, gal, jgg, kees, leon, linux-kernel,
linux-pci, linux-rdma, msanalla, mbloch, ohartoov, saeedm, shayd
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 2
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] mlx5_lag_dev_pcie_bw()
(drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c) skips the PCIe…
- [Low] This mlx5 'Fixes:' patch adds a new exported PCI core API,
pcie_bandwidth_mbps() in drivers/pci/pci.c, declared in…
- [Low] Concern: at this commit, mlx5_lag_reset_vports_speed() (lag.c),
called from mlx5_do_bond() when LAG is disabled, still writes each…
--
net/mlx5: Lag, limit aggregated TX speed by PCIe bandwidth
This patch clamps the aggregated LAG TX speed, and the max speed sent to
vports, by the PCIe bandwidth of the ports that contribute to it. In
Socket Direct groups, the bandwidth of each mdev is summed per port. It
also adds a new PCI core helper, pcie_bandwidth_mbps(). If the PCIe
bandwidth cannot be read, the speed calculation now fails, except for RC
integrated endpoints.
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index 57755ceab8a75..05e6d0aa31fcf 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> @@ -1432,10 +1432,55 @@ static bool mlx5_lag_should_disable_lag(struct mlx5_lag *ldev, bool do_bond)
> ldev->mode != MLX5_LAG_MODE_MPESW;
> }
>
> +static int mlx5_lag_dev_pcie_bw(struct mlx5_core_dev *dev, u32 *bw)
> +{
> + if (pci_pcie_type(dev->pdev) == PCI_EXP_TYPE_RC_END) {
> + *bw = U32_MAX;
> + return 0;
> + }
> +
> + *bw = pcie_bandwidth_mbps(dev->pdev);
> + return *bw ? 0 : -EIO;
> +}
[Severity: Medium]
PCI_EXP_TYPE_RC_END is the only case exempted here. What happens for a
bonded PF where pci_is_pcie() is false? One example is a VFIO-assigned
PF on a conventional PCI segment of a q35 guest, where the Express
capability is hidden.
In that case pcie_capability_reg_implemented() returns false, so the
LNKSTA read reports success and leaves the value at 0:
drivers/pci/access.c:pcie_capability_read_word() {
*val = 0;
...
if (pcie_capability_reg_implemented(dev, pos)) {
...
}
...
return 0;
}
CLS 0 maps to PCI_SPEED_UNKNOWN, so pcie_dev_speed_mbps() returns
-EINVAL and pcie_bandwidth_mbps() returns 0. Since pcie_flags_reg is 0,
pci_pcie_type() reports PCI_EXP_TYPE_ENDPOINT. The RC_END check does not
match, and this function returns -EIO. The same thing happens for any CLS
value from 7 to 0xF, which pcie_link_speed[] maps to PCI_SPEED_UNKNOWN.
If so, wouldn't mlx5_lag_get_devices_oper_speed() and
mlx5_lag_get_devices_max_speed() fail on every call?
mlx5_do_bond()
mlx5_lag_set_vports_agg_speed()
mlx5_lag_update_agg_speed()
mlx5_lag_get_devices_oper_speed()
mlx5_lag_port_pcie_bw()
mlx5_lag_dev_pcie_bw() <- -EIO
mlx5_lag_update_agg_speed() then stores SPEED_UNKNOWN:
err = mlx5_lag_get_devices_oper_speed(ldev, &speed);
if (err) {
WRITE_ONCE(ldev->agg_speed_mbps, SPEED_UNKNOWN);
return err;
}
Because of this, mlx5_lag_set_vports_agg_speed() returns early and never
propagates vport max_tx_speed. mlx5_lag_query_aggregated_speed() also
returns -ENODATA to mlx5_ib_query_port_speed_from_bond(), both for RoCE
LAG and for the SR-IOV LAG uplink rep. Before this patch these paths
returned a valid speed.
The commit message says:
A device that has no Link Status register, an RC integrated endpoint,
has no PCIe budget to apply and is not clamped.
pcie_bandwidth_mbps() returns the same 0 whether Link Status is missing,
the read failed, or the CLS encoding is unknown, so callers cannot tell
these cases apart. Should the "not clamped" case cover every device
without an implemented Link Status register, not just RC_END?
The later patch in this series, "net/mlx5: Lag, save and restore vport
max_tx_speed across LAG", treats pcie_bandwidth_mbps() == 0 as "no
clamp" in mlx5_lag_get_restore_speed(). That does not match the -EIO
handling here.
[ ... ]
> @@ -1539,10 +1598,26 @@ static int mlx5_lag_get_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
[ ... ]
> + if (take_max) {
> + speed = min(speed, dev_pci_bw);
> + *max_speed = max(*max_speed, speed);
> + } else {
> + pci_bw = min(pci_bw, dev_pci_bw);
> + *max_speed += speed;
> + }
> }
>
> + if (!take_max)
> + *max_speed = min(*max_speed, pci_bw);
> +
> return 0;
> }
[Severity: Low]
The commit message says FW programs max_tx_speed as the minimum of port
speed and PCIe bandwidth, and that writing a value computed by other
rules changes the meaning of the field. At this commit, the LAG disable
path still writes an unclamped value:
mlx5_do_bond()
mlx5_lag_should_disable_lag()
mlx5_lag_reset_vports_speed()
drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c:mlx5_lag_reset_vports_speed() {
...
ret = mlx5_port_oper_linkspeed(mdev, &speed);
...
speed = speed / MLX5_MAX_TX_SPEED_UNIT;
mlx5_lag_modify_device_vports_speed(mdev, speed);
...
}
After LAG teardown, can vports end up with a max_tx_speed above the
PCIe link bandwidth?
The last patch in this series, "net/mlx5: Lag, save and restore vport
max_tx_speed across LAG", rewrites mlx5_lag_reset_vports_speed(). It
restores the per-vport value that FW programmed, and its fallback in
mlx5_lag_get_restore_speed() is clamped by pcie_bandwidth_mbps(). So
this only affects the bisection window between this patch and that one.
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8cc6a89130b53..064eef237d6f4 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6006,6 +6006,22 @@ int pcie_link_speed_mbps(struct pci_dev *pdev)
> }
> EXPORT_SYMBOL(pcie_link_speed_mbps);
>
> +u32 pcie_bandwidth_mbps(struct pci_dev *pdev)
> +{
> + u16 lnksta;
> + u32 width;
> + int speed;
> +
> + if (pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnksta))
> + return 0;
> + speed = pcie_dev_speed_mbps(to_pcie_link_speed(lnksta));
> + if (speed <= 0)
> + return 0;
> + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, lnksta);
> + return speed * width;
> +}
> +EXPORT_SYMBOL_GPL(pcie_bandwidth_mbps);
[Severity: Low]
This isn't a bug, but this mlx5 Fixes: patch adds a new exported PCI core
API with no kernel-doc, and the commit has no Acked-by from the PCI
maintainer.
The name is close to pcie_bandwidth_available(), whose kernel-doc also
says it returns Mb/s, but the two behave differently.
pcie_bandwidth_available() walks upstream bridges and subtracts encoding
overhead:
next_bw = next_width * PCIE_SPEED2MBS_ENC(next_speed);
pcie_bandwidth_mbps() returns the nominal rate times width for the
endpoint link only. It also returns 0 on any failure, while
pcie_link_speed_mbps() just above returns a negative errno.
Could the nominal, endpoint-only behaviour and the 0-on-error convention
be documented in a kernel-doc comment here, instead of only in the
changelog?
Is EXPORT_SYMBOL_GPL intentional? The neighbouring pcie_link_speed_mbps()
and pcie_bandwidth_available() both use EXPORT_SYMBOL.
The include/linux/pci.h hunk also adds a stray blank line after the new
declaration.
mlx5's en/params.c already compares link speed against
pcie_bandwidth_available(). With this patch the same driver has two
different definitions of the PCIe budget.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022193053.3815866-1-tariqt%40nvidia.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net V2 3/4] {RDMA,net}/mlx5: notify IB uplink on LAG aggregate speed change
2026-10-22 19:30 [PATCH net V2 0/4] net/mlx5: fixes for LAG max_tx_speed update flow Tariq Toukan
2026-10-22 19:30 ` [PATCH net V2 1/4] {RDMA,net}/mlx5: cache and use TX-enabled aggregate speed for vports Tariq Toukan
2026-10-22 19:30 ` [PATCH net V2 2/4] net/mlx5: Lag, limit aggregated TX speed by PCIe bandwidth Tariq Toukan
@ 2026-10-22 19:30 ` Tariq Toukan
2026-10-26 20:32 ` netdev-bot+sashiko
2026-10-22 19:30 ` [PATCH net V2 4/4] net/mlx5: Lag, save and restore vport max_tx_speed across LAG Tariq Toukan
2026-10-22 19:37 ` [PATCH net V2 0/4] net/mlx5: fixes for LAG max_tx_speed update flow netdev-bot+sinfo
4 siblings, 1 reply; 12+ messages in thread
From: Tariq Toukan @ 2026-10-22 19:30 UTC (permalink / raw)
To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
netdev, Paolo Abeni
Cc: Bjorn Helgaas, Carolina Jubran, Cosmin Ratiu, Edward Srouji,
Gal Pressman, Jason Gunthorpe, Kees Cook, Leon Romanovsky,
open list, linux-pci, linux-rdma, Maher Sanalla, Mark Bloch,
Or Har-Toov, Saeed Mahameed, Shay Drori, Tariq Toukan
From: Or Har-Toov <ohartoov@nvidia.com>
In switchdev LAG, a link-state change fires two
IB_EVENT_DEVICE_SPEED_CHANGE events: the physical port fires immediately
with the port state change, and once the LAG reconverges and pushes the
new aggregate speed to every vport via MODIFY_VPORT_STATE, FW fires a
PORT_CHANGE event per vport, which is how VF consumers learn the
aggregate actually changed.
The uplink vport is deliberately skipped when pushing the aggregate
speed, so the IB uplink device never gets a second event once the
aggregate has settled. In RoCE LAG there are no VF vports at all, so
nothing fires after reconvergence.
Raise IB_EVENT_DEVICE_SPEED_CHANGE from the LAG layer, in
mlx5_lag_update_agg_speed(), right after the cached aggregate is
updated. That cache is where the IB layer now takes the effective speed
from, so a consumer that queries after receiving the event is
guaranteed to read the new value.
blocking_notifier_call_chain() holds the blocking_notifier_head rwsem
for the full duration of lag_event();
blocking_notifier_chain_unregister() in mlx5_eth_lag_cleanup() takes the
write side and cannot return until any in-flight callback has completed,
so no event can be dispatched to a freed device after unregistration.
Fixes: 50f1d188c580 ("net/mlx5: Propagate LAG effective max_tx_speed to vports")
Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Reviewed-by: Shay Drori <shayd@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
drivers/infiniband/hw/mlx5/main.c | 10 ++++++++++
.../net/ethernet/mellanox/mlx5/core/lag/lag.c | 20 +++++++++++++++++++
.../net/ethernet/mellanox/mlx5/core/lag/lag.h | 1 +
include/linux/mlx5/device.h | 1 +
4 files changed, 32 insertions(+)
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 6594d38cf199..cfa7c7ba2b73 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -3690,6 +3690,16 @@ static int lag_event(struct notifier_block *nb, unsigned long event, void *data)
rdma_roce_rescan_port(ibdev, portnum + 1);
}
break;
+ case MLX5_DRIVER_EVENT_LAG_SPEED_CHANGE: {
+ struct ib_event speed_event = {};
+
+ if (!dev->ib_active)
+ break;
+ speed_event.device = ibdev;
+ speed_event.event = IB_EVENT_DEVICE_SPEED_CHANGE;
+ ib_dispatch_event(&speed_event);
+ break;
+ }
default:
return NOTIFY_DONE;
}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
index 05e6d0aa31fc..afe0b9909f2d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
@@ -1540,8 +1540,24 @@ static int mlx5_lag_get_devices_oper_speed(struct mlx5_lag *ldev,
return 0;
}
+void mlx5_lag_notify_speed_change(struct mlx5_lag *ldev)
+{
+ struct lag_func *pf;
+ int idx;
+
+ idx = mlx5_lag_get_dev_index_by_seq(ldev, MLX5_LAG_P1);
+ if (idx < 0)
+ return;
+ pf = mlx5_lag_pf(ldev, idx);
+ if (!pf)
+ return;
+ blocking_notifier_call_chain(&pf->dev->priv.lag_nh,
+ MLX5_DRIVER_EVENT_LAG_SPEED_CHANGE, NULL);
+}
+
int mlx5_lag_update_agg_speed(struct mlx5_lag *ldev)
{
+ u32 old_speed;
u32 speed;
int err;
@@ -1553,7 +1569,11 @@ int mlx5_lag_update_agg_speed(struct mlx5_lag *ldev)
return err;
}
+ old_speed = READ_ONCE(ldev->agg_speed_mbps);
WRITE_ONCE(ldev->agg_speed_mbps, speed);
+ if (speed != old_speed)
+ mlx5_lag_notify_speed_change(ldev);
+
return 0;
}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
index 45b0e8fd8f99..420655468d78 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
@@ -259,6 +259,7 @@ void mlx5_lag_rescan_dev_locked(struct mlx5_lag *ldev,
void mlx5_lag_add_devices_filter(struct mlx5_lag *ldev, u32 filter);
struct mlx5_devcom_comp_dev *mlx5_lag_get_devcom_comp(struct mlx5_lag *ldev);
+void mlx5_lag_notify_speed_change(struct mlx5_lag *ldev);
int mlx5_lag_update_agg_speed(struct mlx5_lag *ldev);
void mlx5_lag_reset_agg_speed(struct mlx5_lag *ldev);
int mlx5_lag_query_bond_speed(struct mlx5_core_dev *dev, u32 *speed);
diff --git a/include/linux/mlx5/device.h b/include/linux/mlx5/device.h
index 8cb321a9fb3d..1ec2331c24ee 100644
--- a/include/linux/mlx5/device.h
+++ b/include/linux/mlx5/device.h
@@ -374,6 +374,7 @@ enum mlx5_driver_event {
MLX5_DRIVER_EVENT_AFFILIATION_DONE,
MLX5_DRIVER_EVENT_AFFILIATION_REMOVED,
MLX5_DRIVER_EVENT_ACTIVE_BACKUP_LAG_CHANGE_LOWERSTATE,
+ MLX5_DRIVER_EVENT_LAG_SPEED_CHANGE,
};
enum {
--
2.44.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH net V2 3/4] {RDMA,net}/mlx5: notify IB uplink on LAG aggregate speed change
2026-10-22 19:30 ` [PATCH net V2 3/4] {RDMA,net}/mlx5: notify IB uplink on LAG aggregate speed change Tariq Toukan
@ 2026-10-26 20:32 ` netdev-bot+sashiko
0 siblings, 0 replies; 12+ messages in thread
From: netdev-bot+sashiko @ 2026-10-26 20:32 UTC (permalink / raw)
To: tariqt
Cc: andrew+netdev, davem, edumazet, kuba, netdev, pabeni, bhelgaas,
cjubran, cratiu, edwards, gal, jgg, kees, leon, linux-kernel,
linux-pci, linux-rdma, msanalla, mbloch, ohartoov, saeedm, shayd
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 2 · Medium: 1 · Low: 0
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).
- [High] The fix does not reach the IB uplink device in non-shared-FDB
switchdev (SRIOV) LAG, although the commit message says it fixes the…
- [High] Use-after-free of struct ib_device through async event work
queued by the new MLX5_DRIVER_EVENT_LAG_SPEED_CHANGE handler.
- [Medium] net/mlx5: LAG speed change notified before updating eswitch
vport speeds
--
{RDMA,net}/mlx5: notify IB uplink on LAG aggregate speed change
This patch raises IB_EVENT_DEVICE_SPEED_CHANGE on the IB uplink device
when the cached LAG aggregate speed changes. mlx5_lag_update_agg_speed()
now calls a new helper, mlx5_lag_notify_speed_change(). The helper sends
MLX5_DRIVER_EVENT_LAG_SPEED_CHANGE on the P1 PF's lag_nh chain, and
lag_event() in mlx5_ib turns it into an IB async event.
> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index 6594d38cf1993..cfa7c7ba2b737 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -3690,6 +3690,16 @@ static int lag_event(struct notifier_block *nb, unsigned long event, void *data)
> rdma_roce_rescan_port(ibdev, portnum + 1);
> }
> break;
> + case MLX5_DRIVER_EVENT_LAG_SPEED_CHANGE: {
> + struct ib_event speed_event = {};
> +
> + if (!dev->ib_active)
> + break;
> + speed_event.device = ibdev;
> + speed_event.event = IB_EVENT_DEVICE_SPEED_CHANGE;
> + ib_dispatch_event(&speed_event);
[Severity: High]
Can the work queued here outlive the ib_device?
ib_dispatch_event() copies the event, including the raw ibdev pointer,
into an ib_update_work and queues it on ib_wq. It takes no reference on
the device. Nothing synchronizes the dev->ib_active check with
__mlx5_ib_remove():
__mlx5_ib_remove() {
dev->ib_active = false;
/* Number of stages to cleanup */
while (stage) {
stage--;
if (profile->stage[stage].cleanup)
profile->stage[stage].cleanup(dev);
}
kfree(dev->port);
ib_dealloc_device(&dev->ib_dev);
}
The stages are cleaned up in reverse order. In pf_profile and
raw_eth_profile, MLX5_IB_STAGE_IB_REG comes after MLX5_IB_STAGE_ROCE.
That means ib_unregister_device() runs first, including its last
flush_workqueue(ib_wq) in ib_cache_cleanup_one(). Only after that does
mlx5_eth_lag_cleanup()->mlx5e_lag_event_unregister() run.
One possible ordering:
CPU1 (bond work or mlx5_mpesw_speed_update_work, holding ldev->lock)
mlx5_lag_update_agg_speed()
mlx5_lag_notify_speed_change()
blocking_notifier_call_chain() /* down_read(&lag_nh.rwsem) */
lag_event()
sees dev->ib_active == true
<preempted>
CPU2 (rmmod mlx5_ib or auxiliary unbind)
__mlx5_ib_remove()
dev->ib_active = false;
mlx5_ib_stage_ib_reg_cleanup()
ib_unregister_device() /* last ib_wq flush */
mlx5_eth_lag_cleanup()
mlx5e_lag_event_unregister()
blocking_notifier_chain_unregister() /* waits on rwsem */
CPU1
ib_dispatch_event()
queue_work(ib_wq, &work->work);
up_read(&lag_nh.rwsem)
CPU2
ib_dealloc_device()
... ib_device_release()
kfree_rcu(dev, rcu_head);
ib_wq worker
ib_generic_event_task()
ib_dispatch_event_clients()
down_read(&event->device->event_handler_rwsem); /* freed */
The commit message says:
blocking_notifier_chain_unregister() in mlx5_eth_lag_cleanup() takes the
write side and cannot return until any in-flight callback has completed,
so no event can be dispatched to a freed device after unregistration.
The rwsem waits for lag_event() to return. Does it also wait for the
ib_wq work that lag_event() queued? Nothing seems to flush ib_wq after
mlx5e_lag_event_unregister().
The existing mlx5_ib_event path seems to avoid this, because its
DEVICE_NOTIFIER stage is torn down before IB_REG. The last patch in this
series doesn't appear to change this path.
> + break;
> + }
> default:
> return NOTIFY_DONE;
> }
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index 05e6d0aa31fcf..afe0b9909f2dc 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> @@ -1540,8 +1540,24 @@ static int mlx5_lag_get_devices_oper_speed(struct mlx5_lag *ldev,
> return 0;
> }
>
> +void mlx5_lag_notify_speed_change(struct mlx5_lag *ldev)
> +{
> + struct lag_func *pf;
> + int idx;
> +
> + idx = mlx5_lag_get_dev_index_by_seq(ldev, MLX5_LAG_P1);
> + if (idx < 0)
> + return;
> + pf = mlx5_lag_pf(ldev, idx);
> + if (!pf)
> + return;
> + blocking_notifier_call_chain(&pf->dev->priv.lag_nh,
> + MLX5_DRIVER_EVENT_LAG_SPEED_CHANGE, NULL);
[Severity: Medium]
Is the event sent before the vports it describes have been updated?
mlx5_lag_set_vports_agg_speed() calls mlx5_lag_update_agg_speed()
first. Only after that does it walk the PFs and push the new
max_tx_speed to each eswitch vport with MODIFY_VPORT_STATE:
mlx5_lag_set_vports_agg_speed() {
if (mlx5_lag_update_agg_speed(ldev))
return;
...
mlx5_ldev_for_each(pf_idx, 0, ldev) {
...
mlx5_lag_modify_device_vports_speed(mdev, speed);
}
}
With this patch, mlx5_lag_update_agg_speed() calls
mlx5_lag_notify_speed_change() as soon as agg_speed_mbps is written.
So lag_event() queues IB_EVENT_DEVICE_SPEED_CHANGE on ib_wq while the
FW vport state still holds the old speed. This applies to
mlx5_do_bond(), mlx5_mpesw_speed_update_work() and
mlx5_lag_shared_fdb_create(), which all go through
mlx5_lag_set_vports_agg_speed().
On the switchdev representor ib_device, only the uplink port reads the
cached aggregate. The VF/SF representor ports on the same ib_device
query FW instead:
mlx5_ib_query_port_speed_rep() {
if (rep->vport == MLX5_VPORT_UPLINK) {
if (mlx5_lag_is_sriov(mdev))
return mlx5_ib_query_port_speed_from_bond(...);
...
}
op_mod = MLX5_VPORT_STATE_OP_MOD_ESW_VPORT;
return mlx5_ib_query_port_speed_from_vport(dev->mdev, op_mod,
rep->vport, ...);
}
A consumer could handle the event and query ports 2..N before the
mlx5_lag_modify_device_vports_speed() loop has finished. It would then
read a stale max_tx_speed. I couldn't find anything that raises a
second IB_EVENT_DEVICE_SPEED_CHANGE on the representor ib_device once
the loop completes. In that case the stale value would be all the
consumer ever sees for that change.
The commit message says:
That cache is where the IB layer now takes the effective speed
from, so a consumer that queries after receiving the event is
guaranteed to read the new value.
That holds for the uplink port. Does it hold for the other
representor ports on the same device?
Could the notification be sent from mlx5_lag_set_vports_agg_speed()
after the vport loop, instead of from mlx5_lag_update_agg_speed()? Or
is there a reason the event has to be raised before the vports are
updated? Any callers of mlx5_lag_update_agg_speed() outside
mlx5_lag_set_vports_agg_speed(), such as RoCE LAG, would presumably
still need their own notification.
[Severity: High]
Does this event reach the IB uplink device in switchdev (SRIOV) LAG
when shared FDB is not used? That is the case when
mlx5_lag_shared_fdb_supported() returns false and mlx5_do_bond() takes
the mlx5_activate_lag(..., MLX5_LAG_MODE_SRIOV, false) branch.
The event only goes to P1's lag_nh. mlx5_eth_lag_init() registers
lag_events only if LAG is already active when the IB device
initializes:
mlx5_eth_lag_init() {
...
ns = mlx5_get_flow_namespace(mdev, MLX5_FLOW_NAMESPACE_LAG);
if (!ns || !mlx5_lag_is_active(mdev))
return 0;
...
mlx5e_lag_event_register(dev);
...
}
IB reps are loaded when switchdev mode is enabled, which is before the
bond forms. The non-shared-FDB branch of mlx5_do_bond() doesn't reload
or rescan the IB devices. Only the RoCE LAG and shared FDB paths do:
mlx5_do_bond() {
...
if (!shared_fdb)
mlx5_lag_set_vports_agg_speed(ldev);
} else if (mlx5_lag_should_modify_lag(ldev, do_bond)) {
mlx5_modify_lag(ldev, &tracker);
mlx5_lag_set_vports_agg_speed(ldev);
...
}
mlx5_create_lag() also reinitializes the chain when LAG is activated,
which drops any earlier subscriber:
BLOCKING_INIT_NOTIFIER_HEAD(&dev0->priv.lag_nh);
Without shared FDB, mlx5_ib_vport_rep_load() also leaves lag_master
pointing at the PF itself. So each PF's uplink IB device has
ibdev->mdev set to its own PF. A P2 uplink device would subscribe to
P2's lag_nh, if it subscribed at all, so an event sent only to P1 could
not reach it.
These uplink devices still report the cached aggregate, because
mlx5_ib_query_port_speed_rep() doesn't check for shared FDB:
if (rep->vport == MLX5_VPORT_UPLINK) {
if (mlx5_lag_is_sriov(mdev))
return mlx5_ib_query_port_speed_from_bond(dev,
port_num,
speed);
In this setup, wouldn't the uplink IB device still get only the
immediate physical-port event, possibly before agg_speed_mbps is
updated, and nothing once the aggregate settles? That is the behavior
the commit message says it fixes:
The uplink vport is deliberately skipped when pushing the aggregate
speed, so the IB uplink device never gets a second event once the
aggregate has settled.
Could the notification go through a chain on each PF that every mlx5_ib
device is always subscribed to, instead of only P1's lag_nh? The later
patch in this series, "net/mlx5: Lag, save and restore vport
max_tx_speed across LAG", doesn't appear to touch the notification path.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022193053.3815866-1-tariqt%40nvidia.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net V2 4/4] net/mlx5: Lag, save and restore vport max_tx_speed across LAG
2026-10-22 19:30 [PATCH net V2 0/4] net/mlx5: fixes for LAG max_tx_speed update flow Tariq Toukan
` (2 preceding siblings ...)
2026-10-22 19:30 ` [PATCH net V2 3/4] {RDMA,net}/mlx5: notify IB uplink on LAG aggregate speed change Tariq Toukan
@ 2026-10-22 19:30 ` Tariq Toukan
2026-10-26 20:32 ` netdev-bot+sashiko
2026-10-22 19:37 ` [PATCH net V2 0/4] net/mlx5: fixes for LAG max_tx_speed update flow netdev-bot+sinfo
4 siblings, 1 reply; 12+ messages in thread
From: Tariq Toukan @ 2026-10-22 19:30 UTC (permalink / raw)
To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
netdev, Paolo Abeni
Cc: Bjorn Helgaas, Carolina Jubran, Cosmin Ratiu, Edward Srouji,
Gal Pressman, Jason Gunthorpe, Kees Cook, Leon Romanovsky,
open list, linux-pci, linux-rdma, Maher Sanalla, Mark Bloch,
Or Har-Toov, Saeed Mahameed, Shay Drori, Tariq Toukan
From: Or Har-Toov <ohartoov@nvidia.com>
mlx5_lag_reset_vports_speed() queries the oper link speed at LAG
teardown and writes it back to each vport. When the link is down at
that moment, mlx5_port_oper_linkspeed() returns 0, which FW treats as
"not provided" and ignores, leaving the aggregate rate limit on the
vports after teardown.
Save each vport's max_tx_speed when the vport is enabled, before the
LAG aggregate or a deferred aggregate replay can overwrite it. While a
LAG is active, only a vport with a pending aggregate is saved, as FW
may already hold the aggregate for any other vport. Restore the saved
value per vport at teardown instead of querying the oper speed, and
update agg_max_tx_speed to the pre-LAG value so disabled vports replay
the correct rate when they are later enabled. A vport keeps its
saved value after teardown, as it is the baseline for the next LAG, and
for a disabled vport it is also the pending restore value.
If the max_tx_speed of a vport could not be saved, it stays zero and
mlx5_lag_get_restore_speed() falls back to a link-derived speed at
teardown. The aggregate is still applied to such a vport.
This also covers the case where the oper speed query fails and the PF
is skipped entirely, leaving the aggregate in FW and in
agg_max_tx_speed, as raised in the AI review of v1 of "net/mlx5: Lag,
reset vport speed on teardown". It is unrelated to that patch, so it is
fixed here separately.
Firmware never returns zero as a configured max_tx_speed; zero means
"not supported", so using it as a sentinel for "not yet saved" is safe.
A vport disabled for the entire LAG period was never assigned a
per-vport rate limit, so no original speed exists to restore; deferring
through agg_max_tx_speed on re-enable is correct.
Fixes: 50f1d188c580 ("net/mlx5: Propagate LAG effective max_tx_speed to vports")
Link: https://lore.kernel.org/all/20260915015118.875210-1-kuba@kernel.org/
Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
.../net/ethernet/mellanox/mlx5/core/eswitch.c | 22 ++++++
.../net/ethernet/mellanox/mlx5/core/eswitch.h | 1 +
.../net/ethernet/mellanox/mlx5/core/lag/lag.c | 68 ++++++++++++++++---
3 files changed, 81 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
index fc197d1dc9df..7cf9c15ec65d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
@@ -935,6 +935,27 @@ static void esw_vport_cleanup(struct mlx5_eswitch *esw, struct mlx5_vport *vport
esw_vport_cleanup_acl(esw, vport);
}
+static void mlx5_esw_vport_save_max_tx_speed(struct mlx5_eswitch *esw,
+ struct mlx5_vport *vport)
+{
+ u16 op_mod = MLX5_VPORT_STATE_OP_MOD_ESW_VPORT;
+ int ret;
+
+ if (!MLX5_CAP_ESW(esw->dev, esw_vport_state_max_tx_speed) ||
+ vport->vport == MLX5_VPORT_UPLINK || vport->max_tx_speed)
+ return;
+
+ if (mlx5_lag_is_active(esw->dev) && !vport->agg_max_tx_speed)
+ return;
+
+ ret = mlx5_query_vport_max_tx_speed(esw->dev, op_mod, vport->vport,
+ true, &vport->max_tx_speed, NULL);
+ if (ret)
+ mlx5_core_dbg(esw->dev,
+ "Failed to save vport %d max tx speed, err=%d\n",
+ vport->vport, ret);
+}
+
static void mlx5_esw_vport_set_max_tx_speed(struct mlx5_eswitch *esw,
struct mlx5_vport *vport)
{
@@ -999,6 +1020,7 @@ int mlx5_esw_vport_enable(struct mlx5_eswitch *esw, struct mlx5_vport *vport,
esw->enabled_vports++;
esw_debug(esw->dev, "Enabled VPORT(%d)\n", vport_num);
+ mlx5_esw_vport_save_max_tx_speed(esw, vport);
if (vport->agg_max_tx_speed)
mlx5_esw_vport_set_max_tx_speed(esw, vport);
done:
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index 8b1f93b13ea9..3717fb6de0b6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -261,6 +261,7 @@ struct mlx5_vport {
int index;
struct mlx5_devlink_port *dl_port;
u32 agg_max_tx_speed;
+ u32 max_tx_speed;
};
struct mlx5_esw_indir_table;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
index afe0b9909f2d..3e623c96b2a1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
@@ -1712,11 +1712,37 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev)
}
}
+static int mlx5_lag_get_restore_speed(struct mlx5_core_dev *mdev,
+ struct mlx5_vport *vport, u32 *speed)
+{
+ u32 pci_bw;
+
+ if (vport->max_tx_speed) {
+ *speed = vport->max_tx_speed;
+ return 0;
+ }
+
+ *speed = 0;
+ mlx5_port_oper_linkspeed(mdev, speed);
+ if (!*speed && mlx5_port_max_linkspeed(mdev, speed))
+ return -ENODATA;
+
+ pci_bw = pcie_bandwidth_mbps(mdev->pdev);
+ if (pci_bw)
+ *speed = min(*speed, pci_bw);
+ *speed /= MLX5_MAX_TX_SPEED_UNIT;
+ return 0;
+}
+
void mlx5_lag_reset_vports_speed(struct mlx5_lag *ldev)
{
+ u16 op_mod = MLX5_VPORT_STATE_OP_MOD_ESW_VPORT;
struct mlx5_core_dev *mdev;
+ struct mlx5_eswitch *esw;
+ struct mlx5_vport *vport;
struct lag_func *pf;
- u32 speed;
+ u32 restore_speed;
+ unsigned long i;
int pf_idx;
int ret;
@@ -1728,17 +1754,39 @@ void mlx5_lag_reset_vports_speed(struct mlx5_lag *ldev)
mdev = pf->dev;
if (!mdev)
continue;
-
- ret = mlx5_port_oper_linkspeed(mdev, &speed);
- if (ret) {
- mlx5_core_dbg(mdev,
- "Failed to reset vports speed for device %s. Oper speed is not available (err=%d)\n",
- dev_name(mdev->device), ret);
+ esw = mdev->priv.eswitch;
+ if (!esw || !MLX5_CAP_ESW(mdev, esw_vport_state_max_tx_speed))
continue;
- }
- speed = speed / MLX5_MAX_TX_SPEED_UNIT;
- mlx5_lag_modify_device_vports_speed(mdev, speed);
+ mutex_lock(&esw->state_lock);
+ mlx5_esw_for_each_vport(esw, i, vport) {
+ if (!vport || vport->vport == MLX5_VPORT_UPLINK)
+ continue;
+
+ if (!vport->enabled) {
+ vport->agg_max_tx_speed = vport->max_tx_speed;
+ continue;
+ }
+
+ if (mlx5_lag_get_restore_speed(mdev, vport,
+ &restore_speed)) {
+ vport->agg_max_tx_speed = 0;
+ continue;
+ }
+
+ ret = mlx5_modify_vport_max_tx_speed(mdev, op_mod,
+ vport->vport,
+ true,
+ restore_speed);
+ if (ret) {
+ mlx5_core_dbg(mdev,
+ "Failed to restore vport %d max tx speed, err=%d\n",
+ vport->vport, ret);
+ continue;
+ }
+ vport->agg_max_tx_speed = 0;
+ }
+ mutex_unlock(&esw->state_lock);
}
}
#endif
--
2.44.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH net V2 4/4] net/mlx5: Lag, save and restore vport max_tx_speed across LAG
2026-10-22 19:30 ` [PATCH net V2 4/4] net/mlx5: Lag, save and restore vport max_tx_speed across LAG Tariq Toukan
@ 2026-10-26 20:32 ` netdev-bot+sashiko
0 siblings, 0 replies; 12+ messages in thread
From: netdev-bot+sashiko @ 2026-10-26 20:32 UTC (permalink / raw)
To: tariqt
Cc: andrew+netdev, davem, edumazet, kuba, netdev, pabeni, bhelgaas,
cjubran, cratiu, edwards, gal, jgg, kees, leon, linux-kernel,
linux-pci, linux-rdma, msanalla, mbloch, ohartoov, saeedm, shayd
Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 5 · 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).
- [High] mlx5_esw_vport_save_max_tx_speed() (eswitch.c:948) treats a non-
zero agg_max_tx_speed during an active LAG as meaning the aggregate is…
- [Medium] The new restore logic only runs on one LAG teardown path.
- [Medium] vport->max_tx_speed is read from FW only once per mlx5_vport
object, because of the `|| vport->max_tx_speed) return;` early return…
- [Medium] In mlx5_lag_reset_vports_speed(), when
mlx5_modify_vport_max_tx_speed() fails for an enabled vport, the loop…
- [Medium] mlx5_lag_reset_vports_speed() records agg_max_tx_speed = 0 in
two cases where FW still holds the LAG aggregate.
- [Medium] The save gate in mlx5_esw_vport_save_max_tx_speed() uses
mlx5_lag_is_active(), which reads ldev->mode under lag_lock.
- [Low] The fallback in mlx5_lag_get_restore_speed() clamps with the raw
per-device pcie_bandwidth_mbps(mdev->pdev).
--
net/mlx5: Lag, save and restore vport max_tx_speed across LAG
This commit reads each vport's max_tx_speed from FW when the vport is
enabled. At LAG teardown it writes that saved value back per vport,
instead of writing the oper link speed. If nothing was saved, it falls
back to a link-derived speed clamped by PCIe bandwidth, and disabled
vports get their saved value as agg_max_tx_speed for the next enable.
Link: https://lore.kernel.org/all/20260915015118.875210-1-kuba@kernel.org/
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> index fc197d1dc9dfe..7cf9c15ec65d2 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> @@ -935,6 +935,27 @@ static void esw_vport_cleanup(struct mlx5_eswitch *esw, struct mlx5_vport *vport
> esw_vport_cleanup_acl(esw, vport);
> }
>
> +static void mlx5_esw_vport_save_max_tx_speed(struct mlx5_eswitch *esw,
> + struct mlx5_vport *vport)
> +{
> + u16 op_mod = MLX5_VPORT_STATE_OP_MOD_ESW_VPORT;
> + int ret;
> +
> + if (!MLX5_CAP_ESW(esw->dev, esw_vport_state_max_tx_speed) ||
> + vport->vport == MLX5_VPORT_UPLINK || vport->max_tx_speed)
> + return;
[Severity: Medium]
Once vport->max_tx_speed is non-zero, this returns early. Nothing in the
patch ever sets it back to 0. Can the saved value go stale?
The mlx5_vport objects are allocated once, in mlx5_esw_vports_init()
from mlx5_init_once(). They survive devlink reload, fw_activate, sync
reset and health recovery, because mlx5_load_one_devl_locked() only
runs mlx5_function_setup() and mlx5_load().
After such a reset, FW may recompute its per-vport default (the lower
of port speed and PCIe bandwidth). If the PCIe link or port speed has
changed, mlx5_lag_get_restore_speed() still uses the old saved value at
the next teardown.
For disabled vports, the old value is copied into agg_max_tx_speed.
Since that field is never cleared, mlx5_esw_vport_enable() writes it on
every later enable, even when no LAG is configured.
Should max_tx_speed be reset on reload/reset, or read again from FW
when no LAG is active?
> +
> + if (mlx5_lag_is_active(esw->dev) && !vport->agg_max_tx_speed)
> + return;
[Severity: High]
This check reads a non-zero agg_max_tx_speed during an active LAG as
"the aggregate is pending and FW still holds the original". Is that
always the case?
mlx5_lag_modify_device_vports_speed() sets agg_max_tx_speed for every
vport. For enabled vports it also writes the aggregate to FW right away:
vport->agg_max_tx_speed = speed;
if (!vport->enabled)
continue;
ret = mlx5_modify_vport_max_tx_speed(mdev, op_mod, vport->vport, true, speed);
Nothing clears agg_max_tx_speed once it has been applied.
mlx5_esw_vport_disable() doesn't touch agg_max_tx_speed or
max_tx_speed either.
So take a vport with max_tx_speed == 0 while FW already holds the
aggregate. That state is reachable in two ways:
- An earlier save query failed (only logged at debug level).
- The vport was enabled while mlx5_lag_is_active() was true and
agg_max_tx_speed was 0, so the save was skipped. One example is an
SD-only shared FDB: __mlx5_lag_is_sd_active() makes
mlx5_lag_is_active() return true, but no aggregate is applied. A
later aggregate, for example from MPESW enable, then lands on the
enabled vport.
If that vport is disabled and re-enabled while the LAG is still active,
this check passes. The query then stores the FW aggregate in
vport->max_tx_speed.
At teardown, mlx5_lag_get_restore_speed() returns that value and the
aggregate is written back. Because max_tx_speed is never reset, every
later LAG cycle restores the same wrong baseline.
That seems to contradict the commit message:
If the max_tx_speed of a vport could not be saved, it stays zero and
mlx5_lag_get_restore_speed() falls back to a link-derived speed at
teardown.
Is a separate marker needed here, one that can tell "aggregate pending"
apart from "aggregate applied"?
[Severity: Medium]
Does mlx5_lag_is_active() stay consistent with agg_max_tx_speed here?
mlx5_lag_is_active() reads ldev->mode under lag_lock. ldev->mode is
written under ldev->lock only, in mlx5_activate_lag() and
mlx5_deactivate_lag(). agg_max_tx_speed and the FW aggregate are
written in a separate esw->state_lock section.
On activation, mlx5_do_bond()->mlx5_lag_shared_fdb_create() sets the
mode and drops ldev->lock for rep rescans and reloads. Only after that
does it call mlx5_lag_set_vports_agg_speed().
A vport can be enabled in that window. One way is mlx5_eswitch_enable()
when the FDB already exists: toggle_lag is false, so
mlx5_lag_disable_change() doesn't serialize it. SF/ECPF function
changes are another way. Such a vport sees an active LAG with
agg_max_tx_speed == 0. It skips the save even though FW still holds the
original, and then receives the aggregate.
On teardown, mlx5_do_bond() does:
mlx5_lag_reset_vports_speed(ldev);
mlx5_disable_lag(ldev);
The mode is still set after mlx5_lag_reset_vports_speed() drops
state_lock. A vport enabled in that window also skips the save.
Either way, the vport ends up with max_tx_speed == 0 while FW holds the
aggregate. Isn't that the precondition for the saved-baseline problem
described above?
> +
> + ret = mlx5_query_vport_max_tx_speed(esw->dev, op_mod, vport->vport,
> + true, &vport->max_tx_speed, NULL);
> + if (ret)
> + mlx5_core_dbg(esw->dev,
> + "Failed to save vport %d max tx speed, err=%d\n",
> + vport->vport, ret);
> +}
[ ... ]
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index afe0b9909f2dc..3e623c96b2a1d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> @@ -1712,11 +1712,37 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev)
> }
> }
>
> +static int mlx5_lag_get_restore_speed(struct mlx5_core_dev *mdev,
> + struct mlx5_vport *vport, u32 *speed)
> +{
> + u32 pci_bw;
> +
> + if (vport->max_tx_speed) {
> + *speed = vport->max_tx_speed;
> + return 0;
> + }
> +
> + *speed = 0;
> + mlx5_port_oper_linkspeed(mdev, speed);
> + if (!*speed && mlx5_port_max_linkspeed(mdev, speed))
> + return -ENODATA;
> +
> + pci_bw = pcie_bandwidth_mbps(mdev->pdev);
> + if (pci_bw)
> + *speed = min(*speed, pci_bw);
[Severity: Low]
This might not be an issue, but the fallback clamps with the per-device
pcie_bandwidth_mbps(mdev->pdev). The aggregate path uses
mlx5_lag_port_pcie_bw() instead, which sums bandwidth across
Socket-Direct group members and treats RC_END as unlimited.
The restore value is per vport, and the FW default is described as
coming from the endpoint's own link status. So the per-device clamp may
be intended. For RC_END, pcie_bandwidth_mbps() returns 0 and the
if (pci_bw) check skips the clamp.
Could a short comment explain why the per-device value is used here
rather than the SD-aware helper?
> + *speed /= MLX5_MAX_TX_SPEED_UNIT;
> + return 0;
> +}
[ ... ]
> @@ -1728,17 +1754,39 @@ void mlx5_lag_reset_vports_speed(struct mlx5_lag *ldev)
> mdev = pf->dev;
> if (!mdev)
> continue;
> -
> - ret = mlx5_port_oper_linkspeed(mdev, &speed);
> - if (ret) {
> - mlx5_core_dbg(mdev,
> - "Failed to reset vports speed for device %s. Oper speed is not available (err=%d)\n",
> - dev_name(mdev->device), ret);
> + esw = mdev->priv.eswitch;
> + if (!esw || !MLX5_CAP_ESW(mdev, esw_vport_state_max_tx_speed))
> continue;
> - }
>
> - speed = speed / MLX5_MAX_TX_SPEED_UNIT;
> - mlx5_lag_modify_device_vports_speed(mdev, speed);
> + mutex_lock(&esw->state_lock);
> + mlx5_esw_for_each_vport(esw, i, vport) {
[Severity: Medium]
mlx5_lag_reset_vports_speed() is only called from the
mlx5_lag_should_disable_lag() branch of mlx5_do_bond(). That predicate
excludes MPESW:
return !do_bond && __mlx5_lag_is_active(ldev) &&
ldev->mode != MLX5_LAG_MODE_MPESW;
Should the other teardown paths restore vport speeds as well?
mlx5_lag_disable_change() goes straight to mlx5_lag_disable_mpesw() or
mlx5_disable_lag(). It is called from mlx5_eswitch_enable(),
mlx5_eswitch_disable() and the devlink eswitch mode change.
Explicit MPESW disable goes through
mlx5_mpesw_work()->mlx5_lag_disable_mpesw()->mlx5_lag_shared_fdb_destroy(),
and doesn't restore either.
The aggregate is still applied when these LAGs come up, through
mlx5_lag_shared_fdb_create()->mlx5_lag_set_vports_agg_speed() and
mlx5_mpesw_speed_update_work().
After those teardowns, FW and agg_max_tx_speed both still hold the
aggregate. mlx5_esw_vport_enable() writes it again on every later
enable.
With the new save step, a vport whose max_tx_speed is still 0 then
reads the leftover aggregate from FW and keeps it as its saved
original. Every later mlx5_do_bond() teardown then restores the
aggregate.
The missing restore calls on these paths were already there before
this patch. But the commit message says the saved value is restored
"at teardown", and the interaction with the new save step is new.
> + if (!vport || vport->vport == MLX5_VPORT_UPLINK)
> + continue;
> +
> + if (!vport->enabled) {
> + vport->agg_max_tx_speed = vport->max_tx_speed;
> + continue;
> + }
> +
> + if (mlx5_lag_get_restore_speed(mdev, vport,
> + &restore_speed)) {
> + vport->agg_max_tx_speed = 0;
> + continue;
> + }
[Severity: Medium]
Both of these branches set agg_max_tx_speed to 0 without writing FW.
Could FW still hold the aggregate in either case?
For a disabled vport with max_tx_speed == 0, the pending restore is
dropped. That vport may have been enabled partway through the LAG
without a saved value (the mlx5_lag_is_active() check skipped the save,
or the query failed). It then received the aggregate and was disabled
before teardown.
The patch already assumes FW keeps max_tx_speed across a vport disable.
That is why disabled vports get a replay value.
For an enabled vport, mlx5_lag_get_restore_speed() returns -ENODATA
when nothing was saved and the PTYS query fails.
In both cases, the next mlx5_esw_vport_enable() after the LAG is gone
sees max_tx_speed == 0 and an inactive LAG. It queries FW and saves the
leftover aggregate as the original.
Before this patch, disabled vports got agg_max_tx_speed set to the oper
speed, so re-enabling them corrected FW.
The commit message covers "a vport disabled for the entire LAG period".
Does the same reasoning hold for a vport that was disabled partway
through the LAG?
> +
> + ret = mlx5_modify_vport_max_tx_speed(mdev, op_mod,
> + vport->vport,
> + true,
> + restore_speed);
> + if (ret) {
> + mlx5_core_dbg(mdev,
> + "Failed to restore vport %d max tx speed, err=%d\n",
> + vport->vport, ret);
> + continue;
> + }
[Severity: Medium]
If mlx5_modify_vport_max_tx_speed() fails here, agg_max_tx_speed keeps
the LAG aggregate. Is that intended?
Before, mlx5_lag_modify_device_vports_speed() set agg_max_tx_speed to
the teardown speed before trying the FW write. A later enable would
then retry with a non-LAG value.
Now, on a later enable with no LAG active, mlx5_esw_vport_enable()
runs:
if (vport->agg_max_tx_speed)
mlx5_esw_vport_set_max_tx_speed(esw, vport);
which writes the stale aggregate to FW again. If max_tx_speed is still
0, the save step runs first and records the aggregate still in FW as
the baseline.
That looks like the state the commit message says is fixed:
leaving the aggregate in FW and in agg_max_tx_speed
Would setting vport->agg_max_tx_speed = restore_speed in this failure
branch let the next enable retry the restore?
> + vport->agg_max_tx_speed = 0;
> + }
> + mutex_unlock(&esw->state_lock);
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022193053.3815866-1-tariqt%40nvidia.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net V2 0/4] net/mlx5: fixes for LAG max_tx_speed update flow
2026-10-22 19:30 [PATCH net V2 0/4] net/mlx5: fixes for LAG max_tx_speed update flow Tariq Toukan
` (3 preceding siblings ...)
2026-10-22 19:30 ` [PATCH net V2 4/4] net/mlx5: Lag, save and restore vport max_tx_speed across LAG Tariq Toukan
@ 2026-10-22 19:37 ` netdev-bot+sinfo
2026-10-25 14:59 ` Or Har-Toov
4 siblings, 1 reply; 12+ messages in thread
From: netdev-bot+sinfo @ 2026-10-22 19:37 UTC (permalink / raw)
To: Tariq Toukan
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
netdev, Paolo Abeni, Bjorn Helgaas, Carolina Jubran, Cosmin Ratiu,
Edward Srouji, Gal Pressman, Jason Gunthorpe, Kees Cook,
Leon Romanovsky, open list, linux-pci, linux-rdma, Maher Sanalla,
Mark Bloch, Or Har-Toov, Saeed Mahameed, Shay Drori
Hi!
This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:
- How the issue was discovered, e.g. hit in production, hit during
development, syzbot report, manual code inspection, LLM or static
analysis tool scan.
- Whether the issue was actually triggered, or is only theoretical
(e.g. found by code inspection). If it was triggered please include
the symptoms, like the stack trace or error messages.
- What hardware the change was tested on. For driver fixes please
mention the device (and if relevant firmware version) used for
testing, or say that the change was not tested on real hardware.
Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.
The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net V2 0/4] net/mlx5: fixes for LAG max_tx_speed update flow
2026-10-22 19:37 ` [PATCH net V2 0/4] net/mlx5: fixes for LAG max_tx_speed update flow netdev-bot+sinfo
@ 2026-10-25 14:59 ` Or Har-Toov
0 siblings, 0 replies; 12+ messages in thread
From: Or Har-Toov @ 2026-10-25 14:59 UTC (permalink / raw)
To: netdev-bot+sinfo, Tariq Toukan
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
netdev, Paolo Abeni, Bjorn Helgaas, Carolina Jubran, Cosmin Ratiu,
Edward Srouji, Gal Pressman, Jason Gunthorpe, Kees Cook,
Leon Romanovsky, open list, linux-pci, linux-rdma, Maher Sanalla,
Mark Bloch, Saeed Mahameed, Shay Drori
On 22/10/2026 22:37, netdev-bot+sinfo@kernel.org wrote:
>
>
> Hi!
>
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
>
> - How the issue was discovered, e.g. hit in production, hit during
> development, syzbot report, manual code inspection, LLM or static
> analysis tool scan.
>
> - Whether the issue was actually triggered, or is only theoretical
> (e.g. found by code inspection). If it was triggered please include
> the symptoms, like the stack trace or error messages.
>
> - What hardware the change was tested on. For driver fixes please
> mention the device (and if relevant firmware version) used for
> testing, or say that the change was not tested on real hardware.
>
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
>
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.
Hi,
Bugs 1 and 2 were found during internal integration and testing of the
flow added by 50f1d188c580 ("net/mlx5: Propagate LAG effective
max_tx_speed to vports").
Bug 3 was found by sashiko.
All three were reproduced on CX8.
Or
^ permalink raw reply [flat|nested] 12+ messages in thread