Re: [dpdk-dev] [RFC,v3] ethdev: extend RSS offload types

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Tue, Jul 30, 2019 at 06:06:56AM +0000, Ori Kam wrote:
> Hi Simei,
> 
> 
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of simei
> > Sent: Monday, July 29, 2019 5:44 AM
> > To: qi.z.zhang@intel.com; jingjing.wu@intel.com; ferruh.yigit@intel.com;
> > Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Cc: dev@dpdk.org; simei.su@intel.com
> > Subject: [dpdk-dev] [RFC,v3] ethdev: extend RSS offload types
> > 
> > From: Simei Su <simei.su@intel.com>
> > 
> > Make it easier to represent to define macro values as (1ULL << ###).
> > 
> > This RFC reserves several bits as input set selection from bottom
> > of the 64 bits. The flow type is combined with input set to
> > represent rss types.
> > 
> 
> 
> Why reserve from the bottom? and not from the first available space?

I assume the reason is that maintaining the existing model with
RTE_ETH_FLOW_* sibling macros doesn't make sense for these, so this approach
doesn't impact future ETH_RSS_* definitions...

> > for example:
> >     ETH_RSS_IPV4 | ETH_RSS_INSET_L3_SRC: hash on src ip address only
> >     ETH_RSS_IPV4_UDP | ETH_RSS_INSET_L4_DST: hash on src/dst IP and
> > 				            dst UDP port
> >     ETH_RSS_L2_PAYLOAD | ETH_RSS_INSET_L2_DST: hash on dst mac address
> > 
> 
> What happens when the user set ETH_RSS_IPV4? From what I understand from your RFC this will do nothing
> since no bits where enabled, am I correct? 
> If I'm correct this may break applications.

Also my thought, again I assume that ETH_RSS_INSET_* flags only act as
modifiers to ETH_RSS_IPV4 and friends, if none are provided, then both
source and destination are taken into account. If that's the design, it must
be properly documented still.

Looks like it's time to end the relationship between RTE_ETH_FLOW_* and
ETH_RSS_* seeing both serve different purposes, and these new macros only
make sense for RSS.

I suggest a prior patch that converts all those definitions:

 #define ETH_RSS_IPV4               (1ULL << RTE_ETH_FLOW_IPV4)
 [...]

To their numerical counterparts directly, in which case we have two options:

Without breaking ABI, e.g.:

 #define ETH_RSS_IPV4               (1ULL << 2)
 [...]

Or going further if we're ready to break ABI a tiny little bit, starting
over from zero and use unique flags for IPv4, IPv6, TCP and UDP without
distinguishing between NONFRAG, FRAG and IPV6_EX, which never made sense for
RSS, and separating L4 from L3 to save even more, that is:
 
 #define ETH_RSS_ETH                (1ULL << 0)
 #define ETH_RSS_IPV4               (1ULL << 1)
 #define ETH_RSS_IPV6               (1ULL << 2)
 #define ETH_RSS_UDP                (1ULL << 3)
 #define ETH_RSS_TCP                (1ULL << 4)
 #define ETH_RSS_SCTP               (1ULL << 5)
 [...]

Then the flags you would like to add would have to be more explicit. I think
Qi's original suggestion with "ONLY" was better in this regard than "INSET":

 #define ETH_RSS_L2_SRC_ONLY        (1ULL << 6)
 #define ETH_RSS_L2_DST_ONLY        (1ULL << 7)
 [...]

Otherwise there's still the negative approach:

 #define ETH_RSS_L2_NO_SRC          (1ULL << 6)
 #define ETH_RSS_L2_NO_DST          (1ULL << 7)
 [...]

Not sure which is better. Thoughts?

-- 
Adrien Mazarguil
6WIND