Re: [dpdk-dev] [PATCH v6 1/3] ethdev: add level support for RSS offload types

[Ferruh Yigit <ferruh.yigit@intel.com>]

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