[dpdk-dev] [RFC] eventdev: add crypto adapter API header

[Akhil Goyal]

Hi Nithin,

On 12/13/2017 5:58 PM, Nithin Dabilpuram wrote:
> Hi Jerin, Declan,
> 
> On Wednesday 13 December 2017 04:56 PM, Jerin Jacob wrote:
>> -----Original Message-----
>>> Date: Wed, 13 Dec 2017 11:03:06 +0000
>>> From: "Doherty, Declan" <declan.doherty at intel.com>
>>> To: Jerin Jacob <jerin.jacob at caviumnetworks.com>, Abhinandan Gujjar
>>>   <abhinandan.gujjar at intel.com>
>>> CC: dev at dpdk.org, narender.vangati at intel.com, Nikhil Rao
>>>   <nikhil.rao at intel.com>, Gage Eads <gage.eads at intel.com>,
>>>   hemant.agrawal at nxp.com, nidadavolu.murthy at cavium.com,
>>>   nithin.dabilpuram at cavium.com, narayanaprasad.athreya at cavium.com
>>> Subject: Re: [RFC] eventdev: add crypto adapter API header
>>> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>>>   Thunderbird/52.5.0
>>>
>>> On 29/11/2017 11:41 AM, Jerin Jacob wrote:
>>>> -----Original Message-----
>>> ...
>>>
>>>> Adding Declan and Hemant.
>>>>> IMO, RTE_EVENT_CRYPTO_ENQ_MULTI_EVENTQ may not be very useful
>>>> from application perceptive as the scope is very limited.
>>>> In real world use cases, we will be attaching destination event 
>>>> queue information
>>>> to the session, not to the queue pair.
>>>>
>>>>
>>>> IMO, RTE_EVENT_CRYPTO_ENQ_MBUF_MULTI_EVENTQ scheme may not very
>>>> convenient for application writers as
>>>> # it relies on mbuf private area memory. So it has additional memory 
>>>> alloc/free
>>>> requirements.
>>>> # additional overhead for application/PMD to write/read the event 
>>>> queue metadata
>>>> information per packet.
>>>>
>>>> Since we already have meta data structure in the crypto
>>>> area, How about adding the destination event queue attributes
>>>> in the PMD crypto session area and for, _session less_, we can add it
>>>> in rte_crypto_op stricture? This will help in:
>>>>
>>>> # Offloading HW specific meta data generation for event queue 
>>>> attributes
>>>> to slow path.
>>>> # From the application perspective, most likely the event queue 
>>>> parameters
>>>> will be different for each session not per packet nor per event queue
>>>> pair.
>>>>
>>> Hey Jerin,
>> Hey Declan,
>>
>>> given my limited experience with eventdev, your proposed approach in 
>>> general
>>> makes sense to me, in that a packet flow having crypto processing 
>>> done will
>>> always be forwarded the same next stage event queue. So storing this 
>>> state
>>> in the crypto session, or crypto op in the case of sessionless ops, 
>>> seems
>>> sensible.
>>>
>>>> Something like below to share my view. Exact details may be we can 
>>>> work it out.
>>>>
>>> I terms of your proposed changes below, my main concern would be 
>>> introducing
>>> dependencies on the eventdev library into cryptodev, as with this new 
>>> crypto
>>> adpater you will have a dependency on cryptodev in eventdev.
>> I agree with your dependency concern.
>>
>>> I think the best approach would be to support opaque metadata in both 
>>> the
>>> crypto op and crypto session structures, as this would allow other uses
>>> cases to be supported which aren't specific to eventdev to also store
>>> metadata across cryptodev processing.
>> Make sense. Just to add, adding a pointer would be overhead. I think, we
>> can reserve a few bytes as byte array and then later typecast with
>> eventdev api in eventdev library.
>>
>> uint8_t eventdev_metadata[SOMEBYTES];
>>
>> Thoughts?
> Can we have this info in structure rte_crypto_sym_xform instead of 
> rte_crypto_op
> so that for session less or session full we have just one api say as below
> to update the event information.
> 
> rte_cryptodev_sym_xform_event_init(struct rte_crypto_sym_xform *xforms,
>                                                              struct 
> rte_event ev)
> 
> IMO, this is better because for both session_less or session_full modes, 
> app has to
> prepare sym_xform structure and in case of session_less make the struct 
> rte_crypto_op
> point to sym_xform and in session_full pass it to 
> rte_cryptodev_sym_session_create().
> 
> The same can be followed for asym/security sessions in future if needed.
> 
IMO, the metadata that we are talking here is per packet and not per 
session. So moving it to xform will not serve the purpose as it is raw 
information which fills the session parameters of the driver.

And yes security sessions will surely support eventdev crypto adapter APIs.

