[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.

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

Signed-off-by: Simei Su <simei.su@intel.com>
---
 lib/librte_ethdev/rte_ethdev.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)
  

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?

> 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.

> Signed-off-by: Simei Su <simei.su@intel.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index dc6596b..8af6355 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -508,6 +508,19 @@ struct rte_eth_rss_conf {
>  #define ETH_RSS_GENEVE             (1ULL << RTE_ETH_FLOW_GENEVE)
>  #define ETH_RSS_NVGRE              (1ULL << RTE_ETH_FLOW_NVGRE)
> 
> +/*
> + * The following six macros are used combined with ETH_RSS_* to
> + * represent rss types. The structure rte_flow_action_rss.types is
> + * 64-bit wide and we reserve couple bits here for input set selection
> + * from bottom of the 64 bits.
> + */
> +#define	ETH_RSS_INSET_L2_SRC       (1ULL << 63)
> +#define	ETH_RSS_INSET_L2_DST       (1ULL << 62)
> +#define	ETH_RSS_INSET_L3_SRC       (1ULL << 61)
> +#define	ETH_RSS_INSET_L3_DST       (1ULL << 60)
> +#define	ETH_RSS_INSET_L4_SRC       (1ULL << 59)
> +#define	ETH_RSS_INSET_L4_DST       (1ULL << 58)
> +
>  #define ETH_RSS_IP ( \
>  	ETH_RSS_IPV4 | \
>  	ETH_RSS_FRAG_IPV4 | \
> --
> 1.8.3.1

Thanks,
Ori Kam
  

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?