Re: [dpdk-dev] [PATCH v2 0/7] crypto/security session framework rework

["Zhang, Roy Fan" <roy.fan.zhang@intel.com>]

Hi,

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Friday, October 15, 2021 7:47 PM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; david.marchand@redhat.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; g.singh@nxp.com; jianjay.zhou@huawei.com;
> asomalap@amd.com; ruifeng.wang@arm.com; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Nicolau, Radu <radu.nicolau@intel.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;
> jianwang@trustnetic.com
> Subject: RE: [PATCH v2 0/7] crypto/security session framework rework
> 
> > > Hi Akhil,
> > >
> > > I tried to fix the problems of seg faults.
> > > The seg-faults are gone now but all asym tests are failing too.
> > > The reason is the rte_cryptodev_queue_pair_setup() checks the session
> > > mempool same for sym and asym.
> > > Since we don't have a rte_cryptodev_asym_session_pool_create() the
> > > session mempool created by
> > > test_cryptodev_asym.c  with rte_mempool_create() will fail the
> mempool
> > > check when setting up the queue pair.
> > >
> > > If you think my fix may be useful (although not resolving asym issue) I can
> > > send it.
> > >
> > Is it a different fix than what I proposed below? If yes, you can send the
> diff.
> > I already made the below changes for all the PMDs.
> > I will try to fix the asym issue, but I suppose it can be dealt in the app
> > Which can be fixed separately in RC2.
> >
> > Also, found the root cause of multi process issue, working on making the
> > patches.
> > Will send v3 soon with all 3 issues(docsis/mp/sessless) fixed atleast.
> > For Asym, may send a separate patch.
> >
> For Asym issue, it looks like the APIs are not written properly and has many
> Issues compared to sym.
> Looking at the API rte_cryptodev_queue_pair_setup(), it only support
> mp_session(or priv_sess_mp) for symmetric sessions even without my
> changes.
> 
> Hence, a qp does not have mempool for sessionless Asym processing and
> looking at current
> Drivers, only QAT support asym session less and it does not use mempool
> stored in qp.
> 
> Hence IMO, it is safe to remove the check from
> rte_cryptodev_queue_pair_setup()
>         if (!qp_conf->mp_session) {
>                 CDEV_LOG_ERR("Invalid mempools\n");
>                 return -EINVAL;
>         }
> Or we can have give a CDEV_LOG_INFO (to indicate session mempool not
> present, session less won't work) instead of CDEV_LOG_ERR and fall through.
> 

Yes this is a valid fix. It will make queue pair setup work as before.
The old code was like this:

	if ((qp_conf->mp_session && !qp_conf->mp_session_private) ||
			(!qp_conf->mp_session && qp_conf->mp_session_private)) {
		CDEV_LOG_ERR("Invalid mempools\n");
		return -EINVAL;
	}

The requirement was either you provide 2 mempools (one for session and one for
session private) or you don't provide session mempool when creating queue pair at
all. Only otherwise the error is returned.

> For sym case, it is checking again in next line if session_mp is there or not.
> 
> I hope, the asym cases will work once we remove the above check and pass
> Null in the asym app while setting up queue pairs. What say?

It shall be working.  Thanks.
> 
> 
>