Re: [dpdk-dev] [PATCH] doc: announce ABI change for crypto info struct

["Verma, Shally" <Shally.Verma@cavium.com>]

>-----Original Message-----
>From: De Lara Guarch, Pablo [mailto:pablo.de.lara.guarch@intel.com]
>Sent: 30 January 2018 16:51
>To: Verma, Shally <Shally.Verma@cavium.com>; Akhil Goyal <akhil.goyal@nxp.com>; Trahe, Fiona <fiona.trahe@intel.com>;
>hemant.agrawal@nxp.com; Doherty, Declan <declan.doherty@intel.com>; Griffin, John <john.griffin@intel.com>; Jain, Deepak K
><deepak.k.jain@intel.com>; jck@semihalf.com; tdu@semihalf.com; dima@marvell.com; nsamsono@marvell.com;
>jianbo.liu@arm.com; Jacob, Jerin <Jerin.JacobKollanukkaran@cavium.com>; Athreya, Narayana Prasad
><NarayanaPrasad.Athreya@cavium.com>; Murthy, Nidadavolu <Nidadavolu.Murthy@cavium.com>
>Cc: dev@dpdk.org
>Subject: RE: [dpdk-dev] [PATCH] doc: announce ABI change for crypto info struct
>
>Hi Shally/Ahkil,
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Verma, Shally
>> Sent: Tuesday, January 30, 2018 7:56 AM
>> To: Akhil Goyal <akhil.goyal@nxp.com>; De Lara Guarch, Pablo
>> <pablo.de.lara.guarch@intel.com>; Trahe, Fiona <fiona.trahe@intel.com>;
>> hemant.agrawal@nxp.com; Doherty, Declan <declan.doherty@intel.com>;
>> Griffin, John <john.griffin@intel.com>; Jain, Deepak K
>> <deepak.k.jain@intel.com>; jck@semihalf.com; tdu@semihalf.com;
>> dima@marvell.com; nsamsono@marvell.com; jianbo.liu@arm.com; Jacob,
>> Jerin <Jerin.JacobKollanukkaran@cavium.com>; Athreya, Narayana Prasad
>> <NarayanaPrasad.Athreya@cavium.com>; Murthy, Nidadavolu
>> <Nidadavolu.Murthy@cavium.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] doc: announce ABI change for crypto info
>> struct
>>
>> I do see current cryptodev unit testcase (inside \test dir) uses
>> info.sym.max_nb_sessions param for session mempool_create. So, such
>> testcases change are also in proposal?
>
>Yes, for these tests, we can just define a macro in the tests, instead of using the info structure.

[Shally] Ok, then you mean applications will choose any random number during mempool_create and not dependent on device max_nb_sessions?

