Re: [dpdk-dev] [PATCH v2 1/2] net/mlx4: advertise supported RSS hash functions

[Shahaf Shuler <shahafs@mellanox.com>]

Hi Ophir, 

Thursday, May 10, 2018 1:27 AM, Ophir Munk:
> Subject: [PATCH v2 1/2] net/mlx4: advertise supported RSS hash functions
> 
> Advertise mlx4 supported RSS functions as part of dev_infos_get callback.
> Previous to this commit RSS support was reported as none. Since the
> introduction of [1] it is required that all RSS configurations will be verified.
> 
> [1] commit 8863a1fbfc66 ("ethdev: add supported hash function check")
> 
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> ---
> v1:
> Initial release
> v2:
> Split into 2 commits following reviews
> 
>  drivers/net/mlx4/mlx4_ethdev.c |  3 +++
>  drivers/net/mlx4/mlx4_flow.c   | 49
> ++++++++++++++++++++++++++++++++++++++++++
>  drivers/net/mlx4/mlx4_flow.h   |  1 +
>  3 files changed, 53 insertions(+)
> 
> diff --git a/drivers/net/mlx4/mlx4_ethdev.c
> b/drivers/net/mlx4/mlx4_ethdev.c index 9a76670..8fbb781 100644
> --- a/drivers/net/mlx4/mlx4_ethdev.c
> +++ b/drivers/net/mlx4/mlx4_ethdev.c
> @@ -544,6 +544,7 @@ mlx4_mac_addr_set(struct rte_eth_dev *dev, struct
> ether_addr *mac_addr)
>  	return mlx4_mac_addr_add(dev, mac_addr, 0, 0);  }
> 
> +

Remove this extra line. 

>  /**
>   * DPDK callback to get information about the device.
>   *
> @@ -587,6 +588,8 @@ 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_dpdk_rss_types(

I think better to ident mlx4_ibv_to_dpdk_rss_types to the line below along with hw_rss_sup 

> +			priv->hw_rss_sup);


>  }
> 
>  /**
> diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c
> index 397a150..41dac16 100644
> --- a/drivers/net/mlx4/mlx4_flow.c
> +++ b/drivers/net/mlx4/mlx4_flow.c
> @@ -134,6 +134,55 @@ mlx4_conv_rss_types(struct priv *priv, uint64_t
> types)  }
> 
>  /**
> + * 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

Missing period

> + *
> + * @return
> + *   A valid dpdk RSS hash fields supported by mlx4 (may return 0)

DPDK RSS hash fields supported by mlx4.  is enough. 

> + */
> +uint64_t
> +mlx4_ibv_to_dpdk_rss_types(uint64_t types) {
> +	enum { IPV4, IPV6, TCP, UDP, };
> +	const uint64_t in[] = {
> +		[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,
> +	};
> +	const uint64_t out[RTE_DIM(in)] = {
> +		[IPV4] = (ETH_RSS_IPV4 |
> +			  ETH_RSS_FRAG_IPV4 |
> +			  ETH_RSS_NONFRAG_IPV4_OTHER),
> +		[IPV6] = (ETH_RSS_IPV6 |
> +			  ETH_RSS_FRAG_IPV6 |
> +			  ETH_RSS_NONFRAG_IPV6_OTHER |
> +			  ETH_RSS_IPV6_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),
> +	};
> +	uint64_t conv = 0;
> +	unsigned int i;
> +
> +	for (i = 0; i != RTE_DIM(in); ++i)
> +		if ((types & in[i]) == in[i])

There is an issue with this logic. Sorry for not noticing in the previous version. 

Let's say the caps report support for only UDP, no IP. On the logic above it will report both 
ETH_RSS_NONFRAG_IPV4_UDP
ETH_RSS_NONFRAG_IPV6_UDP

It looks like for the L4 to be valid RSS hash it needs the l3 to be valid. 

> +			conv |= out[i];
> +	return conv;
> +}
> +
> +/**
>   * Merge Ethernet pattern item into flow rule handle.
>   *
>   * Additional mlx4-specific constraints on supported fields:
> diff --git a/drivers/net/mlx4/mlx4_flow.h b/drivers/net/mlx4/mlx4_flow.h
> index 2c8dff3..ec990df 100644
> --- a/drivers/net/mlx4/mlx4_flow.h
> +++ b/drivers/net/mlx4/mlx4_flow.h
> @@ -49,6 +49,7 @@ struct rte_flow {
>  /* mlx4_flow.c */
> 
>  uint64_t mlx4_conv_rss_types(struct priv *priv, uint64_t rss_hf);
> +uint64_t mlx4_ibv_to_dpdk_rss_types(uint64_t ibv_rss_types);
>  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.7.4