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 58F92A0352; Mon, 4 Nov 2019 16:25:03 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 2450C37A2; Mon, 4 Nov 2019 16:25:03 +0100 (CET) Received: from mx0b-0016f401.pphosted.com (mx0a-0016f401.pphosted.com [67.231.148.174]) by dpdk.org (Postfix) with ESMTP id C08B73256 for ; Mon, 4 Nov 2019 16:25:01 +0100 (CET) Received: from pps.filterd (m0045849.ppops.net [127.0.0.1]) by mx0a-0016f401.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id xA4FHfGB015325; Mon, 4 Nov 2019 07:25:00 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=marvell.com; h=from : to : subject : date : message-id : references : in-reply-to : content-type : content-transfer-encoding : mime-version; s=pfpt0818; bh=DtNqCpMra1pDuoZMbyjbGHdYFVNN7sNUHtfwMVSz4R0=; b=L63TuYBQd7TZuVzziD1Cb15BU8VMnoPK7LomyfhfxZd5rESBeBqLJQ5DxOsZ97zFFu7t Z/MxODJ+7ax9Hp8S6nrk1+QRUNm2DADy9sqwzCLGL2SibOC3cXU2YPzoZHEw8539nYDm GVjpi1lDdwxOuw7843ZBLvFoW45SYuVOC4cx/aBUasrO/SnfmjPJVcftVNb3P3Zxe9Ac KNeyMyMrc3e6pxsXKRekTEluE5JDa55YrfZMLbhm7FYUedQljrPJUFFwwjSPzpJ0zA6S ZlVuJU66M/tMNL3LIQ9xh3vLp3VXE2Moc6AtVhk1TT1Id8XphFbYXqyPApkZkGMMxM2c bg== Received: from sc-exch04.marvell.com ([199.233.58.184]) by mx0a-0016f401.pphosted.com with ESMTP id 2w17n8x6js-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-SHA384 bits=256 verify=NOT); Mon, 04 Nov 2019 07:25:00 -0800 Received: from SC-EXCH02.marvell.com (10.93.176.82) by SC-EXCH04.marvell.com (10.93.176.84) with Microsoft SMTP Server (TLS) id 15.0.1367.3; Mon, 4 Nov 2019 07:24:59 -0800 Received: from NAM05-CO1-obe.outbound.protection.outlook.com (104.47.48.56) by SC-EXCH02.marvell.com (10.93.176.82) with Microsoft SMTP Server (TLS) id 15.0.1367.3 via Frontend Transport; Mon, 4 Nov 2019 07:24:59 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=KfE80bF+A3jeItGs4uINKRv38AQ4+E6kDRMlc3mE9Z5hyin9B8RbPKsMV2Go7VJANxC0xLv3vKYBOKJmAx9ka1FQbWr1TPIrgyz+mWYlHLdiG8ZwyuViFUL2HczIEEwIUteEy0/WRuApmiu28MxrPOMoqsqBU2BoDXS9dBA4ywV/68OdnOHHBhS+QH3LZmI0ywOd1/dZ3EABOwbIgx0+K4ruZP4ncOQvWX5RLhQXegy0AUMmATclcbXLsdWTkwGyxvrv677UxQydVC0286RwR659hSmQOI3oZqHKQOVJT796DZMAxB0ecnwm8DmM2q/qr2siEq+7EqFzFwqzL3pD5g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=DtNqCpMra1pDuoZMbyjbGHdYFVNN7sNUHtfwMVSz4R0=; b=lO3TazN4A2+GhAuTulbvcBBYOaqPOJzIJZW1BZM5FBGTgbXtde74ikHR+tcvuFZ/27nz9SXpmW4jvJwjUGcixy6x589yByS6G+MUoD+wM3JOWHoA+TwudwSxmqHmRtJbMZxAGJJC6gVAMptELi1KK1s82cf3GVbZL+TrLFS2X2A9K4lb8i5/zQ1LaMYVWc8Rms/Cp2P3nK2/IEJ9VNSvamP1SNLPFEU7RVvc9rUXTflyEgNCjtMr+lCgXR+cqPqNV1rAKlFJFSQhDG6elPjJTKQVbVodp87Ouddp6kyUJrN6tkz6XOCvthlDxPQY1YZPOJ3iLne/DQagmflYj3TEfg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=marvell.com; dmarc=pass action=none header.from=marvell.com; dkim=pass header.d=marvell.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=marvell.onmicrosoft.com; s=selector2-marvell-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=DtNqCpMra1pDuoZMbyjbGHdYFVNN7sNUHtfwMVSz4R0=; b=h4Z4esGwBG1nsMRcrReffdelazKJ0anRL+kZGiJknmu6RKZD3H6VjuTZ4eokVL5vpGIbo5nc38OhnGOSKKHI534BqUYhEW35mQyEMK9SyMQNLKdb+0RNhf+GCiCteGYWFVhkTCBVgrDssK+6gBxCO4MXKtspGBcvCwzLjUAxDUU= Received: from BN8PR18MB2867.namprd18.prod.outlook.com (20.179.72.89) by BN8PR18MB2977.namprd18.prod.outlook.com (20.179.76.27) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2408.24; Mon, 4 Nov 2019 15:24:58 +0000 Received: from BN8PR18MB2867.namprd18.prod.outlook.com ([fe80::cc7c:df90:83dd:ad6c]) by BN8PR18MB2867.namprd18.prod.outlook.com ([fe80::cc7c:df90:83dd:ad6c%4]) with mapi id 15.20.2408.024; Mon, 4 Nov 2019 15:24:58 +0000 From: Shally Verma To: "Trybula, ArturX" , "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: AQHVikwCFNfkVivdu0aZlwUMoGELQ6d1EEPAgAXPQQCAAEJmsA== Date: Mon, 4 Nov 2019 15:24:57 +0000 Message-ID: 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: <5B6D1C77E9D7034C93E97BD83D1D9F572F231F97@hasmsx107.ger.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [223.230.111.80] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 1282c35a-ea2f-4efe-1304-08d7613b26fc x-ms-traffictypediagnostic: BN8PR18MB2977: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:3276; x-forefront-prvs: 0211965D06 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(4636009)(39860400002)(396003)(376002)(346002)(366004)(136003)(52084003)(199004)(189003)(13464003)(33656002)(110136005)(316002)(2906002)(305945005)(8936002)(14454004)(81156014)(81166006)(74316002)(8676002)(478600001)(66946007)(66556008)(66446008)(64756008)(76116006)(66476007)(7736002)(25786009)(99286004)(6636002)(26005)(2501003)(66066001)(86362001)(55016002)(7696005)(6436002)(53546011)(6506007)(102836004)(76176011)(6246003)(71190400001)(71200400001)(446003)(9686003)(30864003)(11346002)(14444005)(186003)(256004)(486006)(476003)(229853002)(5660300002)(3846002)(52536014)(6116002); DIR:OUT; SFP:1101; SCL:1; SRVR:BN8PR18MB2977; H:BN8PR18MB2867.namprd18.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: marvell.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: WENa4nJmxvzrJ75S7aziormTk+bGu05rjPFb7WehtlWyA7wiJ8WjqN3Z5rVjuG1vsVqaos8E60Ry3tQjeFSYvUtPjrj9W6561Jl40mOzb5TOF1hH8zuBI4mJ9NhvIS8hlotjZs6aQ24PAuI98OnOUP/b3Dn339EtywOUKSKAHE6ZSIs/8V3igAAJz239Wbn9Fx51ZPum7VfRABOGSY9yV97b55egmNHrW/T5+mrCebdGeiIlPWjO9TFMU0KmcaEWwHTP1Ef1D3uv+dIc4X9J2eXKX1LUau3b46QlmOPTzULLLIT2/VEWxJrv9SDv9qJboL9DQwbs1w9t8L14lFW1zaew1osDrzwEKIPsYI5h+KaqRA88sKB77W0Ky1EQ9m9Td9RNPNenPXHgSDAsZo7BFUWtNS0Z+6+ycWLY/2bNsNJlS9bNlbHPc4Pl8W6PFNdN Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: 1282c35a-ea2f-4efe-1304-08d7613b26fc X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Nov 2019 15:24:58.0184 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: uUWb4V1tVtxag/sGZMNZXxfJnf5CkEzldffHWkHHSMCRQnBYbhhFrlavRLLio4NfH7NtJ24IewAEwujBsxKMmA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR18MB2977 X-OriginatorOrg: marvell.com X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.95,1.0.8 definitions=2019-11-04_09:2019-11-04,2019-11-04 signatures=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 > ; Dybkowski, AdamX > ; akhil.goyal@nxp.com > Subject: RE: [EXT] [PATCH v3 1/1] test/compress: unit tests 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) * > num_bufs .. should be enough. > [Artur] You are absolutely right that sizeof() of any pointer gives the s= ame > result. But to be honest I don't understand your idea. Would 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 . >=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? > > + * 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? > > + * 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? > > + * 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. > > + * 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. > > + * 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 > > + * @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?! > > + > > + 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. > > +} > > + > > + > > +/** > > + * 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 > > + * 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. > > + * @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 > > + * @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 ... > > +/** > > + * 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 > > + * > > + * Operation(s) configuration, depending on CLI parameters. > > + * Operation(s) processing. > > + * -1 returned if function fail. > > + * [Shally] Not needed here.. @return also mention same > > + * @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 > > + * > > + * @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. >=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 ... > > +/* 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 > > + 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