add slice by 8 algorithm to crc32.c
PATCH 1/1 add slice by 8 algorithm to crc32.c
Hello,
Added support for slice by 8 to existing crc32 algorithm. Also modified gen_crc32table.c to only produce table entries that are actually used. The parameters CRC_LE_BITS and CRC_BE_BITS determine the number of bits in the input array that are processed during each step. Generally the more bits the faster the algorithm is but the more table data required.
Using an x86_64 Opteron machine running at 2100MHz the following table was collected with a pre-warmed cache by computing the crc 1000 times on a buffer of 4096 bytes.
BITS Size LE Cycles/byte BE Cycles/byte
----------------------------------------------
1 873 41.65 34.60
2 1097 25.43 29.61
4 1057 13.29 15.28
8 2913 7.13 8.19
32 9684 2.80 2.82
64 18178 1.53 1.53
BITS is the value of CRC_LE_BITS or CRC_BE_BITS. The old default was 8 which actually selected the 32 bit algorithm. In this version the value 8 is used to select the standard 8 bit algorithm and two new values: 32 and 64 are introduced to select the slice by 4 and slice by 8 algorithms respectively.
Where Size is the size of crc32.o's text segment which includes code and table data when both LE and BE versions are set to BITS.
The current version of crc32.c by default uses the slice by 4 algorithm which requires about 2.8 cycles per byte. The slice by 8 algorithm is roughly 2X faster and enables packet processing at over 1GB/sec on a typical 2-3GHz system.
crc32.c +262 -112
@@ -1,4 +1,8 @@/* +* July 20, 2011 Bob Pearson <rpearson at systemfabricworks.com> +* added slice by 8 algorithm to the existing conventional and +* slice by 4 algorithms. +* * Oct 15, 2000 Matt Domsch <[email protected]> * Nicer crc32 functions/docs submitted by [email protected]. Thanks! * Code was from the public domain, copyright abandoned. Code was
@@ -19,7 +23,6 @@* This source code is licensed under the GNU General Public License, * Version 2. See the file COPYING for more details. */ -#include <linux/crc32.h> #include <linux/kernel.h> #include <linux/module.h>
@@ -28,14 +31,17 @@#include <linux/init.h> #include <asm/atomic.h> #include "crc32defs.h" -#if CRC_LE_BITS ==8 -# define tole (x) __constant_cpu_to_le32(x) + +#include <asm/msr.h> + +#if CRC_LE_BITS >8 +# define tole (x) (__force u32) __constant_cpu_to_le32(x)#else # define tole (x) (x)#endif -#if CRC_BE_BITS ==8 -# define tobe (x) __constant_cpu_to_be32(x) +#if CRC_BE_BITS >8 +# define tobe (x) (__force u32) __constant_cpu_to_be32(x)#else # define tobe (x) (x)#endif
@@ -45,54 +51,228 @@ MODULE_AUTHOR("Matt Domsch <Matt_Domsch@MODULE_DESCRIPTION ("Ethernet CRC32 calculations" );MODULE_LICENSE ("GPL" ); -#if CRC_LE_BITS ==8 ||CRC_BE_BITS ==8 +#if CRC_LE_BITS >8 +static inline u32 crc32_le_body (u32 crc,u8 const *buf,size_t len) +{ +const u8 *p8; +const u32 *p32; +int init_bytes, end_bytes; +size_t words; +int i; +u32 q; +u8 i0, i1, i2, i3; + + crc = (__force u32 )__cpu_to_le32 (crc); + +#if CRC_LE_BITS ==64 + p8 = buf; + p32 = (u32 *)(((uintptr_t )p8 +7 ) & ~7 ); + + init_bytes = (uintptr_t )p32 - (uintptr_t )p8; +if (init_bytes > len) + init_bytes = len; + words = (len - init_bytes) >>3 ; + end_bytes = (len - init_bytes) &7 ; +#else + p8 = buf; + p32 = (u32 *)(((uintptr_t )p8 +3 ) & ~3 ); + + init_bytes = (uintptr_t )p32 - (uintptr_t )p8; +if (init_bytes > len) + init_bytes = len; + words = (len - init_bytes) >>2 ; + end_bytes = (len - init_bytes) &3 ; +#endif + +for (i =0 ; i < init_bytes; i++) { +#ifdef __LITTLE_ENDIAN + i0 = *p8++ ^ crc; + crc = t0_le[i0] ^ (crc >>8 ); +#else + i0 = *p8++ ^ (crc >>24 ); + crc = t0_le[i0] ^ (crc <<8 ); +#endif + } + +for (i =0 ; i < words; i++) { +#ifdef __LITTLE_ENDIAN +# if CRC_LE_BITS ==64 +/* slice by 8 algorithm */ + q = *p32++ ^ crc; + i3 = q; + i2 = q >>8 ; + i1 = q >>16 ; + i0 = q >>24 ; + crc = t7_le[i3] ^ t6_le[i2] ^ t5_le[i1] ^ t4_le[i0]; + + q = *p32++; + i3 = q; + i2 = q >>8 ; + i1 = q >>16 ; + i0 = q >>24 ; + crc ^= t3_le[i3] ^ t2_le[i2] ^ t1_le[i1] ^ t0_le[i0]; +# else +/* slice by 4 algorithm */ + q = *p32++ ^ crc; + i3 = q; + i2 = q >>8 ; + i1 = q >>16 ; + i0 = q >>24 ; + crc = t3_le[i3] ^ t2_le[i2] ^ t1_le[i1] ^ t0_le[i0]; +# endif +#else +# if CRC_LE_BITS ==64 + q = *p32++ ^ crc; + i3 = q >>24 ; + i2 = q >>16 ; + i1 = q >>8 ; + i0 = q; + crc = t7_le[i3] ^ t6_le[i2] ^ t5_le[i1] ^ t4_le[i0]; + + q = *p32++; + i3 = q >>24 ; + i2 = q >>16 ; + i1 = q >>8 ; + i0 = q; + crc ^= t3_le[i3] ^ t2_le[i2] ^ t1_le[i1] ^ t0_le[i0]; +# else + q = *p32++ ^ crc; + i3 = q >>24 ; + i2 = q >>16 ; + i1 = q >>8 ; + i0 = q; + crc = t3_le[i3] ^ t2_le[i2] ^ t1_le[i1] ^ t0_le[i0]; +# endif +#endif + } + + p8 = (u8 *)p32; + +for (i =0 ; i < end_bytes; i++) { +#ifdef __LITTLE_ENDIAN + i0 = *p8++ ^ crc; + crc = t0_le[i0] ^ (crc >>8 ); +#else + i0 = *p8++ ^ (crc >>24 ); + crc = t0_le[i0] ^ (crc <<8 ); +#endif + } + +return __le32_to_cpu ((__force __le32 )crc); +} +#endif -static inline u32 -crc32_body (u32 crc,unsigned char const *buf,size_t len,const u32 (*tab)[256 ]) +#if CRC_BE_BITS >8 +static inline u32 crc32_be_body (u32 crc,u8 const *buf,size_t len) { -# ifdef __LITTLE_ENDIAN -# define DO_CRC (x) crc = tab[0][(crc ^ (x)) & 255] ^ (crc >> 8) -# define DO_CRC4 crc = tab[3][(crc) & 255] ^ \ - tab[2][(crc >> 8) & 255] ^ \ - tab[1][(crc >> 16) & 255] ^ \ - tab[0][(crc >> 24) & 255] -# else -# define DO_CRC (x) crc = tab[0][((crc >> 24) ^ (x)) & 255] ^ (crc << 8) -# define DO_CRC4 crc = tab[0][(crc) & 255] ^ \ - tab[1][(crc >> 8) & 255] ^ \ - tab[2][(crc >> 16) & 255] ^ \ - tab[3][(crc >> 24) & 255] -# endif -const u32 *b; -size_t rem_len; +const u8 *p8; +const u32 *p32; +int init_bytes, end_bytes; +size_t words; +int i; +u32 q; +u8 i0, i1, i2, i3; + + crc = (__force u32 )__cpu_to_be32 (crc); + +#if CRC_LE_BITS ==64 + p8 = buf; + p32 = (u32 *)(((uintptr_t )p8 +7 ) & ~7 ); + + init_bytes = (uintptr_t )p32 - (uintptr_t )p8; +if (init_bytes > len) + init_bytes = len; + words = (len - init_bytes) >>3 ; + end_bytes = (len - init_bytes) &7 ; +#else + p8 = buf; + p32 = (u32 *)(((uintptr_t )p8 +3 ) & ~3 ); + + init_bytes = (uintptr_t )p32 - (uintptr_t )p8; +if (init_bytes > len) + init_bytes = len; + words = (len - init_bytes) >>2 ; + end_bytes = (len - init_bytes) &3 ; +#endif + +for (i =0 ; i < init_bytes; i++) { +#ifdef __LITTLE_ENDIAN + i0 = *p8++ ^ crc; + crc = t0_be[i0] ^ (crc >>8 ); +#else + i0 = *p8++ ^ (crc >>24 ); + crc = t0_be[i0] ^ (crc <<8 ); +#endif + } -/* Align it */ -if (unlikely ((long )buf &3 && len)) { -do { -DO_CRC (*buf++); - }while ((--len) && ((long )buf)&3 ); - } - rem_len = len &3 ; -/* load data 32 bits wide, xor data 32 bits wide. */ - len = len >>2 ; - b = (const u32 *)buf; -for (--b; len; --len) { - crc ^= *++b;/* use pre increment for speed */ -DO_CRC4 ; - } - len = rem_len; -/* And the last few bytes */ -if (len) { -u8 *p = (u8 *)(b +1 ) -1 ; -do { -DO_CRC (*++p);/* use pre increment for speed */ - }while (--len); +for (i =0 ; i < words; i++) { +#ifdef __LITTLE_ENDIAN +# if CRC_LE_BITS ==64 +/* slice by 8 algorithm */ + q = *p32++ ^ crc; + i3 = q; + i2 = q >>8 ; + i1 = q >>16 ; + i0 = q >>24 ; + crc = t7_be[i3] ^ t6_be[i2] ^ t5_be[i1] ^ t4_be[i0]; + + q = *p32++; + i3 = q; + i2 = q >>8 ; + i1 = q >>16 ; + i0 = q >>24 ; + crc ^= t3_be[i3] ^ t2_be[i2] ^ t1_be[i1] ^ t0_be[i0]; +# else +/* slice by 4 algorithm */ + q = *p32++ ^ crc; + i3 = q; + i2 = q >>8 ; + i1 = q >>16 ; + i0 = q >>24 ; + crc = t3_be[i3] ^ t2_be[i2] ^ t1_be[i1] ^ t0_be[i0]; +# endif +#else +# if CRC_LE_BITS ==64 + q = *p32++ ^ crc; + i3 = q >>24 ; + i2 = q >>16 ; + i1 = q >>8 ; + i0 = q; + crc = t7_be[i3] ^ t6_be[i2] ^ t5_be[i1] ^ t4_be[i0]; + + q = *p32++; + i3 = q >>24 ; + i2 = q >>16 ; + i1 = q >>8 ; + i0 = q; + crc ^= t3_be[i3] ^ t2_be[i2] ^ t1_be[i1] ^ t0_be[i0]; +# else + q = *p32++ ^ crc; + i3 = q >>24 ; + i2 = q >>16 ; + i1 = q >>8 ; + i0 = q; + crc = t3_be[i3] ^ t2_be[i2] ^ t1_be[i1] ^ t0_be[i0]; +# endif +#endif } -return crc; -#undef DO_CRC -#undef DO_CRC4 + + p8 = (u8 *)p32; + +for (i =0 ; i < end_bytes; i++) { +#ifdef __LITTLE_ENDIAN + i0 = *p8++ ^ crc; + crc = t0_be[i0] ^ (crc >>8 ); +#else + i0 = *p8++ ^ (crc >>24 ); + crc = t0_be[i0] ^ (crc <<8 ); +#endif + } + +return __be32_to_cpu ((__force __be32 )crc); }#endif +/** * crc32_le() - Calculate bitwise little-endian Ethernet AUTODIN II CRC32 * @crc: seed value for computation. ~0 for Ethernet, sometimes 0 for
@@ -100,53 +280,40 @@ crc32_body(u32 crc, unsigned char const* @p: pointer to buffer over which CRC is run * @len: length of buffer @p */ -u32 __pure crc32_le (u32 crc,unsigned char const *p,size_t len); - -#if CRC_LE_BITS ==1 -/* -* In fact, the table-based code will work in this case, but it can be -* simplified by inlining the table in ?: form. -*/ -u32 __pure crc32_le (u32 crc,unsigned char const *p,size_t len) { +#if CRC_LE_BITS ==1 int i;while (len--) { crc ^= *p++;for (i =0 ; i <8 ; i++) crc = (crc >>1 ) ^ ((crc &1 ) ?CRCPOLY_LE :0 ); } -return crc; -} -#else /* Table-based approach */ - -u32 __pure crc32_le (u32 crc,unsigned char const *p,size_t len) -{ -# if CRC_LE_BITS ==8 -const u32 (*tab)[] = crc32table_le; - - crc =__cpu_to_le32 (crc); - crc =crc32_body (crc, p, len, tab); -return __le32_to_cpu (crc); +# elif CRC_LE_BITS ==2 +while (len--) { + crc ^= *p++; + crc = (crc >>2 ) ^ t0_le[crc &0x03 ]; + crc = (crc >>2 ) ^ t0_le[crc &0x03 ]; + crc = (crc >>2 ) ^ t0_le[crc &0x03 ]; + crc = (crc >>2 ) ^ t0_le[crc &0x03 ]; + }# elif CRC_LE_BITS ==4 while (len--) { crc ^= *p++; - crc = (crc >>4 ) ^ crc32table_le[crc &15 ]; - crc = (crc >>4 ) ^ crc32table_le[crc &15 ]; + crc = (crc >>4 ) ^ t0_le[crc &0x0f ]; + crc = (crc >>4 ) ^ t0_le[crc &0x0f ]; } -return crc; -# elif CRC_LE_BITS ==2 +# elif CRC_LE_BITS ==8 while (len--) { crc ^= *p++; - crc = (crc >>2 ) ^ crc32table_le[crc &3 ]; - crc = (crc >>2 ) ^ crc32table_le[crc &3 ]; - crc = (crc >>2 ) ^ crc32table_le[crc &3 ]; - crc = (crc >>2 ) ^ crc32table_le[crc &3 ]; + crc = (crc >>8 ) ^ t0_le[crc &0xff ]; } -return crc; +# else + crc =crc32_le_body (crc, p, len);# endif +return crc; } -#endif +EXPORT_SYMBOL (crc32_le);/** * crc32_be() - Calculate bitwise big-endian Ethernet AUTODIN II CRC32
@@ -155,57 +322,40 @@ u32 __pure crc32_le(u32 crc, unsigned ch* @p: pointer to buffer over which CRC is run * @len: length of buffer @p */ -u32 __pure crc32_be (u32 crc,unsigned char const *p,size_t len); - -#if CRC_BE_BITS ==1 -/* -* In fact, the table-based code will work in this case, but it can be -* simplified by inlining the table in ?: form. -*/ -u32 __pure crc32_be (u32 crc,unsigned char const *p,size_t len) { +#if CRC_BE_BITS ==1 int i;while (len--) { crc ^= *p++ <<24 ;for (i =0 ; i <8 ; i++) - crc = - (crc <<1 ) ^ ((crc &0x80000000 ) ?CRCPOLY_BE : -0 ); + crc = (crc <<1 ) ^ + ((crc &0x80000000 ) ?CRCPOLY_BE :0 ); + } +# elif CRC_BE_BITS ==2 +while (len--) { + crc ^= *p++ <<24 ; + crc = (crc <<2 ) ^ t0_be[crc >>30 ]; + crc = (crc <<2 ) ^ t0_be[crc >>30 ]; + crc = (crc <<2 ) ^ t0_be[crc >>30 ]; + crc = (crc <<2 ) ^ t0_be[crc >>30 ]; } -return crc; -} - -#else /* Table-based approach */ -u32 __pure crc32_be (u32 crc,unsigned char const *p,size_t len) -{ -# if CRC_BE_BITS ==8 -const u32 (*tab)[] = crc32table_be; - - crc =__cpu_to_be32 (crc); - crc =crc32_body (crc, p, len, tab); -return __be32_to_cpu (crc);# elif CRC_BE_BITS ==4 while (len--) { crc ^= *p++ <<24 ; - crc = (crc <<4 ) ^ crc32table_be[crc >>28 ]; - crc = (crc <<4 ) ^ crc32table_be[crc >>28 ]; + crc = (crc <<4 ) ^ t0_be[crc >>28 ]; + crc = (crc <<4 ) ^ t0_be[crc >>28 ]; } -return crc; -# elif CRC_BE_BITS ==2 +# elif CRC_BE_BITS ==8 while (len--) { crc ^= *p++ <<24 ; - crc = (crc <<2 ) ^ crc32table_be[crc >>30 ]; - crc = (crc <<2 ) ^ crc32table_be[crc >>30 ]; - crc = (crc <<2 ) ^ crc32table_be[crc >>30 ]; - crc = (crc <<2 ) ^ crc32table_be[crc >>30 ]; + crc = (crc <<8 ) ^ t0_be[crc >>24 ]; } -return crc; +# else + crc =crc32_be_body (crc, p, len);# endif +return crc; } -#endif - -EXPORT_SYMBOL (crc32_le);EXPORT_SYMBOL (crc32_be);/*
crc32defs.h +9 -7
@@ -6,27 +6,29 @@#define CRCPOLY_LE 0xedb88320#define CRCPOLY_BE 0x04c11db7 -/* How many bits at a time to use. Requires a table of 4<<CRC_xx_BITS bytes. */ +/* How many bits at a time to use. Valid values are 1, 2, 4, 8, 32 and 64. */ /* For less performance-sensitive, use 4 */ #ifndef CRC_LE_BITS -# define CRC_LE_BITS 8 +# define CRC_LE_BITS 64#endif #ifndef CRC_BE_BITS -# define CRC_BE_BITS 8 +# define CRC_BE_BITS 64#endif /* * Little-endian CRC computation. Used with serial bit streams sent * lsbit-first. Be sure to use cpu_to_le32() to append the computed CRC. */ -#if CRC_LE_BITS >8 ||CRC_LE_BITS <1 ||CRC_LE_BITS &CRC_LE_BITS -1 -# error CRC_LE_BITS must be a power of 2 between 1 and 8 +#if CRC_LE_BITS >64 ||CRC_LE_BITS <1 ||CRC_LE_BITS ==16 || \ +CRC_LE_BITS &CRC_LE_BITS -1 +# error "CRC_LE_BITS must be one of {1, 2, 4, 8, 32, 64}"#endif /* * Big-endian CRC computation. Used with serial bit streams sent * msbit-first. Be sure to use cpu_to_be32() to append the computed CRC. */ -#if CRC_BE_BITS >8 ||CRC_BE_BITS <1 ||CRC_BE_BITS &CRC_BE_BITS -1 -# error CRC_BE_BITS must be a power of 2 between 1 and 8 +#if CRC_BE_BITS >64 ||CRC_BE_BITS <1 ||CRC_BE_BITS ==16 || \ +CRC_BE_BITS &CRC_BE_BITS -1 +# error "CRC_BE_BITS must be one of {1, 2, 4, 8, 32, 64}"#endif
gen_crc32table.c +41 -18
110 lines not on this page · raw message
(I don't think [email protected] exists any more)
Quoted 28 lines · On Wed, 20 Jul 2011 17:19:42 -0500 frank zago <[email protected]>…
Hello,
Added support for slice by 8 to existing crc32 algorithm. Also modified
gen_crc32table.cto only produce table entries that are actually used. The parametersCRC_LE_BITSandCRC_BE_BITSdetermine the number of bits in the input array that are processed during each step. Generally the more bits the faster the algorithm is but the more table data required.Using an
x86_64Opteron machine running at 2100MHz the following table was collected with a pre-warmed cache by computing the crc 1000 times on a buffer of 4096 bytes.BITS Size LE Cycles/byte BE Cycles/byte
----------------------------------------------
1 873 41.65 34.60
2 1097 25.43 29.61
4 1057 13.29 15.28
8 2913 7.13 8.19
32 9684 2.80 2.82
64 18178 1.53 1.53BITS is the value of
CRC_LE_BITSorCRC_BE_BITS. The old default was 8 which actually selected the 32 bit algorithm. In this version the value 8 is used to select the standard 8 bit algorithm and two new values: 32 and 64 are introduced to select the slice by 4 and slice by 8 algorithms respectively.
Quoted 7 lines
Where Size is the size of crc32.o's text segment which includes code and table data when both LE and BE versions are set to BITS.
The current version of
crc32.cby default uses the slice by 4 algorithm which requires about 2.8 cycles per byte. The slice by 8 algorithm is roughly 2X faster and enables packet processing at over 1GB/sec on a typical 2-3GHz system.
Are you sure that the code doesn't add any unaligned accesses? Those are very bad on some non-x86 architectures.
crc32.c
Quoted 2 lines
crc32.c +0
-0
Please prepare patches in `patch -p1' form. So that should have been
--- a/lib/crc32.c
+++ a/lib/crc32.c
crc32.c line 35
Quoted 11 lines
...
crc32.c +2
-2@@ -28,14 +31,17 @@#include <linux/init.h> #include <asm/atomic.h> #include "crc32defs.h" -#if CRC_LE_BITS ==8 -# define tole (x) __constant_cpu_to_le32(x) + +#include <asm/msr.h>
That breaks the build on all non-x86. Fortunately the inclusion appears to be unnecessary.
Quoted 14 lines
crc32.c +4
-2+#if CRC_LE_BITS >8 +# define tole (x) (__force u32) __constant_cpu_to_le32(x)#else # define tole (x) (x)#endif -#if CRC_BE_BITS ==8 -# define tobe (x) __constant_cpu_to_be32(x) +#if CRC_BE_BITS >8 +# define tobe (x) (__force u32) __constant_cpu_to_be32(x)#else # define tobe (x) (x)#endif
crc32.c MODULE_AUTHOR("Matt Domsch <Matt_Domsch@ · line 55
Quoted 6 lines
crc32.c +2
-1@@ -45,54 +51,228 @@ MODULE_AUTHOR("Matt Domsch <Matt_Domsch@MODULE_DESCRIPTION ("Ethernet CRC32 calculations" );MODULE_LICENSE ("GPL" ); -#if CRC_LE_BITS ==8 ||CRC_BE_BITS ==8 +#if CRC_LE_BITS >8 +static inline u32 crc32_le_body (u32 crc,u8 const *buf,size_t len)
`inline' is largely ignored by gcc, with gcc-version dependencies. __always_inline is more likely to succeed. But the compiler would inline this all by itself anyway.
Quoted 4 lines
crc32.c +4
-0+{ +const u8 *p8; +const u32 *p32; +int init_bytes, end_bytes;
crc32.c MODULE_AUTHOR("Matt Domsch <Matt_Domsch@ · line 69
Quoted 10 lines
crc32.c +10
-0+size_t words; +int i; +u32 q; +u8 i0, i1, i2, i3; + + crc = (__force u32 )__cpu_to_le32 (crc); + +#if CRC_LE_BITS ==64 + p8 = buf; + p32 = (u32 *)(((uintptr_t )p8 +7 ) & ~7 );
Perhaps using ALIGN() would be clearer.
crc32.c MODULE_AUTHOR("Matt Domsch <Matt_Domsch@ · line 71
Quoted 2 lines
crc32.c +2
-0+ + init_bytes = (uintptr_t )p32 - (uintptr_t )p8;
Please take a look at the types of init_bytes and end_bytes. ptrdiff_t, size_t and uint would all eb more appropriate than `int'.
crc32.c MODULE_AUTHOR("Matt Domsch <Matt_Domsch@ · line 78
Quoted 7 lines
crc32.c +7
-0+if (init_bytes > len) + init_bytes = len; + words = (len - init_bytes) >>3 ; + end_bytes = (len - init_bytes) &7 ; +#else + p8 = buf; + p32 = (u32 *)(((uintptr_t )p8 +3 ) & ~3 );
ALIGN()?
crc32.c MODULE_AUTHOR("Matt Domsch <Matt_Domsch@ · line 73
Quoted 3 lines
crc32.c +3
-0+ init_bytes = (uintptr_t )p32 - (uintptr_t )p8; +if (init_bytes > len) + init_bytes = len;
max()?
crc32.c MODULE_AUTHOR("Matt Domsch <Matt_Domsch@ · line 95
Quoted 13 lines
crc32.c +13
-0+ words = (len - init_bytes) >>2 ; + end_bytes = (len - init_bytes) &3 ; +#endif + +for (i =0 ; i < init_bytes; i++) { +#ifdef __LITTLE_ENDIAN + i0 = *p8++ ^ crc; + crc = t0_le[i0] ^ (crc >>8 ); +#else + i0 = *p8++ ^ (crc >>24 ); + crc = t0_le[i0] ^ (crc <<8 ); +#endif + }
All the ifdeffing in here bursts my brain.
Has this code been carefully tested in LE and BE?
crc32.c MODULE_AUTHOR("Matt Domsch <Matt_Domsch@ · line 75
Quoted 12 lines
...
crc32.c +9
-0+#if CRC_LE_BITS ==64 + p8 = buf; + p32 = (u32 *)(((uintptr_t )p8 +7 ) & ~7 ); + + init_bytes = (uintptr_t )p32 - (uintptr_t )p8; +if (init_bytes > len) + init_bytes = len; + words = (len - init_bytes) >>3 ; + end_bytes = (len - init_bytes) &7 ;
Various dittoes.
Quoted 21 lines
crc32.c +20
-0+#else + p8 = buf; + p32 = (u32 *)(((uintptr_t )p8 +3 ) & ~3 ); + + init_bytes = (uintptr_t )p32 - (uintptr_t )p8; +if (init_bytes > len) + init_bytes = len; + words = (len - init_bytes) >>2 ; + end_bytes = (len - init_bytes) &3 ; +#endif + +for (i =0 ; i < init_bytes; i++) { +#ifdef __LITTLE_ENDIAN + i0 = *p8++ ^ crc; + crc = t0_be[i0] ^ (crc >>8 ); +#else + i0 = *p8++ ^ (crc >>24 ); + crc = t0_be[i0] ^ (crc <<8 ); +#endif + }
Hi Andrew,
A few questions/comments below that will help resending the patch.
Thanks,
Bob Pearson
Quoted 3 lines
Are you sure that the code doesn't add any unaligned accesses? Those are very bad on some non-x86 architectures.
Agreed. P8 is byte aligned. P32 is 32/64 bit aligned for the 32/64 bit algorithms. I will try to make it clearer.
Quoted 5 lines
crc32.c
Quoted 3 lines
crc32.c +0
-0Please prepare patches in `patch -p1' form. So that should have been
OK
Quoted 7 lines
lib/crc32.c +0
-0Quoted 4 lines
...
Quoted 10 lines
crc32.c line 35
Quoted 8 lines
crc32.c +2
-2@@ -28,14 +31,17 @@#include <linux/init.h> #include <asm/atomic.h> #include "crc32defs.h" -#if CRC_LE_BITS ==8 -# define tole (x) __constant_cpu_to_le32(x) + +#include <asm/msr.h> That breaks the build on all non-x86. Fortunately the inclusion appears
to be
Quoted 1 line
unnecessary.
Oops! Was part of some performance measurement code that got taken out.
Quoted 1 line
General comment. The use of le/be in this and the previous version of crc32.c is confusing because it does not refer to little/big endian cpu architecture but rather to the arbitrary choice made by different protocols as to whether the bits in a byte are presented to the CRC in little/big *BIT* order. Some protocols (e.g. Ethernet, InfiniBand) process the bits starting from the LSB and some (e.g. atm) process the bits in MSB order. These routines (crc32_le and crc32_be) compute the CRC as a u32 in cpu byte order. The caller has to then get the CRC into the correct order for inclusion into the message. As a result there are four versions of the calculation:
BE bit order on BE byte order cpu, BE bit order on LE byte order cpu, etc.
Some calculations are simplified by arranging the bits sequentially in WORDS when we are going to process them in a certain order within each byte. This means that the tables used to compute crc32_be are easier to use in BE byte order and that tables used to compute crc32_le are easier to use in LE byte order. That is the logic behind the following weird looking macros which are kept the same as the previous version except for the inclusion of __force to pass sparse with -D__CHECK_ENDIAN__.
Quoted 6 lines
Quoted 5 lines
crc32.c +2
-0+#if CRC_LE_BITS >8 +# define tole (x) (__force u32) __constant_cpu_to_le32(x)#else # define tole (x) (x)#endif
Quoted 6 lines
crc32.c MODULE_AUTHOR("Matt Domsch <Matt_Domsch@ · line 55
Quoted 3 lines
crc32.c +2
-1-#if CRC_LE_BITS ==8 ||CRC_BE_BITS ==8 +#if CRC_LE_BITS >8 +static inline u32 crc32_le_body (u32 crc,u8 const *buf,size_t len)`inline' is largely ignored by gcc, with gcc-version dependencies.
__always_inlineis more likely to succeed. But the compiler would inline
this
Quoted 1 line
all by itself anyway.
Agreed. Inherited from previous version and happy to delete it.
Quoted 3 lines
crc32.c line 43
Quoted 1 line
crc32.c +1
-0+ p32 = (u32 *)(((uintptr_t )p8 +7 ) & ~7 );Perhaps using
ALIGN()would be clearer.
OK by me. Its cleaner.
Quoted 6 lines
crc32.c line 45
Quoted 3 lines
crc32.c +2
-0+ + init_bytes = (uintptr_t )p32 - (uintptr_t )p8;Please take a look at the types of
init_bytesandend_bytes.ptrdiff_t,size_tand uint would all eb more appropriate than `int'.
Is there a performance difference on 32 bit machines from using a long to hold something that is in the range [0,7]? Which of these would you recommend?
Quoted 3 lines
crc32.c line 46
Quoted 1 line
crc32.c +1
-0+ p32 = (u32 *)(((uintptr_t )p8 +3 ) & ~3 );
ALIGN()?
Sure.
Quoted 6 lines
crc32.c MODULE_AUTHOR("Matt Domsch <Matt_Domsch@ · line 73
Quoted 4 lines
crc32.c +3
-0+ init_bytes = (uintptr_t )p32 - (uintptr_t )p8; +if (init_bytes > len) + init_bytes = len;
max()?
Maybe. First line is the main thought and the if handles the exception where the unrolled loop doesn't have a middle. Happy to oblige though. How about an unlikely in the if?
Quoted 11 lines
crc32.c MODULE_AUTHOR("Matt Domsch <Matt_Domsch@ · line 95
Quoted 9 lines
crc32.c +9
-0+for (i =0 ; i < init_bytes; i++) { +#ifdef __LITTLE_ENDIAN + i0 = *p8++ ^ crc; + crc = t0_le[i0] ^ (crc >>8 ); +#else + i0 = *p8++ ^ (crc >>24 ); + crc = t0_le[i0] ^ (crc <<8 ); +#endif + }All the ifdeffing in here bursts my brain.
Agreed. But this is basically the same as the original code except that the #ifdefs were buried in the macros. I couldn't figure out how to save the CRC4 macros and for consistent style decided to get rid of all of them. I'm not sure how to avoid it except to replicate the subroutines 4X.
Quoted 2 lines
Has this code been carefully tested in LE and BE?
Yes. My Big Endian test machine collection just consists of an old Ultra Sparc machine. But we have 64 and 32 bit X86 systems. The patch adds one new option to an existing collection of different ways to compute each CRC, which have not really changed. In particular the 1 bit versions are identical by inspection to the original. That allows us to compare the results for different inputs and optional versions of the algorithm and of course the unpatched file.
One change to the original was the removal of the 2D arrays in the table and replacement with 1D arrays. I have the opinion that this makes the inner loop shorter but to do this required making two copies of the 'body' subroutine. Perhaps someone can comment on this. I have looked and I can't see any way to shorten the code in the slice by 8 code from what gcc does but almost anything that I do to the loop makes it longer.
I have doubts about the need to have 6 versions with different trade-offs between table size and speed but I was afraid to make radical changes to the original code. People who make small embedded systems probably care a lot but I don't have a good feel for the issue.
Quoted 1 line
I left in the unit test and documentation without change but from the looks of it no one has run it for a while and I have doubts about whether it works anymore. What do you think about moving the documentation to Documentation and ditching the unit tests?
Quoted 2 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the
Quoted 3 lines
body of a message to [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Quoted 10 lines · On Thu, 28 Jul 2011 20:47:36 -0500 "Bob Pearson"…
Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ + init_bytes = (uintptr_t)p32 - (uintptr_t)p8;Please take a look at the types of
init_bytesandend_bytes.ptrdiff_t,size_tand uint would all eb more appropriate than `int'.Is there a performance difference on 32 bit machines from using a long to hold something that is in the range [0,7]? Which of these would you recommend?
I'm not aware of 64-bit being any faster than 32-bit on any machine. But I'm not aware of much.
Making this a signed quantity was inappropriate. unsigned int, perhaps.
IMO, unsigned should have been the default in C - negative numbers are relatively rare. Whatever.
Quoted 13 lines
Quoted 3 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ p32 = (u32 *)(((uintptr_t)p8 + 3) & ~3);
ALIGN()?Sure.
Quoted 6 lines
crc32.c MODULE_AUTHOR("Matt Domsch <Matt_Domsch@ · line 73
Quoted 4 lines
crc32.c +3
-0+ init_bytes = (uintptr_t )p32 - (uintptr_t )p8; +if (init_bytes > len) + init_bytes = len;
max()?
Quoted 3 lines
Maybe. First line is the main thought and the if handles the exception where the unrolled loop doesn't have a middle. Happy to oblige though. How about an unlikely in the if?
Don't care much ;) It's just that the reader looks at those two lines, absorbs them then says "ah, it's taking the minimum". If it had just used "min()" then review and reading are more efficient, and reliable. Plus min() has typechecking features which can catch slipups.