[dpdk-dev] [PATCH 3/6] cryptodev: remove max number of sessions

[De Lara Guarch, Pablo]

Hi Tomasz,

> -----Original Message-----
> From: Tomasz Duszynski [mailto:tdu at semihalf.com]
> Sent: Wednesday, June 13, 2018 7:12 AM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>
> Cc: Tomasz Duszynski <tdu at semihalf.com>; Doherty, Declan
> <declan.doherty at intel.com>; akhil.goyal at nxp.com; ravi1.kumar at amd.com;
> jerin.jacob at caviumnetworks.com; Zhang, Roy Fan <roy.fan.zhang at intel.com>;
> Trahe, Fiona <fiona.trahe at intel.com>; jianjay.zhou at huawei.com;
> dev at 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:tdu at semihalf.com]
> > > Sent: Tuesday, June 12, 2018 12:38 PM
> > > To: De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>
> > > Cc: Doherty, Declan <declan.doherty at intel.com>; akhil.goyal at nxp.com;
> > > ravi1.kumar at amd.com; jerin.jacob at caviumnetworks.com; Zhang, Roy Fan
> > > <roy.fan.zhang at intel.com>; Trahe, Fiona <fiona.trahe at intel.com>;
> > > tdu at semihalf.com; jianjay.zhou at huawei.com; dev at 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.guarch at 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?

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