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

[Tomasz Duszynski <tdu@semihalf.com>]

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

>
> 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