Re: [dpdk-dev] [PATCH 1/4] cryptodev: Initial DPDK Crypto APIs and device framework release

[Declan Doherty <declan.doherty@intel.com>]

On 20/08/15 20:07, Neil Horman wrote:
> On Thu, Aug 20, 2015 at 03:07:20PM +0100, Declan Doherty wrote:
>> Co-authored-by: Des O Dea <des.j.o.dea@intel.com>
>> Co-authored-by: John Griffin <john.griffin@intel.com>
>> Co-authored-by: Fiona Trahe <fiona.trahe@intel.com>
>>
>> This patch contains the initial proposed APIs and device framework for
>> integrating crypto packet processing into DPDK.
>>
>> features include:
>>   - Crypto device configuration / management APIs
>>   - Definitions of supported cipher algorithms and operations.
>>   - Definitions of supported hash/authentication algorithms and
>>     operations.
>>   - Crypto session management APIs
>>   - Crypto operation data structures and APIs allocation of crypto
>>     operation structure used to specify the crypto operations to
>>     be performed  on a particular mbuf.
>>   - Extension of mbuf to contain crypto operation data pointer and
>>     extra flags.
>>   - Burst enqueue / dequeue APIs for processing of crypto operations.
>>
>> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
>
> Hey, only had a qick read so some of this might be off base, but a few comments
> in line
>

Hey Neil, thanks for the feedback, I've reply in line below.


>> <snip>
>
> So, this seems a bit...not sure what the word is..specific perhaps?  For an API.
> That is to say, if a underlying device supports performing multiple operations
> in a single transaction, I'm not sure that should be exposed in this way.  As
> the number of devices and chain combinations grow, so too will this list, and if
> there are multiple simmilar (but distinct) chain operations, an application will
> have to know which chains are applicable to which devices which is sub-optimal
>
> Instead, perhaps it would be better to simply ennumerate the list of crypto
> primitives that a device supports (HASH/CIPHER/etc), and allow the application
> to define the desired chain when creating a session.  That is to say, an
> appilcation can create a session that requests a given chain of operations
> (using an array of the primitive enum perhaps).  The implementing PMD is then
> responsible for implementing that chain in hardware or software if need be.  If
> you need to report on the disposition of the implementation, you can do so via
> return code (i.e. SESSION_OK_HW_IMPL or SESSION_OK_SW_IMPL).
>
> Using this method, more complex operations can be implemented with chain macros
> (e.g. AEAD is just an array of primitives [CIPHER, HASH])
>

ok, we may have let the hardware which we have available to us bias the 
scope of the API a little. I guess something similar to the approach 
taken by the OCF in BSD may be appropriate. This has a more generic 
structure for specifying any type of crypto operations and a next 
pointer to the next operations in the chain, this would make the 
provisioning of operations and there chaining much more generic? I'll 
put together a prototype and come back to the mailing list with it for 
further comment.

>> <snip>
> These are all defined in rte_ethdev.  You should just move those to a public
> place rather then re-creating them
>

Will do.

>> <snip>
> Shouldn't there be some interlock here if a device is being removed to block on
> closure of all the sessions that may be open against it, and serializtion
> againsnt any list modifications for tracking of these devices?
>

Good point, I hadn't considered the releasing of session resources on
device closure. I'll also look further at the device management, at this 
point we just had the minimal infrastructure in place to allow
functional testing.

>> <snip>
> So, I'm a bit confused here.  How do you communicate with a cryptodev.  I see
> you're creating queue pairs here, which I think are intended for input/output,
> but you also allow the creation of sessions.  The former seems to have no
> linkage to the latter, so you have sessionless queue pairs and sessions without
> method to perform operations on?  I'm clearly missing something, but I can't see
> the relationship.
>

All data path communication with the cryptodev is done via the burst 
enqueue and dequeue functions, see the rte_cryptodev.h .

So the session structure is a container for all the immutable data used 
to perform a crypto operation on a particular packet flow. We have a 
crypto operation data struct which contains the mutable data such as 
data offsets and lengths, initialization vectorsand the location of the 
digest, and additional data etc.

The crypto operation can be session based or session-less. If session 
based, it will have a pointer to a valid session for use with a specific 
cryptodev otherwise all the immutable crypto operation data parameters 
need to be set in the crypto operation data struct which is attached to 
each mbuf which will be enqueued with the cryptodev
for processing.

Once the crypto operation data struct is completed, it is attached
to the specific mbuf which contains the data to be operated on. The 
cryptodev can then handle a burst of mbuf's for processing enqueued on 
it using the burst_enqueue function.

The data associations from the mbuf, crypto_op and crypto_session are 
connected as below:

mbuf
   |-> data_payload etc..
   --> crypto_op
          |-> crypto session
          |-> digest data ptr / length
          |-> iv data ptr / length
          --> data offsets

One of the main reason for using session is that there are some
computationally costly precomputes which are required for authentication 
algos and key expansions for the cipher algos. By using a crypto session 
you can do these calculation once outside of the data-path and then the 
session can be used for every packet in a flow which requires that 
particular crypto transformation.

The one drawback to this is that if you are dealing with a system that 
has tens of thousands or millions of flows, then you end up caching a 
huge amount of data in the crypto session which is more than likely also 
stored in the application layer above.

The cryptodev can then handle burst of mbuf's for processing passed into 
it using the burst_enqueue function. The rte_cryptodev_burst_enqueue is 
analogous to an rte_eth_dev_tx_burst, but we use the enqueue/dequeue 
naming convention as tx/rx don't really what sense in the case of a 
crypto device where the input and output queues are intrinsically linked.


Cheers
Declan