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

[Ophir Munk <ophirmu@mellanox.com>]

Hi,
PATCH v2 was sent

> -----Original Message-----
> From: Shahaf Shuler
> Sent: Wednesday, May 09, 2018 5:01 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
> 
> Wednesday, May 9, 2018 2:54 PM, Ophir Munk:
> > Subject: RE: [PATCH v1] net/mlx4: report on supported RSS hash
> > functions
> >
> > 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.
> 
> OK. Those can be 2 patches from the same series.
> 
> >
> > [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