[v4,07/10] ethdev: add IPsec SA expiry event subtypes

  From: Vamsi Attunuru <vattunuru@marvell.com>

Patch adds new event subtypes for notifying expiry
events upon reaching IPsec SA soft packet expiry and
hard packet/byte expiry limits.

Signed-off-by: Vamsi Attunuru <vattunuru@marvell.com>
---
 lib/ethdev/rte_ethdev.h | 9 +++++++++
 1 file changed, 9 insertions(+)
  

16/04/2022 21:25, Akhil Goyal:
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -3828,6 +3828,12 @@ enum rte_eth_event_ipsec_subtype {
>  	RTE_ETH_EVENT_IPSEC_SA_TIME_EXPIRY,
>  	/** Soft byte expiry of SA */
>  	RTE_ETH_EVENT_IPSEC_SA_BYTE_EXPIRY,
> +	/** Soft packet expiry of SA */

Is there a reference explaining what exactly is a "soft packet expiry"?
I think you should also mention what should be done
in the event handler.

> +	RTE_ETH_EVENT_IPSEC_SA_PKT_EXPIRY,
> +	/** Hard byte expiry of SA */
> +	RTE_ETH_EVENT_IPSEC_SA_BYTE_HARD_EXPIRY,
> +	/** Hard packet expiry of SA */
> +	RTE_ETH_EVENT_IPSEC_SA_PKT_HARD_EXPIRY,

Same comment for the 3 events.

>  	/** Max value of this enum */
>  	RTE_ETH_EVENT_IPSEC_MAX
>  };

What is the impact of this "MAX" value on ABI compatibility?
  

Hi Thomas,

> 16/04/2022 21:25, Akhil Goyal:
> > --- a/lib/ethdev/rte_ethdev.h
> > +++ b/lib/ethdev/rte_ethdev.h
> > @@ -3828,6 +3828,12 @@ enum rte_eth_event_ipsec_subtype {
> >  	RTE_ETH_EVENT_IPSEC_SA_TIME_EXPIRY,
> >  	/** Soft byte expiry of SA */
> >  	RTE_ETH_EVENT_IPSEC_SA_BYTE_EXPIRY,
> > +	/** Soft packet expiry of SA */
> 
> Is there a reference explaining what exactly is a "soft packet expiry"?

SA expiry is a very common procedure in case of IPsec.
And all stacks must support this feature.
You can refer https://docs.strongswan.org/strongswan-docs/5.9/config/rekeying.html
For details.
Time expiry means after x seconds SA will expire.
Packet expiry means after x packets processing SA will expire.
Byte expiry means after x bytes of packet processing SA will expire.

> I think you should also mention what should be done
> in the event handler.

I believe this is quite obvious as per IPsec specifications.
Application need to start rekeying or SA need to be created again.

> 
> > +	RTE_ETH_EVENT_IPSEC_SA_PKT_EXPIRY,
> > +	/** Hard byte expiry of SA */
> > +	RTE_ETH_EVENT_IPSEC_SA_BYTE_HARD_EXPIRY,
> > +	/** Hard packet expiry of SA */
> > +	RTE_ETH_EVENT_IPSEC_SA_PKT_HARD_EXPIRY,
> 
> Same comment for the 3 events.
> 
> >  	/** Max value of this enum */
> >  	RTE_ETH_EVENT_IPSEC_MAX
> >  };
> 
> What is the impact of this "MAX" value on ABI compatibility?
I see no issues reported while running ABI check.
There is no array being used inside library based on MAX.
  

Hi Thomas, Akhil,

> Is there a reference explaining what exactly is a "soft packet expiry"?

