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

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

On 9/1/2020 4:27 AM, kirankumark@marvell.com wrote:
> From: Kiran Kumar K <kirankumark@marvell.com>
> 
> This patch reserves 2 bits as input selection to select Inner and
> outer encapsulation level for RSS computation. It is combined with existing
> ETH_RSS_* to choose Inner or outer layers.
> 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:
> * Re-worked to keep it in sync with rte_flow_action_rss and support upto
> 3 levels.
> * Addressed testpmd review comments.
> 
>   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..13e49bbd7 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.
> + * bit 50 and 51 are reserved for this.
> + */
> +
> +/** level 0, requests the default behavior. Depending on the packet
> + * type, it can mean outermost, innermost, anything in between or even no RSS.
> + * It basically stands for the innermost encapsulation level RSS
> + * can be performed on according to PMD and device capabilities.
> + */
> +#define ETH_RSS_LEVEL_0         (0ULL << 50)

I can see from history how this is involved, but the 'ETH_RSS_LEVEL_0' naming is 
not really clear what it is, the naming in v6 is more clear.

What about following one:
0 -> LEVEL_PMD_DEFAULT
1 -> LEVEL_OUTER
2 -> LEVEL_INNER
3 -> LEVEL_INNER_OUTER

This doesn't exactly match to rte_flow one, but closer than v6 one. This ends 
with max level 2. And defines a way to say both inner and outer.

> +
> +/** level 1,  requests RSS to be performed on the outermost packet
> + * encapsulation level.
> + */
> +#define ETH_RSS_LEVEL_1         (1ULL << 50)
> +
> +/** level 2,  requests RSS to be performed on the
> + * specified inner packet encapsulation level, from outermost to
> + * innermost (lower to higher values).
> + */
> +#define ETH_RSS_LEVEL_2	        (2ULL << 50)

I can see you are trying to copy rte_flow usage, but this doesn't really makes 
sense here. Where the value of the level is defined in this case? If not defined 
how the PMD knows which level to use?

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