[net 0/2] rxfh with custom RSS fixes

[net 0/2] rxfh with custom RSS fixes

[Joe Damato]

Greetings:

While attempting to get the RX flow hash key for a custom RSS context on
my mlx5 NIC, I got an error:

$ sudo ethtool -u eth1 rx-flow-hash tcp4 context 1
Cannot get RX network flow hashing options: Invalid argument

I dug into this a bit and noticed two things:

1. ETHTOOL_GRXFH supports custom RSS contexts, but ETHTOOL_SRXFH does
not. I moved the copy logic out of ETHTOOL_GRXFH and into a helper so
that both ETHTOOL_{G,S}RXFH now call it, which fixes ETHTOOL_SRXFH. This
is patch 1/2.

2. mlx5 defaulted to RSS context 0 for both ETHTOOL_{G,S}RXFH paths. I
have modified the driver to support custom contexts for both paths. It
is now possible to get and set the flow hash key for custom RSS contexts
with mlx5. This is patch 2/2.

See commit messages for more details.

The patches include the relevant fixes tags, as I think both commits are
fixing previous code, but if this change is preferred for net-next I can
resend.

Thanks.

Joe Damato (2):
  net: ethtool: Unify ETHTOOL_{G,S}RXFH rxnfc copy
  net/mlx5: Fix flowhash key set/get for custom RSS

 .../ethernet/mellanox/mlx5/core/en/rx_res.c   | 23 +++++-
 .../ethernet/mellanox/mlx5/core/en/rx_res.h   |  5 +-
 .../mellanox/mlx5/core/en_fs_ethtool.c        | 33 +++++---
 net/ethtool/ioctl.c                           | 75 ++++++++++---------
 4 files changed, 84 insertions(+), 52 deletions(-)

-- 
2.25.1

[net 1/2] net: ethtool: Unify ETHTOOL_{G,S}RXFH rxnfc copy

[Joe Damato]

ETHTOOL_GRXFH correctly copies in the full struct ethtool_rxnfc when
FLOW_RSS is set; ETHTOOL_SRXFH needs a similar code path to handle the
FLOW_RSS case so that ethtool can set the flow hash for custom RSS
contexts (if supported by the driver).

The copy code from ETHTOOL_GRXFH has been pulled out in to a helper so
that it can be called in both ETHTOOL_{G,S}RXFH code paths.

Fixes: 84a1d9c48200 ("net: ethtool: extend RXNFC API to support RSS spreading of filter matches")
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 net/ethtool/ioctl.c | 75 +++++++++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 37 deletions(-)

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 4a51e0ec295c..7d40e7913e76 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -907,6 +907,38 @@ static int ethtool_rxnfc_copy_to_compat(void __user *useraddr,
 	return 0;
 }
 
+static int ethtool_rxnfc_copy_struct(u32 cmd, struct ethtool_rxnfc *info,
+				     size_t *info_size, void __user *useraddr)
+{
+	/* struct ethtool_rxnfc was originally defined for
+	 * ETHTOOL_{G,S}RXFH with only the cmd, flow_type and data
+	 * members.  User-space might still be using that
+	 * definition.
+	 */
+	if (cmd == ETHTOOL_GRXFH || cmd == ETHTOOL_SRXFH)
+		*info_size = (offsetof(struct ethtool_rxnfc, data) +
+			      sizeof(info->data));
+
+	if (ethtool_rxnfc_copy_from_user(info, useraddr, *info_size))
+		return -EFAULT;
+
+	if ((cmd == ETHTOOL_GRXFH || cmd == ETHTOOL_SRXFH) && info->flow_type & FLOW_RSS) {
+		*info_size = sizeof(*info);
+		if (ethtool_rxnfc_copy_from_user(info, useraddr, *info_size))
+			return -EFAULT;
+		/* Since malicious users may modify the original data,
+		 * we need to check whether FLOW_RSS is still requested.
+		 */
+		if (!(info->flow_type & FLOW_RSS))
+			return -EINVAL;
+	}
+
+	if (info->cmd != cmd)
+		return -EINVAL;
+
+	return 0;
+}
+
 static int ethtool_rxnfc_copy_to_user(void __user *useraddr,
 				      const struct ethtool_rxnfc *rxnfc,
 				      size_t size, const u32 *rule_buf)
@@ -944,16 +976,9 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
 	if (!dev->ethtool_ops->set_rxnfc)
 		return -EOPNOTSUPP;
 
