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

["Trybula, ArturX" <arturx.trybula@intel.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 .
[Artur] Ok, it will be modified according your suggestion.

> 
> 
> > +/**
> > + * Source buffers preparation (for compression).
> > + *
[Shally] Can we have more details here like: API that allocates input (and output buffers?) to perform compression operations?
[Artur] Ok, it will be modified according your suggestion.

> > + * 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?
[Artur] Yes, we can.

> > + *   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?
[Artur] Yes, we can.

> > + *   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.
[Artur] Ok, it will be modified according your suggestion.

> > + * 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.
[Artur] In some way it is not a perfect solution, but from my point of view it's very clear. 
There is no info in the interim test param (I suppose you meant interim_data_params) that could be useful in this case. There are pointers to xforms, but which one should I check? There must be some switch to choose the right flow. I can add another input param to the function, to indicate if it's a compress or decompress operation. But it seems to me, that it would be too much for this one case. Finally I can add another input param if you like this idea?

> > + * 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. 
[Artur] Ok

> > + * @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?!
[Artur] I discussed this question with Fiona. The test was design for QAT and other tools like QAT. In this case Zlib is used just for a verification if the compression using QAT was correct. This condition should be preserved.

> > +
> > +			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.
[Artur] I don't see any reason why priv data could be corrupted. It would be a bigger problem if it happened.
All the cases are covered and the return value must be correct. If ops_processed is not NULL than priv_data has to be correct or we have a problem with QAT PMD. Verification if the filed orig_idx is correct is even more complicated. From my perspective mentioned verification shouldn't be considered as a part of this function. It's too late.
Another input param discussed below: op_type?
 
> > +}
> > +
> > +
> > +/**
> > + * 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
[Artur] Ok

> > + * 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.
[Artur] Similar issue was discussed above. So let's do both cases in the same way. I'm going to define another enum. It will be an extra input param of these functions:
1. test_mbufs_calculate_data_size
2. test_mbufs_destination_preparation
Is it ok for you?


> > + * @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
[Artur] Ok

> > + * @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
[Artur] What object do you mean precisely?

...
> > +/**
> > + * 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 
[Artur] Ok

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

> > + * @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
[Artur] Ok

> > + *
> > + * @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.
[Artur] Ok

> 
> > +
> > +	/* 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
[Artur] Ok

...
> > +/* 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 
[Artur] Ok

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