The SA lifetime/expiry is described in security library.
https://elixir.bootlin.com/dpdk/latest/source/lib/security/rte_security.h#L295

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Tuesday, April 19, 2022 3:44 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; david.marchand@redhat.com;
> hemant.agrawal@nxp.com; Anoob Joseph <anoobj@marvell.com>;
> konstantin.ananyev@intel.com; ciara.power@intel.com;
> ferruh.yigit@intel.com; andrew.rybchenko@oktetlabs.ru; Nithin Kumar
> Dabilpuram <ndabilpuram@marvell.com>; Vamsi Krishna Attunuru
> <vattunuru@marvell.com>
> Subject: RE: [EXT] Re: [PATCH v4 07/10] ethdev: add IPsec SA expiry event
> subtypes
> 
> Hi Thomas,
> 
> > 16/04/2022 21:25, Akhil Goyal:
> > > --- a/lib/ethdev/rte_ethdev.h
> > > +++ b/lib/ethdev/rte_ethdev.h
> > > @@ -3828,6 +3828,12 @@ enum rte_eth_event_ipsec_subtype {
> > >  	RTE_ETH_EVENT_IPSEC_SA_TIME_EXPIRY,
> > >  	/** Soft byte expiry of SA */
> > >  	RTE_ETH_EVENT_IPSEC_SA_BYTE_EXPIRY,
> > > +	/** Soft packet expiry of SA */
> >
> > Is there a reference explaining what exactly is a "soft packet expiry"?
> 
> SA expiry is a very common procedure in case of IPsec.
> And all stacks must support this feature.
> You can refer https://docs.strongswan.org/strongswan-
> docs/5.9/config/rekeying.html
> For details.
> Time expiry means after x seconds SA will expire.
> Packet expiry means after x packets processing SA will expire.
> Byte expiry means after x bytes of packet processing SA will expire.
> 
> > I think you should also mention what should be done in the event
> > handler.
> 
> I believe this is quite obvious as per IPsec specifications.
> Application need to start rekeying or SA need to be created again.
> 
> >
> > > +	RTE_ETH_EVENT_IPSEC_SA_PKT_EXPIRY,
> > > +	/** Hard byte expiry of SA */
> > > +	RTE_ETH_EVENT_IPSEC_SA_BYTE_HARD_EXPIRY,
> > > +	/** Hard packet expiry of SA */
> > > +	RTE_ETH_EVENT_IPSEC_SA_PKT_HARD_EXPIRY,
> >
> > Same comment for the 3 events.
> >
> > >  	/** Max value of this enum */
> > >  	RTE_ETH_EVENT_IPSEC_MAX
> > >  };
> >
> > What is the impact of this "MAX" value on ABI compatibility?
> I see no issues reported while running ABI check.
> There is no array being used inside library based on MAX.
  

19/04/2022 12:19, Anoob Joseph:
> Hi Thomas, Akhil,
> 
> > Is there a reference explaining what exactly is a "soft packet expiry"?
> 
> The SA lifetime/expiry is described in security library.
> https://elixir.bootlin.com/dpdk/latest/source/lib/security/rte_security.h#L295

The comment you are referencing is using "soft" for all limits,
even for packets_hard_limit and bytes_hard_limit.
It seems these comments need to be fixed.
  

Hi Thomas,

Yes. The comments need to be fixed. I'll push a patch for that. Thanks for pointing out.

Thanks,
Anoob

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, April 19, 2022 4:08 PM
> To: Anoob Joseph <anoobj@marvell.com>
> Cc: Akhil Goyal <gakhil@marvell.com>; dev@dpdk.org;
> david.marchand@redhat.com; hemant.agrawal@nxp.com;
> konstantin.ananyev@intel.com; ciara.power@intel.com;
> ferruh.yigit@intel.com; andrew.rybchenko@oktetlabs.ru; Nithin Kumar
> Dabilpuram <ndabilpuram@marvell.com>; Vamsi Krishna Attunuru
> <vattunuru@marvell.com>
> Subject: Re: [EXT] Re: [PATCH v4 07/10] ethdev: add IPsec SA expiry event
> subtypes
> 
> 19/04/2022 12:19, Anoob Joseph:
> > Hi Thomas, Akhil,
> >
> > > Is there a reference explaining what exactly is a "soft packet expiry"?
> >
> > The SA lifetime/expiry is described in security library.
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.co
> > m_dpdk_latest_source_lib_security_rte-5Fsecurity.h-
> 23L295&d=DwICAg&c=n
> > KjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B-
> WYLn1v9SyTMrT5EQqh2TU&m=
> >
> d9ONaYIdobFMxqt05JVZ3nT7rJib1rL35ax9X56teaPlvEGKpDNKMSCv1bYxhN8
> 1&s=jqz
> > cuIk0XOpIidta-KGZ4zmUvmkhsnbe_VzT3QXAWe4&e=
> 
> The comment you are referencing is using "soft" for all limits, even for
> packets_hard_limit and bytes_hard_limit.
> It seems these comments need to be fixed.
>
  

