From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 3E767F932 for ; Mon, 13 Feb 2017 15:44:40 +0100 (CET) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Feb 2017 06:44:39 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,156,1484035200"; d="scan'208";a="64538279" Received: from irsmsx109.ger.corp.intel.com ([163.33.3.23]) by fmsmga005.fm.intel.com with ESMTP; 13 Feb 2017 06:44:38 -0800 Received: from irsmsx101.ger.corp.intel.com ([169.254.1.112]) by IRSMSX109.ger.corp.intel.com ([169.254.13.44]) with mapi id 14.03.0248.002; Mon, 13 Feb 2017 14:44:33 +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/DxKqFnCsiAgAAApNA= Date: Mon, 13 Feb 2017 14:44:31 +0000 Message-ID: <348A99DA5F5B7549AA880327E580B43589187ABE@IRSMSX101.ger.corp.intel.com> References: <2d48b97f-bb39-09e9-5ea2-1fc265459a87@intel.com> In-Reply-To: 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 14:44:40 -0000 Hi Akhil > -----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 >=20 > 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 informati= on. > > > >> - 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 hardwar= e > > 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 session= s > > 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 expos= ed > >> in the enqueue command. > >> > >> One of the NXP Crypto Hardware drivers uses per session data structure= s > >> (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 pair= s > > in any PMDs before. If I understand correctly, in the hardware model yo= u > > 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 tha= t > > session until it is reconfigured, is this correct? So if a session need= s > > 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 pai= r. > > > > 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. >=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 not > implement this API. >=20 > Regards, > Akhil >=20