Re: [dpdk-dev] [EXT] [PATCH v3 1/1] test/compress: unit tests refactoring

[Shally Verma <shallyv@marvell.com>]

> -----Original Message-----
> From: Trybula, ArturX <arturx.trybula@intel.com>
> Sent: Monday, November 4, 2019 3:55 PM
> To: Shally Verma <shallyv@marvell.com>; dev@dpdk.org; Trahe, Fiona
> <fiona.trahe@intel.com>; Dybkowski, AdamX
> <adamx.dybkowski@intel.com>; akhil.goyal@nxp.com
> Subject: RE: [EXT] [PATCH v3 1/1] test/compress: unit tests refactoring
> 
> Hi Shally,
> 
> Please find my comments below.
> 
> > ----------------------------------------------------------------------
> > Core engine refactoring (test_deflate_comp_decomp function).
> > Smaller specialized functions created.
> >
> > Signed-off-by: Artur Trybula <arturx.trybula@intel.com>
> > ---
> >  app/test/test_compressdev.c            | 1118 +++++++++++++++++-------
> >  doc/guides/rel_notes/release_19_11.rst |    5 +
> >  2 files changed, 826 insertions(+), 297 deletions(-)
> >
.....
> > +static void
> > +test_objects_init(struct test_private_arrays *prv,
> > +		unsigned int num_bufs)
> > +{
> > +	/* Initialize all arrays to NULL */
> > +	memset(prv->uncomp_bufs, 0, sizeof(struct rte_mbuf *) *
> > num_bufs);
> > +	memset(prv->comp_bufs, 0, sizeof(struct rte_mbuf *) * num_bufs);
> > +	memset(prv->ops, 0, sizeof(struct rte_comp_op *) * num_bufs);
> > +	memset(prv->ops_processed, 0, sizeof(struct rte_comp_op *) *
> > num_bufs);
> > +	memset(prv->priv_xforms, 0, sizeof(void *) * num_bufs);
> > +	memset(prv->compressed_data_size, 0, sizeof(uint32_t) *
> > num_bufs); }
> > +
> Does it really matter what pointer is pointing to? Sizeof(any pointer) *
> num_bufs .. should be enough.
> [Artur] You are absolutely right that sizeof() of any pointer gives the same
> result. But to be honest I don't understand your idea. Would you like to have
> an extra variable to initialise all the pointer arrays?
> Eg:
> uint32_t array_size = sizeof(void *) * num_bufs;
> ...
> 	memset(prv->uncomp_bufs, 0, array_size);
> 	memset(prv->comp_bufs, 0, array_size);
> ...
[Shally] Hmm... ya that sort of. But my 2cents on it. So, its up to you .

