From: "Trahe, Fiona" <fiona.trahe@intel.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
"dev@dpdk.org" <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: [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session
Date: Wed, 14 Nov 2018 00:46:26 +0000 [thread overview]
Message-ID: <348A99DA5F5B7549AA880327E580B43589677E12@IRSMSX101.ger.corp.intel.com> (raw)
In-Reply-To: <2601191342CEEE43887BDE71AB977258010CE4AB41@IRSMSX106.ger.corp.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
next prev parent reply other threads:[~2018-11-14 0:46 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-24 17:48 Konstantin Ananyev
2018-10-05 11:05 ` Ananyev, Konstantin
2018-11-12 21:01 ` Trahe, Fiona
2018-11-12 23:16 ` Trahe, Fiona
2018-11-12 23:24 ` Trahe, Fiona
2018-11-13 18:56 ` Ananyev, Konstantin
2018-11-13 18:56 ` Ananyev, Konstantin
2018-11-14 0:46 ` Trahe, Fiona [this message]
2018-11-14 8:35 ` Ananyev, Konstantin
2018-11-14 10:14 ` Zhang, Roy Fan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=348A99DA5F5B7549AA880327E580B43589677E12@IRSMSX101.ger.corp.intel.com \
--to=fiona.trahe@intel.com \
--cc=akhil.goyal@nxp.com \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
--cc=dima@marvell.com \
--cc=hemant.agrawal@nxp.com \
--cc=jerin.jacob@caviumnetworks.com \
--cc=jianjay.zhou@huawei.com \
--cc=konstantin.ananyev@intel.com \
--cc=nsamsono@marvell.com \
--cc=pablo.de.lara.guarch@intel.com \
--cc=ravi1.kumar@amd.com \
--cc=roy.fan.zhang@intel.com \
--cc=tdu@semihalf.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).