[PATCH net-next v2] eth: mlx5: expose NETIF_F_NTUPLE when ARFS is compiled out

[PATCH net-next v2] eth: mlx5: expose NETIF_F_NTUPLE when ARFS is compiled out

[Jakub Kicinski]

ARFS depends on NTUPLE filters, but the inverse is not true.
Drivers which don't support ARFS commonly still support NTUPLE
filtering. mlx5 has a Kconfig option to disable ARFS (MLX5_EN_ARFS)
and does not advertise NTUPLE filters as a feature at all when ARFS
is compiled out. That's not correct, ntuple filters indeed still work
just fine (as long as MLX5_EN_RXNFC is enabled).

This is needed to make the RSS test not skip all RSS context
related testing.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - hard wire to on (not propagating Simon's review because of this)
v1: https://lore.kernel.org/20240710175502.760194-1-kuba@kernel.org

CC: tariqt@nvidia.com
CC: rrameshbabu@nvidia.com
CC: saeedm@nvidia.com
CC: yuehaibing@huawei.com
CC: horms@kernel.org
CC: jacob.e.keller@intel.com
CC: afaris@nvidia.com
---
 drivers/net/ethernet/mellanox/mlx5/core/en/fs.h     | 13 +++++++++++++
 .../net/ethernet/mellanox/mlx5/core/en_ethtool.c    |  2 +-
 drivers/net/ethernet/mellanox/mlx5/core/en_fs.c     |  5 ++---
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c   |  8 +++++---
 .../net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c   |  8 +++-----
 5 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/fs.h b/drivers/net/ethernet/mellanox/mlx5/core/en/fs.h
index 4d6225e0eec7..1e8b7d330701 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/fs.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/fs.h
@@ -154,6 +154,19 @@ struct mlx5e_tc_table *mlx5e_fs_get_tc(struct mlx5e_flow_steering *fs);
 struct mlx5e_l2_table *mlx5e_fs_get_l2(struct mlx5e_flow_steering *fs);
 struct mlx5_flow_namespace *mlx5e_fs_get_ns(struct mlx5e_flow_steering *fs, bool egress);
 void mlx5e_fs_set_ns(struct mlx5e_flow_steering *fs, struct mlx5_flow_namespace *ns, bool egress);
+
+static inline bool mlx5e_fs_has_arfs(struct net_device *netdev)
+{
+	return IS_ENABLED(CONFIG_MLX5_EN_ARFS) &&
+		netdev->hw_features & NETIF_F_NTUPLE;
+}
+
+static inline bool mlx5e_fs_want_arfs(struct net_device *netdev)
+{
+	return IS_ENABLED(CONFIG_MLX5_EN_ARFS) &&
+		netdev->features & NETIF_F_NTUPLE;
+}
+
 #ifdef CONFIG_MLX5_EN_RXNFC
 struct mlx5e_ethtool_steering *mlx5e_fs_get_ethtool(struct mlx5e_flow_steering *fs);
 #endif
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index 3320f12ba2db..5582c93a62f1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -525,7 +525,7 @@ int mlx5e_ethtool_set_channels(struct mlx5e_priv *priv,
 
 	opened = test_bit(MLX5E_STATE_OPENED, &priv->state);
 
-	arfs_enabled = opened && (priv->netdev->features & NETIF_F_NTUPLE);
+	arfs_enabled = opened && mlx5e_fs_want_arfs(priv->netdev);
 	if (arfs_enabled)
 		mlx5e_arfs_disable(priv->fs);
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c
index 8c5b291a171f..05058710d2c7 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c
@@ -1307,8 +1307,7 @@ int mlx5e_create_flow_steering(struct mlx5e_flow_steering *fs,
 		return -EOPNOTSUPP;
 
 	mlx5e_fs_set_ns(fs, ns, false);
-	err = mlx5e_arfs_create_tables(fs, rx_res,
-				       !!(netdev->hw_features & NETIF_F_NTUPLE));
+	err = mlx5e_arfs_create_tables(fs, rx_res, mlx5e_fs_has_arfs(netdev));
 	if (err) {
 		fs_err(fs, "Failed to create arfs tables, err=%d\n", err);
 		netdev->hw_features &= ~NETIF_F_NTUPLE;
@@ -1355,7 +1354,7 @@ int mlx5e_create_flow_steering(struct mlx5e_flow_steering *fs,
 err_destroy_inner_ttc_table:
 	mlx5e_destroy_inner_ttc_table(fs);
 err_destroy_arfs_tables:
-	mlx5e_arfs_destroy_tables(fs, !!(netdev->hw_features & NETIF_F_NTUPLE));
+	mlx5e_arfs_destroy_tables(fs, mlx5e_fs_has_arfs(netdev));
 
 	return err;
 }
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index ff335527c10a..6f686fabed44 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -5556,8 +5556,10 @@ static void mlx5e_build_nic_netdev(struct net_device *netdev)
 #if IS_ENABLED(CONFIG_MLX5_CLS_ACT)
 		netdev->hw_features      |= NETIF_F_HW_TC;
 #endif
-#ifdef CONFIG_MLX5_EN_ARFS
+#if IS_ENABLED(CONFIG_MLX5_EN_ARFS)
 		netdev->hw_features	 |= NETIF_F_NTUPLE;
+#elif IS_ENABLED(CONFIG_MLX5_EN_RXNFC)
+		netdev->features	 |= NETIF_F_NTUPLE;
 #endif
 	}
 
@@ -5731,7 +5733,7 @@ static int mlx5e_init_nic_rx(struct mlx5e_priv *priv)
 err_tc_nic_cleanup:
 	mlx5e_tc_nic_cleanup(priv);
 err_destroy_flow_steering:
-	mlx5e_destroy_flow_steering(priv->fs, !!(priv->netdev->hw_features & NETIF_F_NTUPLE),
+	mlx5e_destroy_flow_steering(priv->fs, mlx5e_fs_has_arfs(priv->netdev),
 				    priv->profile);
 err_destroy_rx_res:
 	mlx5e_rx_res_destroy(priv->rx_res);
@@ -5747,7 +5749,7 @@ static void mlx5e_cleanup_nic_rx(struct mlx5e_priv *priv)
 {
 	mlx5e_accel_cleanup_rx(priv);
 	mlx5e_tc_nic_cleanup(priv);
-	mlx5e_destroy_flow_steering(priv->fs, !!(priv->netdev->hw_features & NETIF_F_NTUPLE),
+	mlx5e_destroy_flow_steering(priv->fs, mlx5e_fs_has_arfs(priv->netdev),
 				    priv->profile);
 	mlx5e_rx_res_destroy(priv->rx_res);
 	priv->rx_res = NULL;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c b/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c
index 8e0404c0d1ca..0979d672d47f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c
@@ -372,7 +372,7 @@ static int mlx5i_create_flow_steering(struct mlx5e_priv *priv)
 
 	mlx5e_fs_set_ns(priv->fs, ns, false);
 	err = mlx5e_arfs_create_tables(priv->fs, priv->rx_res,
-				       !!(priv->netdev->hw_features & NETIF_F_NTUPLE));
+				       mlx5e_fs_has_arfs(priv->netdev));
 	if (err) {
 		netdev_err(priv->netdev, "Failed to create arfs tables, err=%d\n",
 			   err);
@@ -391,8 +391,7 @@ static int mlx5i_create_flow_steering(struct mlx5e_priv *priv)
 	return 0;
 
 err_destroy_arfs_tables:
-	mlx5e_arfs_destroy_tables(priv->fs,
-				  !!(priv->netdev->hw_features & NETIF_F_NTUPLE));
+	mlx5e_arfs_destroy_tables(priv->fs, mlx5e_fs_has_arfs(priv->netdev));
 
 	return err;
 }
@@ -400,8 +399,7 @@ static int mlx5i_create_flow_steering(struct mlx5e_priv *priv)
 static void mlx5i_destroy_flow_steering(struct mlx5e_priv *priv)
 {
 	mlx5e_destroy_ttc_table(priv->fs);
-	mlx5e_arfs_destroy_tables(priv->fs,
-				  !!(priv->netdev->hw_features & NETIF_F_NTUPLE));
+	mlx5e_arfs_destroy_tables(priv->fs, mlx5e_fs_has_arfs(priv->netdev));
 	mlx5e_ethtool_cleanup_steering(priv->fs);
 }
 
-- 
2.45.2

Re: [PATCH net-next v2] eth: mlx5: expose NETIF_F_NTUPLE when ARFS is compiled out

[Saeed Mahameed]

On 11 Jul 15:37, Jakub Kicinski wrote:
>ARFS depends on NTUPLE filters, but the inverse is not true.
>Drivers which don't support ARFS commonly still support NTUPLE
>filtering. mlx5 has a Kconfig option to disable ARFS (MLX5_EN_ARFS)
>and does not advertise NTUPLE filters as a feature at all when ARFS
>is compiled out. That's not correct, ntuple filters indeed still work
>just fine (as long as MLX5_EN_RXNFC is enabled).
>
>This is needed to make the RSS test not skip all RSS context
>related testing.
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>---
>v2:
> - hard wire to on (not propagating Simon's review because of this)
>v1: https://lore.kernel.org/20240710175502.760194-1-kuba@kernel.org
>
>CC: tariqt@nvidia.com
>CC: rrameshbabu@nvidia.com
>CC: saeedm@nvidia.com
>CC: yuehaibing@huawei.com
>CC: horms@kernel.org
>CC: jacob.e.keller@intel.com
>CC: afaris@nvidia.com
>---
> drivers/net/ethernet/mellanox/mlx5/core/en/fs.h     | 13 +++++++++++++
> .../net/ethernet/mellanox/mlx5/core/en_ethtool.c    |  2 +-
> drivers/net/ethernet/mellanox/mlx5/core/en_fs.c     |  5 ++---
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c   |  8 +++++---
> .../net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c   |  8 +++-----
> 5 files changed, 24 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/fs.h b/drivers/net/ethernet/mellanox/mlx5/core/en/fs.h
>index 4d6225e0eec7..1e8b7d330701 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en/fs.h
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/fs.h
>@@ -154,6 +154,19 @@ struct mlx5e_tc_table *mlx5e_fs_get_tc(struct mlx5e_flow_steering *fs);
> struct mlx5e_l2_table *mlx5e_fs_get_l2(struct mlx5e_flow_steering *fs);
> struct mlx5_flow_namespace *mlx5e_fs_get_ns(struct mlx5e_flow_steering *fs, bool egress);
> void mlx5e_fs_set_ns(struct mlx5e_flow_steering *fs, struct mlx5_flow_namespace *ns, bool egress);
>+
>+static inline bool mlx5e_fs_has_arfs(struct net_device *netdev)
>+{
>+	return IS_ENABLED(CONFIG_MLX5_EN_ARFS) &&
>+		netdev->hw_features & NETIF_F_NTUPLE;
>+}
>+
>+static inline bool mlx5e_fs_want_arfs(struct net_device *netdev)
>+{
>+	return IS_ENABLED(CONFIG_MLX5_EN_ARFS) &&
>+		netdev->features & NETIF_F_NTUPLE;
>+}
>+
> #ifdef CONFIG_MLX5_EN_RXNFC
> struct mlx5e_ethtool_steering *mlx5e_fs_get_ethtool(struct mlx5e_flow_steering *fs);
> #endif
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>index 3320f12ba2db..5582c93a62f1 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>@@ -525,7 +525,7 @@ int mlx5e_ethtool_set_channels(struct mlx5e_priv *priv,
>
> 	opened = test_bit(MLX5E_STATE_OPENED, &priv->state);
>
>-	arfs_enabled = opened && (priv->netdev->features & NETIF_F_NTUPLE);
>+	arfs_enabled = opened && mlx5e_fs_want_arfs(priv->netdev);
> 	if (arfs_enabled)
> 		mlx5e_arfs_disable(priv->fs);
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c
>index 8c5b291a171f..05058710d2c7 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c
>@@ -1307,8 +1307,7 @@ int mlx5e_create_flow_steering(struct mlx5e_flow_steering *fs,
> 		return -EOPNOTSUPP;
>
> 	mlx5e_fs_set_ns(fs, ns, false);
>-	err = mlx5e_arfs_create_tables(fs, rx_res,
>-				       !!(netdev->hw_features & NETIF_F_NTUPLE));
>+	err = mlx5e_arfs_create_tables(fs, rx_res, mlx5e_fs_has_arfs(netdev));
> 	if (err) {
> 		fs_err(fs, "Failed to create arfs tables, err=%d\n", err);
> 		netdev->hw_features &= ~NETIF_F_NTUPLE;
>@@ -1355,7 +1354,7 @@ int mlx5e_create_flow_steering(struct mlx5e_flow_steering *fs,
> err_destroy_inner_ttc_table:
> 	mlx5e_destroy_inner_ttc_table(fs);
> err_destroy_arfs_tables:
>-	mlx5e_arfs_destroy_tables(fs, !!(netdev->hw_features & NETIF_F_NTUPLE));
>+	mlx5e_arfs_destroy_tables(fs, mlx5e_fs_has_arfs(netdev));
>
> 	return err;
> }
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>index ff335527c10a..6f686fabed44 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>@@ -5556,8 +5556,10 @@ static void mlx5e_build_nic_netdev(struct net_device *netdev)
> #if IS_ENABLED(CONFIG_MLX5_CLS_ACT)
> 		netdev->hw_features      |= NETIF_F_HW_TC;
> #endif
>-#ifdef CONFIG_MLX5_EN_ARFS
>+#if IS_ENABLED(CONFIG_MLX5_EN_ARFS)
> 		netdev->hw_features	 |= NETIF_F_NTUPLE;
>+#elif IS_ENABLED(CONFIG_MLX5_EN_RXNFC)
>+		netdev->features	 |= NETIF_F_NTUPLE;

Why default ON when RXNFC and OFF when ARFS ?
Default should be off always, and this needs to be advertised in 
hw_features in both cases.

I think this should be
#if IS_ENABLED(ARFS) || IS_ENABLED(RXFNC)
	netdev->hw_features |= NTUPLE;

Otherwise LGTM

Acked-by: Saeed Mahameed <saeedm@nvidia.com>

Re: [PATCH net-next v2] eth: mlx5: expose NETIF_F_NTUPLE when ARFS is compiled out

[Jakub Kicinski]

On Thu, 11 Jul 2024 16:05:29 -0700 Saeed Mahameed wrote:
> >+#if IS_ENABLED(CONFIG_MLX5_EN_ARFS)
> > 		netdev->hw_features	 |= NETIF_F_NTUPLE;
> >+#elif IS_ENABLED(CONFIG_MLX5_EN_RXNFC)
> >+		netdev->features	 |= NETIF_F_NTUPLE;  
> 
> Why default ON when RXNFC and OFF when ARFS ?
> Default should be off always, and this needs to be advertised in 
> hw_features in both cases.
> 
> I think this should be
> #if IS_ENABLED(ARFS) || IS_ENABLED(RXFNC)
> 	netdev->hw_features |= NTUPLE;

That's what I thought, but on reflection since the filters can be
added, and disable doesn't actually do anything - "fixed on" started
to sound more appropriate. The additional "[fixed]" could also be useful
for troubleshooting, to signal that this is a different situation than
ARFS=y. No hard preference tho.

> Otherwise LGTM
> 
> Acked-by: Saeed Mahameed <saeedm@nvidia.com>

Thanks!

Re: [PATCH net-next v2] eth: mlx5: expose NETIF_F_NTUPLE when ARFS is compiled out

[Saeed Mahameed]

On 11 Jul 16:16, Jakub Kicinski wrote:
>On Thu, 11 Jul 2024 16:05:29 -0700 Saeed Mahameed wrote:
>> >+#if IS_ENABLED(CONFIG_MLX5_EN_ARFS)
>> > 		netdev->hw_features	 |= NETIF_F_NTUPLE;
>> >+#elif IS_ENABLED(CONFIG_MLX5_EN_RXNFC)
>> >+		netdev->features	 |= NETIF_F_NTUPLE;
>>
>> Why default ON when RXNFC and OFF when ARFS ?
>> Default should be off always, and this needs to be advertised in
>> hw_features in both cases.
>>
>> I think this should be
>> #if IS_ENABLED(ARFS) || IS_ENABLED(RXFNC)
>> 	netdev->hw_features |= NTUPLE;
>
>That's what I thought, but on reflection since the filters can be
>added, and disable doesn't actually do anything - "fixed on" started
>to sound more appropriate. The additional "[fixed]" could also be useful
>for troubleshooting, to signal that this is a different situation than
>ARFS=y. No hard preference tho.
>

Agreed, Thanks.

>> Otherwise LGTM
>>
>> Acked-by: Saeed Mahameed <saeedm@nvidia.com>
>
>Thanks!

Re: [PATCH net-next v2] eth: mlx5: expose NETIF_F_NTUPLE when ARFS is compiled out

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 11 Jul 2024 15:37:22 -0700 you wrote:
> ARFS depends on NTUPLE filters, but the inverse is not true.
> Drivers which don't support ARFS commonly still support NTUPLE
> filtering. mlx5 has a Kconfig option to disable ARFS (MLX5_EN_ARFS)
> and does not advertise NTUPLE filters as a feature at all when ARFS
> is compiled out. That's not correct, ntuple filters indeed still work
> just fine (as long as MLX5_EN_RXNFC is enabled).
> 
> [...]

Here is the summary with links:
  - [net-next,v2] eth: mlx5: expose NETIF_F_NTUPLE when ARFS is compiled out
    https://git.kernel.org/netdev/net-next/c/3771266bf841

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html