Re: [dpdk-dev] [PATCH 2/2] net/mlx4: refactor RSS conversion functions

[Ophir Munk <ophirmu@mellanox.com>]

Hi Adrien,
I like the idea of having one conversion function from dpdk to verbs and vice versa.
I have some comments inline regarding the implementation.

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Tuesday, May 15, 2018 6:51 PM
> To: Shahaf Shuler <shahafs@mellanox.com>
> Cc: dev@dpdk.org; Ophir Munk <ophirmu@mellanox.com>
> Subject: [PATCH 2/2] net/mlx4: refactor RSS conversion functions
> 
> 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>
> ---
>  drivers/net/mlx4/mlx4_ethdev.c |   3 +-
>  drivers/net/mlx4/mlx4_flow.c   | 147 ++++++++++--------------------------
>  drivers/net/mlx4/mlx4_flow.h   |   4 +-
>  3 files changed, 44 insertions(+), 110 deletions(-)
> 
> 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);

Calling with parameters (priv, 0, 1) parameters in not as intuitive as calling with (priv, DEFAULT_RSS, DPDK_TO_VERBS)
Would you consider using definitions for the 0, 1 parameters to increase readability?

>  }
> 
>  /**
> diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c
> index ebc9eeb8b..90d206b55 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,76 @@ 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[] = {
> +	enum {
> +		INNER, IPV4, IPV6, TCP, UDP,
> +		IPV4_TCP, IPV4_UDP, IPV6_TCP, IPV6_UDP,
> +	};
> +	static const uint64_t dpdk[] = {
> +		[INNER] = 0,
>  		[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),
> +			  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 |
> +			      ETH_RSS_IPV6_TCP_EX),
> +		[IPV6_UDP] = (ETH_RSS_NONFRAG_IPV6_UDP |
> +			      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,
>  		[IPV6] = IBV_RX_HASH_SRC_IPV6 | IBV_RX_HASH_DST_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_UDP] = verbs[IPV6] | verbs[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) {
> +		if (!verbs_to_dpdk)
> +			return priv->hw_rss_sup;
> +		types = priv->hw_rss_sup;
> +	}
> +	for (i = 0; i != RTE_DIM(dpdk); ++i)
>  		if (types & 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;
>  }
> 

1. I wonder if [INNER] case is correct. Assuming we are given IBV_RX_HASH_INNER I think we should return with error ENOTSUP.
However in the implementation above we return 0 as a valid DPDK RSS value.
Returning a 0 value is also confusing since it is used in dpdk as "Best effort" RSS HF.

2. Assuming we are given = IBV_RX_HASH_SRC_IPV4 alone (without IBV_RX_HASH_DST_IPV4) I think we should return with error ENOSUP since mlx4 does not support a hash based on SRC IPV4 alone. However in the implementation above we return a valid answer of all the flags from [IPV4], [IPV4_TCP] and [IPV4_UDP]. Can you please explain this logic?
The same question repeats for all other "_DST_" only or "_SRC_" only alone constants.

3. Given "IBV_RX_HASH_SRC_PORT_TCP | IBV_RX_HASH_DST_PORT_TCP" alone we are returning a DPDK reply which includes TCP flags + all IPV4 and IPV6 flags.  Can you please explain this logic?

As mentioned I like the idea of one function to transform either direction between verbs and dpdk but please explain the current implementation following the questions above.

>  /**
> - * 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 +825,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,
> --
> 2.11.0