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: Wednesday, November 6, 2019 5:03 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; Ashish Gupta
> <ashishg@marvell.com>
> Subject: RE: [EXT] [PATCH v3 1/1] test/compress: unit tests refactoring
> 
> 
> > -----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(-)
> > >
> .....
...

> >
...
> > > + * 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?
> 


[Shally] Believe you agreed below that you'll add an param op_type?

...

> > > +	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.
> 
[Shally] ohh ok I noticed not_zlib_compr means zlib will decompress. I thought it otherwise. So ignore my feedback.

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

[Shally] I think we are on same page here. I meant same thing that I don't see reason why func should return 0. It will always return some non-zero value. So my point was value 0 seems redundant here.

> 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?
> 
[Shally] 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?
> 
My suggestion was to move extbuf_info, out_of_space_zlib too to priv_data .. but its just suggestion... you can ignore If it complicate the purpose

> ...
...
> > > 2.17.1