19/04/2022 12:14, Akhil Goyal:
> Hi Thomas,
> 
> > 16/04/2022 21:25, Akhil Goyal:
> > > --- a/lib/ethdev/rte_ethdev.h
> > > +++ b/lib/ethdev/rte_ethdev.h
> > > @@ -3828,6 +3828,12 @@ enum rte_eth_event_ipsec_subtype {
> > >  	RTE_ETH_EVENT_IPSEC_SA_TIME_EXPIRY,
> > >  	/** Soft byte expiry of SA */
> > >  	RTE_ETH_EVENT_IPSEC_SA_BYTE_EXPIRY,
> > > +	/** Soft packet expiry of SA */
> > 
> > Is there a reference explaining what exactly is a "soft packet expiry"?
> 
> SA expiry is a very common procedure in case of IPsec.
> And all stacks must support this feature.
> You can refer https://docs.strongswan.org/strongswan-docs/5.9/config/rekeying.html
> For details.
> Time expiry means after x seconds SA will expire.
> Packet expiry means after x packets processing SA will expire.
> Byte expiry means after x bytes of packet processing SA will expire.

I think you should use the syntax @ref packets_soft_limit
so it is clear where the event come from.

> > I think you should also mention what should be done
> > in the event handler.
> 
> I believe this is quite obvious as per IPsec specifications.
> Application need to start rekeying or SA need to be created again.

Yes indeed.

> > > +	RTE_ETH_EVENT_IPSEC_SA_PKT_EXPIRY,
> > > +	/** Hard byte expiry of SA */
> > > +	RTE_ETH_EVENT_IPSEC_SA_BYTE_HARD_EXPIRY,
> > > +	/** Hard packet expiry of SA */
> > > +	RTE_ETH_EVENT_IPSEC_SA_PKT_HARD_EXPIRY,
> > 
> > Same comment for the 3 events.
> > 
> > >  	/** Max value of this enum */
> > >  	RTE_ETH_EVENT_IPSEC_MAX
> > >  };
> > 
> > What is the impact of this "MAX" value on ABI compatibility?
> 
> I see no issues reported while running ABI check.
> There is no array being used inside library based on MAX.

No need of array inside the library, the events are exposed to the app.
I'm surprised libabigail is OK with changing an enum value.
  

> > Time expiry means after x seconds SA will expire.
> > Packet expiry means after x packets processing SA will expire.
> > Byte expiry means after x bytes of packet processing SA will expire.
> 
> I think you should use the syntax @ref packets_soft_limit
> so it is clear where the event come from.

OK will update the comments.

> 
> 
> > > > +	RTE_ETH_EVENT_IPSEC_SA_PKT_EXPIRY,
> > > > +	/** Hard byte expiry of SA */
> > > > +	RTE_ETH_EVENT_IPSEC_SA_BYTE_HARD_EXPIRY,
> > > > +	/** Hard packet expiry of SA */
> > > > +	RTE_ETH_EVENT_IPSEC_SA_PKT_HARD_EXPIRY,
> > >
> > > Same comment for the 3 events.
> > >
> > > >  	/** Max value of this enum */
> > > >  	RTE_ETH_EVENT_IPSEC_MAX
> > > >  };
> > >
> > > What is the impact of this "MAX" value on ABI compatibility?
> >
> > I see no issues reported while running ABI check.
> > There is no array being used inside library based on MAX.
> 
> No need of array inside the library, the events are exposed to the app.
> I'm surprised libabigail is OK with changing an enum value.
> 
@Ray Can you please check if it is an issue to add more values in this enum?
  

Akhil Goyal <gakhil@marvell.com> writes:

