From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id A83D512A8 for ; Wed, 6 May 2015 18:11:26 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga103.jf.intel.com with ESMTP; 06 May 2015 09:11:25 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,380,1427785200"; d="scan'208";a="724721902" Received: from irsmsx103.ger.corp.intel.com ([163.33.3.157]) by orsmga002.jf.intel.com with ESMTP; 06 May 2015 09:11:26 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.178]) by IRSMSX103.ger.corp.intel.com ([169.254.3.76]) with mapi id 14.03.0224.002; Wed, 6 May 2015 17:11:24 +0100 From: "Ananyev, Konstantin" To: "De Lara Guarch, Pablo" , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v3 3/6] hash: update jhash function with the latest available Thread-Index: AQHQh0Ifa916QN+t0kuol3MAxqDHRp1uDeowgACTEICAAGVfwA== Date: Wed, 6 May 2015 16:11:23 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772582142512E@irsmsx105.ger.corp.intel.com> References: <1429874587-17939-1-git-send-email-pablo.de.lara.guarch@intel.com> <1430837034-21031-1-git-send-email-pablo.de.lara.guarch@intel.com> <1430837034-21031-4-git-send-email-pablo.de.lara.guarch@intel.com> <2601191342CEEE43887BDE71AB97725821424ED1@irsmsx105.ger.corp.intel.com> In-Reply-To: Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3 3/6] hash: update jhash function with the latest available X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 May 2015 16:11:27 -0000 Hi Pablo, > -----Original Message----- > From: De Lara Guarch, Pablo > Sent: Wednesday, May 06, 2015 10:36 AM > To: Ananyev, Konstantin; dev@dpdk.org > Subject: RE: [dpdk-dev] [PATCH v3 3/6] hash: update jhash function with t= he latest available >=20 > Hi Konstantin, >=20 > > -----Original Message----- > > From: Ananyev, Konstantin > > Sent: Wednesday, May 06, 2015 1:36 AM > > To: De Lara Guarch, Pablo; dev@dpdk.org > > Subject: RE: [dpdk-dev] [PATCH v3 3/6] hash: update jhash function with= the > > latest available > > > > > > Hi Pablo, > > > > > -----Original Message----- > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Pablo de Lara > > > Sent: Tuesday, May 05, 2015 3:44 PM > > > To: dev@dpdk.org > > > Subject: [dpdk-dev] [PATCH v3 3/6] hash: update jhash function with t= he > > latest available > > > > > > Jenkins hash function was developed originally in 1996, > > > and was integrated in first versions of DPDK. > > > The function has been improved in 2006, > > > achieving up to 60% better performance, compared to the original one. > > > > > > This patch integrates that code into the rte_jhash library. > > > > > > Signed-off-by: Pablo de Lara > > > --- > > > lib/librte_hash/rte_jhash.h | 261 > > +++++++++++++++++++++++++++++++------------ > > > 1 files changed, 188 insertions(+), 73 deletions(-) > > > > > > diff --git a/lib/librte_hash/rte_jhash.h b/lib/librte_hash/rte_jhash.= h > > > index a4bf5a1..0e96b7c 100644 > > > --- a/lib/librte_hash/rte_jhash.h > > > +++ b/lib/librte_hash/rte_jhash.h > > > @@ -1,7 +1,7 @@ > > > /*- > > > * BSD LICENSE > > > * > > > - * Copyright(c) 2010-2014 Intel Corporation. All rights reserved. > > > + * Copyright(c) 2010-2015 Intel Corporation. All rights reserved. > > > * All rights reserved. > > > * > > > * Redistribution and use in source and binary forms, with or with= out > > > @@ -45,38 +45,68 @@ extern "C" { > > > #endif > > > > > > #include > > > +#include > > > +#include > > > > > > /* jhash.h: Jenkins hash support. > > > * > > > - * Copyright (C) 1996 Bob Jenkins (bob_jenkins@burtleburtle.net) > > > + * Copyright (C) 2006 Bob Jenkins (bob_jenkins@burtleburtle.net) > > > * > > > * http://burtleburtle.net/bob/hash/ > > > * > > > * These are the credits from Bob's sources: > > > * > > > - * lookup2.c, by Bob Jenkins, December 1996, Public Domain. > > > - * hash(), hash2(), hash3, and mix() are externally useful functions= . > > > - * Routines to test the hash are included if SELF_TEST is defined. > > > - * You can use this free for any purpose. It has no warranty. > > > + * lookup3.c, by Bob Jenkins, May 2006, Public Domain. > > > + * > > > + * These are functions for producing 32-bit hashes for hash table lo= okup. > > > + * hashword(), hashlittle(), hashlittle2(), hashbig(), mix(), and fi= nal() > > > + * are externally useful functions. Routines to test the hash are i= ncluded > > > + * if SELF_TEST is defined. You can use this free for any purpose. = It's in > > > + * the public domain. It has no warranty. > > > * > > > * $FreeBSD$ > > > */ > > > > > > +#define rot(x, k) (((x) << (k)) | ((x) >> (32-(k)))) > > > + > > > /** @internal Internal function. NOTE: Arguments are modified. */ > > > #define __rte_jhash_mix(a, b, c) do { \ > > > - a -=3D b; a -=3D c; a ^=3D (c>>13); \ > > > - b -=3D c; b -=3D a; b ^=3D (a<<8); \ > > > - c -=3D a; c -=3D b; c ^=3D (b>>13); \ > > > - a -=3D b; a -=3D c; a ^=3D (c>>12); \ > > > - b -=3D c; b -=3D a; b ^=3D (a<<16); \ > > > - c -=3D a; c -=3D b; c ^=3D (b>>5); \ > > > - a -=3D b; a -=3D c; a ^=3D (c>>3); \ > > > - b -=3D c; b -=3D a; b ^=3D (a<<10); \ > > > - c -=3D a; c -=3D b; c ^=3D (b>>15); \ > > > + a -=3D c; a ^=3D rot(c, 4); c +=3D b; \ > > > + b -=3D a; b ^=3D rot(a, 6); a +=3D c; \ > > > + c -=3D b; c ^=3D rot(b, 8); b +=3D a; \ > > > + a -=3D c; a ^=3D rot(c, 16); c +=3D b; \ > > > + b -=3D a; b ^=3D rot(a, 19); a +=3D c; \ > > > + c -=3D b; c ^=3D rot(b, 4); b +=3D a; \ > > > +} while (0) > > > + > > > +#define __rte_jhash_final(a, b, c) do { \ > > > + c ^=3D b; c -=3D rot(b, 14); \ > > > + a ^=3D c; a -=3D rot(c, 11); \ > > > + b ^=3D a; b -=3D rot(a, 25); \ > > > + c ^=3D b; c -=3D rot(b, 16); \ > > > + a ^=3D c; a -=3D rot(c, 4); \ > > > + b ^=3D a; b -=3D rot(a, 14); \ > > > + c ^=3D b; c -=3D rot(b, 24); \ > > > } while (0) > > > > > > /** The golden ratio: an arbitrary value. */ > > > -#define RTE_JHASH_GOLDEN_RATIO 0x9e3779b9 > > > +#define RTE_JHASH_GOLDEN_RATIO 0xdeadbeef > > > + > > > +#if RTE_BYTE_ORDER =3D=3D RTE_LITTLE_ENDIAN > > > +#define RTE_JHASH_BYTE0_SHIFT 0 > > > +#define RTE_JHASH_BYTE1_SHIFT 8 > > > +#define RTE_JHASH_BYTE2_SHIFT 16 > > > +#define RTE_JHASH_BYTE3_SHIFT 24 > > > +#else > > > +#define RTE_JHASH_BYTE0_SHIFT 24 > > > +#define RTE_JHASH_BYTE1_SHIFT 16 > > > +#define RTE_JHASH_BYTE2_SHIFT 8 > > > +#define RTE_JHASH_BYTE3_SHIFT 0 > > > +#endif > > > + > > > +#define LOWER8b_MASK rte_le_to_cpu_32(0xff) > > > +#define LOWER16b_MASK rte_le_to_cpu_32(0xffff) > > > +#define LOWER24b_MASK rte_le_to_cpu_32(0xffffff) > > > > > > /** > > > * The most generic version, hashes an arbitrary sequence > > > @@ -95,42 +125,119 @@ extern "C" { > > > static inline uint32_t > > > rte_jhash(const void *key, uint32_t length, uint32_t initval) > > > { > > > - uint32_t a, b, c, len; > > > - const uint8_t *k =3D (const uint8_t *)key; > > > - const uint32_t *k32 =3D (const uint32_t *)key; > > > + uint32_t a, b, c; > > > + union { > > > + const void *ptr; > > > + size_t i; > > > + } u; > > > > > > - len =3D length; > > > - a =3D b =3D RTE_JHASH_GOLDEN_RATIO; > > > - c =3D initval; > > > + /* Set up the internal state */ > > > + a =3D b =3D c =3D RTE_JHASH_GOLDEN_RATIO + ((uint32_t)length) + ini= tval; > > > > > > - while (len >=3D 12) { > > > - a +=3D k32[0]; > > > - b +=3D k32[1]; > > > - c +=3D k32[2]; > > > + u.ptr =3D key; > > > > > > - __rte_jhash_mix(a,b,c); > > > + /* Check key alignment. For x86 architecture, first case is always > > optimal */ > > > + if (!strcmp(RTE_ARCH,"x86_64") || !strcmp(RTE_ARCH,"i686") || (u.i > > & 0x3) =3D=3D 0) { > > > > Wonder why strcmp(), why not something like: 'if defined(RTE_ARCH_I686) > > || defined(RTE_ARCH_X86_64)' as in all other places? > > Another question what would be in case of RTE_ARCH=3D"x86_x32"? > > Konstantin >=20 > Functionally is the same and using this method, I can integrate all condi= tions in one line, so it takes less code. > I also checked the assembly code, and the compiler removes the check if i= t is Intel architecture, so performance remains the same. Well, yes I think most modern compilers treat strcmp() as a builtin funct= ion and are able to optimise these strcmp() calls off for that case. But we probably can't guarantee that it would always be the case for all d= ifferent compiler/libc combinations. Again, by some reason user might need to use ' -fno-builtin' flag while bui= lding his stuff. So I would use pre-processor macros here, it is more predictable. Again, that way it is consistent with other places. =20 Actually I wonder do you really need such sort of diversity for aligned/non= -aligned case? Wonder wouldn't something like that work for you: #infdef RTE_ARCH_X86 const uint32_t *k =3D (uint32_t *)((uintptr_t)key & (uintptr_t)~3); const uint32_t s =3D ((uintptr_t)key & 3) * CHAR_BIT; #else /*X86*/ const uint32_t *k =3D key; const uint32_t s =3D 0; #endif while (len > 12) { a +=3D k[0] >> s | (uint64_t)k[1] << (32 - s); b +=3D k[1] >> s | (uint64_t)k[2] << (32 - s); c +=3D k[2] >> s | (uint64_t)k[3] << (32 - s); k +=3D 3; length -=3D 12; } switch (length) { case 12: a +=3D k[0] >> s | (uint64_t)k[1] << (32 - s); b +=3D k[1] >> s | (uint64_t)k[2] << (32 - s); c +=3D k[2] >> s | (uint64_t)k[3] << (32 - s); break; case 11: a +=3D k[0] >> s | (uint64_t)k[1] << (32 - s); b +=3D k[1] >> s | (uint64_t)k[2] << (32 - s); c +=3D (k[2] >> s | (uint64_t)k[3] << (32 - s)) & & LOWER24b_MASK; break; ... case 1: a +=3D (k[0] >> s | (uint64_t)k[1] << (32 - s)) & LOWER8b_MASK; break; ... In that way, even for non-aligned you don't need do 4B reads. For x86, compiler would do it's optimisation work and strip off '>> s | (ui= nt64_t)k[..] << (32 - s);'. >=20 > Re x86_x32, you are right, probably I need to include it. Although, I jus= t realized that it is not used in any other place. > Wonder if we should include it somewhere else? E.g. rte_hash_crc.h Yep, that's true we are not doing it for hash_crc also... Would probably good to have some sort of ' RTE_ARCH_X86' - that would be de= fined for all x86 targets and use it whenever applicable. But I suppose, that's a subject for another patch.=20 Konstantin