DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
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
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


  reply index

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

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org
	public-inbox-index dpdk-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git