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

[Trahe, Fiona]

Hi Akhil, 

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

It may be useful to add max_nb_sessions_per_qp to 
struct rte_cryptodev_info.sym
I'm assuming where there is a limit this would be the same for all qps on the device?
0 meaning unlimited, >0 meaning limited to that number.
This could be used by the application to know whether it needs to use the attach API or not. 
This will cause an ABI breakage, so must be flagged first before changing.

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