>> > Time expiry means after x seconds SA will expire.
>> > Packet expiry means after x packets processing SA will expire.
>> > Byte expiry means after x bytes of packet processing SA will expire.
>> 
>> I think you should use the syntax @ref packets_soft_limit
>> so it is clear where the event come from.
>
> OK will update the comments.
>
>> 
>> 
>> > > > +	RTE_ETH_EVENT_IPSEC_SA_PKT_EXPIRY,
>> > > > +	/** Hard byte expiry of SA */
>> > > > +	RTE_ETH_EVENT_IPSEC_SA_BYTE_HARD_EXPIRY,
>> > > > +	/** Hard packet expiry of SA */
>> > > > +	RTE_ETH_EVENT_IPSEC_SA_PKT_HARD_EXPIRY,
>> > >
>> > > Same comment for the 3 events.
>> > >
>> > > >  	/** Max value of this enum */
>> > > >  	RTE_ETH_EVENT_IPSEC_MAX
>> > > >  };
>> > >
>> > > What is the impact of this "MAX" value on ABI compatibility?
>> >
>> > I see no issues reported while running ABI check.
>> > There is no array being used inside library based on MAX.
>> 
>> No need of array inside the library, the events are exposed to the app.
>> I'm surprised libabigail is OK with changing an enum value.
>> 
> @Ray Can you please check if it is an issue to add more values in this enum?

Well look there is two seperate things going on here.

Why isn't libabigail complaining about the _MAX value changing. I'll
need to look at libabigail to see what the issue is, so lets put this
one aside for a moment. 

This second issue is it safe for the _MAX value to change?
We have a lot of back and forth argument on these, and previously
concluded that we should probably look to remove _MAX values in the
22.11 release.

The core issue is that we need look at how a user is likely to use
rte_eth_event_ipsec_subtype. Take a look at the example below:-

/root/src/dpdk/examples/ipsec-secgw/ipsec-secgw.c:2592:0

For instance, can we guarantee that an application built against DPDK
21.11, but running against 22.xx will never recieve one of the new
values in event_desc->subtype (or by any other means)?
  

Hi Ray,
> >> > > > +	RTE_ETH_EVENT_IPSEC_SA_PKT_EXPIRY,
> >> > > > +	/** Hard byte expiry of SA */
> >> > > > +	RTE_ETH_EVENT_IPSEC_SA_BYTE_HARD_EXPIRY,
> >> > > > +	/** Hard packet expiry of SA */
> >> > > > +	RTE_ETH_EVENT_IPSEC_SA_PKT_HARD_EXPIRY,
> >> > >
> >> > > Same comment for the 3 events.
> >> > >
> >> > > >  	/** Max value of this enum */
> >> > > >  	RTE_ETH_EVENT_IPSEC_MAX
> >> > > >  };
> >> > >
> >> > > What is the impact of this "MAX" value on ABI compatibility?
> >> >
> >> > I see no issues reported while running ABI check.
> >> > There is no array being used inside library based on MAX.
> >>
> >> No need of array inside the library, the events are exposed to the app.
> >> I'm surprised libabigail is OK with changing an enum value.
> >>
> > @Ray Can you please check if it is an issue to add more values in this enum?
> 
> Well look there is two seperate things going on here.
> 
> Why isn't libabigail complaining about the _MAX value changing. I'll
> need to look at libabigail to see what the issue is, so lets put this
> one aside for a moment.
> 
> This second issue is it safe for the _MAX value to change?
> We have a lot of back and forth argument on these, and previously
> concluded that we should probably look to remove _MAX values in the
> 22.11 release.

Agreed.
> 
> The core issue is that we need look at how a user is likely to use
> rte_eth_event_ipsec_subtype. Take a look at the example below:-
> 
> /root/src/dpdk/examples/ipsec-secgw/ipsec-secgw.c:2592:0
> 
> For instance, can we guarantee that an application built against DPDK
> 21.11, but running against 22.xx will never recieve one of the new
> values in event_desc->subtype (or by any other means)?

ok we can defer the 7/10, 8/10, 9/10 patch to next release then.