From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: Akhil Goyal <gakhil@marvell.com>, "dev@dpdk.org" <dev@dpdk.org>
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>,
"Zhang, Roy Fan" <roy.fan.zhang@intel.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>
Subject: Re: [dpdk-dev] [PATCH v3 6/8] cryptodev: rework session framework
Date: Thu, 21 Oct 2021 10:38:33 +0000 [thread overview]
Message-ID: <DM6PR11MB4491DB7D16085E89F0D3B6A49ABF9@DM6PR11MB4491.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CO6PR18MB44843EC3B92B90A43E4FEA09D8BF9@CO6PR18MB4484.namprd18.prod.outlook.com>
> > > As per current design, rte_cryptodev_sym_session_create() and
> > > rte_cryptodev_sym_session_init() use separate mempool objects
> > > for a single session.
> > > And structure rte_cryptodev_sym_session is not directly used
> > > by the application, it may cause ABI breakage if the structure
> > > is modified in future.
> > >
> > > To address these two issues, the rte_cryptodev_sym_session_create
> > > will take one mempool object for both the session and session
> > > private data. The API rte_cryptodev_sym_session_init will now not
> > > take mempool object.
> > > rte_cryptodev_sym_session_create will now return an opaque session
> > > pointer which will be used by the app in rte_cryptodev_sym_session_init
> > > and other APIs.
> > >
> > > With this change, rte_cryptodev_sym_session_init will send
> > > pointer to session private data of corresponding driver to the PMD
> > > based on the driver_id for filling the PMD data.
> > >
> > > In data path, opaque session pointer is attached to rte_crypto_op
> > > and the PMD can call an internal library API to get the session
> > > private data pointer based on the driver id.
> > >
> > > Note: currently nb_drivers are getting updated in RTE_INIT which
> > > result in increasing the memory requirements for session.
> > > User can compile off drivers which are not in use to reduce the
> > > memory consumption of a session.
> > >
> > > Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> > > ---
> >
> > With that patch ipsec-secgw functional tests crashes for AES_GCM test-cases.
> > To be more specific:
> > examples/ipsec-secgw/test/run_test.sh -4 tun_aesgcm
> >
> > [24126592.561071] traps: dpdk-ipsec-secg[3254860] general protection fault
> > ip:7f3ac2397027 sp:7ffeaade8848 error:0 in
> > libIPSec_MB.so.1.0.0[7f3ac238f000+2a20000]
> >
> > Looking a bit deeper, it fails at:
> > #0 0x00007ff9274f4027 in aes_keyexp_128_enc_avx512 ()
> > from /lib/libIPSec_MB.so.1
> > #1 0x00007ff929f0ac97 in aes_gcm_pre_128_avx_gen4 ()
> > from /lib/libIPSec_MB.so.1
> > #2 0x0000561757073753 in aesni_gcm_session_configure
> > (mb_mgr=0x56175c5fe400,
> > session=0x17e3b72d8, xform=0x17e05d7c0)
> > at ../drivers/crypto/ipsec_mb/pmd_aesni_gcm.c:132
> > #3 0x00005617570592af in ipsec_mb_sym_session_configure (
> > dev=0x56175be0c940 <rte_crypto_devices>, xform=0x17e05d7c0,
> > sess=0x17e3b72d8) at ../drivers/crypto/ipsec_mb/ipsec_mb_ops.c:330
> > #4 0x0000561753b4d6ae in rte_cryptodev_sym_session_init (dev_id=0
> > '\000',
> > sess_opaque=0x17e3b4940, xforms=0x17e05d7c0)
> > at ../lib/cryptodev/rte_cryptodev.c:1736
> > #5 0x0000561752ef99b7 in create_lookaside_session (
> > ipsec_ctx=0x56175aa6a210 <lcore_conf+1105232>, sa=0x17e05d140,
> > ips=0x17e05d140) at ../examples/ipsec-secgw/ipsec.c:145
> > #6 0x0000561752f0cf98 in fill_ipsec_session (ss=0x17e05d140,
> > ctx=0x56175aa6a210 <lcore_conf+1105232>, sa=0x17e05d140)
> > at ../examples/ipsec-secgw/ipsec_process.c:89
> > #7 0x0000561752f0d7dd in ipsec_process (
> > ctx=0x56175aa6a210 <lcore_conf+1105232>, trf=0x7ffd192326a0)
> > at ../examples/ipsec-secgw/ipsec_process.c:300
> > #8 0x0000561752f21027 in process_pkts_outbound (
> > --Type <RET> for more, q to quit, c to continue without paging--
> > ipsec_ctx=0x56175aa6a210 <lcore_conf+1105232>,
> > traffic=0x7ffd192326a0)
> > at ../examples/ipsec-secgw/ipsec-secgw.c:839
> > #9 0x0000561752f21b2e in process_pkts (
> > qconf=0x56175aa57340 <lcore_conf+1027712>, pkts=0x7ffd19233c20,
> > nb_pkts=1 '\001', portid=1) at ../examples/ipsec-secgw/ipsec-secgw.c:1072
> > #10 0x0000561752f224db in ipsec_poll_mode_worker ()
> > at ../examples/ipsec-secgw/ipsec-secgw.c:1262
> > #11 0x0000561752f38adc in ipsec_launch_one_lcore (args=0x56175c549700)
> > at ../examples/ipsec-secgw/ipsec_worker.c:654
> > #12 0x0000561753cbc523 in rte_eal_mp_remote_launch (
> > f=0x561752f38ab5 <ipsec_launch_one_lcore>, arg=0x56175c549700,
> > call_main=CALL_MAIN) at ../lib/eal/common/eal_common_launch.c:64
> > #13 0x0000561752f265ed in main (argc=12, argv=0x7ffd19234168)
> > at ../examples/ipsec-secgw/ipsec-secgw.c:2978
> > (gdb) frame 2
> > #2 0x0000561757073753 in aesni_gcm_session_configure
> > (mb_mgr=0x56175c5fe400,
> > session=0x17e3b72d8, xform=0x17e05d7c0)
> > at ../drivers/crypto/ipsec_mb/pmd_aesni_gcm.c:132
> > 132 mb_mgr->gcm128_pre(key, &sess->gdata_key);
> >
> > Because of un-expected unaligned memory access:
> > (gdb) disas
> > Dump of assembler code for function aes_keyexp_128_enc_avx512:
> > 0x00007ff9274f400b <+0>: endbr64
> > 0x00007ff9274f400f <+4>: cmp $0x0,%rdi
> > 0x00007ff9274f4013 <+8>: je 0x7ff9274f41b4
> > <aes_keyexp_128_enc_avx512+425>
> > 0x00007ff9274f4019 <+14>: cmp $0x0,%rsi
> > 0x00007ff9274f401d <+18>: je 0x7ff9274f41b4
> > <aes_keyexp_128_enc_avx512+425>
> > 0x00007ff9274f4023 <+24>: vmovdqu (%rdi),%xmm1
> > => 0x00007ff9274f4027 <+28>: vmovdqa %xmm1,(%rsi)
> >
> > (gdb) print/x $rsi
> > $12 = 0x17e3b72e8
> >
> > And this is caused because now AES_GCM session private data is not 16B-bits
> > aligned anymore:
> > (gdb) print ((struct aesni_gcm_session *)sess->sess_data[index].data)
> > $29 = (struct aesni_gcm_session *) 0x17e3b72d8
> >
> > print &((struct aesni_gcm_session *)sess->sess_data[index].data)-
> > >gdata_key
> > $31 = (struct gcm_key_data *) 0x17e3b72e8
> >
> > As I understand the reason for that is that we changed the way how
> > sess_data[index].data
> > is populated. Now it is just:
> > sess->sess_data[index].data = (void *)((uint8_t *)sess +
> > rte_cryptodev_sym_get_header_session_size() +
> > (index * sess->priv_sz));
> >
> > So, as I can see, there is no guarantee that PMD's private sess data will be
> > aligned on 16B
> > as expected.
> >
> Agreed, that there is no guarantee that the sess_priv will be aligned.
> I believe this is requirement from the PMD side for a particular alignment.
Yes, it is PMD specific requirement.
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.
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.
> Is it possible for the PMD to use __rte_aligned for the fields which are required to
The data structure inside PMD is properly aligned.
The problem is that now cryptodev layer might provide to PMD memory that is not properly aligned.
> Be aligned. For aesni_gcm it is 16B aligned requirement, for some other PMD it may be
> 64B alignment.
next prev parent reply other threads:[~2021-10-21 10:38 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 [this message]
2021-10-21 12:30 ` Akhil Goyal
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=DM6PR11MB4491DB7D16085E89F0D3B6A49ABF9@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=jianjay.zhou@huawei.com \
--cc=jianwang@trustnetic.com \
--cc=jiawenwu@trustnetic.com \
--cc=matan@nvidia.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).