From: Shally Verma <shallyv@marvell.com> To: "Trybula, ArturX" <arturx.trybula@intel.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:00:21 +0000 Message-ID: <BN8PR18MB2867D1DF3774AC6ED8A4EB05AD790@BN8PR18MB2867.namprd18.prod.outlook.com> (raw) In-Reply-To: <5B6D1C77E9D7034C93E97BD83D1D9F572F23270C@hasmsx107.ger.corp.intel.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
next prev parent reply other threads:[~2019-11-06 15:00 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 [this message] 2019-11-06 15:33 ` Trybula, ArturX 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=BN8PR18MB2867D1DF3774AC6ED8A4EB05AD790@BN8PR18MB2867.namprd18.prod.outlook.com \ --to=shallyv@marvell.com \ --cc=adamx.dybkowski@intel.com \ --cc=akhil.goyal@nxp.com \ --cc=arturx.trybula@intel.com \ --cc=ashishg@marvell.com \ --cc=dev@dpdk.org \ --cc=fiona.trahe@intel.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 patches and discussions This inbox may be cloned and mirrored by anyone: git clone --mirror http://inbox.dpdk.org/dev/0 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 dev dev/ http://inbox.dpdk.org/dev \ dev@dpdk.org public-inbox-index dev Example config snippet for mirrors. Newsgroup available over NNTP: nntp://inbox.dpdk.org/inbox.dpdk.dev AGPL code for this site: git clone https://public-inbox.org/public-inbox.git