From: Akhil Goyal <gakhil@marvell.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.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 12:30:18 +0000 [thread overview]
Message-ID: <CO6PR18MB44846C9555B062C0B65DF33BD8BF9@CO6PR18MB4484.namprd18.prod.outlook.com> (raw)
In-Reply-To: <DM6PR11MB4491DB7D16085E89F0D3B6A49ABF9@DM6PR11MB4491.namprd11.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 *
----------------------------------------------------------------------------------------
>
> 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.
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.
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.
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 12:30 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 [this message]
2021-10-21 13:11 ` Ananyev, Konstantin
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=CO6PR18MB44846C9555B062C0B65DF33BD8BF9@CO6PR18MB4484.namprd18.prod.outlook.com \
--to=gakhil@marvell.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=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=konstantin.ananyev@intel.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).