[PATCH] doc: announce rte_ipsec API changes

[PATCH] doc: announce rte_ipsec API changes

[Aakash Sasidharan]

In case of event mode operations where event device can help in atomic sequence
number increment across cores, sequence number need to be provided by the
application instead of being updated in rte_ipsec or the PMD.
To support this, a new flag ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE`` will be added
to disable sequence number update inside IPsec library and the API
rte_ipsec_pkt_crypto_prepare will be extended to include ``sqn`` as an additional
parameter to specify sequence number to be used for IPsec from the application.

Signed-off-by: Aakash Sasidharan <asasidharan@marvell.com>
Change-Id: I0dd7729d8775106445c8e7cbe1a04c1ac5613b3d
---
 doc/guides/rel_notes/deprecation.rst | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 6948641ff6..bc1d93cca7 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -133,6 +133,13 @@ Deprecation Notices
   Since these functions are not called directly by the application,
   the API remains unaffected.
 
+* ipsec: The rte_ipsec library is updated to support sequence number provided
+  by application. A new flag ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE`` is introduced
+  to disable sequence number assignment in lib IPsec.
+  The API rte_ipsec_pkt_crypto_prepare is extended to include ``sqn`` as an
+  additional parameter allowing application to specify the sequence number to be
+  used for the IPsec operation.
+
 * pipeline: The pipeline library legacy API (functions rte_pipeline_*)
   will be deprecated and subsequently removed in DPDK 24.11 release.
   Before this, the new pipeline library API (functions rte_swx_pipeline_*)
-- 
2.25.1

[PATCH v2] doc: announce rte_ipsec API changes

[Aakash Sasidharan]

In case of event mode operations where event device can help in atomic
sequence number increment across cores, sequence number need to be
provided by the application instead of being updated in rte_ipsec or the
PMD. To support this, a new flag ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE``
will be added to disable sequence number update inside IPsec library and
the API rte_ipsec_pkt_crypto_prepare will be extended to include ``sqn``
as an additional parameter to specify sequence number to be used for
IPsec from the application.

Signed-off-by: Aakash Sasidharan <asasidharan@marvell.com>
---
 doc/guides/rel_notes/deprecation.rst | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 6948641ff6..bc1d93cca7 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -133,6 +133,13 @@ Deprecation Notices
   Since these functions are not called directly by the application,
   the API remains unaffected.
 
+* ipsec: The rte_ipsec library is updated to support sequence number provided
+  by application. A new flag ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE`` is introduced
+  to disable sequence number assignment in lib IPsec.
+  The API rte_ipsec_pkt_crypto_prepare is extended to include ``sqn`` as an
+  additional parameter allowing application to specify the sequence number to be
+  used for the IPsec operation.
+
 * pipeline: The pipeline library legacy API (functions rte_pipeline_*)
   will be deprecated and subsequently removed in DPDK 24.11 release.
   Before this, the new pipeline library API (functions rte_swx_pipeline_*)
-- 
2.25.1

RE: [PATCH v2] doc: announce rte_ipsec API changes

[Konstantin Ananyev]

Hi,

> In case of event mode operations where event device can help in atomic
> sequence number increment across cores, sequence number need to be
> provided by the application instead of being updated in rte_ipsec or the
> PMD. To support this, a new flag ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE``
> will be added to disable sequence number update inside IPsec library and
> the API rte_ipsec_pkt_crypto_prepare will be extended to include ``sqn``
> as an additional parameter to specify sequence number to be used for
> IPsec from the application.

Could you probably elaborate a bit more:
Why such change is necessary for event-dev mode, what exactly will
be affected in librte_ipsec (would it be for outbound mode, or both),
etc.
 
> 
> Signed-off-by: Aakash Sasidharan <asasidharan@marvell.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 6948641ff6..bc1d93cca7 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -133,6 +133,13 @@ Deprecation Notices
>    Since these functions are not called directly by the application,
>    the API remains unaffected.
> 
> +* ipsec: The rte_ipsec library is updated to support sequence number provided
> +  by application. A new flag ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE`` is introduced
> +  to disable sequence number assignment in lib IPsec.
> +  The API rte_ipsec_pkt_crypto_prepare is extended to include ``sqn`` as an
> +  additional parameter allowing application to specify the sequence number to be
> +  used for the IPsec operation.
> +
>  * pipeline: The pipeline library legacy API (functions rte_pipeline_*)
>    will be deprecated and subsequently removed in DPDK 24.11 release.
>    Before this, the new pipeline library API (functions rte_swx_pipeline_*)
> --
> 2.25.1

RE: [PATCH v2] doc: announce rte_ipsec API changes

[Aakash Sasidharan]

