> > > 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 <gak...@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.
