Re: [dpdk-dev] [PATCH v3 2/2] cryptodev: change symmetric session structure

["Zhang, Roy Fan" <roy.fan.zhang@intel.com>]

Hi Pablo,

Thanks for the review. Comments inline.

Regards,
Fan

> -----Original Message-----
> From: De Lara Guarch, Pablo
> Sent: Wednesday, January 9, 2019 11:01 AM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; dev@dpdk.org
> Cc: akhil.goyal@nxp.com; Trahe, Fiona <fiona.trahe@intel.com>
> Subject: RE: [PATCH v3 2/2] cryptodev: change symmetric session structure
> 
> Hi Fan,
> 
> > -----Original Message-----
> > From: Zhang, Roy Fan
> > Sent: Friday, December 21, 2018 1:56 PM
> > To: dev@dpdk.org
> > Cc: akhil.goyal@nxp.com; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>; Trahe, Fiona <fiona.trahe@intel.com>
> > Subject: [PATCH v3 2/2] cryptodev: change symmetric session structure
> >
> > This patch changes the symmetric session structure of cryptodev.
> > The symmetric session now contains extra information for secure access
> > purposes. The patch also includes the updates to the PMDs, test
> > applications, and examples to fit the change.
> >
> 
> Programmer's guide needs to be updated to reflect these changes.
> 
[Fan: Will do]
> > Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
> 
> ...
> 
> > diff --git a/lib/librte_cryptodev/Makefile
> > b/lib/librte_cryptodev/Makefile index a8f94c097..ade108b90 100644
> > --- a/lib/librte_cryptodev/Makefile
> > +++ b/lib/librte_cryptodev/Makefile
> > @@ -12,6 +12,7 @@ LIBABIVER := 5
> 
> This needs to be bumped to 6 (here and in meson.build), probably in the
> previous patch.
> 
[Fan: No problem]
> >  # build flags
> >  CFLAGS += -O3
> >  CFLAGS += $(WERROR_FLAGS)
> > +CFLAGS += -DALLOW_EXPERIMENTAL_API
> >  LDLIBS += -lrte_eal -lrte_mempool -lrte_ring -lrte_mbuf  LDLIBS +=
> > -lrte_kvargs
> >
> 
> ...
> 
> > +++ b/lib/librte_cryptodev/rte_cryptodev.c
> 
> ...
> 
> > +struct rte_mempool *__rte_experimental
> > +rte_cryptodev_sym_session_pool_create(const char *name, uint32_t
> > nb_elts,
> > +	uint32_t elt_size, uint32_t cache_size, uint16_t user_data_size,
> > +	int socket_id)
> 
> Is elt_size a required parameter? This mempool is created specifically for
> crypto sessions, so the size of it is known and can be transparent to the user
> (plus, I see that in all apps, this is always 0).
> 
[Fan: Yes. If the user wants to use the same mempool for both session header and private data, this elt size can be used to pass the private session data's size, so the library can create a mempool with big enough elt size. The API comments of the header file has described that and I will add it to the programmer's guide] 
> > +		CDEV_LOG_INFO("elt_size %u is expanded to %u\n",
> elt_size,
> > +				obj_sz);
> 
> ...
> 
> > +++ b/lib/librte_cryptodev/rte_cryptodev_version.map
> > @@ -105,4 +105,6 @@ EXPERIMENTAL {
> >  	rte_cryptodev_sym_session_set_user_data;
> >  	rte_crypto_asym_op_strings;
> >  	rte_crypto_asym_xform_strings;
> > +	rte_cryptodev_sym_session_pool_create;
> > +	rte_cryptodev_sym_get_existing_header_session_size;
> 
> As far as I know, this new API is mandatory to be used, and therefore, I think
> this should not be marked as experimental, as the crypto library is a stable
> library.
[Fan: Will do.]