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

[Ophir Munk <ophirmu@mellanox.com>]

Hi,
PATCH v3 was sent

> -----Original Message-----
> From: Shahaf Shuler
> Sent: Thursday, May 10, 2018 8:20 AM
> 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 v2 1/2] net/mlx4: advertise supported RSS hash
> functions
> 
> 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