* [dpdk-dev] [PATCH] hash: fix CRC32c computation
@ 2015-12-22 9:34 Didier Pallard
2015-12-23 9:12 ` Qiu, Michael
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Didier Pallard @ 2015-12-22 9:34 UTC (permalink / raw)
To: dev, bruce.richardson, pablo.de.lara.guarch
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] = {0};
static uint32_t crc32c_trivial(uint8_t *buffer, uint32_t length, uint32_t crc)
{
uint32_t i, j;
for (i = 0; i < length; ++i)
{
crc = crc ^ buffer[i];
for (j = 0; j < 8; j++)
crc = (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 <didier.pallard@6wind.com>
Acked-by: David Marchand <david.marchand@6wind.com>
---
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 <rte_common.h>
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] = {{
crc32c_tables[(n)-1][((crc) >> 8) & 0xFF])
static inline uint32_t
+crc32c_1byte(uint8_t data, uint32_t init_val)
+{
+ uint32_t crc;
+ crc = init_val;
+ crc ^= 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 = init_val;
+ crc ^= data;
+
+ crc = 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 = 0;
uintptr_t pd = (uintptr_t) data;
for (i = 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 += 8;
}
- switch (7 - (data_len & 0x07)) {
- case 0:
- temp |= (uint64_t) *((const uint8_t *)pd + 6) << 48;
- /* Fallthrough */
- case 1:
- temp |= (uint64_t) *((const uint8_t *)pd + 5) << 40;
- /* Fallthrough */
- case 2:
- temp |= (uint64_t) *((const uint8_t *)pd + 4) << 32;
- temp |= *(const uint32_t *)pd;
- init_val = rte_hash_crc_8byte(temp, init_val);
- break;
- case 3:
+ if (data_len & 0x4) {
init_val = rte_hash_crc_4byte(*(const uint32_t *)pd, init_val);
- break;
- case 4:
- temp |= *((const uint8_t *)pd + 2) << 16;
- /* Fallthrough */
- case 5:
- temp |= *((const uint8_t *)pd + 1) << 8;
- /* Fallthrough */
- case 6:
- temp |= *(const uint8_t *)pd;
- init_val = rte_hash_crc_4byte(temp, init_val);
- /* Fallthrough */
- default:
- break;
+ pd += 4;
+ }
+
+ if (data_len & 0x2) {
+ init_val = rte_hash_crc_2byte(*(const uint16_t *)pd, init_val);
+ pd += 2;
}
+ if (data_len & 0x1)
+ init_val = rte_hash_crc_1byte(*(const uint8_t *)pd, init_val);
+
return init_val;
}
--
2.1.4
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [dpdk-dev] [PATCH] hash: fix CRC32c computation
2015-12-22 9:34 [dpdk-dev] [PATCH] hash: fix CRC32c computation Didier Pallard
@ 2015-12-23 9:12 ` Qiu, Michael
2015-12-23 11:37 ` Vincent JARDIN
2016-02-09 9:34 ` [dpdk-dev] [PATCH v2] " Didier Pallard
2016-02-10 14:35 ` [dpdk-dev] [PATCH] hash: fix " Pattan, Reshma
2 siblings, 1 reply; 13+ messages in thread
From: Qiu, Michael @ 2015-12-23 9:12 UTC (permalink / raw)
To: Didier Pallard, dev, Richardson, Bruce, De Lara Guarch, Pablo
Is it suitable to put so many code in commit log?
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] = {0};
>
> static uint32_t crc32c_trivial(uint8_t *buffer, uint32_t length, uint32_t crc)
> {
> uint32_t i, j;
> for (i = 0; i < length; ++i)
> {
> crc = crc ^ buffer[i];
> for (j = 0; j < 8; j++)
> crc = (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 <didier.pallard@6wind.com>
> Acked-by: David Marchand <david.marchand@6wind.com>
> ---
> 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 <rte_common.h>
>
> 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] = {{
> crc32c_tables[(n)-1][((crc) >> 8) & 0xFF])
>
> static inline uint32_t
> +crc32c_1byte(uint8_t data, uint32_t init_val)
> +{
> + uint32_t crc;
> + crc = init_val;
> + crc ^= 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 = init_val;
> + crc ^= data;
> +
> + crc = 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 = 0;
> uintptr_t pd = (uintptr_t) data;
>
> for (i = 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 += 8;
> }
>
> - switch (7 - (data_len & 0x07)) {
> - case 0:
> - temp |= (uint64_t) *((const uint8_t *)pd + 6) << 48;
> - /* Fallthrough */
> - case 1:
> - temp |= (uint64_t) *((const uint8_t *)pd + 5) << 40;
> - /* Fallthrough */
> - case 2:
> - temp |= (uint64_t) *((const uint8_t *)pd + 4) << 32;
> - temp |= *(const uint32_t *)pd;
> - init_val = rte_hash_crc_8byte(temp, init_val);
> - break;
> - case 3:
> + if (data_len & 0x4) {
> init_val = rte_hash_crc_4byte(*(const uint32_t *)pd, init_val);
> - break;
> - case 4:
> - temp |= *((const uint8_t *)pd + 2) << 16;
> - /* Fallthrough */
> - case 5:
> - temp |= *((const uint8_t *)pd + 1) << 8;
> - /* Fallthrough */
> - case 6:
> - temp |= *(const uint8_t *)pd;
> - init_val = rte_hash_crc_4byte(temp, init_val);
> - /* Fallthrough */
> - default:
> - break;
> + pd += 4;
> + }
> +
> + if (data_len & 0x2) {
> + init_val = rte_hash_crc_2byte(*(const uint16_t *)pd, init_val);
> + pd += 2;
> }
>
> + if (data_len & 0x1)
> + init_val = rte_hash_crc_1byte(*(const uint8_t *)pd, init_val);
> +
> return init_val;
> }
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [dpdk-dev] [PATCH] hash: fix CRC32c computation
2015-12-23 9:12 ` Qiu, Michael
@ 2015-12-23 11:37 ` Vincent JARDIN
[not found] ` <E115CCD9D858EF4F90C690B0DCB4D8973C8A16BD@IRSMSX108.ger.corp.intel.com>
0 siblings, 1 reply; 13+ messages in thread
From: Vincent JARDIN @ 2015-12-23 11:37 UTC (permalink / raw)
To: Qiu, Michael; +Cc: dev
Le 23 déc. 2015 10:12, "Qiu, Michael" <michael.qiu@intel.com> a é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] = {0};
> >
> > static uint32_t crc32c_trivial(uint8_t *buffer, uint32_t length,
uint32_t crc)
> > {
> > uint32_t i, j;
> > for (i = 0; i < length; ++i)
> > {
> > crc = crc ^ buffer[i];
> > for (j = 0; j < 8; j++)
> > crc = (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 <didier.pallard@6wind.com>
> > Acked-by: David Marchand <david.marchand@6wind.com>
> > ---
> > 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 <rte_common.h>
> >
> > 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] = {{
> > crc32c_tables[(n)-1][((crc) >> 8) & 0xFF])
> >
> > static inline uint32_t
> > +crc32c_1byte(uint8_t data, uint32_t init_val)
> > +{
> > + uint32_t crc;
> > + crc = init_val;
> > + crc ^= 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 = init_val;
> > + crc ^= data;
> > +
> > + crc = 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 = 0;
> > uintptr_t pd = (uintptr_t) data;
> >
> > for (i = 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 += 8;
> > }
> >
> > - switch (7 - (data_len & 0x07)) {
> > - case 0:
> > - temp |= (uint64_t) *((const uint8_t *)pd + 6) << 48;
> > - /* Fallthrough */
> > - case 1:
> > - temp |= (uint64_t) *((const uint8_t *)pd + 5) << 40;
> > - /* Fallthrough */
> > - case 2:
> > - temp |= (uint64_t) *((const uint8_t *)pd + 4) << 32;
> > - temp |= *(const uint32_t *)pd;
> > - init_val = rte_hash_crc_8byte(temp, init_val);
> > - break;
> > - case 3:
> > + if (data_len & 0x4) {
> > init_val = rte_hash_crc_4byte(*(const uint32_t *)pd,
init_val);
> > - break;
> > - case 4:
> > - temp |= *((const uint8_t *)pd + 2) << 16;
> > - /* Fallthrough */
> > - case 5:
> > - temp |= *((const uint8_t *)pd + 1) << 8;
> > - /* Fallthrough */
> > - case 6:
> > - temp |= *(const uint8_t *)pd;
> > - init_val = rte_hash_crc_4byte(temp, init_val);
> > - /* Fallthrough */
> > - default:
> > - break;
> > + pd += 4;
> > + }
> > +
> > + if (data_len & 0x2) {
> > + init_val = rte_hash_crc_2byte(*(const uint16_t *)pd,
init_val);
> > + pd += 2;
> > }
> >
> > + if (data_len & 0x1)
> > + init_val = rte_hash_crc_1byte(*(const uint8_t *)pd,
init_val);
> > +
> > return init_val;
> > }
> >
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [dpdk-dev] [PATCH v2] hash: fix CRC32c computation
2015-12-22 9:34 [dpdk-dev] [PATCH] hash: fix CRC32c computation Didier Pallard
2015-12-23 9:12 ` Qiu, Michael
@ 2016-02-09 9:34 ` Didier Pallard
2016-02-10 12:16 ` De Lara Guarch, Pablo
2016-02-19 11:00 ` [dpdk-dev] [PATCH v3 0/2] Fix " Didier Pallard
2016-02-10 14:35 ` [dpdk-dev] [PATCH] hash: fix " Pattan, Reshma
2 siblings, 2 replies; 13+ messages in thread
From: Didier Pallard @ 2016-02-09 9:34 UTC (permalink / raw)
To: dev, bruce.richardson, pablo.de.lara.guarch
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 not tested.
code showing the problem:
uint8_t null_test[32] = {0};
static uint32_t crc32c_trivial(uint8_t *buffer, uint32_t length, uint32_t crc)
{
uint32_t i, j;
for (i = 0; i < length; ++i)
{
crc = crc ^ buffer[i];
for (j = 0; j < 8; j++)
crc = (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 <didier.pallard@6wind.com>
Acked-by: David Marchand <david.marchand@6wind.com>
---
v2: Fix ARM64 compilation issue.
v1: Initial version.
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..7dd6334 100644
--- a/lib/librte_hash/rte_crc_arm64.h
+++ b/lib/librte_hash/rte_crc_arm64.h
@@ -50,6 +50,28 @@ extern "C" {
#include <rte_common.h>
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], %w[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], %w[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] = {{
crc32c_tables[(n)-1][((crc) >> 8) & 0xFF])
static inline uint32_t
+crc32c_1byte(uint8_t data, uint32_t init_val)
+{
+ uint32_t crc;
+ crc = init_val;
+ crc ^= 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 = init_val;
+ crc ^= data;
+
+ crc = 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 = 0;
uintptr_t pd = (uintptr_t) data;
for (i = 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 += 8;
}
- switch (7 - (data_len & 0x07)) {
- case 0:
- temp |= (uint64_t) *((const uint8_t *)pd + 6) << 48;
- /* Fallthrough */
- case 1:
- temp |= (uint64_t) *((const uint8_t *)pd + 5) << 40;
- /* Fallthrough */
- case 2:
- temp |= (uint64_t) *((const uint8_t *)pd + 4) << 32;
- temp |= *(const uint32_t *)pd;
- init_val = rte_hash_crc_8byte(temp, init_val);
- break;
- case 3:
+ if (data_len & 0x4) {
init_val = rte_hash_crc_4byte(*(const uint32_t *)pd, init_val);
- break;
- case 4:
- temp |= *((const uint8_t *)pd + 2) << 16;
- /* Fallthrough */
- case 5:
- temp |= *((const uint8_t *)pd + 1) << 8;
- /* Fallthrough */
- case 6:
- temp |= *(const uint8_t *)pd;
- init_val = rte_hash_crc_4byte(temp, init_val);
- /* Fallthrough */
- default:
- break;
+ pd += 4;
+ }
+
+ if (data_len & 0x2) {
+ init_val = rte_hash_crc_2byte(*(const uint16_t *)pd, init_val);
+ pd += 2;
}
+ if (data_len & 0x1)
+ init_val = rte_hash_crc_1byte(*(const uint8_t *)pd, init_val);
+
return init_val;
}
--
2.1.4
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [dpdk-dev] [PATCH v2] hash: fix CRC32c computation
2016-02-09 9:34 ` [dpdk-dev] [PATCH v2] " Didier Pallard
@ 2016-02-10 12:16 ` De Lara Guarch, Pablo
2016-02-19 11:00 ` [dpdk-dev] [PATCH v3 0/2] Fix " Didier Pallard
1 sibling, 0 replies; 13+ messages in thread
From: De Lara Guarch, Pablo @ 2016-02-10 12:16 UTC (permalink / raw)
To: Didier Pallard, dev, Richardson, Bruce
Hi Didier,
> -----Original Message-----
> From: Didier Pallard [mailto:didier.pallard@6wind.com]
> Sent: Tuesday, February 09, 2016 9:34 AM
> To: dev@dpdk.org; Richardson, Bruce; De Lara Guarch, Pablo
> Cc: jean-mickael.guerin@6wind.com; thomas.monjalon@6wind.com
> Subject: [PATCH v2] hash: fix CRC32c computation
>
> 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 not tested.
>
> code showing the problem:
>
> uint8_t null_test[32] = {0};
>
> static uint32_t crc32c_trivial(uint8_t *buffer, uint32_t length, uint32_t crc)
> {
> uint32_t i, j;
> for (i = 0; i < length; ++i)
> {
> crc = crc ^ buffer[i];
> for (j = 0; j < 8; j++)
> crc = (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 <didier.pallard@6wind.com>
> Acked-by: David Marchand <david.marchand@6wind.com>
It compiles fine now, thanks!
Could you add also tests for not multiple of 4 bytes keys in test_hash_functions.c,
so we make sure from now on that it works (and you can demonstrate that your fix works)?
You could send a patchset with those new tests first and then the fix.
Also, a note in release notes would be welcome :)
Thanks!
Pablo
^ permalink raw reply [flat|nested] 13+ messages in thread* [dpdk-dev] [PATCH v3 0/2] Fix CRC32c computation
2016-02-09 9:34 ` [dpdk-dev] [PATCH v2] " Didier Pallard
2016-02-10 12:16 ` De Lara Guarch, Pablo
@ 2016-02-19 11:00 ` Didier Pallard
2016-02-19 11:00 ` [dpdk-dev] [PATCH v3 1/2] test: fix CRC hash function autotest Didier Pallard
` (2 more replies)
1 sibling, 3 replies; 13+ messages in thread
From: Didier Pallard @ 2016-02-19 11:00 UTC (permalink / raw)
To: dev
CRC32c computation is not valid when buffer length is not a multiple of 4 bytes.
Values returned by rte_hash_crc functions does not match the one
computed by a trivial crc32c implementation.
First patch fixes crc hash function autotests, to outline the problem.
Second patch fixes CRC32c computation.
Didier Pallard (2):
test: fix CRC hash function autotest
hash: fix CRC32c computation
app/test/test_hash_functions.c | 17 +++--
doc/guides/rel_notes/release_16_04.rst | 5 ++
lib/librte_hash/rte_crc_arm64.h | 64 +++++++++++++++++
lib/librte_hash/rte_hash_crc.h | 125 ++++++++++++++++++++++++++-------
4 files changed, 178 insertions(+), 33 deletions(-)
--
2.1.4
^ permalink raw reply [flat|nested] 13+ messages in thread* [dpdk-dev] [PATCH v3 1/2] test: fix CRC hash function autotest
2016-02-19 11:00 ` [dpdk-dev] [PATCH v3 0/2] Fix " Didier Pallard
@ 2016-02-19 11:00 ` Didier Pallard
2016-02-19 11:00 ` [dpdk-dev] [PATCH v3 2/2] hash: fix CRC32c computation Didier Pallard
2016-02-19 15:08 ` [dpdk-dev] [PATCH v3 0/2] Fix " De Lara Guarch, Pablo
2 siblings, 0 replies; 13+ messages in thread
From: Didier Pallard @ 2016-02-19 11:00 UTC (permalink / raw)
To: dev
Add some small key lengthes (below 4 bytes), and fix odd key lengthes
expected returned values for CRC computation to match real CRC values.
Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
---
app/test/test_hash_functions.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/app/test/test_hash_functions.c b/app/test/test_hash_functions.c
index 3ad6d80..f767a48 100644
--- a/app/test/test_hash_functions.c
+++ b/app/test/test_hash_functions.c
@@ -54,26 +54,30 @@
* e.g.: key size = 4, key = 0x03020100
* key size = 8, key = 0x0706050403020100
*/
-static uint32_t hash_values_jhash[2][10] = {{
+static uint32_t hash_values_jhash[2][12] = {{
+ 0x8ba9414b, 0xdf0d39c9,
0xe4cf1d42, 0xd4ccb93c, 0x5e84eafc, 0x21362cfe,
0x2f4775ab, 0x9ff036cc, 0xeca51474, 0xbc9d6816,
0x12926a31, 0x1c9fa888
},
{
+ 0x5c62c303, 0x1b8cf784,
0x8270ac65, 0x05fa6668, 0x762df861, 0xda088f2f,
0x59614cd4, 0x7a94f690, 0xdc1e4993, 0x30825494,
0x91d0e462, 0x768087fc
}
};
-static uint32_t hash_values_crc[2][10] = {{
+static uint32_t hash_values_crc[2][12] = {{
+ 0x00000000, 0xf26b8303,
0x91545164, 0x06040eb1, 0x9bb99201, 0xcc4c4fe4,
- 0x14a90993, 0xf8a5dd8c, 0xc62beb31, 0x32bf340e,
- 0x72f9d22b, 0x4a11475e
+ 0x14a90993, 0xf8a5dd8c, 0xcaa1ad0b, 0x7ac1e03e,
+ 0x43f44466, 0x4a11475e
},
{
+ 0xbdfd3980, 0x70204542,
0x98cd4c70, 0xd52c702f, 0x41fc0e1c, 0x3905f65c,
- 0x94bff47f, 0x1bab102d, 0xd2911ed7, 0xe8faa813,
- 0x6bea184b, 0x53028d3e
+ 0x94bff47f, 0x1bab102d, 0xf4a2c645, 0xbf441539,
+ 0x789c104f, 0x53028d3e
}
};
@@ -89,6 +93,7 @@ static uint32_t hash_values_crc[2][10] = {{
static rte_hash_function hashtest_funcs[] = {rte_jhash, rte_hash_crc};
static uint32_t hashtest_initvals[] = {0, 0xdeadbeef};
static uint32_t hashtest_key_lens[] = {
+ 1, 2, /* Unusual key sizes */
4, 8, 16, 32, 48, 64, /* standard key sizes */
9, /* IPv4 SRC + DST + protocol, unpadded */
13, /* IPv4 5-tuple, unpadded */
--
2.1.4
^ permalink raw reply [flat|nested] 13+ messages in thread* [dpdk-dev] [PATCH v3 2/2] hash: fix CRC32c computation
2016-02-19 11:00 ` [dpdk-dev] [PATCH v3 0/2] Fix " Didier Pallard
2016-02-19 11:00 ` [dpdk-dev] [PATCH v3 1/2] test: fix CRC hash function autotest Didier Pallard
@ 2016-02-19 11:00 ` Didier Pallard
2016-02-19 15:08 ` [dpdk-dev] [PATCH v3 0/2] Fix " De Lara Guarch, Pablo
2 siblings, 0 replies; 13+ messages in thread
From: Didier Pallard @ 2016-02-19 11:00 UTC (permalink / raw)
To: dev
Fix crc32c hash functions to return a valid crc32c value for
data lengthes not multiple of 4 bytes.
ARM code is not tested.
Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
Acked-by: David Marchand <david.marchand@6wind.com>
---
doc/guides/rel_notes/release_16_04.rst | 5 ++
lib/librte_hash/rte_crc_arm64.h | 64 +++++++++++++++++
lib/librte_hash/rte_hash_crc.h | 125 ++++++++++++++++++++++++++-------
3 files changed, 167 insertions(+), 27 deletions(-)
diff --git a/doc/guides/rel_notes/release_16_04.rst b/doc/guides/rel_notes/release_16_04.rst
index eb1b3b2..45b35dd 100644
--- a/doc/guides/rel_notes/release_16_04.rst
+++ b/doc/guides/rel_notes/release_16_04.rst
@@ -68,6 +68,11 @@ Drivers
Libraries
~~~~~~~~~
+* **hash: Fixed CRC32c hash computation for non multiple of 4 bytes sizes.**
+
+ Fix crc32c hash functions to return a valid crc32c value for data lengthes
+ not multiple of 4 bytes.
+
Examples
~~~~~~~~
diff --git a/lib/librte_hash/rte_crc_arm64.h b/lib/librte_hash/rte_crc_arm64.h
index 02e26bc..7dd6334 100644
--- a/lib/librte_hash/rte_crc_arm64.h
+++ b/lib/librte_hash/rte_crc_arm64.h
@@ -50,6 +50,28 @@ extern "C" {
#include <rte_common.h>
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], %w[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], %w[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..63e74aa 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] = {{
crc32c_tables[(n)-1][((crc) >> 8) & 0xFF])
static inline uint32_t
+crc32c_1byte(uint8_t data, uint32_t init_val)
+{
+ uint32_t crc;
+ crc = init_val;
+ crc ^= 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 = init_val;
+ crc ^= data;
+
+ crc = 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 2 bytes 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 = 0;
uintptr_t pd = (uintptr_t) data;
for (i = 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 += 8;
}
- switch (7 - (data_len & 0x07)) {
- case 0:
- temp |= (uint64_t) *((const uint8_t *)pd + 6) << 48;
- /* Fallthrough */
- case 1:
- temp |= (uint64_t) *((const uint8_t *)pd + 5) << 40;
- /* Fallthrough */
- case 2:
- temp |= (uint64_t) *((const uint8_t *)pd + 4) << 32;
- temp |= *(const uint32_t *)pd;
- init_val = rte_hash_crc_8byte(temp, init_val);
- break;
- case 3:
+ if (data_len & 0x4) {
init_val = rte_hash_crc_4byte(*(const uint32_t *)pd, init_val);
- break;
- case 4:
- temp |= *((const uint8_t *)pd + 2) << 16;
- /* Fallthrough */
- case 5:
- temp |= *((const uint8_t *)pd + 1) << 8;
- /* Fallthrough */
- case 6:
- temp |= *(const uint8_t *)pd;
- init_val = rte_hash_crc_4byte(temp, init_val);
- /* Fallthrough */
- default:
- break;
+ pd += 4;
+ }
+
+ if (data_len & 0x2) {
+ init_val = rte_hash_crc_2byte(*(const uint16_t *)pd, init_val);
+ pd += 2;
}
+ if (data_len & 0x1)
+ init_val = rte_hash_crc_1byte(*(const uint8_t *)pd, init_val);
+
return init_val;
}
--
2.1.4
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [dpdk-dev] [PATCH v3 0/2] Fix CRC32c computation
2016-02-19 11:00 ` [dpdk-dev] [PATCH v3 0/2] Fix " Didier Pallard
2016-02-19 11:00 ` [dpdk-dev] [PATCH v3 1/2] test: fix CRC hash function autotest Didier Pallard
2016-02-19 11:00 ` [dpdk-dev] [PATCH v3 2/2] hash: fix CRC32c computation Didier Pallard
@ 2016-02-19 15:08 ` De Lara Guarch, Pablo
2016-03-01 13:31 ` Thomas Monjalon
2 siblings, 1 reply; 13+ messages in thread
From: De Lara Guarch, Pablo @ 2016-02-19 15:08 UTC (permalink / raw)
To: Didier Pallard, dev
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Didier Pallard
> Sent: Friday, February 19, 2016 11:00 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 0/2] Fix CRC32c computation
>
> CRC32c computation is not valid when buffer length is not a multiple of 4
> bytes.
> Values returned by rte_hash_crc functions does not match the one
> computed by a trivial crc32c implementation.
>
> First patch fixes crc hash function autotests, to outline the problem.
> Second patch fixes CRC32c computation.
>
> Didier Pallard (2):
> test: fix CRC hash function autotest
> hash: fix CRC32c computation
>
> app/test/test_hash_functions.c | 17 +++--
> doc/guides/rel_notes/release_16_04.rst | 5 ++
> lib/librte_hash/rte_crc_arm64.h | 64 +++++++++++++++++
> lib/librte_hash/rte_hash_crc.h | 125 ++++++++++++++++++++++++++-----
> --
> 4 files changed, 178 insertions(+), 33 deletions(-)
>
> --
> 2.1.4
Series-acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
Not sure if you need to include a "Fixes" line in the commit messages.
In the first commit, probably you should, the commit that you are fixing is
6298d2c55ae8 ("app/test: add new functional tests for hash functions").
In the second commit, it is a bit more difficult, as we don't know that the commit is,
that code was integrated a while ago, before 1.2.3, which is the first public release in dpdk.org.
Also, there is a typo "lengthes", in both commit messages.
You can leave the ack in both patches. Thanks!!
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [dpdk-dev] [PATCH v3 0/2] Fix CRC32c computation
2016-02-19 15:08 ` [dpdk-dev] [PATCH v3 0/2] Fix " De Lara Guarch, Pablo
@ 2016-03-01 13:31 ` Thomas Monjalon
0 siblings, 0 replies; 13+ messages in thread
From: Thomas Monjalon @ 2016-03-01 13:31 UTC (permalink / raw)
To: Didier Pallard; +Cc: dev
> > CRC32c computation is not valid when buffer length is not a multiple of 4
> > bytes.
> > Values returned by rte_hash_crc functions does not match the one
> > computed by a trivial crc32c implementation.
> >
> > First patch fixes crc hash function autotests, to outline the problem.
> > Second patch fixes CRC32c computation.
> >
> > Didier Pallard (2):
> > test: fix CRC hash function autotest
> > hash: fix CRC32c computation
>
> Series-acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
>
> Not sure if you need to include a "Fixes" line in the commit messages.
> In the first commit, probably you should, the commit that you are fixing is
> 6298d2c55ae8 ("app/test: add new functional tests for hash functions").
Thanks
> In the second commit, it is a bit more difficult, as we don't know that the commit is,
> that code was integrated a while ago, before 1.2.3, which is the first public release in dpdk.org.
Yes it helps to know the bug was there since the beginning.
> Also, there is a typo "lengthes", in both commit messages.
>
> You can leave the ack in both patches. Thanks!!
Applied, thanks
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [dpdk-dev] [PATCH] hash: fix CRC32c computation
2015-12-22 9:34 [dpdk-dev] [PATCH] hash: fix CRC32c computation Didier Pallard
2015-12-23 9:12 ` Qiu, Michael
2016-02-09 9:34 ` [dpdk-dev] [PATCH v2] " Didier Pallard
@ 2016-02-10 14:35 ` Pattan, Reshma
2016-02-11 8:21 ` Didier Pallard
2 siblings, 1 reply; 13+ messages in thread
From: Pattan, Reshma @ 2016-02-10 14:35 UTC (permalink / raw)
To: Didier Pallard; +Cc: dev
Hi,
Small typo.
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Didier Pallard
> Sent: Tuesday, December 22, 2015 9:35 AM
> To: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; De Lara
> Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Subject: [dpdk-dev] [PATCH] hash: fix CRC32c computation
> 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
> + * Use single crc32 instruction to perform a hash on a byte value.
"on a byte value" should be ===> "on 2 bytes".
> + * 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);
> +}
> +
Thanks,
Reshma
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [dpdk-dev] [PATCH] hash: fix CRC32c computation
2016-02-10 14:35 ` [dpdk-dev] [PATCH] hash: fix " Pattan, Reshma
@ 2016-02-11 8:21 ` Didier Pallard
0 siblings, 0 replies; 13+ messages in thread
From: Didier Pallard @ 2016-02-11 8:21 UTC (permalink / raw)
To: Pattan, Reshma; +Cc: dev
Ok, i will fix the typo and propose a second serie of patch including
unit tests.
thanks
On 02/10/2016 03:35 PM, Pattan, Reshma wrote:
> Hi,
>
> Small typo.
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Didier Pallard
>> Sent: Tuesday, December 22, 2015 9:35 AM
>> To: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; De Lara
>> Guarch, Pablo <pablo.de.lara.guarch@intel.com>
>> Subject: [dpdk-dev] [PATCH] hash: fix CRC32c computation
>> 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
>> + * Use single crc32 instruction to perform a hash on a byte value.
> "on a byte value" should be ===> "on 2 bytes".
>
>> + * 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);
>> +}
>> +
> Thanks,
> Reshma
--
Didier PALLARD
6WIND
Software Engineer
Tel: +33 1 39 30 92 46
Mob: +33 6 49 11 40 14
Fax: +33 1 39 30 92 11
didier.pallard@6wind.com
www.6wind.com
This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and contains information that is confidential and proprietary to 6WIND. All unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.
Ce courriel ainsi que toutes les pièces jointes, est uniquement destiné à son ou ses destinataires. Il contient des informations confidentielles qui sont la propriété de 6WIND. Toute révélation, distribution ou copie des informations qu'il contient est strictement interdite. Si vous avez reçu ce message par erreur, veuillez immédiatement le signaler à l'émetteur et détruire toutes les données reçues
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-03-01 13:33 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-22 9:34 [dpdk-dev] [PATCH] hash: fix CRC32c computation Didier Pallard
2015-12-23 9:12 ` Qiu, Michael
2015-12-23 11:37 ` Vincent JARDIN
[not found] ` <E115CCD9D858EF4F90C690B0DCB4D8973C8A16BD@IRSMSX108.ger.corp.intel.com>
2016-02-08 14:43 ` De Lara Guarch, Pablo
2016-02-09 9:34 ` [dpdk-dev] [PATCH v2] " Didier Pallard
2016-02-10 12:16 ` De Lara Guarch, Pablo
2016-02-19 11:00 ` [dpdk-dev] [PATCH v3 0/2] Fix " Didier Pallard
2016-02-19 11:00 ` [dpdk-dev] [PATCH v3 1/2] test: fix CRC hash function autotest Didier Pallard
2016-02-19 11:00 ` [dpdk-dev] [PATCH v3 2/2] hash: fix CRC32c computation Didier Pallard
2016-02-19 15:08 ` [dpdk-dev] [PATCH v3 0/2] Fix " De Lara Guarch, Pablo
2016-03-01 13:31 ` Thomas Monjalon
2016-02-10 14:35 ` [dpdk-dev] [PATCH] hash: fix " Pattan, Reshma
2016-02-11 8:21 ` Didier Pallard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).