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?
> > > > Thanks, > > Pablo > > > > > > > > > > > > > > > If your PMD has a limitation on the maximum number of sessions, then > > > > maybe this change won't work for you (removing the maximum number of > > > sessions), so let me know and we can discuss this. > > > > > > > > Thanks, > > > > Pablo > > > > > > > > P.S. Please, next time, strip out the code that you are not > > > > commenting, as it was hard to find this question :) > > > > > > > > > > -- > > > - Tomasz Duszyński > > -- > - Tomasz Duszyński