> -----Original Message-----
> From: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> Sent: Tuesday, July 23, 2024 9:35 PM
> To: Aakash Sasidharan <asasidharan@marvell.com>
> Cc: Akhil Goyal <gakhil@marvell.com>; Jerin Jacob <jerinj@marvell.com>;
> Anoob Joseph <anoobj@marvell.com>; Vidya Sagar Velumuri
> <vvelumuri@marvell.com>; dev@dpdk.org; konstantin.v.ananyev@yandex.ru;
> vladimir.medvedkin@intel.com
> Subject: [EXTERNAL] RE: [PATCH v2] doc: announce rte_ipsec API changes
> 
> Hi,
> 
> > In case of event mode operations where event device can help in atomic
> > sequence number increment across cores, sequence number need to be
> > provided by the application instead of being updated in rte_ipsec or
> > the PMD. To support this, a new flag
> > ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE``
> > will be added to disable sequence number update inside IPsec library
> > and the API rte_ipsec_pkt_crypto_prepare will be extended to include
> > ``sqn`` as an additional parameter to specify sequence number to be
> > used for IPsec from the application.
> 
> Could you probably elaborate a bit more:
> Why such change is necessary for event-dev mode, what exactly will be
> affected in librte_ipsec (would it be for outbound mode, or both), etc.
> 

[Aakash] When using eventdev, it is possible to have multiple cores process packets from the same flow at the same time, but still have ordering maintained.

Sequence for IPsec would be like below,
1. Ethdev Rx computes flow hash and submits packets to an ORDERED eventdev queue.
    One flow would always hit one event dev queue.
    One eventdev queue can be attached to multiple eventdev ports.
2. Lcores receives packets via these eventdev ports.
    Lcores can now process the packets from the same flow in parallel.
3. Lcores submit the packets to an ATOMIC queue
    This is needed as IPsec seq no update needs to be done atomically.
4. After seq no update, packets are moved to ORDERED queue.
    Lcores can now processes the packets in parallel again.
5. During Tx, eventdev ensures packet ordering based on ORDERED queue.

Since lib IPsec takes care of sequence number assignment, complete rte_ipsec_pkt_crypto_prepare() routine need to be made as ATOMIC stage.
But apart from seq no update, rest of the operations can be done in parallel.

In addition, we are also looking at another use case when a set of packets from a session can be IPsec processed by rte_security device and some packets from the same session would need to be SW processed with lib IPsec. Here again the sequence number assignment would need to occur at central place so that sequence number is not repeated.

Initially we are looking at outbound only. But similar kind of use case would be applicable for inbound also.

> >
> > Signed-off-by: Aakash Sasidharan <asasidharan@marvell.com>
> > ---
> >  doc/guides/rel_notes/deprecation.rst | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/doc/guides/rel_notes/deprecation.rst
> > b/doc/guides/rel_notes/deprecation.rst
> > index 6948641ff6..bc1d93cca7 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -133,6 +133,13 @@ Deprecation Notices
> >    Since these functions are not called directly by the application,
> >    the API remains unaffected.
> >
> > +* ipsec: The rte_ipsec library is updated to support sequence number
> > +provided
> > +  by application. A new flag ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE``
> > +is introduced
> > +  to disable sequence number assignment in lib IPsec.
> > +  The API rte_ipsec_pkt_crypto_prepare is extended to include ``sqn``
> > +as an
> > +  additional parameter allowing application to specify the sequence
> > +number to be
> > +  used for the IPsec operation.
> > +
> >  * pipeline: The pipeline library legacy API (functions rte_pipeline_*)
> >    will be deprecated and subsequently removed in DPDK 24.11 release.
> >    Before this, the new pipeline library API (functions
> > rte_swx_pipeline_*)
> > --
> > 2.25.1

RE: [PATCH v2] doc: announce rte_ipsec API changes

[Konstantin Ananyev]

> > > In case of event mode operations where event device can help in atomic
> > > sequence number increment across cores, sequence number need to be
> > > provided by the application instead of being updated in rte_ipsec or
> > > the PMD. To support this, a new flag
> > > ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE``
> > > will be added to disable sequence number update inside IPsec library
> > > and the API rte_ipsec_pkt_crypto_prepare will be extended to include
> > > ``sqn`` as an additional parameter to specify sequence number to be
> > > used for IPsec from the application.
> >
> > Could you probably elaborate a bit more:
> > Why such change is necessary for event-dev mode, what exactly will be
> > affected in librte_ipsec (would it be for outbound mode, or both), etc.
> >
> 
> [Aakash] When using eventdev, it is possible to have multiple cores process packets from the same flow at the same time, but still
> have ordering maintained.
> 
> Sequence for IPsec would be like below,
> 1. Ethdev Rx computes flow hash and submits packets to an ORDERED eventdev queue.
>     One flow would always hit one event dev queue.
>     One eventdev queue can be attached to multiple eventdev ports.
> 2. Lcores receives packets via these eventdev ports.
>     Lcores can now process the packets from the same flow in parallel.
> 3. Lcores submit the packets to an ATOMIC queue
>     This is needed as IPsec seq no update needs to be done atomically.
> 4. After seq no update, packets are moved to ORDERED queue.
>     Lcores can now processes the packets in parallel again.
> 5. During Tx, eventdev ensures packet ordering based on ORDERED queue.
> 
> Since lib IPsec takes care of sequence number assignment, complete rte_ipsec_pkt_crypto_prepare() routine need to be made as
> ATOMIC stage.
> But apart from seq no update, rest of the operations can be done in parallel.

