diff mbox

[dpdk-dev,v2,2/2] net/mlx4: refactor RSS conversion functions

Message ID 20180521154829.6297-2-adrien.mazarguil@6wind.com (mailing list archive)
State Accepted, archived
Delegated to: Shahaf Shuler
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Adrien Mazarguil May 21, 2018, 3:50 p.m. UTC 
  Since commit 97b2217ae5bc ("net/mlx4: advertise supported RSS hash
functions"), this PMD includes two similar-looking functions that convert
RSS hash fields between Verbs and DPDK formats.

This patch refactors them as a single two-way function and gets rid of
redundant helper macros.

Note the loss of the "static" keyword on the out[] (now verbs[]) array
introduced by commit cbd737416c34 ("net/mlx4: avoid constant recreations in
function") is what prevents the reliance on macro definitions for static
initializers at the expense of a few extra instructions. An acceptable
trade-off given this function is not involved in data plane operations.

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Cc: Ophir Munk <ophirmu@mellanox.com>

--

v2 changes:

- Split hash types on the DPDK side and added corresponding Verbs entries
  for a proper 1:1 mapping between them.
- Replaced logical AND ("any of") with exact match in loop on array
  entries.
- The combination of these changes address the issue where e.g. enabling
  only IPv4 on the Verbs side return IPv4 + TCP + UDP on the DPDK side of
  the conversion.
---
 drivers/net/mlx4/mlx4_ethdev.c |   3 +-
 drivers/net/mlx4/mlx4_flow.c   | 170 ++++++++++++------------------------
 drivers/net/mlx4/mlx4_flow.h   |   4 +-
 3 files changed, 60 insertions(+), 117 deletions(-)

Comments

Ferruh Yigit May 22, 2018, 9:41 a.m. UTC | #1 
On 5/21/2018 4:50 PM, Adrien Mazarguil wrote:
> Since commit 97b2217ae5bc ("net/mlx4: advertise supported RSS hash
> functions"), this PMD includes two similar-looking functions that convert
> RSS hash fields between Verbs and DPDK formats.
> 
> This patch refactors them as a single two-way function and gets rid of
> redundant helper macros.
> 
> Note the loss of the "static" keyword on the out[] (now verbs[]) array
> introduced by commit cbd737416c34 ("net/mlx4: avoid constant recreations in
> function") is what prevents the reliance on macro definitions for static
> initializers at the expense of a few extra instructions. An acceptable
> trade-off given this function is not involved in data plane operations.
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Cc: Ophir Munk <ophirmu@mellanox.com>

<...>

> @@ -119,106 +84,85 @@ struct mlx4_drop {
>   * @param priv
>   *   Pointer to private structure.
>   * @param types
> - *   Hash types in DPDK format (see struct rte_eth_rss_conf).
> + *   Depending on @p verbs_to_dpdk, hash types in either DPDK (see struct
> + *   rte_eth_rss_conf) or Verbs format.
> + * @param verbs_to_dpdk
> + *   A zero value converts @p types from DPDK to Verbs, a nonzero value
> + *   performs the reverse operation.
>   *
>   * @return
> - *   A valid Verbs RSS hash fields mask for mlx4 on success, (uint64_t)-1
> - *   otherwise and rte_errno is set.
> + *   Converted RSS hash fields on success, (uint64_t)-1 otherwise and
> + *   rte_errno is set.
>   */
>  uint64_t
> -mlx4_conv_rss_types(struct priv *priv, uint64_t types)
> +mlx4_conv_rss_types(struct priv *priv, uint64_t types, int verbs_to_dpdk)
>  {
> -	enum { IPV4, IPV6, TCP, UDP, };
> -	static const uint64_t in[] = {
> -		[IPV4] = (ETH_RSS_IPV4 |
> -			  ETH_RSS_FRAG_IPV4 |
> -			  ETH_RSS_NONFRAG_IPV4_TCP |
> -			  ETH_RSS_NONFRAG_IPV4_UDP |
> -			  ETH_RSS_NONFRAG_IPV4_OTHER),
> -		[IPV6] = (ETH_RSS_IPV6 |
> -			  ETH_RSS_FRAG_IPV6 |
> -			  ETH_RSS_NONFRAG_IPV6_TCP |
> -			  ETH_RSS_NONFRAG_IPV6_UDP |
> -			  ETH_RSS_NONFRAG_IPV6_OTHER |
> -			  ETH_RSS_IPV6_EX |
> -			  ETH_RSS_IPV6_TCP_EX |
> -			  ETH_RSS_IPV6_UDP_EX),
> -		[TCP] = (ETH_RSS_NONFRAG_IPV4_TCP |
> -			 ETH_RSS_NONFRAG_IPV6_TCP |
> -			 ETH_RSS_IPV6_TCP_EX),
> -		[UDP] = (ETH_RSS_NONFRAG_IPV4_UDP |
> -			 ETH_RSS_NONFRAG_IPV6_UDP |
> -			 ETH_RSS_IPV6_UDP_EX),
> +	enum {
> +		INNER,
> +		IPV4, IPV4_1, IPV4_2, IPV6, IPV6_1, IPV6_2, IPV6_3,
> +		TCP, UDP,
> +		IPV4_TCP, IPV4_UDP, IPV6_TCP, IPV6_TCP_1, IPV6_UDP, IPV6_UDP_1,
> +	};
> +	static const uint64_t dpdk[] = {
> +		[INNER] = 0,
> +		[IPV4] = ETH_RSS_IPV4,
> +		[IPV4_1] = ETH_RSS_FRAG_IPV4,
> +		[IPV4_2] = ETH_RSS_NONFRAG_IPV4_OTHER,
> +		[IPV6] = ETH_RSS_IPV6,
> +		[IPV6_1] = ETH_RSS_FRAG_IPV6,
> +		[IPV6_2] = ETH_RSS_NONFRAG_IPV6_OTHER,
> +		[IPV6_3] = ETH_RSS_IPV6_EX,
> +		[TCP] = 0,
> +		[UDP] = 0,
> +		[IPV4_TCP] = ETH_RSS_NONFRAG_IPV4_TCP,
> +		[IPV4_UDP] = ETH_RSS_NONFRAG_IPV4_UDP,
> +		[IPV6_TCP] = ETH_RSS_NONFRAG_IPV6_TCP,
> +		[IPV6_TCP_1] = ETH_RSS_IPV6_TCP_EX,
> +		[IPV6_UDP] = ETH_RSS_NONFRAG_IPV6_UDP,
> +		[IPV6_UDP_1] = ETH_RSS_IPV6_UDP_EX,
>  	};
> -	static const uint64_t out[RTE_DIM(in)] = {
> +	const uint64_t verbs[RTE_DIM(dpdk)] = {
> +		[INNER] = IBV_RX_HASH_INNER,
>  		[IPV4] = IBV_RX_HASH_SRC_IPV4 | IBV_RX_HASH_DST_IPV4,
> +		[IPV4_1] = verbs[IPV4],
> +		[IPV4_2] = verbs[IPV4],
>  		[IPV6] = IBV_RX_HASH_SRC_IPV6 | IBV_RX_HASH_DST_IPV6,
> +		[IPV6_1] = verbs[IPV6],
> +		[IPV6_2] = verbs[IPV6],
> +		[IPV6_3] = verbs[IPV6],
>  		[TCP] = IBV_RX_HASH_SRC_PORT_TCP | IBV_RX_HASH_DST_PORT_TCP,
>  		[UDP] = IBV_RX_HASH_SRC_PORT_UDP | IBV_RX_HASH_DST_PORT_UDP,
> +		[IPV4_TCP] = verbs[IPV4] | verbs[TCP],

This gives following build error with ICC [1]. Is it guarantied that in above
assignment the executing order will be the same order with code?


[1]
.../dpdk/drivers/net/mlx4/mlx4_flow.c(135): error #592: variable "verbs" is used
before its value is set
                [IPV4_TCP] = verbs[IPV4] | verbs[TCP],
                             ^


> +		[IPV4_UDP] = verbs[IPV4] | verbs[UDP],
> +		[IPV6_TCP] = verbs[IPV6] | verbs[TCP],
> +		[IPV6_TCP_1] = verbs[IPV6_TCP],
> +		[IPV6_UDP] = verbs[IPV6] | verbs[UDP],
> +		[IPV6_UDP_1] = verbs[IPV6_UDP],
>  	};

<...>
Adrien Mazarguil May 22, 2018, 9:58 a.m. UTC | #2 
On Tue, May 22, 2018 at 10:41:35AM +0100, Ferruh Yigit wrote:
> On 5/21/2018 4:50 PM, Adrien Mazarguil wrote:
> > Since commit 97b2217ae5bc ("net/mlx4: advertise supported RSS hash
> > functions"), this PMD includes two similar-looking functions that convert
> > RSS hash fields between Verbs and DPDK formats.
> > 
> > This patch refactors them as a single two-way function and gets rid of
> > redundant helper macros.
> > 
> > Note the loss of the "static" keyword on the out[] (now verbs[]) array
> > introduced by commit cbd737416c34 ("net/mlx4: avoid constant recreations in
> > function") is what prevents the reliance on macro definitions for static
> > initializers at the expense of a few extra instructions. An acceptable
> > trade-off given this function is not involved in data plane operations.
<snip>
> > -	static const uint64_t out[RTE_DIM(in)] = {
> > +	const uint64_t verbs[RTE_DIM(dpdk)] = {
> > +		[INNER] = IBV_RX_HASH_INNER,
> >  		[IPV4] = IBV_RX_HASH_SRC_IPV4 | IBV_RX_HASH_DST_IPV4,
> > +		[IPV4_1] = verbs[IPV4],
> > +		[IPV4_2] = verbs[IPV4],
> >  		[IPV6] = IBV_RX_HASH_SRC_IPV6 | IBV_RX_HASH_DST_IPV6,
> > +		[IPV6_1] = verbs[IPV6],
> > +		[IPV6_2] = verbs[IPV6],
> > +		[IPV6_3] = verbs[IPV6],
> >  		[TCP] = IBV_RX_HASH_SRC_PORT_TCP | IBV_RX_HASH_DST_PORT_TCP,
> >  		[UDP] = IBV_RX_HASH_SRC_PORT_UDP | IBV_RX_HASH_DST_PORT_UDP,
> > +		[IPV4_TCP] = verbs[IPV4] | verbs[TCP],
> 
> This gives following build error with ICC [1]. Is it guarantied that in above
> assignment the executing order will be the same order with code?
> 
> [1]
> .../dpdk/drivers/net/mlx4/mlx4_flow.c(135): error #592: variable "verbs" is used
> before its value is set
>                 [IPV4_TCP] = verbs[IPV4] | verbs[TCP],

Didn't see this error with GCC nor clang (compilation and validation OK with
both), however ICC is correct, initialization order is not guaranteed
here. This shortcut is unsafe, I'll have to come up with something else.
diff mbox

Patch

diff --git a/drivers/net/mlx4/mlx4_ethdev.c b/drivers/net/mlx4/mlx4_ethdev.c
index 0a9c2e2f5..30deb3ef0 100644
--- a/drivers/net/mlx4/mlx4_ethdev.c
+++ b/drivers/net/mlx4/mlx4_ethdev.c
@@ -587,8 +587,7 @@  mlx4_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info)
 			ETH_LINK_SPEED_20G |
 			ETH_LINK_SPEED_40G |
 			ETH_LINK_SPEED_56G;
-	info->flow_type_rss_offloads =
-		mlx4_ibv_to_rss_types(priv->hw_rss_sup);
+	info->flow_type_rss_offloads = mlx4_conv_rss_types(priv, 0, 1);
 }
 
 /**
diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c
index ebc9eeb8b..ddb7108dc 100644
--- a/drivers/net/mlx4/mlx4_flow.c
+++ b/drivers/net/mlx4/mlx4_flow.c
@@ -42,41 +42,6 @@ 
 #include "mlx4_rxtx.h"
 #include "mlx4_utils.h"
 
-/** IBV supported RSS hash functions combinations */
-#define MLX4_IBV_IPV4_HF ( \
-	IBV_RX_HASH_SRC_IPV4 | \
-	IBV_RX_HASH_DST_IPV4)
-#define MLX4_IBV_IPV6_HF ( \
-	IBV_RX_HASH_SRC_IPV6 | \
-	IBV_RX_HASH_DST_IPV6)
-#define MLX4_IBV_TCP_HF ( \
-	IBV_RX_HASH_SRC_PORT_TCP | \
-	IBV_RX_HASH_DST_PORT_TCP)
-#define MLX4_IBV_UDP_HF ( \
-	IBV_RX_HASH_SRC_PORT_UDP | \
-	IBV_RX_HASH_DST_PORT_UDP)
-
-/** Supported RSS hash functions combinations */
-#define MLX4_RSS_IPV4_HF ( \
-	ETH_RSS_IPV4 | \
-	ETH_RSS_FRAG_IPV4 | \
-	ETH_RSS_NONFRAG_IPV4_OTHER)
-#define MLX4_RSS_IPV6_HF ( \
-	ETH_RSS_IPV6 | \
-	ETH_RSS_FRAG_IPV6 | \
-	ETH_RSS_NONFRAG_IPV6_OTHER | \
-	ETH_RSS_IPV6_EX)
-#define MLX4_RSS_IPV4_TCP_HF ( \
-	ETH_RSS_NONFRAG_IPV4_TCP)
-#define MLX4_RSS_IPV6_TCP_HF ( \
-	ETH_RSS_NONFRAG_IPV6_TCP | \
-	ETH_RSS_IPV6_TCP_EX)
-#define MLX4_RSS_IPV4_UDP_HF ( \
-	ETH_RSS_NONFRAG_IPV4_UDP)
-#define MLX4_RSS_IPV6_UDP_HF ( \
-	ETH_RSS_NONFRAG_IPV6_UDP | \
-	ETH_RSS_IPV6_UDP_EX)
-
 /** Static initializer for a list of subsequent item types. */
 #define NEXT_ITEM(...) \
 	(const enum rte_flow_item_type []){ \
@@ -111,7 +76,7 @@  struct mlx4_drop {
 };
 
 /**
- * Convert DPDK RSS hash types to their Verbs equivalent.
+ * Convert supported RSS hash field types between DPDK and Verbs formats.
  *
  * This function returns the supported (default) set when @p types has
  * special value 0.
@@ -119,106 +84,85 @@  struct mlx4_drop {
  * @param priv
  *   Pointer to private structure.
  * @param types
- *   Hash types in DPDK format (see struct rte_eth_rss_conf).
+ *   Depending on @p verbs_to_dpdk, hash types in either DPDK (see struct
+ *   rte_eth_rss_conf) or Verbs format.
+ * @param verbs_to_dpdk
+ *   A zero value converts @p types from DPDK to Verbs, a nonzero value
+ *   performs the reverse operation.
  *
  * @return
- *   A valid Verbs RSS hash fields mask for mlx4 on success, (uint64_t)-1
- *   otherwise and rte_errno is set.
+ *   Converted RSS hash fields on success, (uint64_t)-1 otherwise and
+ *   rte_errno is set.
  */
 uint64_t
-mlx4_conv_rss_types(struct priv *priv, uint64_t types)
+mlx4_conv_rss_types(struct priv *priv, uint64_t types, int verbs_to_dpdk)
 {
-	enum { IPV4, IPV6, TCP, UDP, };
-	static const uint64_t in[] = {
-		[IPV4] = (ETH_RSS_IPV4 |
-			  ETH_RSS_FRAG_IPV4 |
-			  ETH_RSS_NONFRAG_IPV4_TCP |
-			  ETH_RSS_NONFRAG_IPV4_UDP |
-			  ETH_RSS_NONFRAG_IPV4_OTHER),
-		[IPV6] = (ETH_RSS_IPV6 |
-			  ETH_RSS_FRAG_IPV6 |
-			  ETH_RSS_NONFRAG_IPV6_TCP |
-			  ETH_RSS_NONFRAG_IPV6_UDP |
-			  ETH_RSS_NONFRAG_IPV6_OTHER |
-			  ETH_RSS_IPV6_EX |
-			  ETH_RSS_IPV6_TCP_EX |
-			  ETH_RSS_IPV6_UDP_EX),
-		[TCP] = (ETH_RSS_NONFRAG_IPV4_TCP |
-			 ETH_RSS_NONFRAG_IPV6_TCP |
-			 ETH_RSS_IPV6_TCP_EX),
-		[UDP] = (ETH_RSS_NONFRAG_IPV4_UDP |
-			 ETH_RSS_NONFRAG_IPV6_UDP |
-			 ETH_RSS_IPV6_UDP_EX),
+	enum {
+		INNER,
+		IPV4, IPV4_1, IPV4_2, IPV6, IPV6_1, IPV6_2, IPV6_3,
+		TCP, UDP,
+		IPV4_TCP, IPV4_UDP, IPV6_TCP, IPV6_TCP_1, IPV6_UDP, IPV6_UDP_1,
+	};
+	static const uint64_t dpdk[] = {
+		[INNER] = 0,
+		[IPV4] = ETH_RSS_IPV4,
+		[IPV4_1] = ETH_RSS_FRAG_IPV4,
+		[IPV4_2] = ETH_RSS_NONFRAG_IPV4_OTHER,
+		[IPV6] = ETH_RSS_IPV6,
+		[IPV6_1] = ETH_RSS_FRAG_IPV6,
+		[IPV6_2] = ETH_RSS_NONFRAG_IPV6_OTHER,
+		[IPV6_3] = ETH_RSS_IPV6_EX,
+		[TCP] = 0,
+		[UDP] = 0,
+		[IPV4_TCP] = ETH_RSS_NONFRAG_IPV4_TCP,
+		[IPV4_UDP] = ETH_RSS_NONFRAG_IPV4_UDP,
+		[IPV6_TCP] = ETH_RSS_NONFRAG_IPV6_TCP,
+		[IPV6_TCP_1] = ETH_RSS_IPV6_TCP_EX,
+		[IPV6_UDP] = ETH_RSS_NONFRAG_IPV6_UDP,
+		[IPV6_UDP_1] = ETH_RSS_IPV6_UDP_EX,
 	};
-	static const uint64_t out[RTE_DIM(in)] = {
+	const uint64_t verbs[RTE_DIM(dpdk)] = {
+		[INNER] = IBV_RX_HASH_INNER,
 		[IPV4] = IBV_RX_HASH_SRC_IPV4 | IBV_RX_HASH_DST_IPV4,
+		[IPV4_1] = verbs[IPV4],
+		[IPV4_2] = verbs[IPV4],
 		[IPV6] = IBV_RX_HASH_SRC_IPV6 | IBV_RX_HASH_DST_IPV6,
+		[IPV6_1] = verbs[IPV6],
+		[IPV6_2] = verbs[IPV6],
+		[IPV6_3] = verbs[IPV6],
 		[TCP] = IBV_RX_HASH_SRC_PORT_TCP | IBV_RX_HASH_DST_PORT_TCP,
 		[UDP] = IBV_RX_HASH_SRC_PORT_UDP | IBV_RX_HASH_DST_PORT_UDP,
+		[IPV4_TCP] = verbs[IPV4] | verbs[TCP],
+		[IPV4_UDP] = verbs[IPV4] | verbs[UDP],
+		[IPV6_TCP] = verbs[IPV6] | verbs[TCP],
+		[IPV6_TCP_1] = verbs[IPV6_TCP],
+		[IPV6_UDP] = verbs[IPV6] | verbs[UDP],
+		[IPV6_UDP_1] = verbs[IPV6_UDP],
 	};
+	const uint64_t *in = verbs_to_dpdk ? verbs : dpdk;
+	const uint64_t *out = verbs_to_dpdk ? dpdk : verbs;
 	uint64_t seen = 0;
 	uint64_t conv = 0;
 	unsigned int i;
 
-	if (!types)
-		return priv->hw_rss_sup;
-	for (i = 0; i != RTE_DIM(in); ++i)
-		if (types & in[i]) {
+	if (!types) {
+		if (!verbs_to_dpdk)
+			return priv->hw_rss_sup;
+		types = priv->hw_rss_sup;
+	}
+	for (i = 0; i != RTE_DIM(dpdk); ++i)
+		if (in[i] && (types & in[i]) == in[i]) {
 			seen |= types & in[i];
 			conv |= out[i];
 		}
-	if ((conv & priv->hw_rss_sup) == conv && !(types & ~seen))
+	if ((verbs_to_dpdk || (conv & priv->hw_rss_sup) == conv) &&
+	    !(types & ~seen))
 		return conv;
 	rte_errno = ENOTSUP;
 	return (uint64_t)-1;
 }
 
 /**
- * Convert verbs RSS types to their DPDK equivalents.
- *
- * This function returns a group of RSS DPDK types given their equivalent group
- * of verbs types.
- * For example both source IPv4 and destination IPv4 verbs types are converted
- * into their equivalent RSS group types. If each of these verbs types existed
- * exclusively - no conversion would take place.
- *
- * @param types
- *   RSS hash types in verbs format.
- *
- * @return
- *   DPDK RSS hash fields supported by mlx4.
- */
-uint64_t
-mlx4_ibv_to_rss_types(uint64_t types)
-{
-	enum { IPV4, IPV6, IPV4_TCP, IPV6_TCP, IPV4_UDP, IPV6_UDP};
-
-	static const uint64_t in[] = {
-		[IPV4] = MLX4_IBV_IPV4_HF,
-		[IPV6] = MLX4_IBV_IPV6_HF,
-		[IPV4_TCP] = MLX4_IBV_IPV4_HF | MLX4_IBV_TCP_HF,
-		[IPV6_TCP] = MLX4_IBV_IPV6_HF | MLX4_IBV_TCP_HF,
-		[IPV4_UDP] = MLX4_IBV_IPV4_HF | MLX4_IBV_UDP_HF,
-		[IPV6_UDP] = MLX4_IBV_IPV6_HF | MLX4_IBV_UDP_HF,
-	};
-	static const uint64_t out[RTE_DIM(in)] = {
-		[IPV4] = MLX4_RSS_IPV4_HF,
-		[IPV6] = MLX4_RSS_IPV6_HF,
-		[IPV4_TCP] = MLX4_RSS_IPV4_HF | MLX4_RSS_IPV4_TCP_HF,
-		[IPV6_TCP] = MLX4_RSS_IPV6_HF | MLX4_RSS_IPV6_TCP_HF,
-		[IPV4_UDP] = MLX4_RSS_IPV4_HF | MLX4_RSS_IPV4_UDP_HF,
-		[IPV6_UDP] = MLX4_RSS_IPV6_HF | MLX4_RSS_IPV6_UDP_HF,
-	};
-	uint64_t conv = 0;
-	unsigned int i;
-
-	for (i = 0; i != RTE_DIM(in); ++i)
-		if ((types & in[i]) == in[i])
-			conv |= out[i];
-	return conv;
-}
-
-/**
  * Merge Ethernet pattern item into flow rule handle.
  *
  * Additional mlx4-specific constraints on supported fields:
@@ -890,7 +834,7 @@  mlx4_flow_prepare(struct priv *priv,
 				goto exit_action_not_supported;
 			}
 			rte_errno = 0;
-			fields = mlx4_conv_rss_types(priv, rss->types);
+			fields = mlx4_conv_rss_types(priv, rss->types, 0);
 			if (fields == (uint64_t)-1 && rte_errno) {
 				msg = "unsupported RSS hash type requested";
 				goto exit_action_not_supported;
diff --git a/drivers/net/mlx4/mlx4_flow.h b/drivers/net/mlx4/mlx4_flow.h
index 2e82903bd..2917ebe95 100644
--- a/drivers/net/mlx4/mlx4_flow.h
+++ b/drivers/net/mlx4/mlx4_flow.h
@@ -48,8 +48,8 @@  struct rte_flow {
 
 /* mlx4_flow.c */
 
-uint64_t mlx4_conv_rss_types(struct priv *priv, uint64_t types);
-uint64_t mlx4_ibv_to_rss_types(uint64_t ibv_rss_types);
+uint64_t mlx4_conv_rss_types(struct priv *priv, uint64_t types,
+			     int verbs_to_dpdk);
 int mlx4_flow_sync(struct priv *priv, struct rte_flow_error *error);
 void mlx4_flow_clean(struct priv *priv);
 int mlx4_filter_ctrl(struct rte_eth_dev *dev,