netdev - Re: [PATCH net-next v2] eth: mlx5: expose NETIF_F

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZpBlOWzyihXUad_V@x130>
Date: Thu, 11 Jul 2024 16:05:29 -0700
From: Saeed Mahameed <saeed@...nel.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
	pabeni@...hat.com, tariqt@...dia.com, rrameshbabu@...dia.com,
	saeedm@...dia.com, yuehaibing@...wei.com, horms@...nel.org,
	jacob.e.keller@...el.com, afaris@...dia.com
Subject: Re: [PATCH net-next v2] eth: mlx5: expose NETIF_F_NTUPLE when ARFS
 is compiled out

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@...nel.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@...dia.com
>CC: rrameshbabu@...dia.com
>CC: saeedm@...dia.com
>CC: yuehaibing@...wei.com
>CC: horms@...nel.org
>CC: jacob.e.keller@...el.com
>CC: afaris@...dia.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@...dia.com>