On 1/15/2020 2:11 PM, Iremonger, Bernard wrote:
> Hi Ferruh,
>
> <snip>
>
>> Subject: Re: [dpdk-dev] [PATCH v5 12/14] librte_ethdev: add ESP and AH
>> flow types to RSS
>>
>> 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.iremon...@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.
>
> This patch is required to handle RSS for the esp-ah.pkg DDP profile, it does
> not affect the i40e FD and testpmd changes in this patch set.
> The esp-ah.pkg DDP profile is not released yet.
>
> Given that the merge deadline is today, I propose to drop this patch from the
> v6 patch set.
+1, if it can be separated, better to do it to not block rest of the work.
>
>>
>>>
>>>> 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 | \
>>>>>>>
>>>
> Regards,
>
> Bernard.
>
