diff mbox series

[v5,12/14] librte_ethdev: add ESP and AH flow types to RSS

Message ID 1579010128-15794-13-git-send-email-bernard.iremonger@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series net/i40e: ESP support |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Iremonger, Bernard Jan. 14, 2020, 1:55 p.m. UTC 
  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@intel.com>
---
 lib/librte_ethdev/rte_ethdev.h | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Comments

Ferruh Yigit Jan. 14, 2020, 6:45 p.m. UTC | #1 
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@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
>  
>  /*
>   * 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 | \
>
Qi Zhang Jan. 15, 2020, 12:13 a.m. UTC | #2 
> -----Original Message-----
> From: Iremonger, Bernard <bernard.iremonger@intel.com>
> Sent: Tuesday, January 14, 2020 9:55 PM
> To: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Doherty, Declan <declan.doherty@intel.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Byrne, Stephen1
> <stephen1.byrne@intel.com>; Zhang, Helin <helin.zhang@intel.com>;
> Iremonger, Bernard <bernard.iremonger@intel.com>
> Subject: [PATCH v5 12/14] librte_ethdev: add ESP and AH flow types to RSS
> 
> 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@intel.com>
> ---
>  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
> 
>  /*
>   * 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)
> +
> +
> +
> +

Empty lines need to be removed

Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>

> 
>  /*
>   * 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 | \
> --
> 2.7.4
Andrew Rybchenko Jan. 15, 2020, 9:13 a.m. UTC | #3 
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@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?

>>  
>>  /*
>>   * 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 | \
>>
Iremonger, Bernard Jan. 15, 2020, 9:41 a.m. UTC | #4 
Hi Qi,

<snip>

> > Subject: [PATCH v5 12/14] librte_ethdev: add ESP and AH flow types to
> > RSS
> >
> > 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@intel.com>
> > ---
> >  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
> >
> >  /*
> >   * 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)
> > +
> > +
> > +
> > +
> 
> Empty lines need to be removed
> 
> Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>

I will remove in v6 patch.
Can I carry forward your Reviewed-by:  ?
 
> >
> >  /*
> >   * 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 | \
> > --
> > 2.7.4
> 
Regards,

Bernard.
Ferruh Yigit Jan. 15, 2020, 10:44 a.m. UTC | #5 
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@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.

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 | \
>>>
>
Andrew Rybchenko Jan. 15, 2020, 10:55 a.m. UTC | #6 
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@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)

> 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 | \
>>>>
Ferruh Yigit Jan. 15, 2020, 12:28 p.m. UTC | #7 
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@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 | \
>>>>>
>
Iremonger, Bernard Jan. 15, 2020, 2:11 p.m. UTC | #8 
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.iremonger@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.

> 
> >
> >> 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.
Ferruh Yigit Jan. 15, 2020, 2:36 p.m. UTC | #9 
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.iremonger@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.
>
diff mbox series

Patch

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
 
 /*
  * 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 | \