On Fri, Jan 23, 2015 at 10:39 AM, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On 23 January 2015 at 16:28, Steve Capper steve.capper@linaro.org wrote:
Hi Yazen,
This is looking good, just a few minor comments below.
Cheers,
Steve
On Fri, Jan 23, 2015 at 09:13:42AM -0600, Yazen Ghannam wrote:
ARMv8 defines a set of optional CRC32/CRC32C instructions. This patch defines an optimized function that uses these instructions when available rather than table-based lookup. Optimized function based on a Hadoop patch by Ed Nevill.
Autotools updated to check for compiler support. Optimized function is selected at runtime based on HWCAP_CRC32. Added crc32c "performance" unit test and arch unit test.
Tested on AMD Seattle. Passes all crc32c unit tests. Unit test shows ~4x performance increase versus sctp.
Signed-off-by: Yazen Ghannam yazen.ghannam@linaro.org
configure.ac | 1 + m4/ax_arm.m4 | 18 +++++++++++-- src/arch/arm.c | 2 ++ src/arch/arm.h | 1 + src/common/Makefile.am | 10 +++++++- src/common/crc32c.cc | 6 +++++ src/common/crc32c_aarch64.c | 58 ++++++++++++++++++++++++++++++++++++++++++ src/common/crc32c_aarch64.h | 25 ++++++++++++++++++ src/test/common/test_crc32c.cc | 11 ++++++++ src/test/test_arch.cc | 3 +++ 10 files changed, 132 insertions(+), 3 deletions(-) create mode 100644 src/common/crc32c_aarch64.c create mode 100644 src/common/crc32c_aarch64.h
[...]
diff --git a/src/test/test_arch.cc b/src/test/test_arch.cc index b129262..27cdd1d 100644 --- a/src/test/test_arch.cc +++ b/src/test/test_arch.cc @@ -50,6 +50,9 @@ TEST(Arch, all) expected = (strstr(flags, " neon ") || strstr(flags, " asimd ")) ? 1 : 0; EXPECT_EQ(expected, ceph_arch_neon);
- expected = strstr(flags, " crc32 ") ? 1 : 0;
- EXPECT_EQ(expected, ceph_arch_aarch64_crc32);
The test_arch.cc file looks very dangerous to me, it's strstr scanning for CPU flags from /proc/cpuinfo with no regard for architecture. (It is quite likely another chip in future introduces the crc32 CPU flag).
I see it's trying to sanity check that the HW_CAPS logic matches up with the cpuflags. Could you please surround the crc32 check with:
#ifdef __arch64__ #endif /* __arch64__ */
Just to pin it down to ARM.
Any way we could use getauxval(AT_HWCAP) here? It has been around since before AArch64 glibc support stabilised, afaik, and parsing /proc/cpuinfo is one of the things we really get rid of. (Perhaps in a separate patch)
-- Ard.
This is already done when the system is probed (ceph_arch_probe()). It seems that this unit test is testing that ceph_arch_probe() works by comparing the results with what is in /proc/cpuinfo.
I'll be taking Steve's suggestion a bit further and adding #ifdef for all the archs.
Yazen