Re: [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session

["Zhang, Roy Fan" <roy.fan.zhang@intel.com>]

Hi Fiona,

> -----Original Message-----
> From: Trahe, Fiona
> Sent: Wednesday, November 14, 2018 12:46 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Akhil Goyal
> <akhil.goyal@nxp.com>; Doherty, Declan <declan.doherty@intel.com>; Ravi
> Kumar <ravi1.kumar@amd.com>; Jerin Jacob
> <jerin.jacob@caviumnetworks.com>; Zhang, Roy Fan
> <roy.fan.zhang@intel.com>; Tomasz Duszynski <tdu@semihalf.com>;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Natalie Samsonov
> <nsamsono@marvell.com>; Dmitri Epshtein <dima@marvell.com>; Jay Zhou
> <jianjay.zhou@huawei.com>; Trahe, Fiona <fiona.trahe@intel.com>; Zhang,
> Roy Fan <roy.fan.zhang@intel.com>
> Subject: RE: [RFC] cryptodev: proposed changes in
> rte_cryptodev_sym_session
> 
> Hi Konstantin,
> 
> 
> //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
> 
> 
> > > [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
> 
> > > [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
> 
> > > >  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).
> > > [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 session.
> > 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 convention)
> > 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 for
> 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 user
> 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.
> 
> 
> > > > @@ -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 = 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 anyway.
> [Fiona] Except that you removed the NULL check below and are using that
> invalid index to deref the array.
> 
> > > 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 = get_sym_session_private_data(sess, i);
> > > > -		if (sess_priv != NULL)
> > > > +		if (sess->sess_data[i].refcnt != 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 session 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 
have an extra parameter "struct rte_cryptodev_sym_session *sess"
To use this parameter, the "sess" can either be NULL or a valid session pointer,
If it is NULL, the function call returns the current header size WITHOUT the private
data length. This is reasonable as it is user who knows the size of the private data
he wants to put in.
If it is not NULL, the function call returns the calculated header size based on
The sess's private data size and nb_drivers information. Here is the suggested
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 = {0};
		s.nb_drivers = nb_drivers;
		s.priv_size = 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.
> 
> >
> > >
> > > > +{
> > > > +	struct rte_cryptodev_sym_session *sess = 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 = NULL;
> > > > +
> > > > +	return (sizeof(*sess) + priv_size +
> > > > +		rte_cryptodev_sym_session_max_data_size());
> > > > +}
> > > > +
> 
> 
> 
> > > >
> > > > +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