Thanks for explanation.
Basically you are seeking ability to split rte_ipsec_pkt_crypto_prepare() for outbound
into two stages:
1. update sqn
2. all other preps
To be able to do step #2 in parallel, correct?
My thought always was that step #2 is not that expensive in terms of performance,
and there probably not much point to make it parallel.
But I suppose you measured step#2 overhead on your platform
and concluded that it worth it...

One concern I have with the way you suggested -
now we need to store/update sa.sqn by some external entity.
Another thing - don't really want to pollute crypto_prepare() API
with new parameters which meaning is a bit obscure and depends on
other API calls...  

Wouldn't it be easier and probably more straightforward to just introduce 2 new
functions here that would represent step #1 and step #2?
Then we can keep crypto_prepare() intact, and user will have a choice:
- either use  original crypto_prepare() - nothing needs to be changed
-  or instead call these new functions on his own, if he wants to.

> In addition, we are also looking at another use case when a set of packets from a session can be IPsec processed by rte_security
> device and some packets from the same session would need to be SW processed with lib IPsec. Here again the sequence number
> assignment would need to occur at central place so that sequence number is not repeated.

Interesting, and how SW/HW SQN will be synchronized in that case? 
 
> Initially we are looking at outbound only. But similar kind of use case would be applicable for inbound also.
> 
> > >
> > > Signed-off-by: Aakash Sasidharan <asasidharan@marvell.com>
> > > ---
> > >  doc/guides/rel_notes/deprecation.rst | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/doc/guides/rel_notes/deprecation.rst
> > > b/doc/guides/rel_notes/deprecation.rst
> > > index 6948641ff6..bc1d93cca7 100644
> > > --- a/doc/guides/rel_notes/deprecation.rst
> > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > @@ -133,6 +133,13 @@ Deprecation Notices
> > >    Since these functions are not called directly by the application,
> > >    the API remains unaffected.
> > >
> > > +* ipsec: The rte_ipsec library is updated to support sequence number
> > > +provided
> > > +  by application. A new flag ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE``
> > > +is introduced
> > > +  to disable sequence number assignment in lib IPsec.
> > > +  The API rte_ipsec_pkt_crypto_prepare is extended to include ``sqn``
> > > +as an
> > > +  additional parameter allowing application to specify the sequence
> > > +number to be
> > > +  used for the IPsec operation.
> > > +
> > >  * pipeline: The pipeline library legacy API (functions rte_pipeline_*)
> > >    will be deprecated and subsequently removed in DPDK 24.11 release.
> > >    Before this, the new pipeline library API (functions
> > > rte_swx_pipeline_*)
> > > --
> > > 2.25.1
> 

RE: [PATCH v2] doc: announce rte_ipsec API changes

[Aakash Sasidharan]

> -----Original Message-----
> From: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> Sent: Wednesday, July 24, 2024 10:39 PM
> To: Aakash Sasidharan <asasidharan@marvell.com>
> Cc: Akhil Goyal <gakhil@marvell.com>; Jerin Jacob <jerinj@marvell.com>;
> Anoob Joseph <anoobj@marvell.com>; Vidya Sagar Velumuri
> <vvelumuri@marvell.com>; dev@dpdk.org; konstantin.v.ananyev@yandex.ru;
> vladimir.medvedkin@intel.com
> Subject: [EXTERNAL] RE: [PATCH v2] doc: announce rte_ipsec API changes
> 
> > > > In case of event mode operations where event device can help in
> > > > atomic sequence number increment across cores, sequence number
> > > > need to be provided by the application instead of being updated in
> > > > rte_ipsec or the PMD. To support this, a new flag
> > > > ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE``
> > > > will be added to disable sequence number update inside IPsec
> > > > library and the API rte_ipsec_pkt_crypto_prepare will be extended
> > > > to include ``sqn`` as an additional parameter to specify sequence
> > > > number to be used for IPsec from the application.
> > >
> > > Could you probably elaborate a bit more:
> > > Why such change is necessary for event-dev mode, what exactly will
> > > be affected in librte_ipsec (would it be for outbound mode, or both), etc.
> > >
> >
> > [Aakash] When using eventdev, it is possible to have multiple cores
> > process packets from the same flow at the same time, but still have ordering
> maintained.
> >
> > Sequence for IPsec would be like below, 1. Ethdev Rx computes flow
> > hash and submits packets to an ORDERED eventdev queue.
> >     One flow would always hit one event dev queue.
> >     One eventdev queue can be attached to multiple eventdev ports.
> > 2. Lcores receives packets via these eventdev ports.
> >     Lcores can now process the packets from the same flow in parallel.
> > 3. Lcores submit the packets to an ATOMIC queue
> >     This is needed as IPsec seq no update needs to be done atomically.
> > 4. After seq no update, packets are moved to ORDERED queue.
> >     Lcores can now processes the packets in parallel again.
> > 5. During Tx, eventdev ensures packet ordering based on ORDERED queue.
> >
> > Since lib IPsec takes care of sequence number assignment, complete
> > rte_ipsec_pkt_crypto_prepare() routine need to be made as ATOMIC stage.
> > But apart from seq no update, rest of the operations can be done in parallel.
> 
> Thanks for explanation.
> Basically you are seeking ability to split rte_ipsec_pkt_crypto_prepare() for
> outbound into two stages:
> 1. update sqn
> 2. all other preps
> To be able to do step #2 in parallel, correct?
> My thought always was that step #2 is not that expensive in terms of
> performance, and there probably not much point to make it parallel.
> But I suppose you measured step#2 overhead on your platform and
> concluded that it worth it...
> 
> One concern I have with the way you suggested - now we need to
> store/update sa.sqn by some external entity.
> Another thing - don't really want to pollute crypto_prepare() API with new
> parameters which meaning is a bit obscure and depends on other API calls...
> 
> Wouldn't it be easier and probably more straightforward to just introduce 2
> new functions here that would represent step #1 and step #2?
> Then we can keep crypto_prepare() intact, and user will have a choice:
> - either use  original crypto_prepare() - nothing needs to be changed
> -  or instead call these new functions on his own, if he wants to.
> 

