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 15240A0350; Mon, 11 May 2020 14:10:40 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 5C92F1BF74; Mon, 11 May 2020 14:10:39 +0200 (CEST) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 9C67E4C9D for ; Mon, 11 May 2020 14:10:37 +0200 (CEST) IronPort-SDR: 5vcqpgmam359ywPDMsFNYLOVvpEdEt1TDLg0RU/uz4GC2Da3bTscF4KnfJU3SweQcXSwrbL15F wQjTS/AoggHA== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 May 2020 05:10:36 -0700 IronPort-SDR: SlbARPDwgO84OFOuRVjm1Ez8eeKQkKGcTbgrx5R9nSfnCcQRuj4GjtNVOHRNVVTkwhKud+PmbF kRO2SrCZvRMw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,379,1583222400"; d="scan'208";a="296922973" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga002.fm.intel.com with ESMTP; 11 May 2020 05:10:36 -0700 Received: from fmsmsx603.amr.corp.intel.com (10.18.126.83) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 11 May 2020 05:10:36 -0700 Received: from fmsmsx605.amr.corp.intel.com (10.18.126.85) by fmsmsx603.amr.corp.intel.com (10.18.126.83) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Mon, 11 May 2020 05:10:35 -0700 Received: from FMSEDG002.ED.cps.intel.com (10.1.192.134) by fmsmsx605.amr.corp.intel.com (10.18.126.85) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Mon, 11 May 2020 05:10:35 -0700 Received: from NAM11-BN8-obe.outbound.protection.outlook.com (104.47.58.171) by edgegateway.intel.com (192.55.55.69) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 11 May 2020 05:10:34 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=W5aw1F788zFZhoxGuzPF1tOdhlndJJzQ0ut1fiS/IrSSnrVqDGw2IslAK12Jv8uIB6sVNhGWAKjm5aY5m+IgbHvJoDusbkltzcgbokX3n4L+FU3bxRVVw3dYzMUw4A/yc50LbEG9qYzI+dvlVE6RVClPIAth/x3mi0oy5FTguVVt3Qiw9xg4Y18KbVHXabWN/UK0Dg9QTVLmEG9kolQpmKWY2PoQT6aE/DPvioH3mFuPcm2PbpJDoU1UoVwks13HCE3pWI1i4O1jiayxET6heik7UykaBbvWkueroVsLLj8vrUE5seLZg+1HuK7G5He8dVwiwYXumnH0fdyKfb1kEw== 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=gXYcR02vd8uBINo2UjfOuHSZKWLAHzc/lCPNshtaUHM=; b=g0ZWCkOhWm4puY17IVxtyIp4s3wGY9RQQZw2SlgxS2Zw5BfAad4AfeVAzlJPdMHxZ38hoL+wuiGq1FhRvRf4/+FK1U6yeEXK23RKhUlhxVmovF3iWcIvaylryqVoOEAvcG7UFAHfGLNACcqpjHbSSNQsWF0wxnxAD4k+ru7XaFnkdSxeYYfqmqll34XpV2fGJkILFndZPJqcLp2xI9D08KLpuh+PvxtURTkD9rHm2/mUBFt794QUM/IFrzs0qB2CriVlSNQlgAlqIHyYvFUOYk71821ilcz2SoH88ZWUVSJUZMBBf5W/2ohypooEDGiqGsDgcdJ5puJrYm4/DGTo4g== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=gXYcR02vd8uBINo2UjfOuHSZKWLAHzc/lCPNshtaUHM=; b=IoCsukYE4hgzzsFZgugK86ohEU+AC0w/UfsdVlHubuTHSu1qV5U2A5++sW0DziHCHzeMCDG6vVSu4dMHHnkwRsDrONDz9IbSzMY4gQQGKNU4wEpvOrFZR0cvqk3Z9P/8vEbzSfOTSAekmThNJMHO9R1DArhHCJTbMWGHoTkMKgw= Received: from DM6PR11MB3308.namprd11.prod.outlook.com (2603:10b6:5:d::22) by DM6PR11MB4380.namprd11.prod.outlook.com (2603:10b6:5:14e::20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2979.30; Mon, 11 May 2020 12:10:32 +0000 Received: from DM6PR11MB3308.namprd11.prod.outlook.com ([fe80::c918:3387:da92:c700]) by DM6PR11MB3308.namprd11.prod.outlook.com ([fe80::c918:3387:da92:c700%7]) with mapi id 15.20.2979.033; Mon, 11 May 2020 12:10:32 +0000 From: "Ananyev, Konstantin" To: Pavan Nikhilesh Bhagavatula , "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: AQHWHlDpPeIPCcs0i0ui77Z53FWYRaieMNWwgAPOQICAALYOIIAACp6AgAAAlhCAAAjagIAAEZdw Date: Mon, 11 May 2020 12:10:32 +0000 Message-ID: References: <20200429180515.5704-1-pbhagavatula@marvell.com> In-Reply-To: Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.2.0.6 authentication-results: marvell.com; dkim=none (message not signed) header.d=none;marvell.com; dmarc=none action=none header.from=intel.com; x-originating-ip: [192.198.151.178] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 293a3cb5-d828-4eb1-7f29-08d7f5a44de3 x-ms-traffictypediagnostic: DM6PR11MB4380: x-ld-processed: 46c98d88-e344-4ed4-8496-4ed7712e255d,ExtAddr 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: RitcfIe19xqDjz/1e+VX4dX24puRHwUioawKLxbIX0uMORtC35j2tJeunxK7QSLBlMh1Qw14FDLX6B7xuRJ2PCXhK/MCFpsCD0HLabBLqTbEC2dyPFZlxi0t34NnrbMkcExDMr79mXG1vcKEsaB5Db6nofLR/bthqJCptulbUse8qLERyERX4Ho/qWWy21FWbWgz1d4yEoJlzliuXZsgrZtA9dVfpCFyfXAulsXlppc3Ryp1oxCrMZ718MCuALrw5bLw8Ke9RMfpbT0cnWzmNlQzaw5w4kusWw7x3THHTNlqqpSL93Fmm8UgA8SOL6nb36XfhtjLzvPRV1rkevFk2PR1veyUecpa6Iv0yLTPYJtbqblXkSkIFghdNk7gnRFa3fOHLtx2bIjm8Dksy82kqE2tb1th5m45yQTrC4qYh7XadSnb88cot1ACkKneBXf7I3oKJZaCzN1ApraKhz+0B9+yFF+amsIbuBS6MqtYudmSsuwx45YiAAZeWG60k3f295sm5wvVyZ7zsXz5ZC/w5U/iLniaUM3Ktz8ZqhcZF9A= x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:DM6PR11MB3308.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFTY:; SFS:(346002)(39860400002)(136003)(376002)(366004)(396003)(33430700001)(478600001)(86362001)(6506007)(66476007)(110136005)(316002)(8676002)(76116006)(8936002)(52536014)(66446008)(66556008)(2906002)(66946007)(55016002)(33656002)(64756008)(9686003)(186003)(26005)(7696005)(5660300002)(4326008)(33440700001)(71200400001)(921003); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata: NGBEHD5C3DfeoRMeB4Lw3M75JkIj/X/a4WyYzizRYmSyYrj1xpGEvjoieRAumb2JAx6gs0fLs1DlRia7owihDQW4ZeSb0gIHrWUhjbpeomdPdq62ks6ZupMb8RQnWMBUfj5wBGcit5ZkTEJ8WBn1togGoystmPcQkvcD+REEklfaYStNIlRIW+mIGR7xUE84tHO1DxwKsXWrWRdS8aNztYjBiO6G5JKxL2rQTHK7cvXPCq2nZG82WacshJ0CkovNJ2PEokQOU/TO8/pzeBO2Q6pEXMar3l26dBZwtYk1lJm5WKdtNzDNCkiSzAl1BpVdzrzynlHHyxm9xgiDW5xq1rKzSqZssZcLOPSM3Kc05/0hLC6QZEVePQM7TjfwF/+PFL9h2ieii4xgKnVc1GBebmqpPwEZEKuNNrxHW8CWj5gd86mp2DJljh1XIdLGdnnKjx8DAU0uF7gQ3RonKxtsJ9B3Z+TAczsdGAJCRCMalJusmPvIbmhqXSQsh3jD9BAL Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: 293a3cb5-d828-4eb1-7f29-08d7f5a44de3 X-MS-Exchange-CrossTenant-originalarrivaltime: 11 May 2020 12:10:32.1683 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: jbTZdQbpgnJqMe3gKIVDl5ake2H4TXUiN8s29VFHfLsq/HJz+wSpLqcTYKqeUWbCvRFgvuNcnOpjiqj3ok7bKCATfgHFBjcbesiYO5IgliQ= X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4380 X-OriginatorOrg: intel.com 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" >=20 > >> >> >> 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 t= he > >lib > >> >or > >> >> >> drivers layers. All the libs and drivers use `rte_hash_crc.h` wh= ich > >> >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). >=20 > Yes but I think merging them would be a cleaner, number of constructors w= ould be > one and maybe we could select the best available algorithm on a given pla= tform 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_.= ... >=20 > As Yipeng mentioned do you thing having a indirect call instead of runtim= e branch be > depreciative in terms of performance? I think run-time branch by some global var would be much faster than indire= ct function call (at least on IA). >=20 > > > >> > >> 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. > >> > >>