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

Trybula, ArturX arturx.trybula at intel.com
Wed Nov 6 16:33:25 CET 2019


> 
> 
> > -----Original Message-----
> > From: Trybula, ArturX <arturx.trybula at intel.com>
> > Sent: Monday, November 4, 2019 3:55 PM
> > To: Shally Verma <shallyv at marvell.com>; dev at dpdk.org; Trahe, Fiona 
> > <fiona.trahe at intel.com>; Dybkowski, AdamX
> <adamx.dybkowski at intel.com>;
> > akhil.goyal at 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 at 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?
[Artur] Yes, this is what we agreed.

...

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

> > > +
> > > +			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.
[Artur] Ok, I see your point. It's true that initialization is not necessary. Will be corrected.

> 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
[Artur] Ok. I'll try to optimize the list of params. 

[Artur] Shally, I assume all the issues can be closed and V4 can be prepared tomorrow?

> ...
...
> > > 2.17.1



More information about the dev mailing list