[Aakash] As I understand, your suggestion is to introduce a set of two new APIs by splitting the current logic in crypto_prepare(). This should be okay.
For this, I believe we would need change in the structure rte_ipsec_sa_pkt_func to hold the function pointers for the new APIs.

Assuming that, introduction of the new flag RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE to disable seq no assignment in lib IPsec is fine, shall I send v3 announcing changes in ``struct rte_ipsec_sa_pkt_func``?

> > In addition, we are also looking at another use case when a set of
> > packets from a session can be IPsec processed by rte_security device
> > and some packets from the same session would need to be SW processed
> with lib IPsec. Here again the sequence number assignment would need to
> occur at central place so that sequence number is not repeated.
> 
> Interesting, and how SW/HW SQN will be synchronized in that case?
> 

[Aakash] The design is such that HW would assign sequence number for all cases. HW would then pass this data as a metadata to SW so that it can do SW processing with the assigned sequence number.

> > Initially we are looking at outbound only. But similar kind of use case would
> be applicable for inbound also.
> >
> > > >
> > > > Signed-off-by: Aakash Sasidharan <asasidharan@marvell.com>
> > > > ---
> > > >  doc/guides/rel_notes/deprecation.rst | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/doc/guides/rel_notes/deprecation.rst
> > > > b/doc/guides/rel_notes/deprecation.rst
> > > > index 6948641ff6..bc1d93cca7 100644
> > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > @@ -133,6 +133,13 @@ Deprecation Notices
> > > >    Since these functions are not called directly by the application,
> > > >    the API remains unaffected.
> > > >
> > > > +* ipsec: The rte_ipsec library is updated to support sequence
> > > > +number provided
> > > > +  by application. A new flag
> > > > +``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE``
> > > > +is introduced
> > > > +  to disable sequence number assignment in lib IPsec.
> > > > +  The API rte_ipsec_pkt_crypto_prepare is extended to include
> > > > +``sqn`` as an
> > > > +  additional parameter allowing application to specify the
> > > > +sequence number to be
> > > > +  used for the IPsec operation.
> > > > +
> > > >  * pipeline: The pipeline library legacy API (functions rte_pipeline_*)
> > > >    will be deprecated and subsequently removed in DPDK 24.11 release.
> > > >    Before this, the new pipeline library API (functions
> > > > rte_swx_pipeline_*)
> > > > --
> > > > 2.25.1
> >

RE: [PATCH v2] doc: announce rte_ipsec API changes

[Konstantin Ananyev]

> > > > > In case of event mode operations where event device can help in
> > > > > atomic sequence number increment across cores, sequence number
> > > > > need to be provided by the application instead of being updated in
> > > > > rte_ipsec or the PMD. To support this, a new flag
> > > > > ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE``
> > > > > will be added to disable sequence number update inside IPsec
> > > > > library and the API rte_ipsec_pkt_crypto_prepare will be extended
> > > > > to include ``sqn`` as an additional parameter to specify sequence
> > > > > number to be used for IPsec from the application.
> > > >
> > > > Could you probably elaborate a bit more:
> > > > Why such change is necessary for event-dev mode, what exactly will
> > > > be affected in librte_ipsec (would it be for outbound mode, or both), etc.
> > > >
> > >
> > > [Aakash] When using eventdev, it is possible to have multiple cores
> > > process packets from the same flow at the same time, but still have ordering
> > maintained.
> > >
> > > Sequence for IPsec would be like below, 1. Ethdev Rx computes flow
> > > hash and submits packets to an ORDERED eventdev queue.
> > >     One flow would always hit one event dev queue.
> > >     One eventdev queue can be attached to multiple eventdev ports.
> > > 2. Lcores receives packets via these eventdev ports.
> > >     Lcores can now process the packets from the same flow in parallel.
> > > 3. Lcores submit the packets to an ATOMIC queue
> > >     This is needed as IPsec seq no update needs to be done atomically.
> > > 4. After seq no update, packets are moved to ORDERED queue.
> > >     Lcores can now processes the packets in parallel again.
> > > 5. During Tx, eventdev ensures packet ordering based on ORDERED queue.
> > >
> > > Since lib IPsec takes care of sequence number assignment, complete
> > > rte_ipsec_pkt_crypto_prepare() routine need to be made as ATOMIC stage.
> > > But apart from seq no update, rest of the operations can be done in parallel.
> >
> > Thanks for explanation.
> > Basically you are seeking ability to split rte_ipsec_pkt_crypto_prepare() for
> > outbound into two stages:
> > 1. update sqn
> > 2. all other preps
> > To be able to do step #2 in parallel, correct?
> > My thought always was that step #2 is not that expensive in terms of
> > performance, and there probably not much point to make it parallel.
> > But I suppose you measured step#2 overhead on your platform and
> > concluded that it worth it...
> >
> > One concern I have with the way you suggested - now we need to
> > store/update sa.sqn by some external entity.
> > Another thing - don't really want to pollute crypto_prepare() API with new
> > parameters which meaning is a bit obscure and depends on other API calls...
> >
> > Wouldn't it be easier and probably more straightforward to just introduce 2
> > new functions here that would represent step #1 and step #2?
> > Then we can keep crypto_prepare() intact, and user will have a choice:
> > - either use  original crypto_prepare() - nothing needs to be changed
> > -  or instead call these new functions on his own, if he wants to.
> >
> 
> [Aakash] As I understand, your suggestion is to introduce a set of two new APIs by splitting the current logic in crypto_prepare(). This
> should be okay.
> For this, I believe we would need change in the structure rte_ipsec_sa_pkt_func to hold the function pointers for the new APIs.

