[dpdk-dev] [PATCH v5 12/14] librte_ethdev: add ESP and AH flow types to RSS

[Ferruh Yigit]

On 1/15/2020 10:55 AM, Andrew Rybchenko wrote:
> On 1/15/20 1:44 PM, Ferruh Yigit wrote:
>> On 1/15/2020 9:13 AM, Andrew Rybchenko wrote:
>>> On 1/14/20 9:45 PM, Ferruh Yigit wrote:
>>>> On 1/14/2020 1:55 PM, Bernard Iremonger wrote:
>>>>> Add macros for the following protocols in the DDP esp-ah profile:
>>>>> ESP
>>>>> AH
>>>>>
>>>>> Add the following RSS macro for IPsec:
>>>>> ETH_RSS_IPSEC
>>>>>
>>>>> Signed-off-by: Bernard Iremonger <bernard.iremonger at intel.com>
>>>> +Ori and other ethdev maintainers.
>>>>
>>>> Ori, can you please check this patch?
>>>>
>>>>> ---
>>>>>  lib/librte_ethdev/rte_ethdev.h | 14 +++++++++++++-
>>>>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>>>>> index 18a9def..208ec90 100644
>>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>>> @@ -484,7 +484,9 @@ struct rte_eth_rss_conf {
>>>>>  #define RTE_ETH_FLOW_NVGRE              21 /**< NVGRE protocol based flow */
>>>>>  #define RTE_ETH_FLOW_VXLAN_GPE          22 /**< VXLAN-GPE protocol based flow */
>>>>>  #define RTE_ETH_FLOW_GTPU               23 /**< GTPU protocol based flow */
>>>>> -#define RTE_ETH_FLOW_MAX                24
>>>>> +#define RTE_ETH_FLOW_AH                 24 /**< AH protocol based flow */
>>>>> +#define RTE_ETH_FLOW_ESP                25 /**< ESP protocol based flow */
>>>>> +#define RTE_ETH_FLOW_MAX                26
>>> Isn't changing RTE_ETH_FLOW_MAX value breaking ABI?
>>> Is v20.11 target release of the patch?
>> I can't see how this can cause an ABI break, unless the 'RTE_ETH_FLOW_MAX' value
>> used as size of an array in the middle of a struct.
>> There is 'struct rte_eth_fdir_flex_conf' but the array is at the end there, so
>> it should be OK, unless that struct is not in the middle of another struct.
> 
> rte_eth_fdir_flex_conf -> rte_fdir_conf -> rte_eth_conf (in the middle)

Yes, this looks like an ABI break and this is very annoying not able to even add
a new RTE_FLOW type.

We need to find a proper way to handle this, at first glance I can see stop
using _MAX macros for the array size can work and perhaps we can use another big
enough hardcoded value for all similar array size. Any other option?

But we can do this on 20.11, we need a solution until that time.

> 
>> And there are values calculated from 'RTE_ETH_FLOW_MAX', like
>> 'RTE_FLOW_MASK_ARRAY_SIZE', same concern applies there, it very hard to follow.
>>
>> Bernard,
>>
>> Can you please run the ABI version script to be sure this is not breaking the ABI?
>>
>>
>>>>>  
>>>>>  /*
>>>>>   * Below macros are defined for RSS offload types, they can be used to
>>>>> @@ -511,6 +513,12 @@ struct rte_eth_rss_conf {
>>>>>  #define ETH_RSS_GENEVE             (1ULL << 20)
>>>>>  #define ETH_RSS_NVGRE              (1ULL << 21)
>>>>>  #define ETH_RSS_GTPU               (1ULL << 23)
>>>>> +#define ETH_RSS_AH                 (1ULL << 24)
>>>>> +#define ETH_RSS_ESP                (1ULL << 25)
>>>>> +
>>>>> +
>>>>> +
>>>>> +
>>>>>  
>>>>>  /*
>>>>>   * We use the following macros to combine with above ETH_RSS_* for
>>>>> @@ -571,6 +579,10 @@ rte_eth_rss_hf_refine(uint64_t rss_hf)
>>>>>  	ETH_RSS_NONFRAG_IPV4_SCTP | \
>>>>>  	ETH_RSS_NONFRAG_IPV6_SCTP)
>>>>>  
>>>>> +#define ETH_RSS_IPSEC ( \
>>>>> +	ETH_RSS_AH | \
>>>>> +	ETH_RSS_ESP)
>>>>> +
>>>>>  #define ETH_RSS_TUNNEL ( \
>>>>>  	ETH_RSS_VXLAN  | \
>>>>>  	ETH_RSS_GENEVE | \
>>>>>
>