>
>
> > -----Original Message-----
> > From: Trybula, ArturX <arturx.tryb...@intel.com>
> > Sent: Monday, November 4, 2019 3:55 PM
> > To: Shally Verma <shal...@marvell.com>; dev@dpdk.org; Trahe, Fiona
> > <fiona.tr...@intel.com>; Dybkowski, AdamX
> <adamx.dybkow...@intel.com>;
> > akhil.go...@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.tryb...@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
