Hi Akhil > -----Original Message----- > From: Akhil Goyal [mailto:akhil.go...@nxp.com] > Sent: Monday, February 13, 2017 2:39 PM > To: Doherty, Declan <declan.dohe...@intel.com>; dev@dpdk.org; De Lara > Guarch, Pablo <pablo.de.lara.gua...@intel.com>; Jain, Deepak K > <deepak.k.j...@intel.com> > Cc: hemant.agra...@nxp.com; Trahe, Fiona <fiona.tr...@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 >
