[PATCH net-next v3] net/mlx5e: advertise tcp-data-split support

[PATCH net-next v3] net/mlx5e: advertise tcp-data-split support

[Dimitri Daskalakis]

From: Dimitri Daskalakis <daskald@meta.com>

The ability to set tcp-data-split was introduced in
commit 46bcce5dfd33 ("net/mlx5e: Support ethtool tcp-data-split settings")
This commit dropped reporting from mlx5e_ethtool_get_ringparam()
without explaining why. commit d347b28c492e ("net/mlx5e: Add hds-thresh
query support via ethtool") added support for ETHTOOL_RING_USE_HDS_THRS,
and modified mlx5e_ethtool_get_ringparam() to report an hds_thresh of 0.

Between these two commits, the hds selftest helper _defer_reset_hds()
would skip the reset because neither hds-thresh or tcp-data-split was
present in the ring config. After the second commit this introduced
a KeyError in the hds.set_hds_enable/disable tests.

mlx5 couples tcp-data-split with rx-gro-hw. Users can currently disable
tcp-data-split then enable HW GRO. The kernel reports tcp-data-split
off, but the HW is presumably splitting.

Add back tcp-data-split reporting in mlx5e_ethtool_get_ringparam(),
and de-feature tcp-data-split disable. Users can either enable it
(if HW GRO is enabled), or leave it under driver control.

Additionally, move the call to netdev_update_features() out of
mlx5e_ethtool_set_tcp_data_split(). If mlx5e_ethtool_set_ringparam()
fails, this can cause the driver/kernel feature to de-sync.

This is not a fix since HDS/tcp-data-split is still functional with HW
GRO enabled.

Signed-off-by: Dimitri Daskalakis <daskald@meta.com>
---
Changes in v3:
- Drop redundant hw_features check in mlx5e_ethtool_get_ringparam(). Packet
  merge type can only be MLX5E_PACKET_MERGE_SHAMPO when HW GRO is active.
- Address Bjorn's feedback, and drop premature netdev_update_features() call
  in mlx5e_ethtool_set_tcp_data_split()
- Link to v2: https://lore.kernel.org/all/20261006225925.568263-1-dimitri.daskalakis1@gmail.com/
Changes in v2:
- Leave kernel_param->tcp_data_split as ETHTOOL_TCP_DATA_SPLIT_UNKNOWN if the
  device does not support HW GRO
- Prevent users from disabling tcp-data-split
- Link to v1: https://lore.kernel.org/all/20260923230521.1267511-1-dimitri.daskalakis1@gmail.com/

hds.py before:
 # Interface: eth0, driver: mlx5_core
 TAP version 13
 1..13
 ok 1 hds.get_hds # SKIP tcp-data-split not supported by device
 ok 2 hds.get_hds_thresh
 # Exception while handling defer / cleanup (callback 1 of 1)! ...
 # Defer Exception| KeyError: 'tcp-data-split'
 # Defer Exception|
 not ok 3 hds.set_hds_disable
 # Exception while handling defer / cleanup (callback 1 of 1)!
 ...
 # Defer Exception| KeyError: 'tcp-data-split'
 # Defer Exception|
 not ok 4 hds.set_hds_enable
 ok 5 hds.set_hds_thresh_random # SKIP hds-thresh-max is too small
 ok 6 hds.set_hds_thresh_zero
 ok 7 hds.set_hds_thresh_max
 ok 8 hds.set_hds_thresh_gt
 ok 9 hds.set_xdp # SKIP tcp-data-split not supported by device
 ok 10 hds.enabled_set_xdp # SKIP tcp-data-split not supported by device
 ok 11 hds.ioctl # SKIP tcp-data-split not supported by device
 ok 12 hds.ioctl_set_xdp # SKIP tcp-data-split not supported by device
 ok 13 hds.ioctl_enabled_set_xdp # SKIP tcp-data-split not supported by device

hds.py after:
 # Interface: eth0, driver: mlx5_core
 TAP version 13
 1..13
 ok 1 hds.get_hds # SKIP tcp-data-split not supported by device
 ok 2 hds.get_hds_thresh
 ok 3 hds.set_hds_disable # SKIP disabling of HDS not supported by the device
 ok 4 hds.set_hds_enable # SKIP disabling of HDS not supported by the device
 ok 5 hds.set_hds_thresh_random # SKIP hds-thresh-max is too small
 ok 6 hds.set_hds_thresh_zero
 ok 7 hds.set_hds_thresh_max
 ok 8 hds.set_hds_thresh_gt
 ok 9 hds.set_xdp # SKIP tcp-data-split not supported by device
 ok 10 hds.enabled_set_xdp # SKIP tcp-data-split not supported by device
 ok 11 hds.ioctl # SKIP tcp-data-split not supported by device
 ok 12 hds.ioctl_set_xdp # SKIP tcp-data-split not supported by device
 ok 13 hds.ioctl_enabled_set_xdp # SKIP tcp-data-split not supported by device
 # Totals: pass:4 fail:0 xfail:0 xpass:0 skip:9 error:0
---
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  | 23 +++++++++++++++----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index 261c466a4d36..c30d751cac2f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -378,6 +378,9 @@ void mlx5e_ethtool_get_ringparam(struct mlx5e_priv *priv,
 
 	kernel_param->hds_thresh = 0;
 	kernel_param->hds_thresh_max = 0;
+
+	if (priv->channels.params.packet_merge.type == MLX5E_PACKET_MERGE_SHAMPO)
+		kernel_param->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_ENABLED;
 }
 
 static void mlx5e_get_ringparam(struct net_device *dev,
@@ -403,10 +406,11 @@ static bool mlx5e_ethtool_set_tcp_data_split(struct mlx5e_priv *priv,
 		return false;
 	}
 
-	/* Might need to disable HW-GRO if it was kept on due to hds. */
-	if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_DISABLED &&
-	    dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED)
-		netdev_update_features(priv->netdev);
+	if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_DISABLED) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "TCP-data-split can not be disabled");
+		return false;
+	}
 
 	return true;
 }