>>>> ➜ [master][dpdk.org] $ git diff
>>>> diff --git a/lib/librte_cryptodev/rte_crypto.h
>>>> b/lib/librte_cryptodev/rte_crypto.h
>>>> index 3d672fe7d..b44ef673b 100644
>>>> --- a/lib/librte_cryptodev/rte_crypto.h
>>>> +++ b/lib/librte_cryptodev/rte_crypto.h
>>>> @@ -115,6 +115,9 @@ struct rte_crypto_op {
>>>>           uint8_t reserved[5];
>>>>           /**< Reserved bytes to fill 64 bits for future additions */
>>>>
>>>> +#if 0
>>>> + Crypto completion event attribute. For _session less_ crypto 
>>>> enqueue operation,
>>>> + The will field shall be used by application to post the crypto 
>>>> completion event
>>>> + upon the crypto enqueue operation complete.
>>>>
>>>> + Note: In the case of session based crypto operation, SW based 
>>>> crypto adapter can use
>>>> + this memory to store crypto event completion attributes from the PMD
>>>> + specific session area.
>>>> +
>>>> + Note: ev.event_ptr will point to struct rte_crypto_op *op, So
>>>> + that core can free the ops memory on event_dequeue().
>>>> +#endif
>>>> +
>>>> +       struct rte_event ev;
>>>>
>>>>           struct rte_mempool *mempool;
>>>>           /**< crypto operation mempool which operation is allocated 
>>>> from
>>>>    * */
>>>> diff --git a/lib/librte_cryptodev/rte_cryptodev.h
>>>> b/lib/librte_cryptodev/rte_cryptodev.h
>>>> index dade5548f..540b29e66 100644
>>>> --- a/lib/librte_cryptodev/rte_cryptodev.h
>>>> +++ b/lib/librte_cryptodev/rte_cryptodev.h
>>>> @@ -945,6 +945,13 @@ rte_cryptodev_sym_session_init(uint8_t dev_id,
>>>>                           struct rte_crypto_sym_xform *xforms,
>>>>                           struct rte_mempool *mempool);
>>>>
>>>> +#if 0
>>>> + Create PMD specific session meta data for the destination event queue
>>>> + attribute to post the crypto completion event on crypto work 
>>>> complete.
>>>> +#endif
>>>> +int
>>>> +rte_cryptodev_sym_session_event_init(uint8_t dev_id,
>>>> +                       struct rte_cryptodev_sym_session *sess,
>>>> +                       struct rte_crypto_sym_xform *xforms,
>>>> +                       struct rte_mempool *mempool,
>>>> +                       struct rte_event ev);
>>>> +
>>>>    /**
>>>>     * Frees private data for the device id, based on its device type,
>>>>     * returning it to its mempool.
>>>>
>>>>
>>>>> + *
>>>>> + * The metadata offset is used to configure the location of the
>>>>> + * rte_event_crypto_metadata structure within the mbuf's private 
>>>>> metadata area.
>>>>> + *
>>>>> + * When the application sends crypto operations to the adapter,
>>>>> + * the crypto queue pair identifier needs to be specified, 
>>>>> similarly eventdev
>>>>> + * parameters such as the flow id, scheduling type etc are needed 
>>>>> by the
>>>>> + * adapter when it enqueues mbufs from completed crypto operations 
>>>>> to eventdev.
>>>>> + */
>>>>> +
>>>>> +#ifdef __cplusplus
>>>>> +extern "C" {
>>>>> +#endif
>>>>> +
>>>>> +#include <stdint.h>
>>>>> +#include <rte_service.h>
>>>>> +
>>>>> +#include "rte_eventdev.h"
>>>>> +
>>>>> +#define RTE_EVENT_CRYPTO_ADAPTER_MAX_INSTANCE 32
>>>>> +
>>>>> + /**
>>>>> + * @warning
>>>>> + * @b EXPERIMENTAL: this enum may change without prior notice
>>>>> + *
>>>>> + * Crypto event queue conf type
>>>>> + */
>>>>> +enum rte_event_crypto_conf_type {
>>>>> +    RTE_EVENT_CRYPTO_CONF_TYPE_EVENT = 1,
>>>>> +    /**< Refer RTE_EVENT_CRYPTO_ADAPTER_CAP_MULTI_EVENTQ */
>>>>> +    RTE_EVENT_CRYPTO_CONF_TYPE_MBUF,
>>>>> +    /**< Refer RTE_EVENT_CRYPTO_ADAPTER_CAP_MBUF_MULTI_EVENTQ */
>>>>> +    RTE_EVENT_CRYPTO_CONF_TYPE_MAX
>>>>> +};
>>>> See above.
>>>>
>>>>> +
>>>>> + /**
>>>>> + * @warning
>>>>> + * @b EXPERIMENTAL: this enum may change without prior notice
>>>>> + *
>>>>> + * Crypto event adapter type
>>>>> + */
>>>>> +enum rte_event_crypto_adapter_type {
>>>>> +    RTE_EVENT_CRYPTO_ADAPTER_RX_ONLY = 1,
>>>>> +    /**< Start only Rx part of crypto adapter.
>>>>> +    * Packets dequeued from cryptodev are new to eventdev and
>>>>> +    * events will be treated as RTE_EVENT_OP_NEW */
>>>>> +    RTE_EVENT_CRYPTO_ADAPTER_RX_TX,
>>>>> +    /**< Start both Rx & Tx part of crypto adapter.
>>>>> +    * Packet's event context will be retained and
>>>>> +    * event will be treated as RTE_EVENT_OP_FORWARD */
>>>>> +};
>>>> How about leveraging ev.op based schematics as mentioned above?
>>>>
> 
>