[v6,1/3] ethdev: add level support for RSS offload types

  From: Kiran Kumar K <kirankumark@marvell.com>

This patch reserves 2 bits as input selection to select Inner and
outer layers for RSS computation. It is combined with existing
ETH_RSS_* to choose Inner or outer layers for L2, L3 and L4.
This functionality already exists in rte_flow through level parameter in
RSS action configuration rte_flow_action_rss.

Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
---
V7 Changes:
* Split ethdev and testpmd changes into seperate patches.

 lib/librte_ethdev/rte_ethdev.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

--
2.25.1
  

On 8/21/20 2:03 PM, kirankumark@marvell.com wrote:
> From: Kiran Kumar K <kirankumark@marvell.com>
> 
> This patch reserves 2 bits as input selection to select Inner and
> outer layers for RSS computation. It is combined with existing
> ETH_RSS_* to choose Inner or outer layers for L2, L3 and L4.
> This functionality already exists in rte_flow through level parameter in
> RSS action configuration rte_flow_action_rss.
> 
> Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
> ---
> V7 Changes:
> * Split ethdev and testpmd changes into seperate patches.
> 
>  lib/librte_ethdev/rte_ethdev.h | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 70295d7ab..2a3a76d37 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -552,6 +552,33 @@ struct rte_eth_rss_conf {
>  #define RTE_ETH_RSS_L3_PRE64	   (1ULL << 53)
>  #define RTE_ETH_RSS_L3_PRE96	   (1ULL << 52)
> 
> +/*
> + * We use the following macros to combine with the above layers to choose
> + * inner and outer layers or both for RSS computation.
> + * Note: Default is 0: inner layers, 1: outer layers, 2: both
> + * bit 50 and 51 are reserved for this.
> + */
> +
> +/**
> + * Level 0, It basically stands for the innermost encapsulation level RSS
> + * can be performed on according to PMD and device capabilities.
> + */
> +#define ETH_RSS_LEVEL_INNER        (0ULL << 50)

You're trying to refer to struct rte_flow_action_rss level definition
when values are chosen, but I think it is
inaccurate. Primary definition of the level==0 is PMD default,
not inner. Definition says that typically it is the innermost
encapsulation level RSS can be performed on according to PMD
and device capabilities. But it is secondary, primary
definition is "PMD default". Basically unspecified by the
caller (structure memset to 0) results in default behaviour.

> +/**
> + * Level 1, It basically stands for the outermost encapsulation level RSS
> + * can be performed on according to PMD and device capabilities.
> + */
> +#define ETH_RSS_LEVEL_OUTER        (1ULL << 50)
> +/**
> + * Level 2, It basically stands for the both inner and outermost
> + * encapsulation level RSS can be performed on according to PMD and
> + * device capabilities.
> + */
> +#define ETH_RSS_LEVEL_INNER_OUTER  (2ULL << 50)

Level equal to 2 is the first after the outermost inner level.
Level equal to 3 is the second after the outermost inner level
and so on.

If we really try to follow level definition from
rte_flow_action_rss, the problem is that these defines
are used to define RSS hashing capabilities in
dev_info.flow_type_rss_offloads. Support for level
up to 3 levels, does not mean that we can hash on any,
it could be either outermost or innermost level, but
not the middle level, if 3 levels are present.
Or just innermost recognized: either the first one
if no tunnels recognized, the second if two levels
are recognized, or the third one if three levels are
recognized.

Sorry, that I'm not proposing any solution right now.
Just need more time to thing about it.
You're welcome to try to cover these requirements.

> +#define ETH_RSS_LEVEL_MASK	   (3ULL << 50)
> +
> +#define ETH_RSS_LEVEL(rss_hf) ((rss_hf & ETH_RSS_LEVEL_MASK) >> 50)
> +
>  /**
>   * For input set change of hash filter, if SRC_ONLY and DST_ONLY of
>   * the same level are used simultaneously, it is the same case as
> --
> 2.25.1
>
  

On 8/29/2020 3:52 PM, Andrew Rybchenko wrote:
> On 8/21/20 2:03 PM, kirankumark@marvell.com wrote:
>> From: Kiran Kumar K <kirankumark@marvell.com>
>>
>> This patch reserves 2 bits as input selection to select Inner and
>> outer layers for RSS computation. It is combined with existing
>> ETH_RSS_* to choose Inner or outer layers for L2, L3 and L4.
>> This functionality already exists in rte_flow through level parameter in
>> RSS action configuration rte_flow_action_rss.
>>
>> Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
>> ---
>> V7 Changes:
>> * Split ethdev and testpmd changes into seperate patches.
>>
>>   lib/librte_ethdev/rte_ethdev.h | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>> index 70295d7ab..2a3a76d37 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -552,6 +552,33 @@ struct rte_eth_rss_conf {
>>   #define RTE_ETH_RSS_L3_PRE64	   (1ULL << 53)
>>   #define RTE_ETH_RSS_L3_PRE96	   (1ULL << 52)
>>
>> +/*
>> + * We use the following macros to combine with the above layers to choose
>> + * inner and outer layers or both for RSS computation.
>> + * Note: Default is 0: inner layers, 1: outer layers, 2: both
>> + * bit 50 and 51 are reserved for this.
>> + */
>> +
>> +/**
>> + * Level 0, It basically stands for the innermost encapsulation level RSS
>> + * can be performed on according to PMD and device capabilities.
>> + */
>> +#define ETH_RSS_LEVEL_INNER        (0ULL << 50)
> 
> You're trying to refer to struct rte_flow_action_rss level definition
> when values are chosen, but I think it is
> inaccurate. Primary definition of the level==0 is PMD default,
> not inner. Definition says that typically it is the innermost
> encapsulation level RSS can be performed on according to PMD
> and device capabilities. But it is secondary, primary
> definition is "PMD default". Basically unspecified by the
> caller (structure memset to 0) results in default behaviour.
> 
>> +/**
>> + * Level 1, It basically stands for the outermost encapsulation level RSS
>> + * can be performed on according to PMD and device capabilities.
>> + */
>> +#define ETH_RSS_LEVEL_OUTER        (1ULL << 50)
>> +/**
>> + * Level 2, It basically stands for the both inner and outermost
>> + * encapsulation level RSS can be performed on according to PMD and
>> + * device capabilities.
>> + */
>> +#define ETH_RSS_LEVEL_INNER_OUTER  (2ULL << 50)
> 
> Level equal to 2 is the first after the outermost inner level.
> Level equal to 3 is the second after the outermost inner level
> and so on.
> 
> If we really try to follow level definition from
> rte_flow_action_rss, the problem is that these defines
> are used to define RSS hashing capabilities in
> dev_info.flow_type_rss_offloads. Support for level
> up to 3 levels, does not mean that we can hash on any,
> it could be either outermost or innermost level, but
> not the middle level, if 3 levels are present.
> Or just innermost recognized: either the first one
> if no tunnels recognized, the second if two levels
> are recognized, or the third one if three levels are
> recognized.

Is the more than two levels practically have a usage?
I can see rte_flow accepts any number as level but if it is not practically a 
concern I think we can go with 'inner' & 'outer' only.
And defer more level concern for now.

Even with only 'inner' & 'outer', capability reporting can be a problem tough.
Like if a PMD can calculate hash for inner and outer for TCP but only for outer 
for UDP, how can PMD report this capability?
Again not sure if this is a practically valid concern.

> 
> Sorry, that I'm not proposing any solution right now.
> Just need more time to thing about it.
> You're welcome to try to cover these requirements.
> 
>> +#define ETH_RSS_LEVEL_MASK	   (3ULL << 50)
>> +
>> +#define ETH_RSS_LEVEL(rss_hf) ((rss_hf & ETH_RSS_LEVEL_MASK) >> 50)
>> +
>>   /**
>>    * For input set change of hash filter, if SRC_ONLY and DST_ONLY of
>>    * the same level are used simultaneously, it is the same case as
>> --
>> 2.25.1
>>
>