From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 06A52532C for ; Wed, 14 Nov 2018 11:14:23 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Nov 2018 02:14:23 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,232,1539673200"; d="scan'208";a="86379179" Received: from irsmsx103.ger.corp.intel.com ([163.33.3.157]) by fmsmga008.fm.intel.com with ESMTP; 14 Nov 2018 02:14:21 -0800 Received: from irsmsx101.ger.corp.intel.com ([169.254.1.134]) by IRSMSX103.ger.corp.intel.com ([169.254.3.253]) with mapi id 14.03.0415.000; Wed, 14 Nov 2018 10:14:20 +0000 From: "Zhang, Roy Fan" To: "Trahe, Fiona" , "Ananyev, Konstantin" , "dev@dpdk.org" CC: "De Lara Guarch, Pablo" , Akhil Goyal , "Doherty, Declan" , "Ravi Kumar" , Jerin Jacob , Tomasz Duszynski , Hemant Agrawal , Natalie Samsonov , Dmitri Epshtein , Jay Zhou Thread-Topic: [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session Thread-Index: AQHUO9Lg/kp8P/6LMESj24MvYOaWSaVNHgOAgAFveACAAGHRAIAAm07A Date: Wed, 14 Nov 2018 10:14:19 +0000 Message-ID: <9F7182E3F746AB4EA17801C148F3C604334E09A0@IRSMSX101.ger.corp.intel.com> References: <1535132906-5167-1-git-send-email-konstantin.ananyev@intel.com> <348A99DA5F5B7549AA880327E580B43589676DA2@IRSMSX101.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258010CE4AB41@IRSMSX106.ger.corp.intel.com> <348A99DA5F5B7549AA880327E580B43589677E12@IRSMSX101.ger.corp.intel.com> In-Reply-To: <348A99DA5F5B7549AA880327E580B43589677E12@IRSMSX101.ger.corp.intel.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNzYyYmEwZjAtMjg1NC00MGY1LTg2NmQtNDNiNjczMzg1NDVmIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiOXBaRkJ2SHFiVEhWS1pnZGZGZE5OcDYrb0dXUG9Bc2RDMXhZKzZcL1kxanlINTRERnd5UWszbXlzQ2M3Z1I3a0sifQ== dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action 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] [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: Wed, 14 Nov 2018 10:14:24 -0000 Hi Fiona, > -----Original Message----- > From: Trahe, Fiona > Sent: Wednesday, November 14, 2018 12:46 AM > To: Ananyev, Konstantin ; dev@dpdk.org > Cc: De Lara Guarch, Pablo ; Akhil Goyal > ; Doherty, Declan ; Ravi > Kumar ; Jerin Jacob > ; Zhang, Roy Fan > ; Tomasz Duszynski ; > Hemant Agrawal ; Natalie Samsonov > ; Dmitri Epshtein ; Jay Zhou > ; Trahe, Fiona ; Zhang, > Roy Fan > Subject: RE: [RFC] cryptodev: proposed changes in > rte_cryptodev_sym_session >=20 > Hi Konstantin, >=20 >=20 > //snip/// > > Can you also have a look at related deprecation note: > > http://patches.dpdk.org/patch/46633/ > > and provide the feedback? > > Konstantin > [Fiona] will do >=20 >=20 > > > [Fiona] Ok, I agree with this issue and proposed fix. > > > We need to also document that it's user's responsibility not to call > > > either init() or clear() twice on same device, as that would break > > > the ref count. > > > > I suppose it is obvious constrain, but sure, extra wording can be put > > into the comments/docs, np with that. > [Fiona] ok >=20 > > > [Fiona] I agree, this is needed. > > > But I propose to call it user_data_sz NOT priv_size. > > > See https://patches.dpdk.org/patch/42515/ to understand why. > > > > Hmm that differs with mbuf naming scheme (which I tried to follow > > here), but ok - I don't have strong opinion here. > [Fiona] ok >=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 p= laces > > > > (would be quite useful for ipsec library we develop). > > > [Fiona] Seems unnecessary - the set_user_data can be used. Why have 2 > > > separate user data spaces in the session? - it's confusing. > > > > It allows quickly set/get external metadata associated with that sessio= n. > > As an example - we plan to use it for pointer to ipsec SA associated > > with given session. > > Storing it inside priv_data section (user_data in your naming conventio= n) > > has several limitations: > > - extra function call and extra memory dereference. > > - each app would have to take into account that field when calculates = size > > for session mempool element. > > Also note that inside one app could exist several session pools, > > possibly with different layout for user private data, > > unknown for generic libs. > > > > Again, here I just used current mbuf approach: > > userdata - (pointer to) some external metadata > > (possibly temporally) associated with given mbuf. > > priv_size (you suggest to call it user_data_sz) - > > size of the application private data for given mbuf. > > > > > If these is some good reason, then a different name should be used fo= r > clarity. > > > Not private. And not user. Possibly opaque data. > > > > Ok. > [Fiona] ok, let's go with opaque data so. > The naming problem arises only because the PMD data in the > session is referred to as private data already in various APIs, so when u= ser > data got added it > didn't make sense to also call it private data, so we named it user-data = - so > not consistent > with mbuf. >=20 >=20 > > > > @@ -1301,8 +1346,7 @@ rte_cryptodev_sym_session_free(struct > rte_cryptodev_sym_session *sess) > > > > > > > > /* Check that all device private data has been freed */ > > > > for (i =3D 0; i < nb_drivers; i++) { > > > [Fiona] Use the sess.nb_drivers rather than the global. > > > > Ok, though doesn't really matter here. > > get_sym_session_private_data() will return NULL for invalid index anywa= y. > [Fiona] Except that you removed the NULL check below and are using that > invalid index to deref the array. >=20 > > > Actually maybe name slightly differently to reduce the chance of that > mistake happening, e.g. > > > rename sess.nb_drivers to sess.num_drivers? > > > > > > > - sess_priv =3D get_sym_session_private_data(sess, i); > > > > - if (sess_priv !=3D NULL) > > > > + if (sess->sess_data[i].refcnt !=3D 0) > > > > return -EBUSY; > > > > } > > > > > > > > @@ -1313,6 +1357,23 @@ rte_cryptodev_sym_session_free(struct > rte_cryptodev_sym_session *sess) > > > > return 0; > > > > } > > > > > > > > +unsigned int > > > > +rte_cryptodev_sym_session_max_data_size(void) > > > [Fiona] Suggest renaming this > > > rte_cryptodev_sym_session_max_array_size() > > > > I usually don't care much about names, but that seems just confusing: > > totally unclear which array we are talking about. > > Any better naming ideas? > [Fiona] Isn't it returning the size of the array of structs in the sessio= n hdr? [Fan] I don't think this external API is needed as we already have rte_cryptodev_sym_get_header_session_size(void). There is no point to create an API to do exactly same thing. However, to make the konstantin's idea works this API needs to=20 have an extra parameter "struct rte_cryptodev_sym_session *sess" To use this parameter, the "sess" can either be NULL or a valid session poi= nter, If it is NULL, the function call returns the current header size WITHOUT th= e private data length. This is reasonable as it is user who knows the size of the pri= vate data he wants to put in. If it is not NULL, the function call returns the calculated header size bas= ed on The sess's private data size and nb_drivers information. Here is the sugges= ted Code sample: unsigned int rte_cryptodev_sym_get_header_session_size( struct rte_cryptodev_sym_session *sess) { if (!sess) { struct rte_cryptodev_sym_session s =3D {0}; s.nb_drivers =3D nb_drivers; s.priv_size =3D 0; return (unsigned int)(sizeof(s) + SESSION_DATA_SIZE(&s); } else return (unsigned int)(sizeof(*sess) + SESSION_DATA_SIZE(sess); } > Seems a bit more informative than "data". > Can't think of anything better, if you find array confusing then I > don't feel that strongly about it, stick with data. > Or just get rid of the function altogether and put inside > rte_cryptodev_sym_session_max_size() > Unless it's called elsewhere. > Same applies to the other _data_size fn below. >=20 > > > > > > > > > +{ > > > > + struct rte_cryptodev_sym_session *sess =3D NULL; > > > > + > > > > + return (sizeof(sess->sess_data[0]) * nb_drivers); > > > > +} > > > > + > > > > +size_t > > > > +rte_cryptodev_sym_session_max_size(uint16_t priv_size) > > > > +{ > > > > + struct rte_cryptodev_sym_session *sess =3D NULL; > > > > + > > > > + return (sizeof(*sess) + priv_size + > > > > + rte_cryptodev_sym_session_max_data_size()); > > > > +} > > > > + >=20 >=20 >=20 > > > > > > > > +static inline size_t > > > > +rte_cryptodev_sym_session_data_size(const struct > rte_cryptodev_sym_session *s) > > > > +{ > > > > + return (sizeof(s->sess_data[0]) * s->nb_drivers); > > > > +} > > > > + > > > > +static inline size_t > > > > +rte_cryptodev_sym_session_size(const struct > rte_cryptodev_sym_session *s) > > > > +{ > > > > + return (sizeof(*s) + (s)->priv_size + > > > > + rte_cryptodev_sym_session_data_size(s)); > > > > +} > > > > + > > > [Fiona] Are above 2 fns used? > > > Look like duplicates of the "max" fns? > > > > Not really: rte_cryptodev_sym_session_max_data_size() and > > rte_cryptodev_sym_session_max_size() use global var nb_drivers. > > Returns amount of space required to create a session that > > can work with all attached at that moment drivers. > > Planned to be used by in rte_cryptodev_sym_session_max_size(). > > While these 2 funcs return size of particular session object. > [Fiona] ok