From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id D7012F923 for ; Mon, 13 Feb 2017 16:09:28 +0100 (CET) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Feb 2017 07:09:27 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,156,1484035200"; d="scan'208";a="58116964" Received: from irsmsx106.ger.corp.intel.com ([163.33.3.31]) by orsmga004.jf.intel.com with ESMTP; 13 Feb 2017 07:09:26 -0800 Received: from irsmsx101.ger.corp.intel.com ([169.254.1.112]) by IRSMSX106.ger.corp.intel.com ([169.254.8.197]) with mapi id 14.03.0248.002; Mon, 13 Feb 2017 15:09:25 +0000 From: "Trahe, Fiona" To: Akhil Goyal , "Doherty, Declan" , "dev@dpdk.org" , "De Lara Guarch, Pablo" , "Jain, Deepak K" CC: "hemant.agrawal@nxp.com" , "Trahe, Fiona" Thread-Topic: cryptodev - Session and queue pair relationship Thread-Index: AQHSgYQbBbBxxCwTukqMLAN/x/DxKqFnCsiAgAAApNCAAAJgAA== Date: Mon, 13 Feb 2017 15:09:25 +0000 Message-ID: <348A99DA5F5B7549AA880327E580B43589187B20@IRSMSX101.ger.corp.intel.com> References: <2d48b97f-bb39-09e9-5ea2-1fc265459a87@intel.com> <348A99DA5F5B7549AA880327E580B43589187ABE@IRSMSX101.ger.corp.intel.com> In-Reply-To: <348A99DA5F5B7549AA880327E580B43589187ABE@IRSMSX101.ger.corp.intel.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYzQxZTM2YjQtODJjMi00OTI2LTg0YTgtZWE0NjFlNzg5ZDBiIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6ImhYS3hVMUx1UUk3QVI4SGYybnI1Y2pWMFN4VThNSzVcL1crdG9EVTR6R0dFPSJ9 x-ctpclassification: CTP_IC x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] cryptodev - Session and queue pair relationship X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Feb 2017 15:09:29 -0000 Hi Akhil,=20 > -----Original Message----- > From: Trahe, Fiona > Sent: Monday, February 13, 2017 2:45 PM > To: Akhil Goyal ; Doherty, Declan > ; dev@dpdk.org; De Lara Guarch, Pablo > ; Jain, Deepak K > Cc: hemant.agrawal@nxp.com; Trahe, Fiona > Subject: RE: cryptodev - Session and queue pair relationship >=20 > Hi Akhil >=20 > > -----Original Message----- > > From: Akhil Goyal [mailto:akhil.goyal@nxp.com] > > Sent: Monday, February 13, 2017 2:39 PM > > To: Doherty, Declan ; dev@dpdk.org; De Lara > > Guarch, Pablo ; Jain, Deepak K > > > > Cc: hemant.agrawal@nxp.com; Trahe, Fiona > > 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 o= f > > >> queue pairs > > > > > > Yes, cryptodev assumes no implicit connection between sessions and > > > queue pairs, the current PMDs just use the crypto session to store th= e > > > immutable data (keys etc) for a particular crypto transform or chain = of > > > transforms in a format specific to that PMD with no statefull informa= tion. > > > > > >> - Queue pairs are L-core specific > > > > > > Not exactly, queue pairs like ethdev queues are not thread safe, so w= e > > > assume that only a single l-core will be using a queue pair at any ti= me > > > 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 t= he > > >> systems having large number of queues (hw). > > > > > > Currently none of the software crypto PMDs or Intel QuickAssist hardw= are > > > accelerated PMD make any assumptions regarding coupling/mapping of > > > sessions to queue pairs, so today a users could freely change the que= ue > > > pair which a session is processed on, or even go as far using the am= e > > > session for processing on different queue simultaneously as the sessi= ons > > > are stateless, obviously this could introduce issues for statefull > > > higher level protocol using the cryptodev PMD service but the cryptod= ev > > > 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 eleme= nt > > > 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 exp= osed > > >> in the enqueue command. > > >> > > >> One of the NXP Crypto Hardware drivers uses per session data structu= res > > >> (descriptors) which need to be configured for hardware queues. Thou= gh > > >> this information can be extracted from the first enqueue command for= a > > >> particular session, it will add checks in the data path. Also, it wi= ll > > >> bring down the connection setup rate. > > > > > > We haven't had to support this model of coupling sessions to queue pa= irs > > > 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 t= hat > > > session until it is reconfigured, is this correct? So if a session ne= eds > > > 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 als= o > > >> 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 onl= y > > >> support 1 session/descriptor per qp. > > > > > > I my mind the idea of coupling the session_create function to the que= ue > > > 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 t= he > > > queue pair while the device is still active, but stopping the queue p= air. > > > > > > 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 throug= h > > > the device capabilities, something like a max number of session mappe= d > > > 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 queu= e > > > pairs can all support the same crypto transforms capabilities and tha= t > > > different queue pairs have different capabilities which could get ver= y > > > 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? >=20 > 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=20 struct rte_cryptodev_info.sym I'm assuming where there is a limit this would be the same for all qps on t= he 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 a= ttach API or not.=20 This will cause an ABI breakage, so must be flagged first before changing. >=20 > > > > 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 no= t > > implement this API. > > > > Regards, > > Akhil > >