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 5EB4CA0350; Mon, 11 May 2020 14:32:52 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id B42901C1B4; Mon, 11 May 2020 14:32:51 +0200 (CEST) Received: from mx0b-0016f401.pphosted.com (mx0a-0016f401.pphosted.com [67.231.148.174]) by dpdk.org (Postfix) with ESMTP id B97DE1C0D4 for ; Mon, 11 May 2020 14:32:50 +0200 (CEST) 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 04BCP9Cg024972; Mon, 11 May 2020 05:32:47 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=marvell.com; h=from : to : cc : subject : date : message-id : references : in-reply-to : content-type : content-transfer-encoding : mime-version; s=pfpt0818; bh=6eO4JvTfUu4KzS6VyKlcBjvy9wtTszzRnCi26d4UHhQ=; b=wbM8VV83BxXnCTSoS7yfcgxy65lsUfQ4hTmuwNs3p7VOvOM/hTmeq2GN2zZKMDzM74yj 76MqvZlQwr5KqmC1B12EynS8L5pr2cZOQz1hEtxDJHnxnwqs1NLahfTw3LfJT1I97eiq QHJmgGe9btrDOME6ezT9I0iDX6MmW8YqvruOIYclDrWYv+pk/SjRceA/I+RjFdqUoXdQ aTcUa8F5zIb3WhCtFvuenBvFujFKTV23xP3sPc9bAlrSC9F6y61cKkc7/xN1a2v2ceiP T89DsYxsFdNS544WWbX84FIZYmy9kii2sDJlA4MpgFqaM2q25lBwr65+9JvSvx971sd7 KQ== Received: from sc-exch02.marvell.com ([199.233.58.182]) by mx0a-0016f401.pphosted.com with ESMTP id 30wsvqenn1-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-SHA384 bits=256 verify=NOT); Mon, 11 May 2020 05:32:46 -0700 Received: from DC5-EXCH02.marvell.com (10.69.176.39) by SC-EXCH02.marvell.com (10.93.176.82) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Mon, 11 May 2020 05:32:45 -0700 Received: from SC-EXCH01.marvell.com (10.93.176.81) by DC5-EXCH02.marvell.com (10.69.176.39) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Mon, 11 May 2020 05:32:44 -0700 Received: from NAM12-MW2-obe.outbound.protection.outlook.com (104.47.66.41) by SC-EXCH01.marvell.com (10.93.176.81) with Microsoft SMTP Server (TLS) id 15.0.1497.2 via Frontend Transport; Mon, 11 May 2020 05:32:43 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=eDGpUnBc5tJXFQzXYio7w8cpwfWUPuteNofoD9ybVDC8y/R8cAdE3dYFea8toTidULHd0OqHvqs7PqgLFPSh2SScuXmD3dbMjyPYY+HXiNNPYb7eOi6sppqmxRye34Z12sXn9hTO/O1P7U2XWL1CnmassbxhTDlbJw8buq4smWiqp6DqJA1zG6bTQgIRW3Wr9mqMaZ0aS8MYibz4/eu0S/0Twb3KgmAQOUc+rB1CFCJfK7PQca43/3AECZ+M6sPbJHXVRZdidZ3bJJOPuNIePNe0o7VCjsjTlDFU0O7pOwVHlXkkSgKUuR7F7x5DrBUCBcfiNowBRGLOMVIzCwg2Mg== 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=6eO4JvTfUu4KzS6VyKlcBjvy9wtTszzRnCi26d4UHhQ=; b=jBL2xKENOB9Q46ZC0dF6CR1LY/u2B3l4sp1vJ7eN3xC/PaUCC4EJTnWoUTq0cajDpWidXBfntFhEYCQvjqFWFs+qKWzbn5E3IGjcnxV/IWXu0hojEkgW3kQT1O1XLAnfqT251OmK1NkAhuG3UFgQIQRV5pFG3dLt5aUAPMXRmL5gXBBC2C5i+98XcF2zWudCi9HIcXWeNwoh8MshbYeCgHgIK4wSl19BVTDPzPKvTSjAdGM8PicoiQWMg3vNQP5e8udvzAUkTlNMCpu0V13oq9+jXAILTYkNKPFww5ioCaZiZaYSWeEQeBDzXUgCNSo+XSVnVsUHO32iWShltbiEaw== 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=selector1-marvell-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=6eO4JvTfUu4KzS6VyKlcBjvy9wtTszzRnCi26d4UHhQ=; b=dhdgrxGSDcqR/yBzK682Ok5WgtKzknEcPI7DcTnM+yZ+fVDMF2NXbvot/pWOD7FshJq9aSCxMmzJoKVU3E8iEePz/F0xcHVqeNLzlnh6XLktU+XWa9bZFrvASwMnmUGPLvyp3+Zf+3LqaOoQO5IFrDo76dOB+pfzluemYR4R+2Y= Received: from BYAPR18MB2518.namprd18.prod.outlook.com (20.179.94.82) by BYAPR18MB2488.namprd18.prod.outlook.com (20.179.90.156) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2979.33; Mon, 11 May 2020 12:32:43 +0000 Received: from BYAPR18MB2518.namprd18.prod.outlook.com ([fe80::fca4:5e00:46d9:a289]) by BYAPR18MB2518.namprd18.prod.outlook.com ([fe80::fca4:5e00:46d9:a289%3]) with mapi id 15.20.2979.033; Mon, 11 May 2020 12:32:43 +0000 From: Pavan Nikhilesh Bhagavatula To: "Ananyev, Konstantin" , "Jerin Jacob Kollanukkaran" , "thomas@monjalon.net" , "Wang, Yipeng1" , "Gobriel, Sameh" , "Richardson, Bruce" , Ruifeng Wang CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [RFC] hash: unify crc32 API header for x86 and ARM Thread-Index: AQHWJTgHo7IAeERCXE2KuklKRB3ozKih8AfggAC3mICAAAZJoIAABUUAgAABptCAABsTAIAABZXg Date: Mon, 11 May 2020 12:32:42 +0000 Message-ID: References: <20200429180515.5704-1-pbhagavatula@marvell.com> In-Reply-To: Accept-Language: en-IN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: intel.com; dkim=none (message not signed) header.d=none;intel.com; dmarc=none action=none header.from=marvell.com; x-originating-ip: [223.226.86.58] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: b7e1b20c-3706-44a8-1aac-08d7f5a766ef x-ms-traffictypediagnostic: BYAPR18MB2488: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-forefront-prvs: 04004D94E2 x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: b/EWc7zBaCJn40u8lurekufkO/i7wYOaStYfixD/Amn+z1N7dIhC6G97mkJKm8E7NAi8wR9lKaILk1TqnjK+6D31HHrmOpZBq4jXOlAptaSYRzibYeEN6W9bjg/2/NVOMRL/ymZ645eRPL6jwOEPQvKm8zXw3fu0vGvXajHmRhUOpx9Me35O9atErb4QBY3oMXRNfxUaMMj7tJY1fLUzvpSHAkWFXK3MJDcA2jstlRQzuKRrIL5OSbEa+PqcVpFI/KoEoZ/Fslrxi2T7rzmQ82EUTCdhqImXlKXUc+wNV0/tcWLfF0ddJ96YEVd36+/MOSYgmZhAYN55uAlRPgjfOPfMRJhVeluTxnFPmjjGaUs02s4X7CeiXR913FyAfySFmE7Drp/KOu+z2x71Xx2+FdlwjqHinzFAQKVK7Vl+lsrvCtSl2Pjxlu0AiGDJkL2bf6wPVM/rJN0nL5KnSaCo5ULsqecafu6QYwqRZvnYD9x4id4hyeBKxN2Cj7+Yl5xtzLX3m5EqzVDVypio2K/ebP0PIzWUuv++HFR7ph6RN3U= x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:BYAPR18MB2518.namprd18.prod.outlook.com; PTR:; CAT:NONE; SFTY:; SFS:(4636009)(39850400004)(366004)(136003)(396003)(376002)(346002)(33430700001)(86362001)(186003)(7696005)(8936002)(26005)(8676002)(55236004)(4326008)(71200400001)(316002)(5660300002)(9686003)(55016002)(2906002)(110136005)(6506007)(66476007)(64756008)(66556008)(52536014)(66946007)(478600001)(76116006)(33656002)(33440700001)(66446008)(921003); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata: aQw654U7HwhrNkyTDMnowUdSuISZ+K1AF0699SZgTyzG8pHnzDyLLq9OUzGoeTJHaLq+4yx4VkRubLCeTdr3fMqr1XdfynVdsaENvEAP6FW82Nhd4DIbfVAysaOsf9TgU6DpCw5Zd2+Z9Lq1epZ1Oo6F3aJ2H6EG80JsgHNXwwWj3sduudv6ZN4Ye5gtKNZimRzaxtLs3QJsbtzNH0HZ8ZNUr7/qjqqOFrMQaasN/HOAyIkeNlwyCTd4+HazovcF04X7ZkzTSGSPkUsiIldT+GZJ2sLilB2DK+nvBdV+oNNasO5nRB9GFA5r25uiaHXI5XUoKCoTs4AU/47+fGQSPtxknh/i5VVuM56gfJ5YRSdlxFGP8j8fi7ZmzUaF+l+CK90CSy+JqXoVoNhPGJ079Mof+tHKA+p2CzukrX/UiE8o5RYgTAGYdFGkJSgGZVEIj2vsC4g6ZpysIgAebN5JQk26VZxQQCM12e2QVdwal2w= Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: b7e1b20c-3706-44a8-1aac-08d7f5a766ef X-MS-Exchange-CrossTenant-originalarrivaltime: 11 May 2020 12:32:42.9745 (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: s6HFvuDNf5K/ippvlAejexK75+Sp5qmtruMBJasSUOyPGrJ1h2us9dvBauWsQdmsL6S/QUv2IQjRq/y2w97vxdRFgdWByJVAKpQNeeRniL8= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR18MB2488 X-OriginatorOrg: marvell.com X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.216, 18.0.676 definitions=2020-05-11_04:2020-05-11, 2020-05-11 signatures=0 Subject: Re: [dpdk-dev] [RFC] hash: unify crc32 API header for x86 and ARM 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" >> >> >> >> From: Pavan Nikhilesh >> >> >> >> >> >> >> >> Merge crc32 hash calculation public API headers for x86 and >> >ARM, >> >> >> >> split implementations of x86 and ARM into their respective >> >private >> >> >> >> headers. >> >> >> >> This reduces the ifdef code clutter while keeping current ABI >> >> >intact. >> >> >> >> >> >> >> >> Although we install `rte_crc_arm64.h` it is not used in any of >the >> >lib >> >> >or >> >> >> >> drivers layers. All the libs and drivers use `rte_hash_crc.h` >which >> >> >falls >> >> >> >> back to SW crc32 calculation for ARM platform. >> >> >> >> >> >> >> >> Signed-off-by: Pavan Nikhilesh > >> >> >> >> --- >> >> >> >> >> >> >> >> Currently, if application incorrectly sets CRC32_ARM64 as >crc32 >> >> >> >algorithm >> >> >> >> through `rte_hash_crc_set_alg()` on x86 or vice-versa we >> >fallback >> >> >to >> >> >> >algorithm >> >> >> >> set previously via `rte_hash_crc_set_alg()` instead of setting >> >the >> >> >best >> >> >> >> available. >> >> >> >> This behaviour should probably change to setting the best >> >> >available >> >> >> >algorithm >> >> >> >> and is up for discussion. >> >> >> >> >> >> >> >> app/test/test_hash.c | 6 + >> >> >> >> lib/librte_hash/Makefile | 5 - >> >> >> >> lib/librte_hash/crc_arm64.h | 67 +++++++++++ >> >> >> >> lib/librte_hash/crc_x86.h | 68 +++++++++++ >> >> >> >> lib/librte_hash/meson.build | 3 +- >> >> >> >> lib/librte_hash/rte_crc_arm64.h | 183 ------------------------= -- >---- >> >> >> >> lib/librte_hash/rte_hash_crc.h | 193 +++++++++++++------- >---- >> >--- >> >> >---- >> >> >> >- >> >> >> >> 7 files changed, 219 insertions(+), 306 deletions(-) >> >> >> >> create mode 100644 lib/librte_hash/crc_arm64.h >> >> >> >> create mode 100644 lib/librte_hash/crc_x86.h >> >> >> >> delete mode 100644 lib/librte_hash/rte_crc_arm64.h >> >> >> >> >> >> >> >> diff --git a/app/test/test_hash.c b/app/test/test_hash.c >> >> >> >> index afa3a1a3c..7bd457dac 100644 >> >> >> >> --- a/app/test/test_hash.c >> >> >> >> +++ b/app/test/test_hash.c >> >> >> >> @@ -195,7 +195,13 @@ test_crc32_hash_alg_equiv(void) >> >> >> >> } >> >> >> >> >> >> >> >> /* Resetting to best available algorithm */ >> >> >> >> +#if defined RTE_ARCH_X86 >> >> >> >> rte_hash_crc_set_alg(CRC32_SSE42_x64); >> >> >> >> +#elif defined RTE_ARCH_ARM64 >> >> >> >> + rte_hash_crc_set_alg(CRC32_ARM64); >> >> >> >> +#else >> >> >> >> + rte_hash_crc_set_alg(CRC32_SW); >> >> >> >> +#endif >> >> >> >> >> >> >> >> if (i =3D=3D CRC32_ITERATIONS) >> >> >> >> return 0; >> >> >> >> diff --git a/lib/librte_hash/Makefile >b/lib/librte_hash/Makefile >> >> >> >> index ec9f86499..f640afc42 100644 >> >> >> >> --- a/lib/librte_hash/Makefile >> >> >> >> +++ b/lib/librte_hash/Makefile >> >> >> >> @@ -19,11 +19,6 @@ SRCS-$(CONFIG_RTE_LIBRTE_HASH) >+=3D >> >> >> >rte_fbk_hash.c >> >> >> >> # install this header file >> >> >> >> SYMLINK-$(CONFIG_RTE_LIBRTE_HASH)-include :=3D >rte_hash.h >> >> >> >> SYMLINK-$(CONFIG_RTE_LIBRTE_HASH)-include +=3D >> >> >rte_hash_crc.h >> >> >> >> -ifeq ($(CONFIG_RTE_ARCH_ARM64),y) >> >> >> >> -ifneq ($(findstring >> >RTE_MACHINE_CPUFLAG_CRC32,$(CFLAGS)),) >> >> >> >> -SYMLINK-$(CONFIG_RTE_LIBRTE_HASH)-include +=3D >> >> >rte_crc_arm64.h >> >> >> >> -endif >> >> >> >> -endif >> >> >> >> SYMLINK-$(CONFIG_RTE_LIBRTE_HASH)-include +=3D >> >rte_jhash.h >> >> >> >> SYMLINK-$(CONFIG_RTE_LIBRTE_HASH)-include +=3D >> >rte_thash.h >> >> >> >> SYMLINK-$(CONFIG_RTE_LIBRTE_HASH)-include +=3D >> >> >rte_fbk_hash.h >> >> >> >> diff --git a/lib/librte_hash/crc_arm64.h >> >> >b/lib/librte_hash/crc_arm64.h >> >> >> >> new file mode 100644 >> >> >> >> index 000000000..8e75f8297 >> >> >> > >> >> >> >Wouldn't that break 'make install T=3D...'? >> >> >> >> >> >> My bad I verified with meson and it was building fine. >> >> >> >> >> >> >As now rte_hash_crc.h includes not public headers (crc_x86.h, >> >etc.). >> >> >> >Same question about external apps, where they would get >from >> >> >these >> >> >> >headers? >> >> >> >> >> >> I think in the next version we can directly have the arch specific >> >> >functions >> >> >> Implemented in rte_hash_crc.h. Since its pretty stable code and >> >> >overhead of extra >> >> >> ~120 lines. >> >> > >> >> >Ok... but why not then just leave arch specific headers, as they >are >> >right >> >> >now? >> >> >What is wrong with current approach? >> >> >> >> The problem is if any application directly includes only >> >rte_crc_arm64.h >> >> (completely legal) it will break the build. >> > >> >But we can probably mark rte_crc_arm64.h as internal, and warn >users >> >not to >> >include it directly (same for rte_crc_x86.h and any other arch specific >> >headers). >> >> Yes but I think merging them would be a cleaner, number of >constructors would be >> one and maybe we could select the best available algorithm on a >given platform when >> application requests unsupported one. > >Ok, but we can still have one constructor, and two (or more) different >arch specific headers, >that would be included into main header conditionally by #ifdef >RTE_ARCH_.... > >> >> As Yipeng mentioned do you thing having a indirect call instead of >runtime branch be >> depreciative in terms of performance? > >I think run-time branch by some global var would be much faster than >indirect function call >(at least on IA). > Ok, makes sense as in a tight loop the run-time branch would be hoisted out= . Let me draft a RFC v2. >> >> > >> >> >> >> Example: >> >> >> >> diff --git a/lib/librte_efd/rte_efd.c b/lib/librte_efd/rte_efd.c >> >> index 6a799556d..318670940 100644 >> >> --- a/lib/librte_efd/rte_efd.c >> >> +++ b/lib/librte_efd/rte_efd.c >> >> @@ -19,7 +19,7 @@ >> >> #include >> >> #include >> >> #include >> >> -#include >> >> +#include >> >> #include >> >> >> >> #include "rte_efd.h" >> >> (END) >> >> >> >> Causes: >> >> >> >> ../lib/librte_hash/rte_crc_arm64.h: In function >> >'rte_hash_crc_set_alg': >> >> ../lib/librte_hash/rte_crc_arm64.h:77:7: error: 'CRC32_ARM64' >> >undeclared (first use in this function) >> >> 77 | case CRC32_ARM64: >> >> | ^~~~~~~~~~~ >> >> ../lib/librte_hash/rte_crc_arm64.h:77:7: note: each undeclared >> >identifier is reported only once for each function it appears in >> >> ../lib/librte_hash/rte_crc_arm64.h:79:10: error: 'CRC32_SW' >> >undeclared (first use in this function) >> >> 79 | alg =3D CRC32_SW; >> >> | ^~~~~~~~ >> >> ../lib/librte_hash/rte_crc_arm64.h:82:3: error: 'crc32_alg' >undeclared >> >(first use in this function) >> >> 82 | crc32_alg =3D alg; >> >> | ^~~~~~~~~ >> >> ../lib/librte_hash/rte_crc_arm64.h: In function >> >'rte_hash_crc_init_alg': >> >> ../lib/librte_hash/rte_crc_arm64.h:92:23: error: 'CRC32_ARM64' >> >undeclared (first use in this function) >> >> 92 | rte_hash_crc_set_alg(CRC32_ARM64); >> >> >> >> Thanks, >> >> Pavan. >> >> >> >>