Yes, that was my thought.

> 
> Assuming that, introduction of the new flag RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE to disable seq no assignment in lib IPsec is
> fine, shall I send v3 announcing changes in ``struct rte_ipsec_sa_pkt_func``?

I am definitely not against this new flag, but if we'll have 2 new functions instead,
do you still need it? 

> > > In addition, we are also looking at another use case when a set of
> > > packets from a session can be IPsec processed by rte_security device
> > > and some packets from the same session would need to be SW processed
> > with lib IPsec. Here again the sequence number assignment would need to
> > occur at central place so that sequence number is not repeated.
> >
> > Interesting, and how SW/HW SQN will be synchronized in that case?
> >
> 
> [Aakash] The design is such that HW would assign sequence number for all cases. HW would then pass this data as a metadata to SW
> so that it can do SW processing with the assigned sequence number.

As I can see there are two options to fulfill that requirement:

1. Introduce a new function that would update sa.sqn value.
Something like rte_ipsec_sa_update_sqn(...).
So when metadata from HW arrives, SW can call it and sync sa.sqn with new HW value,
and then continue with conventional rte_ipsec_crypto_prepare(...);

2. Introduce new (extended) variants of ipsec_crypto_prepare/process that would
take SQN (might be something else ?) as extra parameter, something like:

rte_ipcec_xprepare(const struct rte_ipsec_session *ss, struct rte_mbuf *mb[],
        struct rte_crypto_op *cop[], uint16_t num, /* extra params will be there*/);

Which one is better, hard to say for me off-hand...
Might be both are needed.
But probably we don't need to decide right now?
For me it would be enough if you'll outline the plan to change and extend ipsec lib 
API with new data-path functions and probably new flag for session create.   
 
> > > Initially we are looking at outbound only. But similar kind of use case would
> > be applicable for inbound also.
> > >
> > > > >
> > > > > Signed-off-by: Aakash Sasidharan <asasidharan@marvell.com>
> > > > > ---
> > > > >  doc/guides/rel_notes/deprecation.rst | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/doc/guides/rel_notes/deprecation.rst
> > > > > b/doc/guides/rel_notes/deprecation.rst
> > > > > index 6948641ff6..bc1d93cca7 100644
> > > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > > @@ -133,6 +133,13 @@ Deprecation Notices
> > > > >    Since these functions are not called directly by the application,
> > > > >    the API remains unaffected.
> > > > >
> > > > > +* ipsec: The rte_ipsec library is updated to support sequence
> > > > > +number provided
> > > > > +  by application. A new flag
> > > > > +``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE``
> > > > > +is introduced
> > > > > +  to disable sequence number assignment in lib IPsec.
> > > > > +  The API rte_ipsec_pkt_crypto_prepare is extended to include
> > > > > +``sqn`` as an
> > > > > +  additional parameter allowing application to specify the
> > > > > +sequence number to be
> > > > > +  used for the IPsec operation.
> > > > > +
> > > > >  * pipeline: The pipeline library legacy API (functions rte_pipeline_*)
> > > > >    will be deprecated and subsequently removed in DPDK 24.11 release.
> > > > >    Before this, the new pipeline library API (functions
> > > > > rte_swx_pipeline_*)
> > > > > --
> > > > > 2.25.1
> > >
> 

RE: [PATCH v2] doc: announce rte_ipsec API changes

[Aakash Sasidharan]

