Re: [dpdk-dev] [PATCH v1] net/mlx4: report on supported RSS hash functions

[Ophir Munk <ophirmu@mellanox.com>]

Hi Shahaf,

> -----Original Message-----
> From: Shahaf Shuler
> Sent: Wednesday, May 09, 2018 12:39 PM
> To: Ophir Munk <ophirmu@mellanox.com>; dev@dpdk.org; Adrien
> Mazarguil <adrien.mazarguil@6wind.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> <olgas@mellanox.com>
> Subject: RE: [PATCH v1] net/mlx4: report on supported RSS hash functions
> 
> Hi Ophir,
> 
> Tuesday, May 8, 2018 6:43 PM, Ophir Munk:
> > Subject: [PATCH v1] net/mlx4: report on supported RSS hash functions
> >
> > Report on 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>
> > ---
> >  drivers/net/mlx4/mlx4_ethdev.c | 51
> > ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >
> > diff --git a/drivers/net/mlx4/mlx4_ethdev.c
> > b/drivers/net/mlx4/mlx4_ethdev.c index 9a76670..2a1533c 100644
> > --- a/drivers/net/mlx4/mlx4_ethdev.c
> > +++ b/drivers/net/mlx4/mlx4_ethdev.c
> > @@ -545,6 +545,55 @@ mlx4_mac_addr_set(struct rte_eth_dev *dev,
> struct
> > ether_addr *mac_addr)  }
> >
> >  /**
> > + * 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
> > + *   A valid dpdk RSS hash fields supported by mlx4 (may return 0)
> > + */
> > +static 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),
> > +	};
> 
> Since above are constants, why not defining a global array of structs
> containing the ibv_hash and the equivalent dpdk_hash, instead of recreating
> it for each call?

The array is recreated because it should have been declared as "static const" rather than just "const". I prefer this fix.
Alternatively it could have been defined globally outside of the function as suggested which would have avoided recreation as well.
All of those claims are confirmed by inspecting the assembly code of the above alternatives.

> There is a similar concept on mlx5_flow.c:
> 
> /* Initialization data for hash RX queues. */
>  const struct hash_rxq_init hash_rxq_init[] = {
>          [HASH_RXQ_TCPV4] = {
>                  .hash_fields = (IBV_RX_HASH_SRC_IPV4 |
>                                  IBV_RX_HASH_DST_IPV4 |
>                                  IBV_RX_HASH_SRC_PORT_TCP |
>                                  IBV_RX_HASH_DST_PORT_TCP),
>                  .dpdk_rss_hf = ETH_RSS_NONFRAG_IPV4_TCP,
>                  .flow_priority = 0,
>                  .ip_version = MLX5_IPV4,
>          },
> 

I was inspired from "the reversed" function in mlx4_flow.c which defines constants inside the function as well, see [1].
To fix the constants recreation both functions should be handled together.

I suggest:
1. Moving mlx4_ibv_to_dpdk_rss_types() function as is from mlx4_ether.c to mlx4_flow.c so it will be adjacent to mlx4_conv_rss_types() function.
2. Sending a new patch that avoids constants recreation in the 2 functions. 

[1]
uint64_t
mlx4_conv_rss_types(struct priv *priv, uint64_t types)
{
	enum { IPV4, IPV6, TCP, UDP, };
	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),
	};
	const uint64_t out[RTE_DIM(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,
	};
	uint64_t seen = 0;
	uint64_t conv = 0;
	unsigned int i;

	if (types == (uint64_t)-1)
		return priv->hw_rss_sup;
	for (i = 0; i != RTE_DIM(in); ++i)
		if (types & in[i]) {
			seen |= types & in[i];
			conv |= out[i];
		}
	if ((conv & priv->hw_rss_sup) == conv && !(types & ~seen))
		return conv;
	rte_errno = ENOTSUP;
	return (uint64_t)-1;
}

> > +	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;
> > +}
> > +
> > +/**
> >   * DPDK callback to get information about the device.
> >   *
> >   * @param dev
> > @@ -587,6 +636,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(
> > +			priv->hw_rss_sup);
> >  }
> >
> >  /**
> > --
> > 2.7.4