>>
>> Another point, we recently submitted an RFC patch on lib/cryptodev with
>> asymmetric crypto support
>> (https://dpdk.org/dev/patchwork/patch/34308/) which is awaiting review
>> and these fields have role to play there.
>> So, could this change be please viewed in conjunction with asym RFC?
>
>Do you need it for asymmetric? Anyway, this would remove the symmetric function and structures, not applicable for you.

[Shally] I would say addition of asym in lib/cryptodev is not entirely standalone, specifically for PMDs that can support both. 
My key concern are max_nb_sessions_per_qp and related qp_attach_sym/asym APIs which enable management of queue distribution among sym and asym in current proposal, specifically, for PMDs that can support both but have dedicated qp for each. Right now proposal is open for feedback and would prefer to be covered before sym related changes could be applied.

>>
>> Thanks
>> Shally
>>
>> > -----Original Message-----
>> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Akhil Goyal
>> > Sent: 29 January 2018 14:57
>> > To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Trahe,
>> > Fiona <fiona.trahe@intel.com>; hemant.agrawal@nxp.com; Doherty,
>> Declan
>> > <declan.doherty@intel.com>; Griffin, John <john.griffin@intel.com>;
>> > Jain, Deepak K <deepak.k.jain@intel.com>; jck@semihalf.com;
>> > tdu@semihalf.com; dima@marvell.com; nsamsono@marvell.com;
>> > jianbo.liu@arm.com; Jacob, Jerin
>> <Jerin.JacobKollanukkaran@cavium.com>
>> > Cc: dev@dpdk.org
>> > Subject: Re: [dpdk-dev] [PATCH] doc: announce ABI change for crypto
>> > info struct
>> >
>> > Hi Pablo/Fiona,
>> >
>> > On 1/26/2018 4:38 PM, De Lara Guarch, Pablo wrote:
>> > >
>> > >
>> > >> -----Original Message-----
>> > >> From: Trahe, Fiona
>> > >> Sent: Friday, January 26, 2018 10:45 AM
>> > >> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
>> > >> akhil.goyal@nxp.com; hemant.agrawal@nxp.com; Doherty, Declan
>> > >> <declan.doherty@intel.com>; jerin.jacob@intel.com; Griffin, John
>> > >> <john.griffin@intel.com>; Jain, Deepak K <deepak.k.jain@intel.com>;
>> > >> jck@semihalf.com; tdu@semihalf.com; dima@marvell.com;
>> > >> nsamsono@marvell.com; jianbo.liu@arm.com
>> > >> Cc: dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>
>> > >> Subject: RE: [PATCH] doc: announce ABI change for crypto info
>> > >> struct
>> > >>
>> > >> Hi Pablo,
>> > >>
>> > >>> -----Original Message-----
>> > >>> From: De Lara Guarch, Pablo
>> > >>> Sent: Friday, January 26, 2018 9:04 AM
>> > >>> To: akhil.goyal@nxp.com; hemant.agrawal@nxp.com; Doherty,
>> Declan
>> > >>> <declan.doherty@intel.com>; jerin.jacob@intel.com; Trahe, Fiona
>> > >>> <fiona.trahe@intel.com>; Griffin, John <john.griffin@intel.com>;
>> > >>> Jain, Deepak K <deepak.k.jain@intel.com>; jck@semihalf.com;
>> > >>> tdu@semihalf.com; dima@marvell.com; nsamsono@marvell.com;
>> > >>> jianbo.liu@arm.com
>> > >>> Cc: dev@dpdk.org; De Lara Guarch, Pablo
>> > >>> <pablo.de.lara.guarch@intel.com>
>> > >>> Subject: [PATCH] doc: announce ABI change for crypto info struct
>> > >>>
>> > >>> Since the API changes made in 17.08, the session mempool is not
>> > >>> created anymore in each crypto device.
>> > >>> Therefore, there is no need to have, in the cryptodev info
>> > >>> structure, the maximum number of sessions supported per device
>> and
>> > >>> per queue pair.
>> > >>>
>> > >>> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
>> > >>> ---
>> > >>>   doc/guides/rel_notes/deprecation.rst | 5 +++++
>> > >>>   1 file changed, 5 insertions(+)
>> > >>>
>> > >>> diff --git a/doc/guides/rel_notes/deprecation.rst
>> > >>> b/doc/guides/rel_notes/deprecation.rst
>> > >>> index d59ad5988..5588ba7c1 100644
>> > >>> --- a/doc/guides/rel_notes/deprecation.rst
>> > >>> +++ b/doc/guides/rel_notes/deprecation.rst
>> > >>> @@ -59,3 +59,8 @@ Deprecation Notices
>> > >>>     be added between the producer and consumer structures. The
>> > >>> size of
>> > >> the
>> > >>>     structure and the offset of the fields will remain the same on
>> > >>>     platforms with 64B cache line, but will change on other platforms.
>> > >>> +
>> > >>> +* cryptodev: The structure ``sym``, including its fields
>> > >>> +``max_nb_sessions``
>> > >>> +  and ``max_nb_sessions_per_qp``, in structure
>> > >>> +``rte_cryptodev_info``,
>> > >>> +  will be removed in 18.05, as these fields are not relevant
>> > >>> +anymore
>> > >>> +  since the session mempool is not internal in the crypto device
>> > >> anymore.
>> > >>> --
>> > >> [Fiona] max_nb_sessions must be also removed from struct
>> > >> rte_cryptodev_pmd_init_params
>> > >
>> > > Good point. Since this structure is internal, I guess we don't need
>> > > a
>> > deprecation notice for it,
>> > > but I will remove it in the patch for 18.05.
>> > >
>> > >> Regards deprecation of max_nb_sessions from both structs:
>> > >> Acked-by: Fiona Trahe <fiona.trahe@intel.com>
>> > >>
>> > >> If removing the max_nb_sessions_per_qp, then the following
>> > >> functions should also be deprecated.
>> > >> rte_cryptodev_queue_pair_attach_sym_session
>> > >> rte_cryptodev_queue_pair_detach_sym_session
>> > >> These and the max_nb_session_per_qp were added here at request of
>> > NXP:
>> > >> http://dpdk.org/ml/archives/dev/2017-March/060740.html
>> > >
>> > > Akhil, do you agree on this change?
>> > >
>> >
>> > We recently did some changes in the driver to take care of the
>> > dependency for limit on max_nb_sessions_per_qp, but it is not removed
>> > completely. We will need to look into this. But sending the
>> > deprecation notice at this moment is fine. If something comes up, will
>> > let you know later.
>
>Looks good to me. Could you ack this if you agree with it?
>
>Thanks,
>Pablo
>
>> >
>> > -Akhil