From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id D7C2F1DB9 for ; Fri, 5 Oct 2018 13:06:07 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Oct 2018 04:06:06 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,344,1534834800"; d="scan'208";a="78963132" Received: from irsmsx102.ger.corp.intel.com ([163.33.3.155]) by orsmga008.jf.intel.com with ESMTP; 05 Oct 2018 04:05:31 -0700 Received: from irsmsx106.ger.corp.intel.com ([169.254.8.45]) by IRSMSX102.ger.corp.intel.com ([169.254.2.67]) with mapi id 14.03.0319.002; Fri, 5 Oct 2018 12:05:30 +0100 From: "Ananyev, Konstantin" To: "dev@dpdk.org" CC: "De Lara Guarch, Pablo" , Akhil Goyal , "Doherty, Declan" , "Ravi Kumar" , Jerin Jacob , "Zhang, Roy Fan" , "Trahe, Fiona" , Tomasz Duszynski , Hemant Agrawal , Natalie Samsonov , "Dmitri Epshtein" , Jay Zhou Thread-Topic: [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session Thread-Index: AQHUO9Lg2zx23KOBQ0qIus6aAigF26UQvj0A Date: Fri, 5 Oct 2018 11:05:29 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772580102FE270D@IRSMSX106.ger.corp.intel.com> References: <1535132906-5167-1-git-send-email-konstantin.ananyev@intel.com> In-Reply-To: <1535132906-5167-1-git-send-email-konstantin.ananyev@intel.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOTQxOTIyNzMtMGQ0YS00YzhmLTkwZDUtNDcxNWExZDg5YjBlIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiWFRhN2RWdld6QUNWakpIb1JkMVwvUjZEQzhVNnlXNFFkYnZCOFphNnhjNG9lVVwvRVhFd3UwTW5tbUNRUUE3TjBZIn0= x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session 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: Fri, 05 Oct 2018 11:06:08 -0000 Hi everyone, > This RFC for proposes several changes inside rte_cryptodev_sym_session. > Note that this is just RFC not a complete patch, so for now > I modified only the librte_cryptodev itself, > some cryptodev PMD, test-crypto-perf and ipsec-secgw example. > Proposed changes means ABI/API breakage inside cryptodev, > so looking for feedback from crypto-dev lib and crypto-PMD maintainiers. > Below are details and reasoning for proposed changes. >=20 > 1.rte_cryptodev_sym_session_init()/ rte_cryptodev_sym_session_clear() > operate based on cytpodev device id, though inside > rte_cryptodev_sym_session device specific data is addressed > by driver id (not device id). > That creates a problem with current implementation when we have > two or more devices with the same driver used by the same session. > Consider the following example: >=20 > struct rte_cryptodev_sym_session *sess; > rte_cryptodev_sym_session_init(dev_id=3DX, sess, ...); > rte_cryptodev_sym_session_init(dev_id=3DY, sess, ...); > rte_cryptodev_sym_session_clear(dev_id=3DX, sess); >=20 > After that point if X and Y uses the same driver, > then sess can't be used by device Y any more. > The reason for that - driver specific (not device specific) > data per session, plus there is no information > how many device instances use that data. > Probably the simplest way to deal with that issue - > add a reference counter per each driver data. >=20 > 2.rte_cryptodev_sym_session_set_user_data() and > rte_cryptodev_sym_session_get_user_data() - > with current implementation there is no defined way for the user to > determine what is the max allowed size of the private data. > Even within rte_cryptodev_sym_session_set_user_data() we just blindly > copying user provided data without checking memory boundaries violation= . > To overcome that issue I added 'uint16_t priv_size' into > rte_cryptodev_sym_session structure. >=20 > 3.rte_cryptodev_sym_session contains an array of variable size for > driver specific data. > Though number of elements in that array is determined by static > variable nb_drivers, that could be modified by > rte_cryptodev_allocate_driver(). > That construction seems to work ok so far, as right now users register > all their PMDs at startup, though it doesn't mean that it would always > remain like that. > To make it less error prone I added 'uint16_t nb_drivers' into the > rte_cryptodev_sym_session structure. > At least that allows related functions to check that provided > driver id wouldn't overrun variable array boundaries, > again it allows to determine size of already allocated session > without accessing global variable. >=20 > 4.#2 and #3 above implies that now each struct rte_cryptodev_sym_session > would have sort of readonly type data (init once at allocation time, > keep unmodified through session life-time). > That requires more changes in current cryptodev implementation: > Right now inside cryptodev framework both rte_cryptodev_sym_session > and driver specific session data are two completely different sctrucure= s > (e.g. struct struct null_crypto_session and struct null_crypto_session)= . > Though current cryptodev implementation implicitly assumes that driver > will allocate both of them from within the same mempool. > Plus this is done in a manner that they override each other fields > (reuse the same space - sort of implicit C union). > That's probably not the best programming practice, > plus make impossible to have readonly fields inside both of them. > So to overcome that situation I changed an API a bit, to allow > to use two different mempools for these two distinct data structures. >=20 > 5. Add 'uint64_t userdata' inside struct rte_cryptodev_sym_session. > I suppose that self-explanatory, and might be used in a lot of places > (would be quite useful for ipsec library we develop). >=20 > So the new proposed layout for rte_cryptodev_sym_session: > struct rte_cryptodev_sym_session { > uint64_t userdata; > /**< Can be used for external metadata */ > uint16_t nb_drivers; > /**< number of elements in sess_data array */ > uint16_t priv_size; > /**< session private data will be placed after sess_data */ > __extension__ struct { > void *data; > uint16_t refcnt; > } sess_data[0]; > /**< Driver specific session material, variable size */ > }; >=20 >=20 > Signed-off-by: Konstantin Ananyev > --- Ok, didn't hear any objections, so far, so I suppose everyone are ok in general with proposed changes. Will go ahead with deprecation notice for 18.11 then. Konstantin