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

["Iremonger, Bernard" <bernard.iremonger@intel.com>]

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.