> 
> 
> > +/**
> > + * Source buffers preparation (for compression).
> > + *
[Shally] Can we have more details here like: API that allocates input (and output buffers?) to perform compression operations?

> > + * Memory allocation for each buffer from mempool
> > + * -1 returned if function fail, without modifying the mbuf.
> > + *
> > + * @param int_data
[Shally] Can we change it to name test_params?

> > + *   Interim data containing session/transformation objects.
> > + * @param test_data
> > + *   The test parameters set by users (command line parameters).
> > + * @param priv_arrays
[Shally] Can we change it to name test_priv_data?

> > + *   A container used for aggregation all the private test arrays.
> > + * @return
> > + *   - 0: On success.
> > + *   - -1: On error.
> >   */
> >  static int
> > -test_deflate_comp_decomp(const struct interim_data_params *int_data,
> > -		const struct test_data_params *test_data)
> > +test_mbufs_source_preparation(const struct interim_data_params
> > *int_data,
> > +		const struct test_data_params *test_data,
> > +		const struct test_private_arrays *priv_arrays)
> >  {
....

> > +/**
> > + * Data size calculation (for both compression and decompression).
> > + *
[Shally] Can add more details here like calculate size of anticipated output buffer required for both compression and decompression operations based on input test_params.
Caller then allocate output buffer based on size returned by this function.

> > + * Developer is requested to provide input params
> > + * according to the following rule:
> > + *      if ops_processed == NULL -> compression
> > + *      if ops_processed != NULL -> decompression
[Shally] we are trying to  overload its purpose here. Can it be make simpler . we can use interim test param to check if op type is compression/decompression and then use op_processed[] on need basis
This will help in code readability else function looks complex to understand where as purpose is very simple.

> > + * Values bigger than 0 have to be returned to avoid problems
> > + * with memory allocation
> > + *
[Shally] Can be reworded as " function return non-zero on success, 0 on failure. Caller should not allocate memory if 0. 

> > + * @param ops_processed
> > + *   Operations created as a result of compression phase. Should be
> > + *   set to NULL for compression
> > + * @param out_of_space_and_zlib
> > + *   Boolean value to switch into "out of space" buffer if set.
> > + *   To test "out-of-space" data size, zlib_decompress must be set as well.
> > + * @param int_data
> > + *   Interim data containing session/transformation objects.
> > + * @param test_data
> > + *   The test parameters set by users (command line parameters).
> > + * @param i
> > + *   current buffer index
> > + * @return
> > + *   - values bigger than 0
> > + */
> > +static inline uint32_t
> > +test_mbufs_calculate_data_size(
> > +		struct rte_comp_op *ops_processed[], /*can be equal to
> > NULL*/
> > +		unsigned int out_of_space_and_zlib,
> > +		const struct interim_data_params *int_data,
> > +		const struct test_data_params *test_data,
> > +		unsigned int i)
> > +{
> > +	/* local variables: */
> > +	uint32_t data_size = 0;
> > +	struct priv_op_data *priv_data;
> > +	float ratio;
> > +	uint8_t not_zlib_compr; /* true if zlib isn't current compression dev
> > */
> > +	enum overflow_test overflow = test_data->overflow;
> > +
> > +	/* from int_data: */
> > +	const char * const *test_bufs = int_data->test_bufs;
> > +
> > +	if (out_of_space_and_zlib)
> > +		data_size = OUT_OF_SPACE_BUF;
> > +	else {
> > +		if (ops_processed == NULL) {
> > +
> > +			not_zlib_compr = (test_data->zlib_dir ==
> > ZLIB_DECOMPRESS
> > +				|| test_data->zlib_dir == ZLIB_NONE);
> > +
> > +			ratio = (not_zlib_compr &&
> > +				(overflow == OVERFLOW_ENABLED)) ?
> > +				COMPRESS_BUF_SIZE_RATIO_OVERFLOW :
> > +				COMPRESS_BUF_SIZE_RATIO;
[Shally] Why this condition is not true in case of ZLIB compression? For zlib compression too it can overflow right in case of deflate uncompressed output?!

> > +
> > +			data_size = strlen(test_bufs[i]) * ratio;
> > +
> > +		} else {
> > +			priv_data = (struct priv_op_data *)
> > +					(ops_processed[i] + 1);
> > +			data_size = strlen(test_bufs[priv_data->orig_idx]) +
> > 1;
> > +		}
> > +	}
> > +
> > +	return data_size;
[Shally] On the other hand, I don't see reason why it should return 0 unless object priv data is corrupted or not updated with test buf size, which ideally should not be the case. 
Given that, it can  be just updated that func returns expected output buffer size.


> > +}
> > +
> > +
> > +/**
> > + * Memory buffers preparation (for both compression and
> decompression).
> > + *
> > + * Memory allocation for comp/decomp buffers from mempool,
> depending
> > on
[Shally] can be reworded " function allocate output buffer depending on op_type : compression / decompression

> > + * ops_processed value. Developer is requested to provide input params
> > + * according to the following rule:
> > + *      if ops_processed == NULL -> current_bufs = comp_bufs[]
> > + *      if ops_processed != NULL -> current_bufs = decomp_bufs[]
> > + * -1 returned if function fail, without modifying the mbuf.
> > + *
[Shally] Same feedback here. This can be made simpler by adding another op_type in arg or getting this info from test args.

> > + * @param ops_processed
> > + *   Operations created as a result of compression phase. Should be
> > + *   set to NULL for compression
> > + * @param current_bufs
> > + *   mbufs being prepared in the function.
> > + * @param out_of_space_and_zlib
> > + *   Boolean value to switch into "out of space" buffer if set.
> > + *   To test "out-of-space" data size, zlib_decompress must be set as well.
> > + * @param int_data
> > + *   Interim data containing session/transformation objects.
> > + * @param test_data
> > + *   The test parameters set by users (command line parameters).
> > + * @param current_extbuf_info,
> > + *   The structure containing all the information related to external mbufs
> > + * @param current_memzone
> > + *   memzone used by external buffers
[Shally] add "valid if current_extbuf_info is set

> > + * @return
> > + *   - 0: On success.
> > + *   - -1: On error.
> > + */
> > +static int
> > +test_mbufs_destination_preparation(
> > +		struct rte_comp_op *ops_processed[], /*can be equal to
> > NULL*/
> > +		struct rte_mbuf *current_bufs[],
> > +		unsigned int out_of_space_and_zlib,
> > +		const struct interim_data_params *int_data,
> > +		const struct test_data_params *test_data,
> > +		struct rte_mbuf_ext_shared_info *current_extbuf_info,
> > +		const struct rte_memzone *current_memzone) {

[Shally] I would still recommend to make it part of priv array and keep prototype simpler to read

...
> > +/**
> > + * The main compression function.
> > + *
> > + * Operation(s) configuration, depending on CLI parameters.
> > + * Operation(s) processing.
> > + * -1 returned if function fail.
> > + *
> > + * @param int_data
> > + *   Interim data containing session/transformation objects.
> > + * @param test_data
> > + *   The test parameters set by users (command line parameters).
> > + * @param priv_arrays
> > + *   A container used for aggregation all the private test arrays.
> > + * @return
> > + *   - 0: On success.
> > + *   - -1: On error.
> > + */
...

> > +/**
> > + * The main decompression function.
[Shally] add more description : function perform decompression op 

> > + *
> > + * Operation(s) configuration, depending on CLI parameters.
> > + * Operation(s) processing.
> > + * -1 returned if function fail.
> > + *
[Shally] Not needed here.. @return also mention same

> > + * @param int_data
> > + *   Interim data containing session/transformation objects.
> > + * @param test_data
> > + *   The test parameters set by users (command line parameters).
> > + * @param priv_arrays
> > + *   A container used for aggregation all the private test arrays.
> > + * @return
> > + *   - 0: On success.
> > + *   - -1: On error.
> > + */
> > +static int
> > +test_deflate_decomp_run(const struct interim_data_params *int_data,
> > +		const struct test_data_params *test_data,
> > +		struct test_private_arrays *priv_arrays) {
> >
....

> > +/**
> > + * Compresses and decompresses input stream with compressdev API
> and
> > +Zlib API
> > + *
> > + * Basic test function. Common for all the functional tests.
> > + * -1 returned if function fail.
[Shally] Not needed here

> > + *
> > + * @param int_data
> > + *   Interim data containing session/transformation objects.
> > + * @param test_data
> > + *   The test parameters set by users (command line parameters).
> > + * @return
> > + *   - 1: Some operation not supported
> > + *   - 0: On success.
> > + *   - -1: On error.
> > + */
> > +
...

> > +	capa = rte_compressdev_capability_get(0,
> > RTE_COMP_ALGO_DEFLATE);
> > +	if (capa == NULL) {
> > +		RTE_LOG(ERR, USER1,
> > +			"Compress device does not support DEFLATE\n");
> > +		return -1;
> > +	}
> > +	test_objects_init(&priv_arrays, num_bufs);
> Do we need this? Why we just cant call source_prep API? That will also setup
> uncompressed_bufs
> [Artur] This is my approach. Do you prefer to have arrays initialisation moved
> to test_mbufs_source_preparation?
> If "yes", test_objects_init will be removed from the code.
[Shally] Ya. We can minimize code bit here.

> 
> > +
> > +	/* Prepare the source mbufs with the data */
> > +	ret = test_mbufs_source_preparation(int_data, test_data,
> > &priv_arrays);
> For code readability sake, it could test_setup_uncom_bufs()
> [Artur] Good idea. Would it be ok if I change:
> 1. test_mbufs_destination_preparation  into test_setup_uncom_bufs
> 2. test_mbufs_source_preparation into test_setup_com_bufs
> 
[Shally] Ya that look better or we can say , setup_input_bufs , setup_output_bufs .. just few suggestios

...
> > +/* COMPRESSION  */
> > +
> > +	/* Prepare the destination mbufs */
> > +	ret = test_mbufs_destination_preparation(
> Why we cant keep this prototype same as source_preparation() API for
> better readability?
> I mean we can pass priv_arry here too...
> [Artur] To get better readability it has been implemented in this way. That
> was only my point of view but I don't mind to modify the code according to
> your suggestion. It's also ok.
> 
[Shally] Ya. I would prefer 

> > +			NULL, /*can be equal to NULL*/
> > +			comp_bufs,
> > +			out_of_space == 1 && !zlib_compress,
> > +			int_data,
> > +			test_data,
> > +			&compbuf_info,
> > +			test_data->compbuf_memzone);
> > +	if (ret < 0) {
> > +		ret_status = -1;
> > +		goto exit;
> > +	}
> > +
...

> > 2.17.1