On Fri, Jan 23, 2015 at 10:39 AM, Ard Biesheuvel <ard.biesheu...@linaro.org> wrote: > On 23 January 2015 at 16:28, Steve Capper <steve.cap...@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.ghan...@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 _______________________________________________ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
