Hi Tomasz, Pablo,

-----Original Message-----
From: Tomasz Duszynski [mailto:tdu@semihalf.com]
Sent: Wednesday, June 13, 2018 11:11 AM
To: De Lara Guarch, Pablo <redacted>
Cc: Tomasz Duszynski <redacted>; Doherty, Declan <redacted>;
akhil.goyal@nxp.com; ravi1.kumar@amd.com; jerin.jacob@caviumnetworks.com; Zhang, Roy Fan
[off-list ref]; Trahe, Fiona [off-list ref]; jianjay.zhou@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:

quoted

Hi Tomasz,

quoted

-----Original Message-----
From: Tomasz Duszynski [mailto:tdu@semihalf.com]
Sent: Wednesday, June 13, 2018 7:12 AM
To: De Lara Guarch, Pablo <redacted>
Cc: Tomasz Duszynski <redacted>; Doherty, Declan
[off-list ref]; akhil.goyal@nxp.com; ravi1.kumar@amd.com;
jerin.jacob@caviumnetworks.com; Zhang, Roy Fan [off-list ref];
Trahe, Fiona [off-list ref]; 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:

quoted

quoted

-----Original Message-----
From: Tomasz Duszynski [mailto:tdu@semihalf.com]
Sent: Tuesday, June 12, 2018 12:38 PM
To: De Lara Guarch, Pablo <redacted>
Cc: Doherty, Declan <redacted>; akhil.goyal@nxp.com;
ravi1.kumar@amd.com; jerin.jacob@caviumnetworks.com; Zhang, Roy Fan
[off-list ref]; Trahe, Fiona [off-list ref];
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:

quoted

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

...

quoted

quoted

diff --git a/drivers/crypto/mvsam/rte_mrvl_pmd.c

b/drivers/crypto/mvsam/rte_mrvl_pmd.c

quoted

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.");

quoted

 	}

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

quoted

Thanks,
Pablo

quoted

quoted

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.

quoted

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