From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: Akhil Goyal <gakhil@marvell.com>, "dev@dpdk.org" <dev@dpdk.org>,
"Zhang, Roy Fan" <roy.fan.zhang@intel.com>
Cc: "thomas@monjalon.net" <thomas@monjalon.net>,
"david.marchand@redhat.com" <david.marchand@redhat.com>,
"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
Anoob Joseph <anoobj@marvell.com>,
"De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>,
"Trahe, Fiona" <fiona.trahe@intel.com>,
"Doherty, Declan" <declan.doherty@intel.com>,
"matan@nvidia.com" <matan@nvidia.com>,
"g.singh@nxp.com" <g.singh@nxp.com>,
"jianjay.zhou@huawei.com" <jianjay.zhou@huawei.com>,
"asomalap@amd.com" <asomalap@amd.com>,
"ruifeng.wang@arm.com" <ruifeng.wang@arm.com>,
"Nicolau, Radu" <radu.nicolau@intel.com>,
"ajit.khaparde@broadcom.com" <ajit.khaparde@broadcom.com>,
Nagadheeraj Rottela <rnagadheeraj@marvell.com>,
Ankur Dwivedi <adwivedi@marvell.com>,
"Power, Ciara" <ciara.power@intel.com>,
"Wang, Haiyue" <haiyue.wang@intel.com>,
"jiawenwu@trustnetic.com" <jiawenwu@trustnetic.com>,
"jianwang@trustnetic.com" <jianwang@trustnetic.com>,
Jerin Jacob Kollanukkaran <jerinj@marvell.com>,
Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>
Subject: Re: [dpdk-dev] [PATCH v3 6/8] cryptodev: rework session framework
Date: Thu, 21 Oct 2021 13:11:39 +0000 [thread overview]
Message-ID: <DM6PR11MB4491B6FCD5EA0966A96B80509ABF9@DM6PR11MB4491.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CO6PR18MB44846C9555B062C0B65DF33BD8BF9@CO6PR18MB4484.namprd18.prod.outlook.com>
> > The problem is that with new approach you proposed there is no simple way
> > for PMD to
> > fulfil that requirement.
> > In current version of DPDK:
> > - PMD reports size of private data, note that it reports extra space needed
> > to align its data properly inside provided buffer.
> > - Then it ss up to higher layer to allocate mempool with elements big enough
> > to hold
> > PMD private data.
> > - At session init that mempool is passed to PMD sym_session_confgure() and
> > it is
> > PMD responsibility to allocate buffer (from given mempool) for its private
> > data
> > align it properly, and update sess->sess_data[].data.
> > With this patch:
> > - PMD still reports size of private data, but now it is cryptodev layer who
> > allocates
> > memory for PMD private data and updates sess->sess_data[].data.
> >
> > So PMD simply has no way to allocate/align its private data in a way it likes
> > to.
> > Of course it can simply do alignment on the fly for each operation, something
> > like:
> >
> > void *p = get_sym_session_private_data(sess, dev->driver_id);
> > sess_priv = RTE_PTR_ALIGN_FLOOR(p, PMD_SES_ALIGN);
> >
> > But it is way too ugly and error-prone.
> >
> > Another potential problem with that approach (when cryptodev allocates
> > memory for
> > PMD private session data and updates sess->sess_data[].data for it) - it could
> > happen
> > that private data for different PMDs can endup on the same cache-line.
> > If we'll ever have a case with simultaneous session processing by multiple-
> > devices
> > it can cause all sorts of performance problems.
>
> To resolve above 2 issues(performance and pointer CEIL in PMD), can you check
> If following diff in library would work?
> ----------------------------------------------------------------------------------------------------
> diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
> index 9d5e08bba2..7beb5339ea 100644
> --- a/lib/cryptodev/rte_cryptodev.c
> +++ b/lib/cryptodev/rte_cryptodev.c
> @@ -1731,12 +1731,13 @@ rte_cryptodev_sym_session_init(uint8_t dev_id,
> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->sym_session_configure, -ENOTSUP);
>
> if (sess->sess_data[index].refcnt == 0) {
> - sess->sess_data[index].data = (void *)((uint8_t *)sess +
> + sess->sess_data[index].data = RTE_PTR_ALIGN_CEIL(
> + (void *)((uint8_t *)sess +
> rte_cryptodev_sym_get_header_session_size() +
> - (index * sess->priv_sz));
> - sess_iova = rte_mempool_virt2iova(sess) +
> + (index * sess->priv_sz)), RTE_CACHE_LINE_SIZE);
> + sess_iova = RTE_ALIGN_CEIL(rte_mempool_virt2iova(sess) +
> rte_cryptodev_sym_get_header_session_size() +
> - (index * sess->priv_sz);
> + (index * sess->priv_sz), RTE_CACHE_LINE_SIZE);
> ret = dev->dev_ops->sym_session_configure(dev, xforms,
> sess->sess_data[index].data, sess_iova);
> if (ret < 0) {
> @@ -1805,7 +1806,7 @@ get_max_sym_sess_priv_sz(void)
> if (sz > max_sz)
> max_sz = sz;
> }
> - return max_sz;
> + return RTE_ALIGN_CEIL(max_sz,RTE_CACHE_LINE_SIZE);
> }
>
> struct rte_mempool *
Yep, aligning each PMD private data on CACHE_LINE will help to overcome that issue.
Though it means that we need to allocate extra CACHE_LINE bytes for each sess_data
element. That could be a significant amount.
Also I am still not sure that cryptodev layer should allocate/manage space for PMD
private session data. It would be really hard to predict all possible requirements that
each PMD can have. I think better to leave it to PMD itself, as it knows best what
it needs.
> ----------------------------------------------------------------------------------------
> >
> > All in all - these changes for (remove second mempool, change the way we
> > allocate/setup
> > session private data) seems premature to me.
> > So, I think to go ahead with this series (hiding rte_cryptodev_sym_session)
> > for 21.11
> > we need to drop changes for sess_data[] management allocation and keep
> > only changes
> > directly related to hide sym_session.
> > My apologies for not reviewing/testing properly that series earlier.
> >
>
> The changes are huge and will affect a lot of people. We needed help
> From all the pmd owners to look into this.
Agree.
> We can drop this series, citing not enough review happened, but the issues
> that were raised could have been resolved till RC2 for the cases that are currently
> broken.
> However, there is one more issue that was not highlighted here was that, in case of
> Scheduler PMD there are a lot of inappropriate stuff which hampers these changes.
> Because of which we will end up reserving huge memory space which will be unused
> if scheduler PMD is compiled in.
> We can have a simple single API for session creation similar to rte_security.
> And let scheduler PMD manage all the memory by itself for all the PMDs which
> it want to schedule.
Yes, same thoughts here:
if we can have just one device per session (as we have for security) it would help
to come up with simple and clean approach.
From my perspective, probably better to create a simple, clean API first,
and then try to re-work scheduler PMD to work with new one.
> We can defer this series for now, and can work on Asymmetric crypto
> first (probably in 22.02) which is still in experimental state. This will help in getting
> these changes matured enough for sym session which we can take up in 22.11.
Sounds like a good plan to me.
> I believe Intel people are planning for new features in asymmetric crypto.
> It makes more sense that they can align it as per the discussed approach.
>
> Regards,
> Akhil
next prev parent reply other threads:[~2021-10-21 13:13 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-30 14:50 [dpdk-dev] [PATCH 0/3] crypto/security session framework rework Akhil Goyal
2021-09-30 14:50 ` [dpdk-dev] [PATCH 1/3] security: rework session framework Akhil Goyal
2021-09-30 14:50 ` [dpdk-dev] [PATCH 2/3] drivers/net: temporary disable ixgbe and txgbe Akhil Goyal
2021-10-12 12:26 ` Zhang, Roy Fan
2021-10-12 12:29 ` Akhil Goyal
2021-10-12 13:32 ` Zhang, Roy Fan
2021-09-30 14:50 ` [dpdk-dev] [PATCH 3/3] cryptodev: rework session framework Akhil Goyal
2021-10-01 15:53 ` Zhang, Roy Fan
2021-10-04 19:07 ` Akhil Goyal
2021-10-13 19:22 ` [dpdk-dev] [PATCH v2 0/7] crypto/security session framework rework Akhil Goyal
2021-10-13 19:22 ` [dpdk-dev] [PATCH v2 1/7] security: rework session framework Akhil Goyal
2021-10-18 21:34 ` [dpdk-dev] [PATCH v3 0/8] crypto/security session framework rework Akhil Goyal
2021-10-18 21:34 ` [dpdk-dev] [PATCH v3 1/8] security: rework session framework Akhil Goyal
2021-10-18 21:34 ` [dpdk-dev] [PATCH v3 2/8] security: hide security session struct Akhil Goyal
2021-10-18 21:34 ` [dpdk-dev] [PATCH v3 3/8] net/cnxk: rework security session framework Akhil Goyal
2021-10-18 21:34 ` [dpdk-dev] [PATCH v3 4/8] security: pass session iova in PMD sess create Akhil Goyal
2021-10-18 21:34 ` [dpdk-dev] [PATCH v3 5/8] drivers/crypto: support security session get size op Akhil Goyal
2021-10-18 21:34 ` [dpdk-dev] [PATCH v3 6/8] cryptodev: rework session framework Akhil Goyal
2021-10-20 19:27 ` Ananyev, Konstantin
2021-10-21 6:53 ` Akhil Goyal
2021-10-21 10:38 ` Ananyev, Konstantin
2021-10-21 12:30 ` Akhil Goyal
2021-10-21 13:11 ` Ananyev, Konstantin [this message]
2021-10-18 21:34 ` [dpdk-dev] [PATCH v3 7/8] cryptodev: hide sym session structure Akhil Goyal
2021-10-18 21:34 ` [dpdk-dev] [PATCH v3 8/8] cryptodev: pass session iova in configure session Akhil Goyal
2021-10-20 14:36 ` [dpdk-dev] [PATCH v3 0/8] crypto/security session framework rework Hemant Agrawal
2021-10-20 15:45 ` Power, Ciara
2021-10-20 16:41 ` Akhil Goyal
2021-10-20 16:48 ` Akhil Goyal
2021-10-20 18:04 ` Akhil Goyal
2021-10-21 8:43 ` Zhang, Roy Fan
2021-10-13 19:22 ` [dpdk-dev] [PATCH v2 2/7] security: hide security session struct Akhil Goyal
2021-10-13 19:22 ` [dpdk-dev] [PATCH v2 3/7] net/cnxk: rework security session framework Akhil Goyal
2021-10-13 19:22 ` [dpdk-dev] [PATCH v2 4/7] security: pass session iova in PMD sess create Akhil Goyal
2021-10-13 19:22 ` [dpdk-dev] [PATCH v2 5/7] cryptodev: rework session framework Akhil Goyal
2021-10-13 19:22 ` [dpdk-dev] [PATCH v2 6/7] cryptodev: hide sym session structure Akhil Goyal
2021-10-13 19:22 ` [dpdk-dev] [PATCH v2 7/7] cryptodev: pass session iova in configure session Akhil Goyal
2021-10-14 11:47 ` [dpdk-dev] [PATCH v2 0/7] crypto/security session framework rework Akhil Goyal
2021-10-14 12:30 ` Zhang, Roy Fan
2021-10-14 12:34 ` Akhil Goyal
2021-10-14 17:07 ` Zhang, Roy Fan
2021-10-14 18:23 ` Akhil Goyal
2021-10-14 18:57 ` Akhil Goyal
2021-10-15 15:33 ` Zhang, Roy Fan
2021-10-15 17:42 ` Akhil Goyal
2021-10-15 18:47 ` Akhil Goyal
2021-10-16 13:31 ` Zhang, Roy Fan
2021-10-16 13:21 ` Zhang, Roy Fan
2021-10-15 8:12 ` 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=DM6PR11MB4491B6FCD5EA0966A96B80509ABF9@DM6PR11MB4491.namprd11.prod.outlook.com \
--to=konstantin.ananyev@intel.com \
--cc=adwivedi@marvell.com \
--cc=ajit.khaparde@broadcom.com \
--cc=anoobj@marvell.com \
--cc=asomalap@amd.com \
--cc=ciara.power@intel.com \
--cc=david.marchand@redhat.com \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
--cc=fiona.trahe@intel.com \
--cc=g.singh@nxp.com \
--cc=gakhil@marvell.com \
--cc=haiyue.wang@intel.com \
--cc=hemant.agrawal@nxp.com \
--cc=jerinj@marvell.com \
--cc=jianjay.zhou@huawei.com \
--cc=jianwang@trustnetic.com \
--cc=jiawenwu@trustnetic.com \
--cc=matan@nvidia.com \
--cc=ndabilpuram@marvell.com \
--cc=pablo.de.lara.guarch@intel.com \
--cc=radu.nicolau@intel.com \
--cc=rnagadheeraj@marvell.com \
--cc=roy.fan.zhang@intel.com \
--cc=ruifeng.wang@arm.com \
--cc=thomas@monjalon.net \
/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).