> -----Original Message----- > From: Trahe, Fiona > Sent: Tuesday, June 19, 2018 2:20 PM > To: Tomasz Duszynski <t...@semihalf.com>; De Lara Guarch, Pablo > <pablo.de.lara.gua...@intel.com> > Cc: Doherty, Declan <declan.dohe...@intel.com>; akhil.go...@nxp.com; > ravi1.ku...@amd.com; jerin.ja...@caviumnetworks.com; Zhang, Roy Fan > <roy.fan.zh...@intel.com>; jianjay.z...@huawei.com; dev@dpdk.org; Trahe, > Fiona <fiona.tr...@intel.com> > Subject: RE: [PATCH 3/6] cryptodev: remove max number of sessions > > Hi Tomasz, Pablo, > > > -----Original Message----- > > From: Tomasz Duszynski [mailto:t...@semihalf.com] > > Sent: Wednesday, June 13, 2018 11:11 AM > > To: De Lara Guarch, Pablo <pablo.de.lara.gua...@intel.com> > > Cc: Tomasz Duszynski <t...@semihalf.com>; Doherty, Declan > > <declan.dohe...@intel.com>; akhil.go...@nxp.com; ravi1.ku...@amd.com; > > jerin.ja...@caviumnetworks.com; Zhang, Roy Fan > > <roy.fan.zh...@intel.com>; Trahe, Fiona <fiona.tr...@intel.com>; > > jianjay.z...@huawei.com; dev@dpdk.org > > Subject: Re: [PATCH 3/6] cryptodev: remove max number of sessions > > > > On Wed, Jun 13, 2018 at 08:23:36AM +0000, De Lara Guarch, Pablo wrote: > > > Hi Tomasz, > > > > > > > -----Original Message----- > > > > From: Tomasz Duszynski [mailto:t...@semihalf.com] > > > > Sent: Wednesday, June 13, 2018 7:12 AM > > > > To: De Lara Guarch, Pablo <pablo.de.lara.gua...@intel.com> > > > > Cc: Tomasz Duszynski <t...@semihalf.com>; Doherty, Declan > > > > <declan.dohe...@intel.com>; akhil.go...@nxp.com; > > > > ravi1.ku...@amd.com; jerin.ja...@caviumnetworks.com; Zhang, Roy > > > > Fan <roy.fan.zh...@intel.com>; Trahe, Fiona > > > > <fiona.tr...@intel.com>; jianjay.z...@huawei.com; dev@dpdk.org > > > > Subject: Re: [PATCH 3/6] cryptodev: remove max number of sessions > > > > > > > > On Tue, Jun 12, 2018 at 01:53:36PM +0000, De Lara Guarch, Pablo wrote: > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Tomasz Duszynski [mailto:t...@semihalf.com] > > > > > > Sent: Tuesday, June 12, 2018 12:38 PM > > > > > > To: De Lara Guarch, Pablo <pablo.de.lara.gua...@intel.com> > > > > > > Cc: Doherty, Declan <declan.dohe...@intel.com>; > > > > > > akhil.go...@nxp.com; ravi1.ku...@amd.com; > > > > > > jerin.ja...@caviumnetworks.com; Zhang, Roy Fan > > > > > > <roy.fan.zh...@intel.com>; Trahe, Fiona > > > > > > <fiona.tr...@intel.com>; t...@semihalf.com; > > > > > > jianjay.z...@huawei.com; dev@dpdk.org > > > > > > Subject: Re: [PATCH 3/6] cryptodev: remove max number of > > > > > > sessions > > > > > > > > > > > > Hello Pablo, > > > > > > > > > > > > On Fri, Jun 08, 2018 at 11:02:31PM +0100, Pablo de Lara wrote: > > > > > > > Sessions are not created and stored in the crypto device > > > > > > > anymore, since now the session mempool is created at the > application level. > > > > > > > > > > > > > > Therefore the limitation of the maximum number of sessions > > > > > > > that can be created should not be dependent of the crypto device. > > > > > > > > > > > > > > Signed-off-by: Pablo de Lara > > > > > > > <pablo.de.lara.gua...@intel.com> > > > > > > > > > > ... > > > > > > > > > > > > diff --git a/drivers/crypto/mvsam/rte_mrvl_pmd.c > > > > > > b/drivers/crypto/mvsam/rte_mrvl_pmd.c > > > > > > > index 1b6029a56..822b6cac7 100644 > > > > > > > --- a/drivers/crypto/mvsam/rte_mrvl_pmd.c > > > > > > > +++ b/drivers/crypto/mvsam/rte_mrvl_pmd.c > > > > > > > @@ -719,7 +719,6 @@ cryptodev_mrvl_crypto_create(const char > *name, > > > > > > > internals = dev->data->dev_private; > > > > > > > > > > > > > > internals->max_nb_qpairs = init_params->max_nb_queue_pairs; > > > > > > > - internals->max_nb_sessions = init_params->max_nb_sessions; > > > > > > > > > > > > > > /* > > > > > > > * ret == -EEXIST is correct, it means DMA @@ -734,8 > > > > > > > +733,6 @@ cryptodev_mrvl_crypto_create(const char *name, > > > > > > > "DMA memory has been already initialized by a > > > > > > different driver."); > > > > > > > } > > > > > > > > > > > > > > - sam_params.max_num_sessions = internals->max_nb_sessions; > > > > > > > > > > > > This will not fly since library maintains separate list of sessions. > > > > > > We have to initialize this number to something sane. Since we > > > > > > cannot get it from userspace perhaps make that compile-time > > > > > > configurable by adding separate CONFIG_? > > > > > > > > > > Hi Tomasz, > > > > > > > > > > If you need to have an actual limit, you could define it > > > > > internally (not adding an external configuration option), but > > > > > bear in mind that This won't prevent an application from trying to > allocate more sessions. > > > > > > > > You can define arbitrary number of session on condition you have > > > > enough memory. So no hard limit here. What bothers me is the case > > > > where app wants to initialize more session than the library internally > > > > has. > > > > If this happens userspace will get an error. On the other hand > > > > requesting some arbitrary large number of session from library and > > > > hoping app will never use so many wastes memory (which might be > > > > valuable on resource constrained systems). > > > > > > > > That is why keeping the number of sessions in app and library in > > > > sync is important. > > > > > > > > Do we have any option in DPDK now to workaround this? > > > > > > Ok I see, so actually the MUSDK library needs a maximum number of > sessions. > > > I'd say then we should keep this field, but we can add a special case: 0. > > > In this case, the PMD does not have any maximum number of sessions > > > (which would be applicable to most PMDs). > > > > > > So, for this PMD, this special case is not supported. If 0 is > > > passed, either return that unlimited number of sessions is not > > > supported, or set it to a default value (defined inside the PMD, such as > > > 2048). > > > If no value is passed, this number can be set to the default value too. > > > > > > How does this sound? > > > > Who is going to pass that value? App? Or the old way is retained i.e > > PMD parameters? > > > > OK, my understanding is that we have 3 options: > > > > 1. 0 is passed which for most of the drivers translates to "you should > > not care about sessions number created by userspace application". In > > case PMD supports that it returns either success or failure. > > > > 2. Nothing is passed which means PMD should not care about number of > > sessions except mvsam which sets some default value. > > > > 3. Passing some arbitrary value which which sets number of sessions > > for PMDs that care about that (mvsam). In that case app would > > respect that number and not allocate more than specified? This is > > what DPDK has now. > > > > Right? Doesn't is sound like the mechanism that gets removed from DPDK > > with some extra handling added? > > > > Other solution might be to remove the old API as planned and change > > the PMD itself so that it defers library initialization until PMD is > > started. During session configuration necessary information would be > > saved and reused later on (i.e during PMD start). > > > [Fiona] I'd suggest we keep max_nb_sessions in the info struct. > mvsam - or any other PMD which wants an internal PMD limit - should set it via > hard-coding or a PMD-specific item in the config file. > For all other PMDs currently which don't have a limit the config file item > can be > removed. For these, 0, indicating No Limit, should be returned in the info > struct. > But I think it's optional what an app does with this max_nb_sessions. > It could take it into account and create a session pool of (max_nb_sessions * > num_PMDs) and be careful not to send more than max_nb_sessions to a PMD > with a limit. > Or it could just ignore it, create whatever size session pool it wants and if > it calls > rte_cryptodev_sym_session_init() too many times on a PMD handle the - > ENOMEM which I'd expect the PMD to return. Possibly route the session to a > different device with available resources. > Does that work for everyone?
I will send a new version of this patchset, addressing these comments. MVSAM PMD will still be able to set max_nb_sessions to a defined value and rest of the PMDs (except for DPAA) will set this value to 0, indicating that they have no limit in the amount of sessions. Thanks, Pablo i
