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

[Aakash Sasidharan <asasidharan@marvell.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(...);
> 

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