> > > > > > In case of event mode operations where event device can help
> > > > > > in atomic sequence number increment across cores, sequence
> > > > > > number need to be provided by the application instead of being
> > > > > > updated in rte_ipsec or the PMD. To support this, a new flag
> > > > > > ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE``
> > > > > > will be added to disable sequence number update inside IPsec
> > > > > > library and the API rte_ipsec_pkt_crypto_prepare will be
> > > > > > extended to include ``sqn`` as an additional parameter to
> > > > > > specify sequence number to be used for IPsec from the application.
> > > > >
> > > > > Could you probably elaborate a bit more:
> > > > > Why such change is necessary for event-dev mode, what exactly
> > > > > will be affected in librte_ipsec (would it be for outbound mode, or
> both), etc.
> > > > >
> > > >
> > > > [Aakash] When using eventdev, it is possible to have multiple
> > > > cores process packets from the same flow at the same time, but
> > > > still have ordering
> > > maintained.
> > > >
> > > > Sequence for IPsec would be like below, 1. Ethdev Rx computes flow
> > > > hash and submits packets to an ORDERED eventdev queue.
> > > >     One flow would always hit one event dev queue.
> > > >     One eventdev queue can be attached to multiple eventdev ports.
> > > > 2. Lcores receives packets via these eventdev ports.
> > > >     Lcores can now process the packets from the same flow in parallel.
> > > > 3. Lcores submit the packets to an ATOMIC queue
> > > >     This is needed as IPsec seq no update needs to be done atomically.
> > > > 4. After seq no update, packets are moved to ORDERED queue.
> > > >     Lcores can now processes the packets in parallel again.
> > > > 5. During Tx, eventdev ensures packet ordering based on ORDERED
> queue.
> > > >
> > > > Since lib IPsec takes care of sequence number assignment, complete
> > > > rte_ipsec_pkt_crypto_prepare() routine need to be made as ATOMIC
> stage.
> > > > But apart from seq no update, rest of the operations can be done in
> parallel.
> > >
> > > Thanks for explanation.
> > > Basically you are seeking ability to split
> > > rte_ipsec_pkt_crypto_prepare() for outbound into two stages:
> > > 1. update sqn
> > > 2. all other preps
> > > To be able to do step #2 in parallel, correct?
> > > My thought always was that step #2 is not that expensive in terms of
> > > performance, and there probably not much point to make it parallel.
> > > But I suppose you measured step#2 overhead on your platform and
> > > concluded that it worth it...
> > >
> > > One concern I have with the way you suggested - now we need to
> > > store/update sa.sqn by some external entity.
> > > Another thing - don't really want to pollute crypto_prepare() API
> > > with new parameters which meaning is a bit obscure and depends on other
> API calls...
> > >
> > > Wouldn't it be easier and probably more straightforward to just
> > > introduce 2 new functions here that would represent step #1 and step #2?
> > > Then we can keep crypto_prepare() intact, and user will have a choice:
> > > - either use  original crypto_prepare() - nothing needs to be
> > > changed
> > > -  or instead call these new functions on his own, if he wants to.
> > >
> >
> > [Aakash] As I understand, your suggestion is to introduce a set of two
> > new APIs by splitting the current logic in crypto_prepare(). This should be
> okay.
> > For this, I believe we would need change in the structure
> rte_ipsec_sa_pkt_func to hold the function pointers for the new APIs.
> 
> Yes, that was my thought.
> 
> >
> > Assuming that, introduction of the new flag
> > RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE to disable seq no assignment in
> lib IPsec is fine, shall I send v3 announcing changes in ``struct
> rte_ipsec_sa_pkt_func``?
> 
> I am definitely not against this new flag, but if we'll have 2 new functions
> instead, do you still need it?
> 
> > > > In addition, we are also looking at another use case when a set of
> > > > packets from a session can be IPsec processed by rte_security
> > > > device and some packets from the same session would need to be SW
> > > > processed
> > > with lib IPsec. Here again the sequence number assignment would need
> > > to occur at central place so that sequence number is not repeated.
> > >
> > > Interesting, and how SW/HW SQN will be synchronized in that case?
> > >
> >
> > [Aakash] The design is such that HW would assign sequence number for
> > all cases. HW would then pass this data as a metadata to SW so that it can do
> SW processing with the assigned sequence number.
> 
> As I can see there are two options to fulfill that requirement:
> 
> 1. Introduce a new function that would update sa.sqn value.
> Something like rte_ipsec_sa_update_sqn(...).
> So when metadata from HW arrives, SW can call it and sync sa.sqn with new
> HW value, and then continue with conventional rte_ipsec_crypto_prepare(...);
> 

[Aakash] With option 1, one issue that I can see is that, multiple cores can call rte_ipsec_sa_update_sqn() creating a race condition.

> 2. Introduce new (extended) variants of ipsec_crypto_prepare/process that
> would take SQN (might be something else ?) as extra parameter, something
> like:
> 
> rte_ipcec_xprepare(const struct rte_ipsec_session *ss, struct rte_mbuf *mb[],
>         struct rte_crypto_op *cop[], uint16_t num, /* extra params will be
> there*/);
> 
> Which one is better, hard to say for me off-hand...
> Might be both are needed.
> But probably we don't need to decide right now?
> For me it would be enough if you'll outline the plan to change and extend ipsec
> lib
> API with new data-path functions and probably new flag for session create.
> 

[Aakash] Will go ahead with the option of introducing two new APIs:
1. rte_ipsec_pkt_crypto_seq_no_assign
2. rte_ipsec_pkt_crypto_xprepare
along with the config structure changes and the new flag for session create.

