From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 85A0AA04AB; Wed, 6 Nov 2019 16:33:33 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 8B6651C246; Wed, 6 Nov 2019 16:33:32 +0100 (CET) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 622BA1C1E5 for ; Wed, 6 Nov 2019 16:33:30 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Nov 2019 07:33:29 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.68,275,1569308400"; d="scan'208";a="403767559" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga006.fm.intel.com with ESMTP; 06 Nov 2019 07:33:29 -0800 Received: from fmsmsx608.amr.corp.intel.com (10.18.126.88) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 6 Nov 2019 07:33:28 -0800 Received: from fmsmsx608.amr.corp.intel.com (10.18.126.88) by fmsmsx608.amr.corp.intel.com (10.18.126.88) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Wed, 6 Nov 2019 07:33:28 -0800 Received: from hasmsx108.ger.corp.intel.com (10.184.198.18) by fmsmsx608.amr.corp.intel.com (10.18.126.88) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Wed, 6 Nov 2019 07:33:28 -0800 Received: from hasmsx107.ger.corp.intel.com ([169.254.2.187]) by hasmsx108.ger.corp.intel.com ([169.254.9.149]) with mapi id 14.03.0439.000; Wed, 6 Nov 2019 17:33:25 +0200 From: "Trybula, ArturX" To: Shally Verma , "dev@dpdk.org" , "Trahe, Fiona" , "Dybkowski, AdamX" , "akhil.goyal@nxp.com" , Ashish Gupta Thread-Topic: [EXT] [PATCH v3 1/1] test/compress: unit tests refactoring Thread-Index: AQHVikwL8Flm67vmt0eCZaJdlv6k+Kd0+lkAgAXXd3CAAEAYgIAC758AgAAuK4CAACN1QA== Date: Wed, 6 Nov 2019 15:33:25 +0000 Message-ID: <5B6D1C77E9D7034C93E97BD83D1D9F572F2328AC@hasmsx107.ger.corp.intel.com> References: <20190912143431.26917-1-arturx.trybula@intel.com> <20191024091616.31224-1-arturx.trybula@intel.com> <20191024091616.31224-2-arturx.trybula@intel.com> <5B6D1C77E9D7034C93E97BD83D1D9F572F231F97@hasmsx107.ger.corp.intel.com> <5B6D1C77E9D7034C93E97BD83D1D9F572F23270C@hasmsx107.ger.corp.intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiM2QzNmVhNjktZTFlZS00ZTBlLWJmNDAtYmQzMDczNmM3MjEwIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoia0IwdTZVMjRnTGdxdHVzWkJpaUorYlwvMWF3U0hLeStSK0ptZTh0dUtlQVlHeTJCWFBnaHJsOXAyeTdyXC9UVm5rIn0= x-originating-ip: [10.184.70.11] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [EXT] [PATCH v3 1/1] test/compress: unit tests refactoring X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" >=20 >=20 > > -----Original Message----- > > From: Trybula, ArturX > > Sent: Monday, November 4, 2019 3:55 PM > > To: Shally Verma ; dev@dpdk.org; Trahe, Fiona=20 > > ; Dybkowski, AdamX > ; > > akhil.goyal@nxp.com > > Subject: RE: [EXT] [PATCH v3 1/1] test/compress: unit tests=20 > > 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 > > > --- > > > 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 =3D=3D NULL -> compression > > > + * if ops_processed !=3D NULL -> decompression > [Shally] we are trying to overload its purpose here. Can it be make simp= ler . > we can use interim test param to check if op type is=20 > compression/decompression and then use op_processed[] on need basis=20 > This will help in code readability else function looks complex to=20 > understand where as purpose is very simple. > [Artur] In some way it is not a perfect solution, but from my point of=20 > 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=20 > pointers to xforms, but which one should I check? There must be some=20 > switch to choose the right flow. I can add another input param to the=20 > function, to indicate if it's a compress or decompress operation. But=20 > it seems to me, that it would be too much for this one case. Finally I=20 > can add another input param if you like this idea? >=20 [Shally] Believe you agreed below that you'll add an param op_type? [Artur] Yes, this is what we agreed. ... > > > + else { > > > + if (ops_processed =3D=3D NULL) { > > > + > > > + not_zlib_compr =3D (test_data->zlib_dir =3D=3D > > > ZLIB_DECOMPRESS > > > + || test_data->zlib_dir =3D=3D ZLIB_NONE); > > > + > > > + ratio =3D (not_zlib_compr && > > > + (overflow =3D=3D OVERFLOW_ENABLED)) ? > > > + COMPRESS_BUF_SIZE_RATIO_OVERFLOW : > > > + COMPRESS_BUF_SIZE_RATIO; > [Shally] Why this condition is not true in case of ZLIB compression?=20 > For zlib compression too it can overflow right in case of deflate=20 > uncompressed output?! > [Artur] I discussed this question with Fiona. The test was design for=20 > QAT and other tools like QAT. In this case Zlib is used just for a=20 > verification if the compression using QAT was correct. This condition sho= uld be preserved. >=20 [Shally] ohh ok I noticed not_zlib_compr means zlib will decompress. I thou= ght it otherwise. So ignore my feedback. [Artur] Ok > > > + > > > + data_size =3D strlen(test_bufs[i]) * ratio; > > > + > > > + } else { > > > + priv_data =3D (struct priv_op_data *) > > > + (ops_processed[i] + 1); > > > + data_size =3D 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=20 > unless object priv data is corrupted or not updated with test buf=20 > size, which ideally should not be the case. > Given that, it can be just updated that func returns expected output=20 > buffer size. > [Artur] I don't see any reason why priv data could be corrupted. It=20 > would be a bigger problem if it happened. > All the cases are covered and the return value must be correct. If=20 > ops_processed is not NULL than priv_data has to be correct or we have=20 > a problem with QAT PMD. Verification if the filed orig_idx is correct=20 > is even more complicated. From my perspective mentioned verification=20 > 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 va= lue. So my point was value 0 seems redundant here. [Artur] Ok, I see your point. It's true that initialization is not necessar= y. Will be corrected. > Another input param discussed below: op_type? >=20 > > > +} > > > + > > > + > > > +/** > > > + * 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=20 > on op_type : compression / decompression [Artur] Ok >=20 > > > + * ops_processed value. Developer is requested to provide input=20 > > > + params > > > + * according to the following rule: > > > + * if ops_processed =3D=3D NULL -> current_bufs =3D comp_bufs[] > > > + * if ops_processed !=3D NULL -> current_bufs =3D decomp_bufs[] > > > + * -1 returned if function fail, without modifying the mbuf. > > > + * > [Shally] Same feedback here. This can be made simpler by adding=20 > 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=20 > the same way. I'm going to define another enum. It will be an extra=20 > input param of these functions: > 1. test_mbufs_calculate_data_size > 2. test_mbufs_destination_preparation > Is it ok for you? >=20 [Shally] Ok >=20 ... > > > + * @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) { >=20 > [Shally] I would still recommend to make it part of priv array and=20 > keep prototype simpler to read [Artur] What object do you mean precisely? >=20 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.=20 [Artur] Shally, I assume all the issues can be closed and V4 can be prepare= d tomorrow? > ... ... > > > 2.17.1