Messages in this thread |  | | Date | Mon, 24 Feb 2014 04:32:27 -0800 | From | "H. Peter Anvin" <> | Subject | Re: [PATCH 2/3] x86/hash: swap parameters of crc32_u32() |
| |
On 02/24/2014 03:46 AM, Daniel Borkmann wrote: >>>>>> >>>>>> --- 3.14-rc3-x86-hash-crc32.orig/arch/x86/lib/hash.c >>>>>> +++ 3.14-rc3-x86-hash-crc32/arch/x86/lib/hash.c >>>>>> @@ -37,7 +37,7 @@ >>>>>> #include <asm/cpufeature.h> >>>>>> #include <asm/hash.h> >>>>>> >>>>>> -static inline u32 crc32_u32(u32 crc, u32 val) >>>>>> +static inline u32 crc32_u32(u32 val, u32 crc) >>>>>> { >>>>>> #ifdef CONFIG_AS_CRC32 >>>>>> asm ("crc32l %1,%0\n" : "+r" (crc) : "rm" (val)); >>>>>
OK, this whole tread is really confusing, but the change proposed seems actively wrong.
First of all:
static inline uint32_t rte_hash_crc_4byte(uint32_t data, uint32_t init_val) { return _mm_crc32_u32(data, init_val); }
... from the DPDK code is confusing all by itself, because the definition of the _mm_crc32_u32() intrinsic per the Intel SDM is:
unsigned int _mm_crc32_u32(unsigned int crc, unsigned int data);
... where "crc" is the destination operand, i.e. the accumulator if you actually would be computing a CRC32C.
So I'm guessing this hash is deliberately using the CRC32 instruction "backwards", which would actually make sense: an actual CRC is actually a pretty poor hash due to linearity.
This has confused people elsewhere, too:
http://permalink.gmane.org/gmane.comp.networking.dpdk.devel/954
So if this is a bug it is a bug in the upstream code, but I'm guessing the operand reversal is intentional.
Therefore, this patch should be actively NAKed.
Nacked-by: H. Peter Anvin <hpa@zytor.com>
|  |