Re: [dpdk-dev] cryptodev - Session and queue pair relationship

[Akhil Goyal <akhil.goyal@nxp.com>]

On 2/8/2017 2:22 AM, Declan Doherty wrote:
> On 06/02/17 13:35, Akhil Goyal wrote:
>> Hi,
>>
> Hey Akhil, see my thoughts inline
>
>> I have some issues w.r.t the mapping sessions and queue pairs.
>>
>> As per my understanding:
>> - Number of sessions may be large – they are independent of number of
>> queue pairs
>
> Yes, cryptodev assumes no implicit connection between sessions and
> queue pairs, the current PMDs just use the crypto session to store the
> immutable data (keys etc) for a particular crypto transform or chain of
> transforms in a format specific to that PMD with no statefull information.
>
>> - Queue pairs are L-core specific
>
> Not exactly, queue pairs like ethdev queues are not thread safe, so we
> assume that only a single l-core will be using a queue pair at any time
> unless the application layer has introduce a locking mechanism to
> provide thread safety.
>
>> - Depending on the implementation, one queue pair can be mapped to many
>> sessions. Or, Only one queue pair for every session- especially in the
>> systems having large number of queues (hw).
>
> Currently none of the software crypto PMDs or Intel QuickAssist hardware
> accelerated PMD make any assumptions regarding coupling/mapping of
> sessions to queue pairs, so today a users could freely change the queue
> pair which a session is processed on, or even go as far using the  ame
> session for processing on different queue simultaneously as the sessions
> are stateless, obviously this could introduce issues for statefull
> higher level protocol using the cryptodev PMD service but the cryptodev
> API doesn't prohibit this usage model.
>
>
>> - Sessions can be created on the fly – typical rekeying use-cases.
>> Generally done by the control threads.
>>
>
> Sure, there is no restriction on session creation other than an element
> being free in the mempool which the session is being created on.
>
>> There seems to be no straight way for the underlying driver
>> implementation to know, what all sessions are mapped to a particular
>> queue pair. The session and queue pair information is first time exposed
>> in the enqueue command.
>>
>> One of the NXP Crypto Hardware drivers uses per session data structures
>> (descriptors) which need to be configured for hardware queues.  Though
>> this information can be extracted from the first enqueue command for a
>> particular session, it will add checks in the data path. Also, it will
>> bring down the connection setup rate.
>
> We haven't had to support this model of coupling sessions to queue pairs
> in any PMDs before. If I understand correctly, in the hardware model you
> need to support a queue pair can only be configured to support the
> processing of a single session at any one time and it only supports that
> session until it is reconfigured, is this correct? So if a session needs
> to be re-keyed the queue pair would need to be reconfigured?
yes it is correct.
>
>>
>> In the API rte_cryptodev_sym_session_create(), we create session on a
>> particular device, but there is no information of queue pair being
>> shared.
>>
>> 1. We want to propose to change the session create/config API to also
>> take queue pair id as argument.
>> struct rte_cryptodev_sym_session *
>> rte_cryptodev_sym_session_create(uint8_t dev_id,
>>                               struct rte_crypto_sym_xform *xform) to
>> also take “uint16_t qp;”
>>
>> This will also return “in-use” error, if the underlying hardware only
>> support 1 session/descriptor per qp.
>
> I my mind the idea of coupling the session_create function to the queue
> pair of a device doesn't feel right as it would certainly put
> unnecessary constraint on all existing PMDs queue pairs.
>
> One possible approach would be to extend the the queue_pair_setup
> function to take an opaque parameter which would allow you to pass a
> session through and would be  an approach more in keeping with the
> cryptodev current model, but you would then still need to verify that
> the operations being enqueued have the same session as the configured
> device, assuming that the packet are being enqueued from the host.
>
> If you need to re-key or change the session you could re-initialize the
> queue pair while the device is still active, but stopping the queue pair.
>
> Following a sequence something like:
> stop_qp()
> setup_qp()
> start_qp()
>
>
> Another option Fiona suggested would be to add 2 new APIs
>
> rte_cryptodev_queue_pair_attach_sym_session/queue_pair_detach_sym_session this
> would allow dynamic attaching of one or more sessions to device if it
> supported this sort of static mapping of sessions to queue pairs.
>
>
>>
>> 2. Currently the application configures the *nb_descriptors* in the
>> *rte_cryptodev_queue_pair_setup*. Should we add the queue pair
>> capability API?
>>
>
> Regarding capabilities, I think this should be just propagated through
> the device capabilities, something like a max number of session mapped
> per queue pair, which would be zero for all/most current devices, and
> could be 1 or greater for your device. This is assuming that all queue
> pairs can all support the same crypto transforms capabilities and that
> different queue pairs have different capabilities which could get very
> messy to discover.
>
>>
>> Please share your feedback, I will submit the patch accordingly.
>>
>> Regards,
>> Akhil
>>
>>
>>
>
>
Thanks for your feedback Declan,
The suggestion from Fiona looks good. Should I send the patch for this 
or is it already in discussion in some different thread?

Also, if this new API is added, there would be corresponding change in 
the ipsec-secgw application as well.
This API should be optional and underlying implementation may or may not 
implement this API.

Regards,
Akhil