-	/* struct ethtool_rxnfc was originally defined for
-	 * ETHTOOL_{G,S}RXFH with only the cmd, flow_type and data
-	 * members.  User-space might still be using that
-	 * definition. */
-	if (cmd == ETHTOOL_SRXFH)
-		info_size = (offsetof(struct ethtool_rxnfc, data) +
-			     sizeof(info.data));
-
-	if (ethtool_rxnfc_copy_from_user(&info, useraddr, info_size))
-		return -EFAULT;
+	rc = ethtool_rxnfc_copy_struct(cmd, &info, &info_size, useraddr);
+	if (rc)
+		return rc;
 
 	rc = dev->ethtool_ops->set_rxnfc(dev, &info);
 	if (rc)
@@ -978,33 +1003,9 @@ static noinline_for_stack int ethtool_get_rxnfc(struct net_device *dev,
 	if (!ops->get_rxnfc)
 		return -EOPNOTSUPP;
 
-	/* struct ethtool_rxnfc was originally defined for
-	 * ETHTOOL_{G,S}RXFH with only the cmd, flow_type and data
-	 * members.  User-space might still be using that
-	 * definition. */
-	if (cmd == ETHTOOL_GRXFH)
-		info_size = (offsetof(struct ethtool_rxnfc, data) +
-			     sizeof(info.data));
-
-	if (ethtool_rxnfc_copy_from_user(&info, useraddr, info_size))
-		return -EFAULT;
-
-	/* If FLOW_RSS was requested then user-space must be using the
-	 * new definition, as FLOW_RSS is newer.
-	 */
-	if (cmd == ETHTOOL_GRXFH && info.flow_type & FLOW_RSS) {
-		info_size = sizeof(info);
-		if (ethtool_rxnfc_copy_from_user(&info, useraddr, info_size))
-			return -EFAULT;
-		/* Since malicious users may modify the original data,
-		 * we need to check whether FLOW_RSS is still requested.
-		 */
-		if (!(info.flow_type & FLOW_RSS))
-			return -EINVAL;
-	}
-
-	if (info.cmd != cmd)
-		return -EINVAL;
+	ret = ethtool_rxnfc_copy_struct(cmd, &info, &info_size, useraddr);
+	if (ret)
+		return ret;
 
 	if (info.cmd == ETHTOOL_GRXCLSRLALL) {
 		if (info.rule_cnt > 0) {
-- 
2.25.1

Re: [net 1/2] net: ethtool: Unify ETHTOOL_{G,S}RXFH rxnfc copy

[Edward Cree]

On 23/07/2023 16:06, Joe Damato wrote:
> ETHTOOL_GRXFH correctly copies in the full struct ethtool_rxnfc when
> FLOW_RSS is set; ETHTOOL_SRXFH needs a similar code path to handle the
> FLOW_RSS case so that ethtool can set the flow hash for custom RSS
> contexts (if supported by the driver).
> 
> The copy code from ETHTOOL_GRXFH has been pulled out in to a helper so
> that it can be called in both ETHTOOL_{G,S}RXFH code paths.
> 
> Fixes: 84a1d9c48200 ("net: ethtool: extend RXNFC API to support RSS spreading of filter matches")
> Signed-off-by: Joe Damato <jdamato@fastly.com>

Acked-by: Edward Cree <ecree.xilinx@gmail.com>

[net 2/2] net/mlx5: Fix flowhash key set/get for custom RSS

[Joe Damato]

mlx5 flow hash field retrieval and set only worked on the default
RSS context, not custom RSS contexts.

For example, before this patch attempting to retrieve the flow hash fields
for RSS context 1 fails:

$ sudo ethtool -u eth1 rx-flow-hash tcp4 context 1
Cannot get RX network flow hashing options: Invalid argument

This patch fixes getting and setting the flow hash fields for contexts
other than the default context.

Getting the flow hash fields for RSS context 1:

sudo ethtool -u eth1 rx-flow-hash tcp4 context 1
For RSS context 1:
TCP over IPV4 flows use these fields for computing Hash flow key:
IP DA

Now, setting the flash hash fields to a custom value:

sudo ethtool -U eth1 rx-flow-hash tcp4 sdfn context 1

And retrieving them again:

sudo ethtool -u eth1 rx-flow-hash tcp4 context 1
For RSS context 1:
TCP over IPV4 flows use these fields for computing Hash flow key:
IP SA
IP DA
L4 bytes 0 & 1 [TCP/UDP src port]
L4 bytes 2 & 3 [TCP/UDP dst port]

Fixes: f01cc58c18d6 ("net/mlx5e: Support multiple RSS contexts")
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 .../ethernet/mellanox/mlx5/core/en/rx_res.c   | 23 ++++++++++---
 .../ethernet/mellanox/mlx5/core/en/rx_res.h   |  5 +--
 .../mellanox/mlx5/core/en_fs_ethtool.c        | 33 ++++++++++++++-----
 3 files changed, 46 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/rx_res.c b/drivers/net/ethernet/mellanox/mlx5/core/en/rx_res.c
index e1095bc36543..bb189c92e4c0 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/rx_res.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rx_res.c
@@ -218,17 +218,32 @@ int mlx5e_rx_res_rss_set_rxfh(struct mlx5e_rx_res *res, u32 rss_idx,
 	return mlx5e_rss_set_rxfh(rss, indir, key, hfunc, res->rss_rqns, res->rss_nch);
 }
 
-u8 mlx5e_rx_res_rss_get_hash_fields(struct mlx5e_rx_res *res, enum mlx5_traffic_types tt)
+int mlx5e_rx_res_rss_get_hash_fields(struct mlx5e_rx_res *res, enum mlx5_traffic_types tt,
+				     u32 rss_idx)
 {
-	struct mlx5e_rss *rss = res->rss[0];
+	struct mlx5e_rss *rss;
+
+	if (rss_idx >= MLX5E_MAX_NUM_RSS)
+		return -EINVAL;
+
+	rss = res->rss[rss_idx];
+	if (!rss)
+		return -EINVAL;
 
 	return mlx5e_rss_get_hash_fields(rss, tt);
 }
 
 int mlx5e_rx_res_rss_set_hash_fields(struct mlx5e_rx_res *res, enum mlx5_traffic_types tt,
-				     u8 rx_hash_fields)
+				     u8 rx_hash_fields, u32 rss_idx)
 {
-	struct mlx5e_rss *rss = res->rss[0];
+	struct mlx5e_rss *rss;
+
+	if (rss_idx >= MLX5E_MAX_NUM_RSS)
+		return -EINVAL;
+
+	rss = res->rss[rss_idx];
+	if (!rss)
+		return -EINVAL;
 
 	return mlx5e_rss_set_hash_fields(rss, tt, rx_hash_fields);
 }
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/rx_res.h b/drivers/net/ethernet/mellanox/mlx5/core/en/rx_res.h
index 5d5f64fab60f..8ac9d67a9603 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/rx_res.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rx_res.h
@@ -48,9 +48,10 @@ int mlx5e_rx_res_rss_get_rxfh(struct mlx5e_rx_res *res, u32 rss_idx,
 int mlx5e_rx_res_rss_set_rxfh(struct mlx5e_rx_res *res, u32 rss_idx,
 			      const u32 *indir, const u8 *key, const u8 *hfunc);
 
-u8 mlx5e_rx_res_rss_get_hash_fields(struct mlx5e_rx_res *res, enum mlx5_traffic_types tt);
+int mlx5e_rx_res_rss_get_hash_fields(struct mlx5e_rx_res *res, enum mlx5_traffic_types tt,
+				     u32 rss_idx);
 int mlx5e_rx_res_rss_set_hash_fields(struct mlx5e_rx_res *res, enum mlx5_traffic_types tt,
-				     u8 rx_hash_fields);
+				     u8 rx_hash_fields, u32 rss_idx);
 int mlx5e_rx_res_packet_merge_set_param(struct mlx5e_rx_res *res,
 					struct mlx5e_packet_merge_param *pkt_merge_param);
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c
index aac32e505c14..50b8f3da4db1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c
@@ -902,8 +902,14 @@ static int mlx5e_set_rss_hash_opt(struct mlx5e_priv *priv,
 	u8 rx_hash_field = 0;
 	int err;
 	int tt;
+	u32 flow_type = 0;
+	u32 rss_idx = 0;
 
-	tt = flow_type_to_traffic_type(nfc->flow_type);
+	if (nfc->flow_type & FLOW_RSS)
+		rss_idx = nfc->rss_context;
+
+	flow_type = flow_type_mask(nfc->flow_type);
+	tt = flow_type_to_traffic_type(flow_type);
 	if (tt < 0)
 		return tt;
 
@@ -911,10 +917,10 @@ static int mlx5e_set_rss_hash_opt(struct mlx5e_priv *priv,
 	 *  on src IP, dest IP, TCP/UDP src port and TCP/UDP dest
 	 *  port.
 	 */
-	if (nfc->flow_type != TCP_V4_FLOW &&
-	    nfc->flow_type != TCP_V6_FLOW &&
-	    nfc->flow_type != UDP_V4_FLOW &&
-	    nfc->flow_type != UDP_V6_FLOW)
+	if (flow_type != TCP_V4_FLOW &&
+	    flow_type != TCP_V6_FLOW &&
+	    flow_type != UDP_V4_FLOW &&
+	    flow_type != UDP_V6_FLOW)
 		return -EOPNOTSUPP;
 
 	if (nfc->data & ~(RXH_IP_SRC | RXH_IP_DST |
@@ -931,7 +937,7 @@ static int mlx5e_set_rss_hash_opt(struct mlx5e_priv *priv,
 		rx_hash_field |= MLX5_HASH_FIELD_SEL_L4_DPORT;
 
 	mutex_lock(&priv->state_lock);
-	err = mlx5e_rx_res_rss_set_hash_fields(priv->rx_res, tt, rx_hash_field);
+	err = mlx5e_rx_res_rss_set_hash_fields(priv->rx_res, tt, rx_hash_field, rss_idx);
 	mutex_unlock(&priv->state_lock);
 
 	return err;
@@ -940,14 +946,23 @@ static int mlx5e_set_rss_hash_opt(struct mlx5e_priv *priv,
 static int mlx5e_get_rss_hash_opt(struct mlx5e_priv *priv,
 				  struct ethtool_rxnfc *nfc)
 {
-	u32 hash_field = 0;
+	int hash_field = 0;
 	int tt;
+	u32 flow_type = 0;
+	u32 rss_idx = 0;
+
+	if (nfc->flow_type & FLOW_RSS)
+		rss_idx = nfc->rss_context;
 
-	tt = flow_type_to_traffic_type(nfc->flow_type);
+	flow_type = flow_type_mask(nfc->flow_type);
+	tt = flow_type_to_traffic_type(flow_type);
 	if (tt < 0)
 		return tt;
 
-	hash_field = mlx5e_rx_res_rss_get_hash_fields(priv->rx_res, tt);
+	hash_field = mlx5e_rx_res_rss_get_hash_fields(priv->rx_res, tt, rss_idx);
+	if (hash_field < 0)
+		return hash_field;
+
 	nfc->data = 0;
 
 	if (hash_field & MLX5_HASH_FIELD_SEL_SRC_IP)
-- 
2.25.1

Re: [net 2/2] net/mlx5: Fix flowhash key set/get for custom RSS

[Tariq Toukan]

On 23/07/2023 18:06, Joe Damato wrote:
> mlx5 flow hash field retrieval and set only worked on the default
> RSS context, not custom RSS contexts.
> 
> For example, before this patch attempting to retrieve the flow hash fields
> for RSS context 1 fails:
> 

Hi,

You are adding new driver functionality, please take it through net-next.

> $ sudo ethtool -u eth1 rx-flow-hash tcp4 context 1
> Cannot get RX network flow hashing options: Invalid argument
> 
> This patch fixes getting and setting the flow hash fields for contexts
> other than the default context.
> 
> Getting the flow hash fields for RSS context 1:
> 
> sudo ethtool -u eth1 rx-flow-hash tcp4 context 1
> For RSS context 1:
> TCP over IPV4 flows use these fields for computing Hash flow key:
> IP DA
> 
> Now, setting the flash hash fields to a custom value:
> 
> sudo ethtool -U eth1 rx-flow-hash tcp4 sdfn context 1
> 
> And retrieving them again:
> 
> sudo ethtool -u eth1 rx-flow-hash tcp4 context 1
> For RSS context 1:
> TCP over IPV4 flows use these fields for computing Hash flow key:
> IP SA
> IP DA
> L4 bytes 0 & 1 [TCP/UDP src port]
> L4 bytes 2 & 3 [TCP/UDP dst port]
> 
> Fixes: f01cc58c18d6 ("net/mlx5e: Support multiple RSS contexts")
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> ---
>   .../ethernet/mellanox/mlx5/core/en/rx_res.c   | 23 ++++++++++---
>   .../ethernet/mellanox/mlx5/core/en/rx_res.h   |  5 +--
>   .../mellanox/mlx5/core/en_fs_ethtool.c        | 33 ++++++++++++++-----
>   3 files changed, 46 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/rx_res.c b/drivers/net/ethernet/mellanox/mlx5/core/en/rx_res.c
> index e1095bc36543..bb189c92e4c0 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/rx_res.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rx_res.c
> @@ -218,17 +218,32 @@ int mlx5e_rx_res_rss_set_rxfh(struct mlx5e_rx_res *res, u32 rss_idx,
>   	return mlx5e_rss_set_rxfh(rss, indir, key, hfunc, res->rss_rqns, res->rss_nch);
>   }
>   
> -u8 mlx5e_rx_res_rss_get_hash_fields(struct mlx5e_rx_res *res, enum mlx5_traffic_types tt)
> +int mlx5e_rx_res_rss_get_hash_fields(struct mlx5e_rx_res *res, enum mlx5_traffic_types tt,
> +				     u32 rss_idx)

For consistency with other functions, please keep the rss_idx next to 
the res argument.

>   {
> -	struct mlx5e_rss *rss = res->rss[0];
> +	struct mlx5e_rss *rss;
> +
> +	if (rss_idx >= MLX5E_MAX_NUM_RSS)
> +		return -EINVAL;
> +
> +	rss = res->rss[rss_idx];
> +	if (!rss)
> +		return -EINVAL;

return -ENOENT; in this case.

>   
>   	return mlx5e_rss_get_hash_fields(rss, tt);
>   }
>   
>   int mlx5e_rx_res_rss_set_hash_fields(struct mlx5e_rx_res *res, enum mlx5_traffic_types tt,
> -				     u8 rx_hash_fields)
> +				     u8 rx_hash_fields, u32 rss_idx)

Same. let rss_idx come as second argument.
>   {
> -	struct mlx5e_rss *rss = res->rss[0];
> +	struct mlx5e_rss *rss;
> +
> +	if (rss_idx >= MLX5E_MAX_NUM_RSS)
> +		return -EINVAL;
> +
> +	rss = res->rss[rss_idx];
> +	if (!rss)
> +		return -EINVAL;

ENOENT

>   
>   	return mlx5e_rss_set_hash_fields(rss, tt, rx_hash_fields);
>   }
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/rx_res.h b/drivers/net/ethernet/mellanox/mlx5/core/en/rx_res.h
> index 5d5f64fab60f..8ac9d67a9603 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/rx_res.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rx_res.h
> @@ -48,9 +48,10 @@ int mlx5e_rx_res_rss_get_rxfh(struct mlx5e_rx_res *res, u32 rss_idx,
>   int mlx5e_rx_res_rss_set_rxfh(struct mlx5e_rx_res *res, u32 rss_idx,
>   			      const u32 *indir, const u8 *key, const u8 *hfunc);
>   
> -u8 mlx5e_rx_res_rss_get_hash_fields(struct mlx5e_rx_res *res, enum mlx5_traffic_types tt);
> +int mlx5e_rx_res_rss_get_hash_fields(struct mlx5e_rx_res *res, enum mlx5_traffic_types tt,
> +				     u32 rss_idx);
>   int mlx5e_rx_res_rss_set_hash_fields(struct mlx5e_rx_res *res, enum mlx5_traffic_types tt,
> -				     u8 rx_hash_fields);
> +				     u8 rx_hash_fields, u32 rss_idx);
>   int mlx5e_rx_res_packet_merge_set_param(struct mlx5e_rx_res *res,
>   					struct mlx5e_packet_merge_param *pkt_merge_param);
>   
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c
> index aac32e505c14..50b8f3da4db1 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c
> @@ -902,8 +902,14 @@ static int mlx5e_set_rss_hash_opt(struct mlx5e_priv *priv,
>   	u8 rx_hash_field = 0;
>   	int err;
>   	int tt;
> +	u32 flow_type = 0;
> +	u32 rss_idx = 0;
>   
> -	tt = flow_type_to_traffic_type(nfc->flow_type);
> +	if (nfc->flow_type & FLOW_RSS)
> +		rss_idx = nfc->rss_context;
> +
> +	flow_type = flow_type_mask(nfc->flow_type);
> +	tt = flow_type_to_traffic_type(flow_type);
>   	if (tt < 0)
>   		return tt;
>   
> @@ -911,10 +917,10 @@ static int mlx5e_set_rss_hash_opt(struct mlx5e_priv *priv,
>   	 *  on src IP, dest IP, TCP/UDP src port and TCP/UDP dest
>   	 *  port.
>   	 */
> -	if (nfc->flow_type != TCP_V4_FLOW &&
> -	    nfc->flow_type != TCP_V6_FLOW &&
> -	    nfc->flow_type != UDP_V4_FLOW &&
> -	    nfc->flow_type != UDP_V6_FLOW)
> +	if (flow_type != TCP_V4_FLOW &&
> +	    flow_type != TCP_V6_FLOW &&
> +	    flow_type != UDP_V4_FLOW &&
> +	    flow_type != UDP_V6_FLOW)
>   		return -EOPNOTSUPP;
>   
>   	if (nfc->data & ~(RXH_IP_SRC | RXH_IP_DST |
> @@ -931,7 +937,7 @@ static int mlx5e_set_rss_hash_opt(struct mlx5e_priv *priv,
>   		rx_hash_field |= MLX5_HASH_FIELD_SEL_L4_DPORT;
>   
>   	mutex_lock(&priv->state_lock);
> -	err = mlx5e_rx_res_rss_set_hash_fields(priv->rx_res, tt, rx_hash_field);
> +	err = mlx5e_rx_res_rss_set_hash_fields(priv->rx_res, tt, rx_hash_field, rss_idx);
>   	mutex_unlock(&priv->state_lock);
>   
>   	return err;
> @@ -940,14 +946,23 @@ static int mlx5e_set_rss_hash_opt(struct mlx5e_priv *priv,
>   static int mlx5e_get_rss_hash_opt(struct mlx5e_priv *priv,
>   				  struct ethtool_rxnfc *nfc)
>   {
> -	u32 hash_field = 0;
> +	int hash_field = 0;
>   	int tt;
> +	u32 flow_type = 0;
> +	u32 rss_idx = 0;

Please maintain the reversed Christmas tree.

> +
> +	if (nfc->flow_type & FLOW_RSS)
> +		rss_idx = nfc->rss_context;
>   
> -	tt = flow_type_to_traffic_type(nfc->flow_type);
> +	flow_type = flow_type_mask(nfc->flow_type);
> +	tt = flow_type_to_traffic_type(flow_type);
>   	if (tt < 0)
>   		return tt;
>   
> -	hash_field = mlx5e_rx_res_rss_get_hash_fields(priv->rx_res, tt);
> +	hash_field = mlx5e_rx_res_rss_get_hash_fields(priv->rx_res, tt, rss_idx);
> +	if (hash_field < 0)
> +		return hash_field;
> +
>   	nfc->data = 0;
>   
>   	if (hash_field & MLX5_HASH_FIELD_SEL_SRC_IP)

Re: [net 2/2] net/mlx5: Fix flowhash key set/get for custom RSS

[Joe Damato]

On Tue, Jul 25, 2023 at 12:59:32PM +0300, Tariq Toukan wrote:
>
>
> On 23/07/2023 18:06, Joe Damato wrote:
> >mlx5 flow hash field retrieval and set only worked on the default
> >RSS context, not custom RSS contexts.
> >
> >For example, before this patch attempting to retrieve the flow hash fields
> >for RSS context 1 fails:
> >
>
> Hi,
>
> You are adding new driver functionality, please take it through net-next.

Thanks for reviewing the code I sent; I made the changes you requested and
sent a v2, but through net-next this time.

Re: [net 0/2] rxfh with custom RSS fixes

[Edward Cree]

On 23/07/2023 16:06, Joe Damato wrote:
> Greetings:
> 
> While attempting to get the RX flow hash key for a custom RSS context on
> my mlx5 NIC, I got an error:
> 
> $ sudo ethtool -u eth1 rx-flow-hash tcp4 context 1
> Cannot get RX network flow hashing options: Invalid argument
> 
> I dug into this a bit and noticed two things:
> 
> 1. ETHTOOL_GRXFH supports custom RSS contexts, but ETHTOOL_SRXFH does
> not. I moved the copy logic out of ETHTOOL_GRXFH and into a helper so
> that both ETHTOOL_{G,S}RXFH now call it, which fixes ETHTOOL_SRXFH. This
> is patch 1/2.

As I see it, this is a new feature, not a fix, so belongs on net-next.
(No existing driver accepts FLOW_RSS in ETHTOOL_SRXFH's cmd->flow_type,
 which is just as well as if they did this would be a uABI break.)

Going forward, ETHTOOL_SRXFH will hopefully be integrated into the new
 RSS context kAPI I'm working on[1], so that we can have a new netlink
 uAPI for RSS configuration that's all in one place instead of the
 piecemeal-grown ethtool API with its backwards-compatible hacks.
But that will take a while, so I think this should go in even though
 it's technically an extension to legacy ethtool; it was part of the
 documented uAPI and userland implements it, it just never got
 implemented on the kernel side (because the initial driver with
 context support, sfc, didn't support SRXFH).

> 2. mlx5 defaulted to RSS context 0 for both ETHTOOL_{G,S}RXFH paths. I
> have modified the driver to support custom contexts for both paths. It
> is now possible to get and set the flow hash key for custom RSS contexts
> with mlx5. This is patch 2/2.

My feeling would be that this isn't a Fix either, but not my place to say.

-ed

[1]: https://lore.kernel.org/netdev/ecaae93a-d41d-4c3d-8e52-2800baa7080d@lunn.ch/T/

Re: [net 0/2] rxfh with custom RSS fixes

[Joe Damato]

On Mon, Jul 24, 2023 at 08:27:43PM +0100, Edward Cree wrote:
> On 23/07/2023 16:06, Joe Damato wrote:
> > Greetings:
> > 
> > While attempting to get the RX flow hash key for a custom RSS context on
> > my mlx5 NIC, I got an error:
> > 
> > $ sudo ethtool -u eth1 rx-flow-hash tcp4 context 1
> > Cannot get RX network flow hashing options: Invalid argument
> > 
> > I dug into this a bit and noticed two things:
> > 
> > 1. ETHTOOL_GRXFH supports custom RSS contexts, but ETHTOOL_SRXFH does
> > not. I moved the copy logic out of ETHTOOL_GRXFH and into a helper so
> > that both ETHTOOL_{G,S}RXFH now call it, which fixes ETHTOOL_SRXFH. This
> > is patch 1/2.
> 
> As I see it, this is a new feature, not a fix, so belongs on net-next.
> (No existing driver accepts FLOW_RSS in ETHTOOL_SRXFH's cmd->flow_type,
>  which is just as well as if they did this would be a uABI break.)
> 
> Going forward, ETHTOOL_SRXFH will hopefully be integrated into the new
>  RSS context kAPI I'm working on[1], so that we can have a new netlink
>  uAPI for RSS configuration that's all in one place instead of the
>  piecemeal-grown ethtool API with its backwards-compatible hacks.
> But that will take a while, so I think this should go in even though
>  it's technically an extension to legacy ethtool; it was part of the
>  documented uAPI and userland implements it, it just never got
>  implemented on the kernel side (because the initial driver with
>  context support, sfc, didn't support SRXFH).
> 
> > 2. mlx5 defaulted to RSS context 0 for both ETHTOOL_{G,S}RXFH paths. I
> > have modified the driver to support custom contexts for both paths. It
> > is now possible to get and set the flow hash key for custom RSS contexts
> > with mlx5. This is patch 2/2.
> 
> My feeling would be that this isn't a Fix either, but not my place to say.

Thanks for the context above; I'll let the Mellanox folks weigh in on what
they think about the code in the second patch before I proceed.

I suspect that you are probably right and that net-next might be a more
appropriate place for this. If the code is ack'd by Mellanox (and they
agree re: net-next), I can re-send this series to net-next with the Fixes
removed and the Ack's added.

Re: [net 0/2] rxfh with custom RSS fixes

[Jakub Kicinski]

On Mon, 24 Jul 2023 20:27:43 +0100 Edward Cree wrote:
> On 23/07/2023 16:06, Joe Damato wrote:
> > Greetings:
> > 
> > While attempting to get the RX flow hash key for a custom RSS context on
> > my mlx5 NIC, I got an error:
> > 
> > $ sudo ethtool -u eth1 rx-flow-hash tcp4 context 1
> > Cannot get RX network flow hashing options: Invalid argument
> > 
> > I dug into this a bit and noticed two things:
> > 
> > 1. ETHTOOL_GRXFH supports custom RSS contexts, but ETHTOOL_SRXFH does
> > not. I moved the copy logic out of ETHTOOL_GRXFH and into a helper so
> > that both ETHTOOL_{G,S}RXFH now call it, which fixes ETHTOOL_SRXFH. This
> > is patch 1/2.  
> 
> As I see it, this is a new feature, not a fix, so belongs on net-next.
> (No existing driver accepts FLOW_RSS in ETHTOOL_SRXFH's cmd->flow_type,
>  which is just as well as if they did this would be a uABI break.)
> 
> Going forward, ETHTOOL_SRXFH will hopefully be integrated into the new
>  RSS context kAPI I'm working on[1], so that we can have a new netlink
>  uAPI for RSS configuration that's all in one place instead of the
>  piecemeal-grown ethtool API with its backwards-compatible hacks.
> But that will take a while, so I think this should go in even though
>  it's technically an extension to legacy ethtool; it was part of the
>  documented uAPI and userland implements it, it just never got
>  implemented on the kernel side (because the initial driver with
>  context support, sfc, didn't support SRXFH).

What's the status on your work? Are you planning to split the RSS
config from ethtool or am I reading too much into what you said?

It'd be great to push the uAPI extensions back and make them
netlink-only, but we can't make Joe wait if it takes a long time
to finish up the basic conversion :(

Re: [net 0/2] rxfh with custom RSS fixes

[Edward Cree]

On 24/07/2023 23:08, Jakub Kicinski wrote:
> What's the status on your work? Are you planning to split the RSS
> config from ethtool or am I reading too much into what you said?

I was just thinking that when netlink dumps get added it would make
 sense to also extend the netlink version of SRSSH (which is what
 calls the rxfh_context ethtool_ops, via the misleadingly named
 ethtool_set_rxfh()) to include the hash fields setting that's
 currently done through ETHTOOL_SRXFH.  In which case I should add
 that data to struct ethtool_rxfh_context, and include it in the
 get/create/modify_rss_context ethtool_ops API.
Since it only occurred to me to consider that setting when I saw
 Joe's patches, I haven't really figured out yet how to go about
 the implementation of that.

More generally the status of my RSS work is that I've been umming
 and ahhing about that mutex you didn't like (I still think it's
 the Right Thing) so I've not made much progress with it.
And I should place on record that probably once I've gotten the
 kernel-driver API done I'll leave the netlink/uAPI stuff for
 someone else to do as I really don't have the relevant expertise.

> It'd be great to push the uAPI extensions back and make them
> netlink-only, but we can't make Joe wait if it takes a long time
> to finish up the basic conversion :(

Yeah as I said upthread I don't think we should make Joe wait, if
 he's got a use case that actually needs it (have you, Joe?  Or
 is it only GRXFH you need and the investigation just led you to
 notice SRXFH was broken?)

-ed

Re: [net 0/2] rxfh with custom RSS fixes

[Joe Damato]

On Tue, Jul 25, 2023 at 09:40:24AM +0100, Edward Cree wrote:
> On 24/07/2023 23:08, Jakub Kicinski wrote:
> > It'd be great to push the uAPI extensions back and make them
> > netlink-only, but we can't make Joe wait if it takes a long time
> > to finish up the basic conversion :(
> 
> Yeah as I said upthread I don't think we should make Joe wait, if
>  he's got a use case that actually needs it (have you, Joe?  Or
>  is it only GRXFH you need and the investigation just led you to
>  notice SRXFH was broken?)

In short, yes: I'd like to be able to get and set the flow hash keys for
custom RSS contexts on mlx5 which is why I included the patch to mlx5 in
this series... but to be fair I am just one user :) I think it's really
up to you all on the direction you want to go.

Longer story: I am working on building a system which relies on custom RSS
contexts, flow rules to associate flows with RSS contexts (and thus
specific sets of queues), and epoll based busy poll. It's a long story ;)

I had considered changing the flow hash key to see if I could alter the
behavior of the system I am working on, but I ran into both issues
immediately (GRXFH and FLOW_RSS was not supported by mlx5, and SRXFH
was broken) which led me down the path of attempting to fix both so that I
could get and set the flow hash keys. I thought the code might be useful,
so I submit it upstream -- I was not aware of the netlink work (but it
looks really useful!).

It seems that the Mellanox folks are OK with the proposed driver change,
so I am going to send a v2 rebased on net-next with their requested changes.

Re: [net 0/2] rxfh with custom RSS fixes

[Jakub Kicinski]

On Tue, 25 Jul 2023 09:40:24 +0100 Edward Cree wrote:
> More generally the status of my RSS work is that I've been umming
>  and ahhing about that mutex you didn't like (I still think it's
>  the Right Thing) so I've not made much progress with it.

I had a look at the code again, and I don't think the mutex is a deal
breaker. More of an aesthetic thing, so to speak. If you strongly
prefer to keep the mutex that's fine.