I will update v3 of the announcement with this.

> > > > Initially we are looking at outbound only. But similar kind of use
> > > > case would
> > > be applicable for inbound also.
> > > >
> > > > > >
> > > > > > Signed-off-by: Aakash Sasidharan <asasidharan@marvell.com>
> > > > > > ---
> > > > > >  doc/guides/rel_notes/deprecation.rst | 7 +++++++
> > > > > >  1 file changed, 7 insertions(+)
> > > > > >
> > > > > > diff --git a/doc/guides/rel_notes/deprecation.rst
> > > > > > b/doc/guides/rel_notes/deprecation.rst
> > > > > > index 6948641ff6..bc1d93cca7 100644
> > > > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > > > @@ -133,6 +133,13 @@ Deprecation Notices
> > > > > >    Since these functions are not called directly by the application,
> > > > > >    the API remains unaffected.
> > > > > >
> > > > > > +* ipsec: The rte_ipsec library is updated to support sequence
> > > > > > +number provided
> > > > > > +  by application. A new flag
> > > > > > +``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE``
> > > > > > +is introduced
> > > > > > +  to disable sequence number assignment in lib IPsec.
> > > > > > +  The API rte_ipsec_pkt_crypto_prepare is extended to include
> > > > > > +``sqn`` as an
> > > > > > +  additional parameter allowing application to specify the
> > > > > > +sequence number to be
> > > > > > +  used for the IPsec operation.
> > > > > > +
> > > > > >  * pipeline: The pipeline library legacy API (functions rte_pipeline_*)
> > > > > >    will be deprecated and subsequently removed in DPDK 24.11
> release.
> > > > > >    Before this, the new pipeline library API (functions
> > > > > > rte_swx_pipeline_*)
> > > > > > --
> > > > > > 2.25.1
> > > >
> >

[PATCH v3] doc: announce rte_ipsec API changes

[Aakash Sasidharan]

In case of event mode operations where event device can help in atomic
sequence number increment across cores, sequence number need to be
provided by the application instead of being updated in rte_ipsec or the
PMD.

To support this, two new APIs rte_ipsec_pkt_crypto_sqn_assign and
rte_ipsec_pkt_crypto_xprepare are introduced decoupling the seq no update
functionality from the existing rte_ipsec_pkt_crypto_prepare API.
Additionally, a new flag ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE`` will be
added to allow disabling of internal sequence number update inside IPsec
library.

Signed-off-by: Aakash Sasidharan <asasidharan@marvell.com>
---
 doc/guides/rel_notes/deprecation.rst | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 6948641ff6..9be7b90b94 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -133,6 +133,14 @@ Deprecation Notices
   Since these functions are not called directly by the application,
   the API remains unaffected.
 
+* ipsec: The rte_ipsec library is updated to support sequence number provided
+  by application. To allow the same, two new APIs are being introduced:
+  rte_ipsec_pkt_crypto_sqn_assign and rte_ipsec_pkt_crypto_xprepare. These APIs
+  separate the seq no update functionality from the existing rte_ipsec_pkt_crypto_prepare
+  API. Corresponding configure structure changes are being made for the new APIs.
+  Additionally a new flag ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE`` is
+  introduced to disable sequence number assignment in lib IPsec.
+
 * pipeline: The pipeline library legacy API (functions rte_pipeline_*)
   will be deprecated and subsequently removed in DPDK 24.11 release.
   Before this, the new pipeline library API (functions rte_swx_pipeline_*)
-- 
2.25.1

RE: [PATCH v3] doc: announce rte_ipsec API changes

[Akhil Goyal]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

> Subject: [PATCH v3] doc: announce rte_ipsec API changes
> 
> In case of event mode operations where event device can help in atomic
> sequence number increment across cores, sequence number need to be
> provided by the application instead of being updated in rte_ipsec or the
> PMD.
> 
> To support this, two new APIs rte_ipsec_pkt_crypto_sqn_assign and
> rte_ipsec_pkt_crypto_xprepare are introduced decoupling the seq no update
> functionality from the existing rte_ipsec_pkt_crypto_prepare API.
> Additionally, a new flag ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE`` will be
> added to allow disabling of internal sequence number update inside IPsec
> library.
> 
> Signed-off-by: Aakash Sasidharan <asasidharan@marvell.com>
Acked-by: Akhil Goyal <gakhil@marvell.com>

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 14899 bytes --]

Re: [PATCH v3] doc: announce rte_ipsec API changes

[Thomas Monjalon]

29/07/2024 13:53, Akhil Goyal:
> > Subject: [PATCH v3] doc: announce rte_ipsec API changes
> > 
> > In case of event mode operations where event device can help in atomic
> > sequence number increment across cores, sequence number need to be
> > provided by the application instead of being updated in rte_ipsec or the
> > PMD.
> > 
> > To support this, two new APIs rte_ipsec_pkt_crypto_sqn_assign and
> > rte_ipsec_pkt_crypto_xprepare are introduced decoupling the seq no update
> > functionality from the existing rte_ipsec_pkt_crypto_prepare API.
> > Additionally, a new flag ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE`` will be
> > added to allow disabling of internal sequence number update inside IPsec
> > library.
> > 
> > Signed-off-by: Aakash Sasidharan <asasidharan@marvell.com>
> Acked-by: Akhil Goyal <gakhil@marvell.com>

Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
Acked-by: Radu Nicolau <radu.nicolau@intel.com>

