[dpdk-dev] [RFC] ipsec: new library for IPsec data-path processing

[Ananyev, Konstantin]

Hi Jerin,


> >
> > >
> > > Anyway, let's pretend we found some smart way to distribute inbound packets for the same SA to multiple HW queues/CPU
> cores.
> > > To make ipsec processing for such case to work correctly just atomicity on check/update segn/replay_window is not enough.
> > > I think it would require some extra synchronization:
> > > make sure that we do final packet processing (seq check/update) at the same order as we received the packets
> > > (packets entered ipsec processing).
> > > I don't really like to introduce such heavy mechanisms on SA level,  after all it supposed to be light and simple.
> > > Though we plan CTX level API to support such scenario.
> > > What I think would be useful addition for SA level API - have an ability to do one update seqn/replay_window and multiple checks
> concurrently.
> > >
> > > > In case of ingress also, the same problem exists. We will not be able to use RSS and spread the traffic to multiple cores.
> Considering
> > > > IPsec being CPU intensive, this would limit the net output of the chip.
> > > That's true - but from other side implementation can offload heavy part
> > > (encrypt/decrypt, auth) to special HW (cryptodev).
> > > In that case single core might be enough for SA and extra synchronization would just slowdown things.
> > > That's why I think it should be configurable  what behavior (ST or MT) to use.
> > I do agree that these are the issues that we need to address to make the
> > library MT safe. Whether the extra synchronization would slow down things is
> > a very subjective question and will heavily depend on the platform. The
> > library should have enough provisions to be able to support MT without
> > causing overheads to ST. Right now, the library assumes ST.
> 
> 
> I agree with Anoob here.
> 
> I have two concerns with librte_ipsec as a separate library
> 
> 1) There is an overlap with rte_security and new proposed library.

I don't think there really is an overlap.
rte_security is a 'framework for management and provisioning of security protocol operations offloaded to hardware based devices'.
While rte_ipsec is aimed to be a library for IPsec data-path processing.
There is no plans for rte_ipsec to 'obsolete' rte_security.
Quite opposite rte_ipsec supposed to work with both rte_cryptodev and rte_security APIs (devices).
It is possible to have an SA that would use both crypto and  security devices.
Or to have an SA that would use multiple crypto devs
(though right now it is up the user level to do load-balancing logic). 

> For IPsec, If an application needs to use rte_security for HW
> implementation and and application needs to use librte_ipsec for
>  SW implementation then it is bad and a lot duplication of work on
> he slow path too.

The plan is that application would need to use just rte_ipsec API for all data-paths
(HW/SW, lookaside/inline). 
Let say right now there is rte_ipsec_inline_process() function if user
prefers to use inline security device to process given group packets,
and rte_ipsec_crypto_process(/prepare) if user decides to use
lookaside security or simple crypto device for it.

> 
> The rte_security spec can support both inline and look-aside IPSec
> protocol support.

AFAIK right now rte_security just provides API to create/free/manipulate security sessions.
I don't see how it can support all the functionality mentioned above,
plus SAD and SPD.

> 
> 2) This library is tuned for fat CPU core in mind like single SA on core
> etc. Which is fine for x86 servers and arm64 server category of machines
> but it does not work very well with NPU class of SoC or FPGA.
> 
> As there  are the different ways to implement the IPSec, For instance,
> use of eventdev can help in situation for handling millions of SA and
> equence number of update and anti reply check can be done by leveraging
> some of the HW specific features like
> ORDERED, ATOMIC schedule type(mapped as eventdev feature)in HW with PIPELINE model.
> 
> # Issues with having one SA one core,
> - In the outbound side, there could be multiple flows using the same SA.
>   Multiple flows could be processed parallel on different lcores,
> but tying one SA to one core would mean we won't be able to do that.
> 
> - In the inbound side, we will have a fat flow hitting one core. If
>   IPsec library assumes single core, we will not be able to to spread
> fat flow to multiple cores. And one SA-one core would mean all ports on
> which we would expect IPsec traffic has to be handled by that core.

I suppose that all refers to the discussion about MT safe API for rte_ipsec, right?
If so, then as I said in my reply to Anoob: 
We will try to make API usable in MT environment for v1,
so you can review and provide comments at early stages.

> 
> I have made a simple presentation. This presentation details ONE WAY to
> implement the IPSec with HW support on NPU.
> 
> https://docs.google.com/presentation/d/1e3IDf9R7ZQB8FN16Nvu7KINuLSWMdyKEw8_0H05rjj4/edit?usp=sharing
> 

Thanks, quite helpful.
Actually from page 3, it looks like your expectations don't contradict in general with proposed API:

