From: Saeed Mahameed <saeedm@nvidia.com>
Try enabling HW GRO when requested.
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
Signed-off-by: Cosmin Ratiu <cratiu@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
.../ethernet/mellanox/mlx5/core/en_ethtool.c | 50 +++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index ea078c9f5d15..b6c3b6c11f86 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -371,6 +371,14 @@ void mlx5e_ethtool_get_ringparam(struct mlx5e_priv *priv,
(priv->channels.params.packet_merge.type == MLX5E_PACKET_MERGE_SHAMPO) ?
ETHTOOL_TCP_DATA_SPLIT_ENABLED :
ETHTOOL_TCP_DATA_SPLIT_DISABLED;
+
+ /* if HW GRO is not enabled due to external limitations but is wanted,
+ * report HDS state as unknown so it won't get turned off explicitly.
+ */
+ if (kernel_param->tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_DISABLED &&
+ priv->netdev->wanted_features & NETIF_F_GRO_HW)
+ kernel_param->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_UNKNOWN;
+
}
static void mlx5e_get_ringparam(struct net_device *dev,
@@ -383,6 +391,43 @@ static void mlx5e_get_ringparam(struct net_device *dev,
mlx5e_ethtool_get_ringparam(priv, param, kernel_param);
}
+static bool mlx5e_ethtool_set_tcp_data_split(struct mlx5e_priv *priv,
+ u8 tcp_data_split)
+{
+ bool enable = (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED);
+ struct net_device *dev = priv->netdev;
+
+ if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_UNKNOWN)
+ return true;
+
+ if (enable && !(dev->hw_features & NETIF_F_GRO_HW)) {
+ netdev_warn(dev, "TCP-data-split is not supported when GRO HW is not supported\n");
+ return false; /* GRO HW is not supported */
+ }
+
+ if (enable && (dev->features & NETIF_F_GRO_HW)) {
+ /* Already enabled */
+ dev->wanted_features |= NETIF_F_GRO_HW;
+ return true;
+ }
+
+ if (!enable && !(dev->features & NETIF_F_GRO_HW)) {
+ /* Already disabled */
+ dev->wanted_features &= ~NETIF_F_GRO_HW;
+ return true;
+ }
+
+ /* Try enable or disable GRO HW */
+ if (enable)
+ dev->wanted_features |= NETIF_F_GRO_HW;
+ else
+ dev->wanted_features &= ~NETIF_F_GRO_HW;
+
+ netdev_change_features(dev);
+
+ return enable == !!(dev->features & NETIF_F_GRO_HW);
+}
+
int mlx5e_ethtool_set_ringparam(struct mlx5e_priv *priv,
struct ethtool_ringparam *param,
struct netlink_ext_ack *extack)
@@ -441,6 +486,10 @@ static int mlx5e_set_ringparam(struct net_device *dev,
{
struct mlx5e_priv *priv = netdev_priv(dev);
+ if (!mlx5e_ethtool_set_tcp_data_split(priv,
+ kernel_param->tcp_data_split))
+ return -EINVAL;
+
return mlx5e_ethtool_set_ringparam(priv, param, extack);
}
@@ -2625,6 +2674,7 @@ const struct ethtool_ops mlx5e_ethtool_ops = {
ETHTOOL_COALESCE_USE_ADAPTIVE |
ETHTOOL_COALESCE_USE_CQE,
.supported_input_xfrm = RXH_XFRM_SYM_OR_XOR,
+ .supported_ring_params = ETHTOOL_RING_USE_TCP_DATA_SPLIT,
.get_drvinfo = mlx5e_get_drvinfo,
.get_link = ethtool_op_get_link,
.get_link_ext_state = mlx5e_get_link_ext_state,
--
2.31.1On Fri, 23 May 2025 00:41:26 +0300 Tariq Toukan wrote:
> + /* if HW GRO is not enabled due to external limitations but is wanted,
> + * report HDS state as unknown so it won't get turned off explicitly.
> + */
> + if (kernel_param->tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_DISABLED &&
> + priv->netdev->wanted_features & NETIF_F_GRO_HW)
> + kernel_param->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_UNKNOWN;
The kernel_param->tcp_data_split here is the user config, right?
It would be cleaner to not support setting SPLIT_DISABLED.
Nothing requires that, you can support just setting AUTO and ENABLED.
> +
nit: extra empty line, please run checkpatch
> }
>
> static void mlx5e_get_ringparam(struct net_device *dev,
> @@ -383,6 +391,43 @@ static void mlx5e_get_ringparam(struct net_device *dev,
> mlx5e_ethtool_get_ringparam(priv, param, kernel_param);
> }
>
> +static bool mlx5e_ethtool_set_tcp_data_split(struct mlx5e_priv *priv,
> + u8 tcp_data_split)
> +{
> + bool enable = (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED);
> + struct net_device *dev = priv->netdev;
> +
> + if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_UNKNOWN)
> + return true;
> +
> + if (enable && !(dev->hw_features & NETIF_F_GRO_HW)) {
> + netdev_warn(dev, "TCP-data-split is not supported when GRO HW is not supported\n");
> + return false; /* GRO HW is not supported */
> + }
> +
> + if (enable && (dev->features & NETIF_F_GRO_HW)) {
> + /* Already enabled */
> + dev->wanted_features |= NETIF_F_GRO_HW;
> + return true;
> + }
> +
> + if (!enable && !(dev->features & NETIF_F_GRO_HW)) {
> + /* Already disabled */
> + dev->wanted_features &= ~NETIF_F_GRO_HW;
> + return true;
> + }
> +
> + /* Try enable or disable GRO HW */
> + if (enable)
> + dev->wanted_features |= NETIF_F_GRO_HW;
> + else
> + dev->wanted_features &= ~NETIF_F_GRO_HW;
Why are you modifying wanted_features? wanted_features is what
*user space* wanted! You should probably operate on hw_features ?
Tho, may be cleaner to return an error and an extack if the user
tries to set HDS and GRO to conflicting values.
On 22 May 15:55, Jakub Kicinski wrote:
>On Fri, 23 May 2025 00:41:26 +0300 Tariq Toukan wrote:
>> + /* if HW GRO is not enabled due to external limitations but is wanted,
>> + * report HDS state as unknown so it won't get turned off explicitly.
>> + */
>> + if (kernel_param->tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_DISABLED &&
>> + priv->netdev->wanted_features & NETIF_F_GRO_HW)
>> + kernel_param->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_UNKNOWN;
>
>The kernel_param->tcp_data_split here is the user config, right?
>It would be cleaner to not support setting SPLIT_DISABLED.
>Nothing requires that, you can support just setting AUTO and ENABLED.
>
I think I agree, AUTO might require some extra work on the driver side to
figure out current internal mode, but it actually makes more sense than
just doing "UNKNOWN", UKNOWN here means that HW GRO needs to be enabled
when disabling TCP HDR split, and we still don't know if that will work..
Cosmin will you look into this ?
>> +
>
>nit: extra empty line, please run checkpatch
>
>> }
>>
>> static void mlx5e_get_ringparam(struct net_device *dev,
>> @@ -383,6 +391,43 @@ static void mlx5e_get_ringparam(struct net_device *dev,
>> mlx5e_ethtool_get_ringparam(priv, param, kernel_param);
>> }
>>
>> +static bool mlx5e_ethtool_set_tcp_data_split(struct mlx5e_priv *priv,
>> + u8 tcp_data_split)
>> +{
>> + bool enable = (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED);
>> + struct net_device *dev = priv->netdev;
>> +
>> + if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_UNKNOWN)
>> + return true;
>> +
>> + if (enable && !(dev->hw_features & NETIF_F_GRO_HW)) {
>> + netdev_warn(dev, "TCP-data-split is not supported when GRO HW is not supported\n");
>> + return false; /* GRO HW is not supported */
>> + }
>> +
>> + if (enable && (dev->features & NETIF_F_GRO_HW)) {
>> + /* Already enabled */
>> + dev->wanted_features |= NETIF_F_GRO_HW;
>> + return true;
>> + }
>> +
>> + if (!enable && !(dev->features & NETIF_F_GRO_HW)) {
>> + /* Already disabled */
>> + dev->wanted_features &= ~NETIF_F_GRO_HW;
>> + return true;
>> + }
>> +
>> + /* Try enable or disable GRO HW */
>> + if (enable)
>> + dev->wanted_features |= NETIF_F_GRO_HW;
>> + else
>> + dev->wanted_features &= ~NETIF_F_GRO_HW;
>
>Why are you modifying wanted_features? wanted_features is what
>*user space* wanted! You should probably operate on hw_features ?
>Tho, may be cleaner to return an error and an extack if the user
>tries to set HDS and GRO to conflicting values.
>
hw_features is hw capabilities, it doesn't mean on/off.. so no we can't
rely on that.
To enable TCP_DATA_SPLIT we tie it to GRO_HW, so we enable GRO_HW when
TCP_DATA_SPLIT is set to on and vise-versa. I agree not the cleanest..
But it is good for user-visibility as you would see both ON if you query
from user, which is the actual state. This is the only way to set HW_GRO
to on by driver and not lose previous state when we turn the other bit
on/off.
Yes, I guess such logic should be in the stack, although I don't see
anything wrong here in terms of correctness.
On Thu, 22 May 2025 16:19:28 -0700 Saeed Mahameed wrote: > >Why are you modifying wanted_features? wanted_features is what > >*user space* wanted! You should probably operate on hw_features ? > >Tho, may be cleaner to return an error and an extack if the user > >tries to set HDS and GRO to conflicting values. > > > > hw_features is hw capabilities, it doesn't mean on/off.. so no we can't > rely on that. > > To enable TCP_DATA_SPLIT we tie it to GRO_HW, so we enable GRO_HW when > TCP_DATA_SPLIT is set to on and vise-versa. I agree not the cleanest.. > But it is good for user-visibility as you would see both ON if you query > from user, which is the actual state. This is the only way to set HW_GRO > to on by driver and not lose previous state when we turn the other bit > on/off. features = on hw_features = off is how we indicate the feature is "on [fixed]" Tho, I'm not sure how much precedent there is for making things fixed at runtime.
On 27/05/2025 19:10, Jakub Kicinski wrote: > On Thu, 22 May 2025 16:19:28 -0700 Saeed Mahameed wrote: >>> Why are you modifying wanted_features? wanted_features is what >>> *user space* wanted! You should probably operate on hw_features ? >>> Tho, may be cleaner to return an error and an extack if the user >>> tries to set HDS and GRO to conflicting values. >>> >> >> hw_features is hw capabilities, it doesn't mean on/off.. so no we can't >> rely on that. >> >> To enable TCP_DATA_SPLIT we tie it to GRO_HW, so we enable GRO_HW when >> TCP_DATA_SPLIT is set to on and vise-versa. I agree not the cleanest.. >> But it is good for user-visibility as you would see both ON if you query >> from user, which is the actual state. This is the only way to set HW_GRO >> to on by driver and not lose previous state when we turn the other bit >> on/off. > > features = on > hw_features = off > > is how we indicate the feature is "on [fixed]" > Tho, I'm not sure how much precedent there is for making things fixed > at runtime. Isn't this something that should be handled through fix_features?
On Wed, 28 May 2025 08:10:46 +0300 Gal Pressman wrote: > > features = on > > hw_features = off > > > > is how we indicate the feature is "on [fixed]" > > Tho, I'm not sure how much precedent there is for making things fixed > > at runtime. > > Isn't this something that should be handled through fix_features? Yes, should work. Off the top of my head after setting HDS to enabled and trying to disable HW-GRO we should see: "on [requested off]" right?
On Thu, 2025-05-22 at 16:19 -0700, Saeed Mahameed wrote: > > > > The kernel_param->tcp_data_split here is the user config, right? > > It would be cleaner to not support setting SPLIT_DISABLED. > > Nothing requires that, you can support just setting AUTO and > > ENABLED. > > > > I think I agree, AUTO might require some extra work on the driver > side to > figure out current internal mode, but it actually makes more sense > than > just doing "UNKNOWN", UKNOWN here means that HW GRO needs to be > enabled > when disabling TCP HDR split, and we still don't know if that will > work.. > > Cosmin will you look into this ? Yes, I will address all comments from this round by the next submission. Cosmin.
On 23 May 16:17, Cosmin Ratiu wrote: >On Thu, 2025-05-22 at 16:19 -0700, Saeed Mahameed wrote: >> > >> > The kernel_param->tcp_data_split here is the user config, right? >> > It would be cleaner to not support setting SPLIT_DISABLED. >> > Nothing requires that, you can support just setting AUTO and >> > ENABLED. >> > >> >> I think I agree, AUTO might require some extra work on the driver >> side to >> figure out current internal mode, but it actually makes more sense >> than >> just doing "UNKNOWN", UKNOWN here means that HW GRO needs to be >> enabled >> when disabling TCP HDR split, and we still don't know if that will >> work.. >> >> Cosmin will you look into this ? > >Yes, I will address all comments from this round by the next >submission. > I thought about it, maybe we should simplify mlx5: when hw_gro is not enabled mlx5 should fail tcp_data_split ethtool settings, and when hw_gro is enabled setting tcp_data_split on mlx5 should be a no-op. I think verbosity is important here as mlx5 doesn't support plain tcp_data_split, it must come with gro_hw .. reporting tcp_data_split in mlx5 can remain the same as before the series. We might need to update some tests and script to try hw_gro if enabling just tcp_data_split didn't work. for the AUTO state I really have no clue what it means and I think we should avoid it. >Cosmin.
© 2016 - 2025 Red Hat, Inc.