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

["Trahe, Fiona" <fiona.trahe@intel.com>]

Hi Akhil

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Monday, February 13, 2017 2:39 PM
> To: Doherty, Declan <declan.doherty@intel.com>; dev@dpdk.org; De Lara
> Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Jain, Deepak K
> <deepak.k.jain@intel.com>
> Cc: hemant.agrawal@nxp.com; Trahe, Fiona <fiona.trahe@intel.com>
> Subject: Re: cryptodev - Session and queue pair relationship
> 
> 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_sess
> ion 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?

No, it's not under discussion in any other thread that I'm aware of.
Go ahead and send it.

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