...
} else if (ev.event_type == RTE_EVENT_TYPE_LCORE && ev.sub_event_id == APP_STATE_SEQ_UPDATE) {
                        sa = ev.flow_queue_id;                                  
                        /* do critical section work per sa */               
                        do_critical_section_work(sa);  

[KA] that's the place where I expect either 
rte_ipsec_inline_process(sa, ...); OR rte_ipsec_crypto_prepare(sa, ...);      	
would be called.
                                                                               
                     /* Issue the crypto request and generate the following on crypto work completion */
[KA] that's the place where I expect rte_ipsec_crypto_process(...) be invoked.

                        ev.flow_queue_id = tx_port;                                  
                        ev.sub_event_id = tx_queue_id;            
                        ev.sched_sync = RTE_SCHED_SYNC_ATOMIC;                 
                        rte_cryptodev_event_enqueue(cryptodev, ev.mbuf, eventdev, ev);                
                }


> I am not saying this should be the ONLY way to do as it does not work
> very well with non NPU/FPGA class of SoC.
> 
> So how about making the proposed IPSec library as plugin/driver to
> rte_security.

As I mentioned above, I don't think that pushing whole IPSec data-path into rte_security
is the best possible approach.
Though I probably understand your concern:
In RFC code we always do whole prepare/process in SW (attach/remove ESP headers/trailers, so paddings etc.),
i.e. right now only device types: RTE_SECURITY_ACTION_TYPE_NONE and RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO are covered.
Though there are devices where most of prepare/process can be done in HW
(RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL/RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL),
plus in future could be devices where prepare/process would be split between HW/SW in a custom way.
Is that so?
To address that issue I suppose we can do:
1. Add support for RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL and RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL
    security devices into ipsec.
    We planned to do it anyway, just don't have it done yet.
2. For custom case - introduce RTE_SECURITY_ACTION_TYPE_INLINE_CUSTOM and RTE_SECURITY_ACTION_TYPE_LOOKASIDE_CUSTOM
    and add into rte_security_ops   new functions: 
    uint16_t lookaside_prepare(struct rte_security_session *sess, struct rte_mbuf *mb[], struct struct rte_crypto_op *cop[], uint16_t num);
    uint16_t lookaside_process(struct rte_security_session *sess, struct rte_mbuf *mb[], struct struct rte_crypto_op *cop[], uint16_t num);
    uint16_t inline_process(struct rte_security_session *sess, struct rte_mbuf *mb[], struct struct rte_crypto_op *cop[], uint16_t num);
    So for custom HW, PMD can overwrite normal prepare/process behavior.

> This would give flexibly for each vendor/platform choose to different
> IPse implementation based on HW support WITHOUT CHANGING THE APPLICATION
> INTERFACE.

Not sure what API changes you are referring to?
As I am aware we do introduce new API, but all existing APIs remain in place.

> 
> IMO, rte_security IPsec look aside support can be simply added by
> creating the virtual crypto device(i.e move the proposed code to the virtual crypto device)
> likewise inline support
> can be added by the virtual ethdev device.

That's probably possible and if someone would like to introduce such abstraction - NP in general
(though my suspicion - it might be too heavy to be really useful).
Though I don't think it should be the only possible way for the user to enable IPsec data-processing inside his app.
Again I guess such virtual-dev will still use rte_ipsec inside.

> This would avoid the need for
> updating ipsec-gw application as well i.e unified interface to application.

I think - it would  really good to simplify existing ipsec-secgw sample app.
Some parts of it seems unnecessary complex to me.
One of the reasons for it -  we don't really have an unified (and transparent) API for ipsec data-path.
Let's look at ipsec_enqueue() and related code (examples/ipsec-secgw/ipsec.c:365)
It is huge (and ugly) -  user has to handle dozen different cases just to enqueue packet for IPsec processing.
One of the aims of rte_ipsec library - hide all that complexities inside the library and provide to
the upper layer clean and transparent API.

> 
> If you don't like the above idea, any scheme of plugin based
> implementation would be fine so that vendor or platform can choose its own implementation.
> It can be based on partial HW implement too. i.e SA look can be used in SW, remaining stuff in HW
> (for example IPsec inline case)

I am surely ok with the idea to give vendors an ability to customize implementation
and enable their HW capabilities.
Do you think proposed additions to the rte_security would be  enough,
or something extra is needed?

Konstantin


> 
> # For protocols like UDP, it makes sense to create librte_udp as there
> no much HW specific offload other than ethdev provides.
> 
> # PDCP could be another library to offload to HW, So talking
> rte_security path makes more sense in that case too.
> 
> Jerin