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

["Trahe, Fiona" <fiona.trahe@intel.com>]

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?
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