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

[Konstantin Ananyev <konstantin.ananyev@huawei.com>]

> > > > > 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
> > >
>