| Message ID | 20240711223722.297676-1-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3771266bf8418904794175fdc83aaf4732114cba |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] eth: mlx5: expose NETIF_F_NTUPLE when ARFS is compiled out | expand |
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>
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!
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!
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!
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); }
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(-)