From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi0-f53.google.com (mail-oi0-f53.google.com [209.85.218.53]) by dpdk.org (Postfix) with ESMTP id 5E6D3A6A for ; Wed, 23 Dec 2015 12:37:45 +0100 (CET) Received: by mail-oi0-f53.google.com with SMTP id l9so93641338oia.2 for ; Wed, 23 Dec 2015 03:37:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=zTwVHJ3ACCgSZl75aeLy6TyayWxJNJZa2xjJu7dQurg=; b=tLepLn2wmLgNLpr2g48FIb/phuRpO3iuxW3ETwfQSuvR2WH3BsrTYKXzasOPc/K/si snnc7dXbtG2/eDpLyXCQ5cVgbBi196ytIIHuzmSn0g6kD9RfEQh0o4Mxycfq1LpXpAEl LG+n8x0p++wc1yO0xHEBu+XcyXA8HKKzJVg8GU3M/JSPRicqQTasg0IJqK0MdDLWpMfd Tdfq352RqTuIZRe1kw4FkENghPPf/9avXgU+/+n6q+aitOToAs9dXB7kwwtDA6sZY3iA EN+e2O1cFsrVm/3jDeEDmCRylsgXs9r1j1YBNeGWybS9OIy1gyPfQ5dmPwCW3NOMOeSK whsg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=zTwVHJ3ACCgSZl75aeLy6TyayWxJNJZa2xjJu7dQurg=; b=awrz2D6LSds6rPrhhd/dGxX2h3cViWqK+AfFzgRYpOIUrni+BtsRIlfBLWZaGgBPjr XuxmaHYsVhpQDf3m1MF8Wz8vFfPw9GyXnd9hnlJjhjPPe0iT3vqZZ8KsnX1IwwDCyMBA bExhGbODepI5q0ObUqjdWwyqSa/iZTLyKB9Prxxp5EC7MZ3BXU2jEF43JqUJDWCw3hLD jk0kWk++Sck20tcXUH+ZqY9EpTrQ4VF3XDlC18bwTYTd8b0o5+oyIff4GLgX6Nm3tuzy WCwiwtueTh+lMAhStne2IM4+SH0Zd2MqP9eONFXh4FQtNSLrEsW6ISOD3+N6QIfv6rYA e4Ow== X-Gm-Message-State: ALoCoQkXq9aEK1M6CJF4PeNyCzk+dx06VD3WiDx5U/RixAUGIkbto9Gyu6f/R7F8/6nK8rNFfewMdTVUKH+lb70CcX2aDL9JehtX8fnCDqpOnQmBJP3MUSQ= MIME-Version: 1.0 X-Received: by 10.202.98.84 with SMTP id w81mr13103762oib.3.1450870664088; Wed, 23 Dec 2015 03:37:44 -0800 (PST) Received: by 10.60.38.132 with HTTP; Wed, 23 Dec 2015 03:37:43 -0800 (PST) Received: by 10.60.38.132 with HTTP; Wed, 23 Dec 2015 03:37:43 -0800 (PST) In-Reply-To: <533710CFB86FA344BFBF2D6802E6028622EFE9FB@SHSMSX101.ccr.corp.intel.com> References: <1450776898-8951-1-git-send-email-didier.pallard@6wind.com> <533710CFB86FA344BFBF2D6802E6028622EFE9FB@SHSMSX101.ccr.corp.intel.com> Date: Wed, 23 Dec 2015 12:37:43 +0100 Message-ID: From: Vincent JARDIN To: "Qiu, Michael" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] hash: fix CRC32c computation 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, 23 Dec 2015 11:37:45 -0000 Le 23 d=C3=A9c. 2015 10:12, "Qiu, Michael" a =C3=A9= crit : > > Is it suitable to put so many code in commit log? It is more explicit than a text/comment. I do not think it should be maintained code. > > Thanks, > Michael > On 12/22/2015 5:36 PM, Didier Pallard wrote: > > As demonstrated by the following code, CRC32c computation is not valid > > when buffer length is not a multiple of 4 bytes: > > (Output obtained by code below) > > > > CRC of 1 NULL bytes expected: 0x527d5351 > > soft: 527d5351 > > rte accelerated: 48674bc7 > > rte soft: 48674bc7 > > CRC of 2 NULL bytes expected: 0xf16177d2 > > soft: f16177d2 > > rte accelerated: 48674bc7 > > rte soft: 48674bc7 > > CRC of 2x1 NULL bytes expected: 0xf16177d2 > > soft: f16177d2 > > rte accelerated: 8c28b28a > > rte soft: 8c28b28a > > CRC of 3 NULL bytes expected: 0x6064a37a > > soft: 6064a37a > > rte accelerated: 48674bc7 > > rte soft: 48674bc7 > > CRC of 4 NULL bytes expected: 0x48674bc7 > > soft: 48674bc7 > > rte accelerated: 48674bc7 > > rte soft: 48674bc7 > > > > Values returned by rte_hash_crc functions does not match the one > > computed by a trivial crc32c implementation. > > > > ARM code is a guess, it is not tested, neither compiled. > > > > code showing the problem: > > > > uint8_t null_test[32] =3D {0}; > > > > static uint32_t crc32c_trivial(uint8_t *buffer, uint32_t length, uint32_t crc) > > { > > uint32_t i, j; > > for (i =3D 0; i < length; ++i) > > { > > crc =3D crc ^ buffer[i]; > > for (j =3D 0; j < 8; j++) > > crc =3D (crc >> 1) ^ 0x80000000 ^ ((~crc & 1) * 0x82f63b78)= ; > > } > > return crc; > > } > > > > void hash_test(void); > > void hash_test(void) > > { > > printf("CRC of 1 nul byte expected: 0x527d5351\n"); > > printf(" soft: %08x\n", crc32c_trivial(null_test, 1, 0)); > > rte_hash_crc_init_alg(); > > printf(" rte accelerated: %08x\n", ~rte_hash_crc(null_test, 1, 0xFFFFFFFF)); > > rte_hash_crc_set_alg(CRC32_SW); > > printf(" rte soft: %08x\n", ~rte_hash_crc(null_test, 1, 0xFFFFFFFF)); > > > > printf("CRC of 2 nul bytes expected: 0xf16177d2\n"); > > printf(" soft: %08x\n", crc32c_trivial(null_test, 2, 0)); > > rte_hash_crc_init_alg(); > > printf(" rte accelerated: %08x\n", ~rte_hash_crc(null_test, 2, 0xFFFFFFFF)); > > rte_hash_crc_set_alg(CRC32_SW); > > printf(" rte soft: %08x\n", ~rte_hash_crc(null_test, 2, 0xFFFFFFFF)); > > > > printf("CRC of 2x1 nul bytes expected: 0xf16177d2\n"); > > printf(" soft: %08x\n", crc32c_trivial(null_test, 1, crc32c_trivial(null_test, 1, 0))); > > rte_hash_crc_init_alg(); > > printf(" rte accelerated: %08x\n", ~rte_hash_crc(null_test, 1, rte_hash_crc(null_test, 1, 0xFFFFFFFF))); > > rte_hash_crc_set_alg(CRC32_SW); > > printf(" rte soft: %08x\n", ~rte_hash_crc(null_test, 1, rte_hash_crc(null_test, 1, 0xFFFFFFFF))); > > > > printf("CRC of 3 nul bytes expected: 0x6064a37a\n"); > > printf(" soft: %08x\n", crc32c_trivial(null_test, 3, 0)); > > rte_hash_crc_init_alg(); > > printf(" rte accelerated: %08x\n", ~rte_hash_crc(null_test, 3, 0xFFFFFFFF)); > > rte_hash_crc_set_alg(CRC32_SW); > > printf(" rte soft: %08x\n", ~rte_hash_crc(null_test, 3, 0xFFFFFFFF)); > > > > printf("CRC of 4 nul bytes expected: 0x48674bc7\n"); > > printf(" soft: %08x\n", crc32c_trivial(null_test, 4, 0)); > > rte_hash_crc_init_alg(); > > printf(" rte accelerated: %08x\n", ~rte_hash_crc(null_test, 4, 0xFFFFFFFF)); > > rte_hash_crc_set_alg(CRC32_SW); > > printf(" rte soft: %08x\n", ~rte_hash_crc(null_test, 4, 0xFFFFFFFF)); > > } > > > > Signed-off-by: Didier Pallard > > Acked-by: David Marchand > > --- > > lib/librte_hash/rte_crc_arm64.h | 64 ++++++++++++++++++++ > > lib/librte_hash/rte_hash_crc.h | 125 +++++++++++++++++++++++++++++++--------- > > 2 files changed, 162 insertions(+), 27 deletions(-) > > > > diff --git a/lib/librte_hash/rte_crc_arm64.h b/lib/librte_hash/rte_crc_arm64.h > > index 02e26bc..44ef460 100644 > > --- a/lib/librte_hash/rte_crc_arm64.h > > +++ b/lib/librte_hash/rte_crc_arm64.h > > @@ -50,6 +50,28 @@ extern "C" { > > #include > > > > static inline uint32_t > > +crc32c_arm64_u8(uint8_t data, uint32_t init_val) > > +{ > > + asm(".arch armv8-a+crc"); > > + __asm__ volatile( > > + "crc32cb %w[crc], %w[crc], %b[value]" > > + : [crc] "+r" (init_val) > > + : [value] "r" (data)); > > + return init_val; > > +} > > + > > +static inline uint32_t > > +crc32c_arm64_u16(uint16_t data, uint32_t init_val) > > +{ > > + asm(".arch armv8-a+crc"); > > + __asm__ volatile( > > + "crc32ch %w[crc], %w[crc], %h[value]" > > + : [crc] "+r" (init_val) > > + : [value] "r" (data)); > > + return init_val; > > +} > > + > > +static inline uint32_t > > crc32c_arm64_u32(uint32_t data, uint32_t init_val) > > { > > asm(".arch armv8-a+crc"); > > @@ -103,6 +125,48 @@ rte_hash_crc_init_alg(void) > > } > > > > /** > > + * Use single crc32 instruction to perform a hash on a 1 byte value. > > + * Fall back to software crc32 implementation in case arm64 crc intrinsics is > > + * not supported > > + * > > + * @param data > > + * Data to perform hash on. > > + * @param init_val > > + * Value to initialise hash generator. > > + * @return > > + * 32bit calculated hash value. > > + */ > > +static inline uint32_t > > +rte_hash_crc_1byte(uint8_t data, uint32_t init_val) > > +{ > > + if (likely(crc32_alg & CRC32_ARM64)) > > + return crc32c_arm64_u8(data, init_val); > > + > > + return crc32c_1byte(data, init_val); > > +} > > + > > +/** > > + * Use single crc32 instruction to perform a hash on a 2 bytes value. > > + * Fall back to software crc32 implementation in case arm64 crc intrinsics is > > + * not supported > > + * > > + * @param data > > + * Data to perform hash on. > > + * @param init_val > > + * Value to initialise hash generator. > > + * @return > > + * 32bit calculated hash value. > > + */ > > +static inline uint32_t > > +rte_hash_crc_2byte(uint16_t data, uint32_t init_val) > > +{ > > + if (likely(crc32_alg & CRC32_ARM64)) > > + return crc32c_arm64_u16(data, init_val); > > + > > + return crc32c_2bytes(data, init_val); > > +} > > + > > +/** > > * Use single crc32 instruction to perform a hash on a 4 byte value. > > * Fall back to software crc32 implementation in case arm64 crc intrinsics is > > * not supported > > diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h > > index 78a34b7..485f8a2 100644 > > --- a/lib/librte_hash/rte_hash_crc.h > > +++ b/lib/librte_hash/rte_hash_crc.h > > @@ -328,6 +328,28 @@ static const uint32_t crc32c_tables[8][256] =3D {{ > > crc32c_tables[(n)-1][((crc) >> 8) & 0xFF]) > > > > static inline uint32_t > > +crc32c_1byte(uint8_t data, uint32_t init_val) > > +{ > > + uint32_t crc; > > + crc =3D init_val; > > + crc ^=3D data; > > + > > + return crc32c_tables[0][crc & 0xff] ^ (crc >> 8); > > +} > > + > > +static inline uint32_t > > +crc32c_2bytes(uint16_t data, uint32_t init_val) > > +{ > > + uint32_t crc; > > + crc =3D init_val; > > + crc ^=3D data; > > + > > + crc =3D CRC32_UPD(crc, 1) ^ (crc >> 16); > > + > > + return crc; > > +} > > + > > +static inline uint32_t > > crc32c_1word(uint32_t data, uint32_t init_val) > > { > > uint32_t crc, term1, term2; > > @@ -367,6 +389,26 @@ crc32c_2words(uint64_t data, uint32_t init_val) > > > > #if defined(RTE_ARCH_I686) || defined(RTE_ARCH_X86_64) > > static inline uint32_t > > +crc32c_sse42_u8(uint8_t data, uint32_t init_val) > > +{ > > + __asm__ volatile( > > + "crc32b %[data], %[init_val];" > > + : [init_val] "+r" (init_val) > > + : [data] "rm" (data)); > > + return init_val; > > +} > > + > > +static inline uint32_t > > +crc32c_sse42_u16(uint16_t data, uint32_t init_val) > > +{ > > + __asm__ volatile( > > + "crc32w %[data], %[init_val];" > > + : [init_val] "+r" (init_val) > > + : [data] "rm" (data)); > > + return init_val; > > +} > > + > > +static inline uint32_t > > crc32c_sse42_u32(uint32_t data, uint32_t init_val) > > { > > __asm__ volatile( > > @@ -453,6 +495,52 @@ rte_hash_crc_init_alg(void) > > } > > > > /** > > + * Use single crc32 instruction to perform a hash on a byte value. > > + * Fall back to software crc32 implementation in case SSE4.2 is > > + * not supported > > + * > > + * @param data > > + * Data to perform hash on. > > + * @param init_val > > + * Value to initialise hash generator. > > + * @return > > + * 32bit calculated hash value. > > + */ > > +static inline uint32_t > > +rte_hash_crc_1byte(uint8_t data, uint32_t init_val) > > +{ > > +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64 > > + if (likely(crc32_alg & CRC32_SSE42)) > > + return crc32c_sse42_u8(data, init_val); > > +#endif > > + > > + return crc32c_1byte(data, init_val); > > +} > > + > > +/** > > + * Use single crc32 instruction to perform a hash on a byte value. > > + * Fall back to software crc32 implementation in case SSE4.2 is > > + * not supported > > + * > > + * @param data > > + * Data to perform hash on. > > + * @param init_val > > + * Value to initialise hash generator. > > + * @return > > + * 32bit calculated hash value. > > + */ > > +static inline uint32_t > > +rte_hash_crc_2byte(uint16_t data, uint32_t init_val) > > +{ > > +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64 > > + if (likely(crc32_alg & CRC32_SSE42)) > > + return crc32c_sse42_u16(data, init_val); > > +#endif > > + > > + return crc32c_2bytes(data, init_val); > > +} > > + > > +/** > > * Use single crc32 instruction to perform a hash on a 4 byte value. > > * Fall back to software crc32 implementation in case SSE4.2 is > > * not supported > > @@ -521,7 +609,6 @@ static inline uint32_t > > rte_hash_crc(const void *data, uint32_t data_len, uint32_t init_val) > > { > > unsigned i; > > - uint64_t temp =3D 0; > > uintptr_t pd =3D (uintptr_t) data; > > > > for (i =3D 0; i < data_len / 8; i++) { > > @@ -529,35 +616,19 @@ rte_hash_crc(const void *data, uint32_t data_len, uint32_t init_val) > > pd +=3D 8; > > } > > > > - switch (7 - (data_len & 0x07)) { > > - case 0: > > - temp |=3D (uint64_t) *((const uint8_t *)pd + 6) << 48; > > - /* Fallthrough */ > > - case 1: > > - temp |=3D (uint64_t) *((const uint8_t *)pd + 5) << 40; > > - /* Fallthrough */ > > - case 2: > > - temp |=3D (uint64_t) *((const uint8_t *)pd + 4) << 32; > > - temp |=3D *(const uint32_t *)pd; > > - init_val =3D rte_hash_crc_8byte(temp, init_val); > > - break; > > - case 3: > > + if (data_len & 0x4) { > > init_val =3D rte_hash_crc_4byte(*(const uint32_t *)pd, init_val); > > - break; > > - case 4: > > - temp |=3D *((const uint8_t *)pd + 2) << 16; > > - /* Fallthrough */ > > - case 5: > > - temp |=3D *((const uint8_t *)pd + 1) << 8; > > - /* Fallthrough */ > > - case 6: > > - temp |=3D *(const uint8_t *)pd; > > - init_val =3D rte_hash_crc_4byte(temp, init_val); > > - /* Fallthrough */ > > - default: > > - break; > > + pd +=3D 4; > > + } > > + > > + if (data_len & 0x2) { > > + init_val =3D rte_hash_crc_2byte(*(const uint16_t *)pd, init_val); > > + pd +=3D 2; > > } > > > > + if (data_len & 0x1) > > + init_val =3D rte_hash_crc_1byte(*(const uint8_t *)pd, init_val); > > + > > return init_val; > > } > > >