Applied, thanks.

RE: [PATCH v3] doc: announce rte_ipsec API changes

[Konstantin Ananyev]

> In case of event mode operations where event device can help in atomic
> sequence number increment across cores, sequence number need to be
> provided by the application instead of being updated in rte_ipsec or the
> PMD.
> 
> To support this, two new APIs rte_ipsec_pkt_crypto_sqn_assign and
> rte_ipsec_pkt_crypto_xprepare are introduced decoupling the seq no update
> functionality from the existing rte_ipsec_pkt_crypto_prepare API.
> Additionally, a new flag ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE`` will be
> added to allow disabling of internal sequence number update inside IPsec
> library.
> 
> Signed-off-by: Aakash Sasidharan <asasidharan@marvell.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 6948641ff6..9be7b90b94 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -133,6 +133,14 @@ Deprecation Notices
>    Since these functions are not called directly by the application,
>    the API remains unaffected.
> 
> +* ipsec: The rte_ipsec library is updated to support sequence number provided
> +  by application. To allow the same, two new APIs are being introduced:
> +  rte_ipsec_pkt_crypto_sqn_assign and rte_ipsec_pkt_crypto_xprepare. These APIs
> +  separate the seq no update functionality from the existing rte_ipsec_pkt_crypto_prepare
> +  API. Corresponding configure structure changes are being made for the new APIs.
> +  Additionally a new flag ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE`` is
> +  introduced to disable sequence number assignment in lib IPsec.
> +
>  * pipeline: The pipeline library legacy API (functions rte_pipeline_*)
>    will be deprecated and subsequently removed in DPDK 24.11 release.
>    Before this, the new pipeline library API (functions rte_swx_pipeline_*)
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
 

> 2.25.1

RE: [PATCH v3] doc: announce rte_ipsec API changes

[Akhil Goyal]

[-- Attachment #1: Type: text/plain, Size: 2110 bytes --]

Hi Radu,

Can you ack this deprecation notice if not issues?

> Subject: [PATCH v3] doc: announce rte_ipsec API changes
> 
> In case of event mode operations where event device can help in atomic
> sequence number increment across cores, sequence number need to be
> provided by the application instead of being updated in rte_ipsec or the
> PMD.
> 
> To support this, two new APIs rte_ipsec_pkt_crypto_sqn_assign and
> rte_ipsec_pkt_crypto_xprepare are introduced decoupling the seq no update
> functionality from the existing rte_ipsec_pkt_crypto_prepare API.
> Additionally, a new flag ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE`` will be
> added to allow disabling of internal sequence number update inside IPsec
> library.
> 
> Signed-off-by: Aakash Sasidharan <asasidharan@marvell.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index 6948641ff6..9be7b90b94 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -133,6 +133,14 @@ Deprecation Notices
>    Since these functions are not called directly by the application,
>    the API remains unaffected.
> 
> +* ipsec: The rte_ipsec library is updated to support sequence number provided
> +  by application. To allow the same, two new APIs are being introduced:
> +  rte_ipsec_pkt_crypto_sqn_assign and rte_ipsec_pkt_crypto_xprepare. These
> APIs
> +  separate the seq no update functionality from the existing
> rte_ipsec_pkt_crypto_prepare
> +  API. Corresponding configure structure changes are being made for the new
> APIs.
> +  Additionally a new flag ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE`` is
> +  introduced to disable sequence number assignment in lib IPsec.
> +
>  * pipeline: The pipeline library legacy API (functions rte_pipeline_*)
>    will be deprecated and subsequently removed in DPDK 24.11 release.
>    Before this, the new pipeline library API (functions rte_swx_pipeline_*)
> --
> 2.25.1


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 15443 bytes --]

Re: [PATCH v3] doc: announce rte_ipsec API changes

[Radu Nicolau]

[-- Attachment #1: Type: text/plain, Size: 763 bytes --]


On 29-Jul-24 12:47 PM, Aakash Sasidharan wrote:
> In case of event mode operations where event device can help in atomic
> sequence number increment across cores, sequence number need to be
> provided by the application instead of being updated in rte_ipsec or the
> PMD.
>
> To support this, two new APIs rte_ipsec_pkt_crypto_sqn_assign and
> rte_ipsec_pkt_crypto_xprepare are introduced decoupling the seq no update
> functionality from the existing rte_ipsec_pkt_crypto_prepare API.
> Additionally, a new flag ``RTE_IPSEC_SAFLAG_SQN_ASSIGN_DISABLE`` will be
> added to allow disabling of internal sequence number update inside IPsec
> library.
>
> Signed-off-by: Aakash Sasidharan<asasidharan@marvell.com>
> ---
Acked-by: Radu Nicolau <radu.nicolau@intel.com>

[-- Attachment #2: Type: text/html, Size: 1310 bytes --]