From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 6150E1288 for ; Mon, 11 May 2015 21:35:35 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga102.fm.intel.com with ESMTP; 11 May 2015 12:35:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,409,1427785200"; d="scan'208";a="492226448" Received: from irsmsx151.ger.corp.intel.com ([163.33.192.59]) by FMSMGA003.fm.intel.com with ESMTP; 11 May 2015 12:35:22 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.73]) by IRSMSX151.ger.corp.intel.com ([169.254.4.102]) with mapi id 14.03.0224.002; Mon, 11 May 2015 20:35:20 +0100 From: "Ananyev, Konstantin" To: "Ravi Kerur (rkerur@gmail.com)" Thread-Topic: [dpdk-dev] [PATCH v2] Implement memcmp using AVX/SSE instructions. Thread-Index: AQHQjBHuQIC0CvBLZEiNp/yP/VFCLp13FljAgAAAK+A= Date: Mon, 11 May 2015 19:35:20 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772582142E475@irsmsx105.ger.corp.intel.com> References: <1431119946-32078-1-git-send-email-rkerur@gmail.com> <1431119989-32124-1-git-send-email-rkerur@gmail.com> <2601191342CEEE43887BDE71AB9772582142E106@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772582142E44A@irsmsx105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB9772582142E44A@irsmsx105.ger.corp.intel.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v2] Implement memcmp using AVX/SSE instructions. 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: Mon, 11 May 2015 19:35:36 -0000 Hi Ravi, >=20 > From: Ravi Kerur [mailto:rkerur@gmail.com] > Sent: Monday, May 11, 2015 6:43 PM > To: Ananyev, Konstantin > Cc: Matt Laswell; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2] Implement memcmp using AVX/SSE instruc= tions. >=20 > Hi Konstantin, >=20 >=20 > On Mon, May 11, 2015 at 2:51 AM, Ananyev, Konstantin wrote: > Hi Ravi, >=20 > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ravi Kerur > > Sent: Friday, May 08, 2015 11:55 PM > > To: Matt Laswell > > Cc: dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH v2] Implement memcmp using AVX/SSE instr= uctions. > > > > On Fri, May 8, 2015 at 3:29 PM, Matt Laswell w= rote: > > > > > > > > > > > On Fri, May 8, 2015 at 4:19 PM, Ravi Kerur wrote: > > > > > >> This patch replaces memcmp in librte_hash with rte_memcmp which is > > >> implemented with AVX/SSE instructions. > > >> > > >> +static inline int > > >> +rte_memcmp(const void *_src_1, const void *_src_2, size_t n) > > >> +{ > > >> +=A0 =A0 =A0 =A0const uint8_t *src_1 =3D (const uint8_t *)_src_1; > > >> +=A0 =A0 =A0 =A0const uint8_t *src_2 =3D (const uint8_t *)_src_2; > > >> +=A0 =A0 =A0 =A0int ret =3D 0; > > >> + > > >> +=A0 =A0 =A0 =A0if (n & 0x80) > > >> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return rte_cmp128(src_1, src_2); > > >> + > > >> +=A0 =A0 =A0 =A0if (n & 0x40) > > >> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return rte_cmp64(src_1, src_2); > > >> + > > >> +=A0 =A0 =A0 =A0if (n & 0x20) { > > >> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D rte_cmp32(src_1, src_2); > > >> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0n -=3D 0x20; > > >> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0src_1 +=3D 0x20; > > >> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0src_2 +=3D 0x20; > > >> +=A0 =A0 =A0 =A0} > > >> > > >> > > > Pardon me for butting in, but this seems incorrect for the first two = cases > > > listed above, as the function as written will only compare the first = 128 or > > > 64 bytes of each source and return the result.=A0 The pattern express= ed in > > > the 32 byte case appears more correct, as it compares the first 32 by= tes > > > and then lets later pieces of the function handle the smaller remaini= ng > > > bits of the sources. Also, if this function is to handle arbitrarily = large > > > source data, the 128 byte case needs to be in a loop. > > > > > > What am I missing? > > > > > > > Current max hash key length supported is 64 bytes, hence no comparison = is > > done after 64 bytes. 128 bytes comparison is added to measure performan= ce > > only and there is no use-case as of now. With the current use-cases its= not > > required but if there is a need to handle large arbitrary data upto 128 > > bytes it can be modified. > So on x86 let say rte_memcmp(k1, k2, 65) might produce invalid results, r= ight? > While on PPC will work as expected (as it calls memcpu underneath)? > That looks really weird to me. > If you plan to use rte_memcmp only for hash comparisons, then probably > you should put it somewhere into librte_hash and name it accordingly: rte= _hash_key_cmp() or something. > And put a big comment around it, that it only works with particular lengt= hs. > If you want it to be a generic function inside EAL, then it probably need= to handle different lengths properly > on all supported architectures. > Konstantin >=20 >=20 > Let me just explain it here and probably add it to document as well. >=20 > rte_memcmp is not >=20 > 1. a replacement to memcmp >=20 > 2. =A0restricted to hash key comparison >=20 > rte_memcmp is >=20 > 1. optimized comparison for 16 to 128 bytes, v1 patch series had this sup= port. Changed some of the logic in v2 due to concerns raised > for unavailable use-cases beyond 64 bytes comparison. >>From what I see in v2 it supposed to work correctly for len in [0,64] and = len=3D128, right? Not sure I get it: so for v1 it was able to handle any length correctly, bu= t then you removed it? If so, I wonder what was the reason? Make it faster? Another thing that looks strange to me: While all rte_cmp*() uses actual data values for comparison results, rte_memcmp_remainder() return value depends not only on data values but als= o on data locations: +static inline int +rte_memcmp_remainder(const uint8_t *src_1u, const uint8_t *src_2u, size_t = n) +{ ... exit: + + return src_1u < src_2u ? -1 : 1; +} If you just test for equal/not equal that doesn't really matter. If this is supposed to be a 'proper' comparison function, then the result i= s sort of unpredictable. > With minor tuning over the weekend I am able to get better performance fo= r > anything between 16 to 128 bytes comparison. >=20 > 2. will be specific to DPDK =A0i.e. currently all memcmp usage in DPDK ar= e for equality or inequality hence "less than" or "greater than" > implementation in rte_memcmp doesn't make sense and will be removed in su= bsequent patches, it will return 0 or 1 for > equal/unequal cases. If you don't plan your function to follow memcmp() semantics and syntax, wh= y to name it rte_memcmp()? I think that will make a lot of confusion around. Why not to name it differently(and put a clear comment in the declaration o= f course)? >=20 > rte_hash will be the first candidate to move to rte_memcmp and subsequent= ly rte_lpm6 which uses 16 bytes comparison will be > moved >=20 > Later on RING_SIZE which uses large size for comparison will be moved. I = am currently studying/understanding that logic and will make > changes to rte_memcmp to support that. Sorry, didn't get you here. Konstantin >=20 > I don't want to make lot of changes in one shot and see that patch series= die a slow death with no takers. >=20 > Thanks, > Ravi >=20 > > > > > > > > -- > > > Matt Laswell > > > infinite io, inc. > > > laswell@infiniteio.com > > > > > >