@@ -468,13 +472,22 @@ static int mlx5e_set_ringparam(struct net_device *dev,
 			       struct netlink_ext_ack *extack)
 {
 	struct mlx5e_priv *priv = netdev_priv(dev);
+	int err;
 
 	if (!mlx5e_ethtool_set_tcp_data_split(priv,
 					      kernel_param->tcp_data_split,
 					      extack))
 		return -EINVAL;
 
-	return mlx5e_ethtool_set_ringparam(priv, param, extack);
+	err = mlx5e_ethtool_set_ringparam(priv, param, extack);
+	if (err)
+		return err;
+
+	/* Disable HW-GRO if it was only kept on for HDS. */
+	if (dev->cfg->hds_config != kernel_param->tcp_data_split)
+		netdev_update_features(priv->netdev);
+
+	return 0;
 }
 
 void mlx5e_ethtool_get_channels(struct mlx5e_priv *priv,
-- 
2.52.0

Re: [PATCH net-next v3] net/mlx5e: advertise tcp-data-split support

[Björn Töpel]

Dimitri Daskalakis <dimitri.daskalakis1@gmail.com> writes:

> From: Dimitri Daskalakis <daskald@meta.com>
>
> The ability to set tcp-data-split was introduced in
> commit 46bcce5dfd33 ("net/mlx5e: Support ethtool tcp-data-split settings")
> This commit dropped reporting from mlx5e_ethtool_get_ringparam()
> without explaining why. commit d347b28c492e ("net/mlx5e: Add hds-thresh
> query support via ethtool") added support for ETHTOOL_RING_USE_HDS_THRS,
> and modified mlx5e_ethtool_get_ringparam() to report an hds_thresh of 0.
>
> Between these two commits, the hds selftest helper _defer_reset_hds()
> would skip the reset because neither hds-thresh or tcp-data-split was
> present in the ring config. After the second commit this introduced
> a KeyError in the hds.set_hds_enable/disable tests.
>
> mlx5 couples tcp-data-split with rx-gro-hw. Users can currently disable
> tcp-data-split then enable HW GRO. The kernel reports tcp-data-split
> off, but the HW is presumably splitting.
>
> Add back tcp-data-split reporting in mlx5e_ethtool_get_ringparam(),
> and de-feature tcp-data-split disable. Users can either enable it
> (if HW GRO is enabled), or leave it under driver control.
>
> Additionally, move the call to netdev_update_features() out of
> mlx5e_ethtool_set_tcp_data_split(). If mlx5e_ethtool_set_ringparam()
> fails, this can cause the driver/kernel feature to de-sync.
>
> This is not a fix since HDS/tcp-data-split is still functional with HW
> GRO enabled.

Hmm, but with this move it is a fix, no? (We can probably get in similar
inconsist state as the one you describe with "ethtool blabla rx 1"?)

Maybe:
Fixes: 46bcce5dfd33 ("net/mlx5e: Support ethtool tcp-data-split
settings")

Regardless;
Reviewed-by: Björn Töpel <bjorn@kernel.org>

Re: [PATCH net-next v3] net/mlx5e: advertise tcp-data-split support

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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] The patch is titled "advertise tcp-data-split support" and
  says it will "Add back tcp-data-split reporting", but…
- [Medium] On an mlx5 device with rx-gro-hw on,
  mlx5e_ethtool_get_ringparam() now reports tcp-data-split='enabled'…
- [Low] The commit message says "This is not a fix" and carries no Fixes:
  tag, but the patch fixes real defects that the message itself…
- [Low] With rx-gro-hw on and dev->cfg->hds_config == UNKNOWN,
  mlx5e_ethtool_get_ringparam() now reports tcp-data-split=enabled from…

--

net/mlx5e: advertise tcp-data-split support

This commit brings back tcp-data-split reporting in
mlx5e_ethtool_get_ringparam() when SHAMPO is active. It also rejects
requests to disable tcp-data-split. Finally, it moves
netdev_update_features() so that it only runs after
mlx5e_ethtool_set_ringparam() succeeds.

> This is not a fix since HDS/tcp-data-split is still functional with HW
> GRO enabled.

[Severity: Low]
Is this accurate? The patch seems to fix problems that the commit message
itself describes:

  - the tcp-data-split reporting that 46bcce5dfd33 dropped from ethtool -g
  - the KeyError in hds.set_hds_enable/disable since d347b28c492e
  - the feature/config de-sync caused by calling netdev_update_features()
    before mlx5e_ethtool_set_ringparam()

For the last one: in the baseline, mlx5e_ethtool_set_tcp_data_split()
called netdev_update_features() before the ring sizes were validated.
mlx5e_fix_features() looks at netdev->cfg_pending:

drivers/net/ethernet/mellanox/mlx5/core/en_main.c:mlx5e_fix_features() {
    ...
	if (cfg && cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
	    !(features & NETIF_F_GRO_HW)) {
    ...
}

Take a request like "ethtool -G ethX rx 1 tcp-data-split off" while
hds_config is ENABLED. HW-GRO/SHAMPO could be turned off first. The
rx_pending minimum check then fails, and the ethtool core discards
cfg_pending. dev->cfg->hds_config stays ENABLED, but the HW is no longer
splitting.

There is no Fixes: tag, so stable tooling won't pick these fixes up for
kernels that contain 46bcce5dfd33 and d347b28c492e. Could the reporting
restore and the netdev_update_features() reordering be split into their
own patches with Fixes: tags? The uAPI behavior change that rejects
tcp-data-split off could then stay separate.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> index 261c466a4d362..c30d751cac2fa 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> @@ -378,6 +378,9 @@ void mlx5e_ethtool_get_ringparam(struct mlx5e_priv *priv,
>  
>  	kernel_param->hds_thresh = 0;
>  	kernel_param->hds_thresh_max = 0;
> +
> +	if (priv->channels.params.packet_merge.type == MLX5E_PACKET_MERGE_SHAMPO)
> +		kernel_param->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_ENABLED;
>  }

[Severity: Medium]
The subject says "advertise tcp-data-split support" and the message says
"Add back tcp-data-split reporting". However, this only ever writes
ENABLED, and only when SHAMPO is active. The code removed by 46bcce5dfd33
also reported DISABLED when SHAMPO was not active.

HW-GRO is off by default on mlx5, since mlx5e_build_nic_netdev() clears
NETIF_F_GRO_HW. In that case:

  - rings_prepare_data() pre-fills tcp_data_split from
    dev->cfg->hds_config (UNKNOWN)
  - the driver leaves it untouched
  - rings_fill_reply() leaves out the attribute:

net/ethtool/rings.c:rings_fill_reply() {
    ...
	    (kr->tcp_data_split &&
	     (nla_put_u8(skb, ETHTOOL_A_RINGS_TCP_DATA_SPLIT,
    ...
}

So user space sees tcp-data-split as unsupported, even though the device
advertises ETHTOOL_RING_USE_TCP_DATA_SPLIT and is in fact not splitting.
The post-patch log in the changelog shows this:

  ok 1 hds.get_hds # SKIP tcp-data-split not supported by device

mlx5e_ethtool_set_tcp_data_split() now also rejects
ETHTOOL_TCP_DATA_SPLIT_DISABLED unconditionally. On a default-configured
device, the off state can then be neither reported nor requested.

Would it be safe to report ETHTOOL_TCP_DATA_SPLIT_DISABLED when SHAMPO is
not active? Both ethtool_ringparam_get_cfg() and the netlink set path
replace the driver-reported value with dev->cfg->hds_config.

[Severity: Low]
With rx-gro-hw on and dev->cfg->hds_config == UNKNOWN, this now reports
enabled. Can hds.set_hds_enable then leave hds_config stuck at ENABLED
after the test finishes?

_defer_reset_hds() saves tcp-data-split as 'enabled' (from SHAMPO state)
set_hds_enable() writes tcp-data-split 'enabled'
  ethnl_set_rings()
    dev->cfg_pending->hds_config = kernel_ringparam.tcp_data_split;
_hds_reset() at cleanup:
    if cur.get('tcp-data-split') != rings.get('tcp-data-split'):

Both values are 'enabled', so 'unknown' is never written back.

After that, mlx5e_fix_features() keeps NETIF_F_GRO_HW forced on, and
"ethtool -K rx-gro-hw off" no longer takes effect. dev_xdp_attach() also
rejects single-buffer XDP:

	if (dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
	    prog && !prog->aux->xdp_has_frags) {

In a full hds.py run, the later set_xdp tests write 'unknown' and hide
this. It shows up when set_hds_enable runs alone, or as the last test that
touches HDS.

The root cause is that the test compares reported state rather than
config, and netdevsim has the same pattern. This patch is what makes it
reachable on mlx5, though. Should _hds_reset() or the driver reporting
be adjusted?

[Severity: Medium]
On mlx5 with rx-gro-hw on, do hds.set_xdp and hds.ioctl_set_xdp now go
from SKIP to FAIL?

Both tests see mode 'enabled', write 'unknown', and then attach a
single-buffer XDP program:

tools/testing/selftests/drivers/net/hds.py:set_xdp() {
    ...
    if mode == 'enabled':
        _defer_reset_hds(cfg, netnl)
        netnl.rings_set({'header': {'dev-index': cfg.ifindex},
                         'tcp-data-split': 'unknown'})

    _xdp_onoff(cfg)
}

If cfg is already UNKNOWN, ethnl_set_rings() returns 0 on !mod and the
'unknown' write does nothing. Otherwise it only clears the config, and
GRO_HW stays wanted. Either way SHAMPO stays active:

mlx5e_xdp_set()
  mlx5e_xdp_allowed()
	if (params->packet_merge.type != MLX5E_PACKET_MERGE_NONE) {
		netdev_warn(netdev, "can't set XDP while HW-GRO/LRO is on, disable them first\n");
		return -EINVAL;

The log in the changelog only covers the HW-GRO off case, where these
tests are skipped. Was the HW-GRO on configuration tested as well?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009025137.2631215-1-dimitri.daskalakis1%40gmail.com