From: "Trybula, ArturX" <arturx.trybula@intel.com>
To: Shally Verma <shallyv@marvell.com>, "dev@dpdk.org" <dev@dpdk.org>,
"Trahe, Fiona" <fiona.trahe@intel.com>,
"Dybkowski, AdamX" <adamx.dybkowski@intel.com>,
"akhil.goyal@nxp.com" <akhil.goyal@nxp.com>,
Ashish Gupta <ashishg@marvell.com>
Subject: Re: [dpdk-dev] [EXT] [PATCH v3 1/1] test/compress: unit tests refactoring
Date: Wed, 6 Nov 2019 15:33:25 +0000 [thread overview]
Message-ID: <5B6D1C77E9D7034C93E97BD83D1D9F572F2328AC@hasmsx107.ger.corp.intel.com> (raw)
In-Reply-To: <BN8PR18MB2867D1DF3774AC6ED8A4EB05AD790@BN8PR18MB2867.namprd18.prod.outlook.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(-)
> > >
> .....
...
> >
...
> > > + * 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
next prev parent reply other threads:[~2019-11-06 15:33 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-09 14:45 [dpdk-dev] [PATCH] " Artur Trybula
2019-09-12 14:34 ` [dpdk-dev] [PATCH v2 0/1] compression: " Artur Trybula
2019-09-12 14:34 ` [dpdk-dev] [PATCH v2 1/1] test/compress: " Artur Trybula
2019-10-24 9:16 ` [dpdk-dev] [PATCH v3 0/1] compression: " Artur Trybula
2019-10-24 9:16 ` [dpdk-dev] [PATCH v3 1/1] test/compress: " Artur Trybula
2019-10-24 9:22 ` Dybkowski, AdamX
2019-10-24 9:27 ` Akhil Goyal
2019-10-31 8:38 ` Akhil Goyal
2019-10-31 18:23 ` [dpdk-dev] [EXT] " Shally Verma
2019-11-04 10:24 ` Trybula, ArturX
2019-11-04 15:24 ` Shally Verma
2019-11-06 7:43 ` Akhil Goyal
2019-11-06 11:33 ` Trybula, ArturX
2019-11-06 15:00 ` Shally Verma
2019-11-06 15:33 ` Trybula, ArturX [this message]
2019-11-06 15:36 ` Shally Verma
2019-11-07 17:26 ` [dpdk-dev] [PATCH v4 0/1] compression: " Artur Trybula
2019-11-07 17:26 ` [dpdk-dev] [PATCH v4 1/1] test/compress: " Artur Trybula
2019-11-08 6:35 ` [dpdk-dev] [EXT] " Shally Verma
2019-11-08 10:16 ` Akhil Goyal
2019-11-08 18:40 ` [dpdk-dev] " Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5B6D1C77E9D7034C93E97BD83D1D9F572F2328AC@hasmsx107.ger.corp.intel.com \
--to=arturx.trybula@intel.com \
--cc=adamx.dybkowski@intel.com \
--cc=akhil.goyal@nxp.com \
--cc=ashishg@marvell.com \
--cc=dev@dpdk.org \
--cc=fiona.trahe@intel.com \
--cc=shallyv@marvell.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).