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 13505A04AB; Wed, 6 Nov 2019 12:33:25 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 1AD841C038; Wed, 6 Nov 2019 12:33:24 +0100 (CET) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 993771C027 for ; Wed, 6 Nov 2019 12:33:21 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Nov 2019 03:33:20 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.68,274,1569308400"; d="scan'208";a="201080471" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga007.fm.intel.com with ESMTP; 06 Nov 2019 03:33:19 -0800 Received: from fmsmsx116.amr.corp.intel.com (10.18.116.20) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 6 Nov 2019 03:33:19 -0800 Received: from hasmsx105.ger.corp.intel.com (10.184.198.19) by fmsmsx116.amr.corp.intel.com (10.18.116.20) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 6 Nov 2019 03:33:18 -0800 Received: from hasmsx107.ger.corp.intel.com ([169.254.2.187]) by HASMSX105.ger.corp.intel.com ([169.254.1.77]) with mapi id 14.03.0439.000; Wed, 6 Nov 2019 13:33:16 +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+lkAgAXXd3CAAEAYgIAC758A Date: Wed, 6 Nov 2019 11:33:15 +0000 Message-ID: <5B6D1C77E9D7034C93E97BD83D1D9F572F23270C@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> 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: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMzhhY2ZhOTktMmJjMi00MDk3LWExYzMtZmI4YjcwNDliZTc0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiOHI5ZHNDTU5STTJ6WlJUc2RzREc5MTNtSkFsNEF1d0luRnlvcytGWWhoVU9rZld0TVBqNm1iODRRWGM4UVBCVCJ9 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" > -----Original Message----- > From: Trybula, ArturX > Sent: Monday, November 4, 2019 3:55 PM > To: Shally Verma ; dev@dpdk.org; Trahe, Fiona=20 > ; Dybkowski, AdamX ;=20 > akhil.goyal@nxp.com > Subject: RE: [EXT] [PATCH v3 1/1] test/compress: unit tests=20 > refactoring >=20 > Hi Shally, >=20 > Please find my comments below. >=20 > > -------------------------------------------------------------------- > > -- 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(-) > > ..... > > +static void > > +test_objects_init(struct test_private_arrays *prv, > > + unsigned int num_bufs) > > +{ > > + /* Initialize all arrays to NULL */ > > + memset(prv->uncomp_bufs, 0, sizeof(struct rte_mbuf *) * > > num_bufs); > > + memset(prv->comp_bufs, 0, sizeof(struct rte_mbuf *) * num_bufs); > > + memset(prv->ops, 0, sizeof(struct rte_comp_op *) * num_bufs); > > + memset(prv->ops_processed, 0, sizeof(struct rte_comp_op *) * > > num_bufs); > > + memset(prv->priv_xforms, 0, sizeof(void *) * num_bufs); > > + memset(prv->compressed_data_size, 0, sizeof(uint32_t) * > > num_bufs); } > > + > Does it really matter what pointer is pointing to? Sizeof(any pointer)=20 > * num_bufs .. should be enough. > [Artur] You are absolutely right that sizeof() of any pointer gives=20 > the same result. But to be honest I don't understand your idea. Would=20 > you like to have an extra variable to initialise all the pointer arrays? > Eg: > uint32_t array_size =3D sizeof(void *) * num_bufs; ... > memset(prv->uncomp_bufs, 0, array_size); > memset(prv->comp_bufs, 0, array_size); ... [Shally] Hmm... ya that sort of. But my 2cents on it. So, its up to you . [Artur] Ok, it will be modified according your suggestion. >=20 >=20 > > +/** > > + * Source buffers preparation (for compression). > > + * [Shally] Can we have more details here like: API that allocates input (and = output buffers?) to perform compression operations? [Artur] Ok, it will be modified according your suggestion. > > + * Memory allocation for each buffer from mempool > > + * -1 returned if function fail, without modifying the mbuf. > > + * > > + * @param int_data [Shally] Can we change it to name test_params? [Artur] Yes, we can. > > + * Interim data containing session/transformation objects. > > + * @param test_data > > + * The test parameters set by users (command line parameters). > > + * @param priv_arrays [Shally] Can we change it to name test_priv_data? [Artur] Yes, we can. > > + * A container used for aggregation all the private test arrays. > > + * @return > > + * - 0: On success. > > + * - -1: On error. > > */ > > static int > > -test_deflate_comp_decomp(const struct interim_data_params *int_data, > > - const struct test_data_params *test_data) > > +test_mbufs_source_preparation(const struct interim_data_params > > *int_data, > > + const struct test_data_params *test_data, > > + const struct test_private_arrays *priv_arrays) > > { .... > > +/** > > + * Data size calculation (for both compression and decompression). > > + * [Shally] Can add more details here like calculate size of anticipated outpu= t buffer required for both compression and decompression operations based o= n input test_params. Caller then allocate output buffer based on size returned by this function. [Artur] Ok, it will be modified according your suggestion. > > + * 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 simple= r . we can use interim test param to check if op type is compression/decomp= ression and then use op_processed[] on need basis This will help in code readability else function looks complex to understan= d 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.=20 There is no info in the interim test param (I suppose you meant interim_dat= a_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? > > + * Values bigger than 0 have to be returned to avoid problems > > + * with memory allocation > > + * [Shally] Can be reworded as " function return non-zero on success, 0 on fai= lure. Caller should not allocate memory if 0.=20 [Artur] Ok > > + * @param ops_processed > > + * Operations created as a result of compression phase. Should be > > + * set to NULL for compression > > + * @param out_of_space_and_zlib > > + * Boolean value to switch into "out of space" buffer if set. > > + * To test "out-of-space" data size, zlib_decompress must be set as = well. > > + * @param int_data > > + * Interim data containing session/transformation objects. > > + * @param test_data > > + * The test parameters set by users (command line parameters). > > + * @param i > > + * current buffer index > > + * @return > > + * - values bigger than 0 > > + */ > > +static inline uint32_t > > +test_mbufs_calculate_data_size( > > + struct rte_comp_op *ops_processed[], /*can be equal to > > NULL*/ > > + unsigned int out_of_space_and_zlib, > > + const struct interim_data_params *int_data, > > + const struct test_data_params *test_data, > > + unsigned int i) > > +{ > > + /* local variables: */ > > + uint32_t data_size =3D 0; > > + struct priv_op_data *priv_data; > > + float ratio; > > + uint8_t not_zlib_compr; /* true if zlib isn't current compression dev > > */ > > + enum overflow_test overflow =3D test_data->overflow; > > + > > + /* from int_data: */ > > + const char * const *test_bufs =3D int_data->test_bufs; > > + > > + if (out_of_space_and_zlib) > > + data_size =3D OUT_OF_SPACE_BUF; > > + 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? For zl= ib compression too it can overflow right in case of deflate uncompressed ou= tput?! [Artur] I discussed this question with Fiona. The test was design for QAT a= nd 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 preserve= d. > > + > > + 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 unles= s object priv data is corrupted or not updated with test buf size, which id= eally should not be the case.=20 Given that, it can be just updated that func returns expected output buffe= r size. [Artur] I don't see any reason why priv data could be corrupted. It would b= e a bigger problem if it happened. All the cases are covered and the return value must be correct. If ops_proc= essed is not NULL than priv_data has to be correct or we have a problem wit= h QAT PMD. Verification if the filed orig_idx is correct is even more compl= icated. From my perspective mentioned verification shouldn't be considered = as a part of this function. It's too late. 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 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 =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 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 sa= me way. I'm going to define another enum. It will be an extra input param o= f these functions: 1. test_mbufs_calculate_data_size 2. test_mbufs_destination_preparation Is it ok for you? > > + * @param ops_processed > > + * Operations created as a result of compression phase. Should be > > + * set to NULL for compression > > + * @param current_bufs > > + * mbufs being prepared in the function. > > + * @param out_of_space_and_zlib > > + * Boolean value to switch into "out of space" buffer if set. > > + * To test "out-of-space" data size, zlib_decompress must be set as = well. > > + * @param int_data > > + * Interim data containing session/transformation objects. > > + * @param test_data > > + * The test parameters set by users (command line parameters). > > + * @param current_extbuf_info, > > + * The structure containing all the information related to external = mbufs > > + * @param current_memzone > > + * memzone used by external buffers [Shally] add "valid if current_extbuf_info is set [Artur] 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 pro= totype simpler to read [Artur] What object do you mean precisely? ... > > +/** > > + * The main compression function. > > + * > > + * Operation(s) configuration, depending on CLI parameters. > > + * Operation(s) processing. > > + * -1 returned if function fail. > > + * > > + * @param int_data > > + * Interim data containing session/transformation objects. > > + * @param test_data > > + * The test parameters set by users (command line parameters). > > + * @param priv_arrays > > + * A container used for aggregation all the private test arrays. > > + * @return > > + * - 0: On success. > > + * - -1: On error. > > + */ ... > > +/** > > + * The main decompression function. [Shally] add more description : function perform decompression op=20 [Artur] Ok > > + * > > + * Operation(s) configuration, depending on CLI parameters. > > + * Operation(s) processing. > > + * -1 returned if function fail. > > + * [Shally] Not needed here.. @return also mention same [Artur] Ok > > + * @param int_data > > + * Interim data containing session/transformation objects. > > + * @param test_data > > + * The test parameters set by users (command line parameters). > > + * @param priv_arrays > > + * A container used for aggregation all the private test arrays. > > + * @return > > + * - 0: On success. > > + * - -1: On error. > > + */ > > +static int > > +test_deflate_decomp_run(const struct interim_data_params *int_data, > > + const struct test_data_params *test_data, > > + struct test_private_arrays *priv_arrays) { > > .... > > +/** > > + * Compresses and decompresses input stream with compressdev API > and > > +Zlib API > > + * > > + * Basic test function. Common for all the functional tests. > > + * -1 returned if function fail. [Shally] Not needed here [Artur] Ok > > + * > > + * @param int_data > > + * Interim data containing session/transformation objects. > > + * @param test_data > > + * The test parameters set by users (command line parameters). > > + * @return > > + * - 1: Some operation not supported > > + * - 0: On success. > > + * - -1: On error. > > + */ > > + ... > > + capa =3D rte_compressdev_capability_get(0, > > RTE_COMP_ALGO_DEFLATE); > > + if (capa =3D=3D NULL) { > > + RTE_LOG(ERR, USER1, > > + "Compress device does not support DEFLATE\n"); > > + return -1; > > + } > > + test_objects_init(&priv_arrays, num_bufs); > Do we need this? Why we just cant call source_prep API? That will also se= tup > uncompressed_bufs > [Artur] This is my approach. Do you prefer to have arrays initialisation = moved > to test_mbufs_source_preparation? > If "yes", test_objects_init will be removed from the code. [Shally] Ya. We can minimize code bit here. [Artur] Ok >=20 > > + > > + /* Prepare the source mbufs with the data */ > > + ret =3D test_mbufs_source_preparation(int_data, test_data, > > &priv_arrays); > For code readability sake, it could test_setup_uncom_bufs() > [Artur] Good idea. Would it be ok if I change: > 1. test_mbufs_destination_preparation into test_setup_uncom_bufs > 2. test_mbufs_source_preparation into test_setup_com_bufs >=20 [Shally] Ya that look better or we can say , setup_input_bufs , setup_outpu= t_bufs .. just few suggestios [Artur] Ok ... > > +/* COMPRESSION */ > > + > > + /* Prepare the destination mbufs */ > > + ret =3D test_mbufs_destination_preparation( > Why we cant keep this prototype same as source_preparation() API for > better readability? > I mean we can pass priv_arry here too... > [Artur] To get better readability it has been implemented in this way. Th= at > was only my point of view but I don't mind to modify the code according t= o > your suggestion. It's also ok. >=20 [Shally] Ya. I would prefer=20 [Artur] Ok > > + NULL, /*can be equal to NULL*/ > > + comp_bufs, > > + out_of_space =3D=3D 1 && !zlib_compress, > > + int_data, > > + test_data, > > + &compbuf_info, > > + test_data->compbuf_memzone); > > + if (ret < 0) { > > + ret_status =3D -1; > > + goto exit; > > + } > > + ... > > 2.17.1