lib/raid6: Build proper files on corresponding arch
PATCH v5 1/2 lib/raid6: Build proper files on corresponding arch
Previously the raid6 test Makefile did not correctly build the files for testing on PowerPC. This patch fixes the bug, so that all appropriate files for PowerPC are built.
This patch also fixes the missing and mismatched ifdef statements to allow the altivec.uc file to be built correctly.
---
Changelog
v5
- moved altivec.uc fix into this patch
- updates commit message
--- lib/raid6/altivec.uc | 3 +++ lib/raid6/test/Makefile | 8 +++++--- 2 files changed, 8 insertions(+), 3 deletions(-)
lib/raid6/altivec.uc +3 -0
@@ -24,10 +24,13 @@ #include <linux/raid/pq.h> +#ifdef CONFIG_ALTIVEC + #include <altivec.h> #ifdef __KERNEL__ # include <asm/cputable.h> # include <asm/switch_to.h> +#endif /* __KERNEL__ */ /* * This is the C data type to use. We use a vector of
lib/raid6/test/Makefile +5 -3
@@ -44,10 +44,12 @@ else ifeq ($(HAS_NEON),yes) CFLAGS += -DCONFIG_KERNEL_MODE_NEON=1 else HAS_ALTIVEC := $(shell printf '\#include <altivec.h>\nvector int a;\n' |\ - gcc -c -x c - >&/dev/null && \ - rm ./-.o && echo yes) + gcc -c -x c - >/dev/null && rm ./-.o && echo yes) ifeq ($(HAS_ALTIVEC),yes) - OBJS += altivec1.o altivec2.o altivec4.o altivec8.o + CFLAGS += -I../../../arch/powerpc/include + CFLAGS += -DCONFIG_ALTIVEC + OBJS += altivec1.o altivec2.o altivec4.o altivec8.o \ + vpermxor1.o vpermxor2.o vpermxor4.o vpermxor8.o endif endif ifeq ($(ARCH),tilegx)
Signature · 1 line
2.9.3
PATCH v5 2/2 raid6/altivec: Add vpermxor implementation for raid6 Q syndrome
The raid6 Q syndrome check has been optimised using the vpermxor instruction. This instruction was made available with POWER8, ISA version 2.07. It allows for both vperm and vxor instructions to be done in a single instruction. This has been tested for correctness on a ppc64le vm with a basic RAID6 setup containing 5 drives.
The performance benchmarks are from the raid6test in the /lib/raid6/test directory. These results are from an IBM Firestone machine with ppc64le architecture. The benchmark results show a 35% speed increase over the best existing algorithm for powerpc (altivec). The raid6test has also been run on a big-endian ppc64 vm to ensure it also works for big-endian architectures.
Performance benchmarks:
raid6: altivecx4 gen() 18773 MB/s raid6: altivecx8 gen() 19438 MB/s raid6: vpermxor4 gen() 25112 MB/s raid6: vpermxor8 gen() 26279 MB/s
---
Changelog
v5
- moved altivec.uc fix into other patch in series
--- include/linux/raid/pq.h | 4 ++ lib/raid6/Makefile | 27 ++++++++++++- lib/raid6/algos.c | 4 ++ lib/raid6/test/Makefile | 14 ++++++- lib/raid6/vpermxor.uc | 104 ++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 151 insertions(+), 2 deletions(-) create mode 100644 lib/raid6/vpermxor.uc
include/linux/raid/pq.h +4 -0
@@ -107,6 +107,10 @@ extern const struct raid6_calls raid6_avx512x2;extern const struct raid6_calls raid6_avx512x4;extern const struct raid6_calls raid6_tilegx8;extern const struct raid6_calls raid6_s390vx8; +extern const struct raid6_calls raid6_vpermxor1; +extern const struct raid6_calls raid6_vpermxor2; +extern const struct raid6_calls raid6_vpermxor4; +extern const struct raid6_calls raid6_vpermxor8;struct raid6_recov_calls {void (*data2)(int ,size_t ,int ,int ,void **);
lib/raid6/Makefile +26 -1
@@ -4,7 +4,8 @@ raid6_pq-y += algos.o recov.o tables.o int1.o int2.o int4.o \ int8.o int16.o int32.o raid6_pq-$(CONFIG_X86) += recov_ssse3.o recov_avx2.o mmx.o sse1.o sse2.o avx2.o avx512.o recov_avx512.o -raid6_pq-$(CONFIG_ALTIVEC) += altivec1.o altivec2.o altivec4.o altivec8.o +raid6_pq-$(CONFIG_ALTIVEC) += altivec1.o altivec2.o altivec4.o altivec8.o \ + vpermxor1.o vpermxor2.o vpermxor4.o vpermxor8.o raid6_pq-$(CONFIG_KERNEL_MODE_NEON) += neon.o neon1.o neon2.o neon4.o neon8.o raid6_pq-$(CONFIG_TILEGX) += tilegx8.o raid6_pq-$(CONFIG_S390) += s390vx8.o recov_s390xc.o
@@ -88,6 +89,30 @@ $(obj)/altivec8.c: UNROLL := 8 $(obj)/altivec8.c: $(src)/altivec.uc $(src)/unroll.awk FORCE $(call if_changed,unroll) +CFLAGS_vpermxor1.o += $(altivec_flags) +targets += vpermxor1.c +$(obj)/vpermxor1.c: UNROLL := 1 +$(obj)/vpermxor1.c: $(src)/vpermxor.uc $(src)/unroll.awk FORCE + $(call if_changed,unroll) + +CFLAGS_vpermxor2.o += $(altivec_flags) +targets += vpermxor2.c +$(obj)/vpermxor2.c: UNROLL := 2 +$(obj)/vpermxor2.c: $(src)/vpermxor.uc $(src)/unroll.awk FORCE + $(call if_changed,unroll) + +CFLAGS_vpermxor4.o += $(altivec_flags) +targets += vpermxor4.c +$(obj)/vpermxor4.c: UNROLL := 4 +$(obj)/vpermxor4.c: $(src)/vpermxor.uc $(src)/unroll.awk FORCE + $(call if_changed,unroll) + +CFLAGS_vpermxor8.o += $(altivec_flags) +targets += vpermxor8.c +$(obj)/vpermxor8.c: UNROLL := 8 +$(obj)/vpermxor8.c: $(src)/vpermxor.uc $(src)/unroll.awk FORCE + $(call if_changed,unroll) + CFLAGS_neon1.o += $(NEON_FLAGS) targets += neon1.c $(obj)/neon1.c: UNROLL := 1
lib/raid6/algos.c +4 -0
@@ -74,6 +74,10 @@ const struct raid6_calls * const raid6_algos[] = { &raid6_altivec2, &raid6_altivec4, &raid6_altivec8, + &raid6_vpermxor1, + &raid6_vpermxor2, + &raid6_vpermxor4, + &raid6_vpermxor8, #endif #if defined(CONFIG_TILEGX) &raid6_tilegx8,
lib/raid6/test/Makefile +13 -1
@@ -97,6 +97,18 @@ altivec4.c: altivec.uc ../unroll.awk altivec8.c: altivec.uc ../unroll.awk $(AWK) ../unroll.awk -vN=8 < altivec.uc > $@ +vpermxor1.c: vpermxor.uc ../unroll.awk + $(AWK) ../unroll.awk -vN=1 < vpermxor.uc > $@ + +vpermxor2.c: vpermxor.uc ../unroll.awk + $(AWK) ../unroll.awk -vN=2 < vpermxor.uc > $@ + +vpermxor4.c: vpermxor.uc ../unroll.awk + $(AWK) ../unroll.awk -vN=4 < vpermxor.uc > $@ + +vpermxor8.c: vpermxor.uc ../unroll.awk + $(AWK) ../unroll.awk -vN=8 < vpermxor.uc > $@ + int1.c: int.uc ../unroll.awk $(AWK) ../unroll.awk -vN=1 < int.uc > $@
@@ -122,7 +134,7 @@ tables.c: mktables ./mktables > tables.c clean: - rm -f *.o *.a mktables mktables.c *.uc int*.c altivec*.c neon*.c tables.c raid6test + rm -f *.o *.a mktables mktables.c *.uc int*.c altivec*.c vpermxor*.c neon*.c tables.c raid6test rm -f tilegx*.c spotless: clean
lib/raid6/vpermxor.uc new file +104 -0
104 lines not on this page · raw message
Signature · 1 line
2.9.3
Re: [v5 2/2] raid6/altivec: Add vpermxor implementation for raid6 Q syndrome
Hi Matt,
Quoted 2 lines
The raid6 Q syndrome check has been optimised using the vpermxor instruction.
Very much a nit, but normally we'd write the change that the patch makes as a command: "Optimise the raid6 Q syndrome generation using the vpermxor instruction" - see
https://www.kernel.org/doc/html/v4.11/process/submitting-patches.html#describe-your-changes
Quoted 12 lines
lib/raid6/vpermxor.uc new file +12
-0+static void noinline raid6_vpermxor$#_gen_syndrome_real(int disks, size_t bytes, + void **ptrs) +{ + u8 **dptr = (u8 **)ptrs; + u8 *p, *q; + int d, z, z0; + unative_t wp$$, wq$$, wd$$; + + z0 = disks - 3; /* Highest data disk */ + p = dptr[z0+1]; /* XOR parity */ + q = dptr[z0+2]; /* RS syndrome */ +
lib/raid6/vpermxor.uc line 63
Quoted 10 lines
lib/raid6/vpermxor.uc new file +10
-0+ for (d = 0; d < bytes; d += NSIZE*$#) { + wp$$ = wq$$ = *(unative_t *)&dptr[z0][d+$$*NSIZE]; + + for (z = z0-1; z>=0; z--) { + wd$$ = *(unative_t *)&dptr[z][d+$$*NSIZE]; + /* P syndrome */ + wp$$ = vec_xor(wp$$, wd$$); + + /*Q syndrome */ + asm("vpermxor %0,%1,%2,%3":"=v"(wq$$):"v"(gf_high), "v"(gf_low), "v"(wq$$));
Initially I thought "why can't we break this over 2 lines?" and then I remembered that the awk script can't handle that. A space between /* and Q would be good though.
lib/raid6/vpermxor.uc line 64
Quoted 1 line
lib/raid6/vpermxor.uc new file +1
-0+ wq$$ = vec_xor(wq$$, wd$$);
I generated some of the unrolled code and inspected it. It's non-trivial to follow but that's justifiable, it's due to:
- the complex maths
- the unrolling process
- consistency with the altivec code, which I think is worth keeping
I am not sure how you could make it any easier to read, so I don't think that should block its acceptance into the kernel.
I am confident that this code works correctly and as described.
Regards,
Daniel
Quoted 2 lines
Signature · 1 line
2.9.3
Re: [v5 2/2] raid6/altivec: Add vpermxor implementation for raid6 Q syndrome
Quoted 9 lines · Daniel Axtens
Hi Matt,
Quoted 2 lines
The raid6 Q syndrome check has been optimised using the vpermxor instruction.
Very much a nit, but normally we'd write the change that the patch makes as a command: "Optimise the raid6 Q syndrome generation using the vpermxor instruction" - see
https://www.kernel.org/doc/html/v4.11/process/submitting-patches.html#describe-your-changes
There's a good list here:
https://chris.beams.io/posts/git-commit/
Which includes "Use the imperative mood in the subject line".
And has a good rule of thumb:
A properly formed Git commit subject line should always be able to complete the following sentence:
If applied, this commit will [your subject line here]
In this case Matt's subject is fine, but IMHO you should also use the imperative mood for the body of the change log - which is basically what you said :)
cheers
Re: [v5 2/2] raid6/altivec: Add vpermxor implementation for raid6 Q syndrome
In reply to Matt BrownOh, one final thing - I just realised there's a .gitignore file in lib/raid6/.gitignore that needs to be updated to include the vpermxor generated files. That should be part of this patch.
Regards,
Daniel
Re: [v5 2/2] raid6/altivec: Add vpermxor implementation for raid6 Q syndrome
Quoted 4 lines · Daniel Axtens
Oh, one final thing - I just realised there's a .gitignore file in
lib/raid6/.gitignorethat needs to be updated to include the vpermxor generated files. That should be part of this patch.
Oh, I managed to miss that!
I'll add that and fix up that comment spacing.
Thanks,
Matt
Quoted 2 lines
Regards,
Daniel
Hi Matt,
Quoted 11 lines
lib/raid6/test/Makefile +1
-2@@ -44,10 +44,12 @@ else ifeq ($(HAS_NEON),yes) CFLAGS += -DCONFIG_KERNEL_MODE_NEON=1 else HAS_ALTIVEC := $(shell printf '\#include <altivec.h>\nvector int a;\n' |\ - gcc -c -x c - >&/dev/null && \ - rm ./-.o && echo yes) + gcc -c -x c - >/dev/null && rm ./-.o && echo yes)From memory the change here (s/>&/>/) was necessary to get the build to
succeed - did we ever figure out why that was? I'm not enough of a shell guru to grok the difference. If it's easy to explain it would be good to put it in the commit message, rather than just saying you fixed an unspecified bug.
lib/raid6/test/Makefile else ifeq ($(HAS_NEON),yes) · line 52
Quoted 6 lines
ifeq ($(HAS_ALTIVEC),yes)lib/raid6/test/Makefile +4
-1- OBJS += altivec1.o altivec2.o altivec4.o altivec8.o + CFLAGS += -I../../../arch/powerpc/include + CFLAGS += -DCONFIG_ALTIVEC + OBJS += altivec1.o altivec2.o altivec4.o altivec8.o \ + vpermxor1.o vpermxor2.o vpermxor4.o vpermxor8.o
You've added vpermxor here, but you don't define them until the next patch, so the tests will fail. Please move the change to OBJS to the next patch.
With that change, I'd be happy to formally Review this patch.
Regards,
Daniel
Quoted 5 lines
endif
endif
ifeq ($(ARCH),tilegx)Signature · 1 line
2.9.3
Quoted 15 lines · Daniel Axtens
Hi Matt,
lib/raid6/test/Makefile else ifeq ($(HAS_NEON),yes) · line 47
Quoted 9 lines
lib/raid6/test/Makefile +1
-2@@ -44,10 +44,12 @@ else ifeq ($(HAS_NEON),yes) CFLAGS += -DCONFIG_KERNEL_MODE_NEON=1 else HAS_ALTIVEC := $(shell printf '\#include <altivec.h>\nvector int a;\n' |\ - gcc -c -x c - >&/dev/null && \ - rm ./-.o && echo yes) + gcc -c -x c - >/dev/null && rm ./-.o && echo yes)From memory the change here (s/>&/>/) was necessary to get the build to succeed - did we ever figure out why that was? I'm not enough of a shell guru to grok the difference.
Using >& redirects stdout and stderr, whereas > only redirects stdout.
So possibly it doesn't fix anything, but rather lets you see any error emitted by the compiler rather than swallowing it?
cheers
Quoted 18 lines · Michael Ellerman
Quoted 15 lines · Daniel Axtens
Hi Matt,
lib/raid6/test/Makefile else ifeq ($(HAS_NEON),yes) · line 47
Quoted 9 lines
lib/raid6/test/Makefile +1
-2@@ -44,10 +44,12 @@ else ifeq ($(HAS_NEON),yes) CFLAGS += -DCONFIG_KERNEL_MODE_NEON=1 else HAS_ALTIVEC := $(shell printf '\#include <altivec.h>\nvector int a;\n' |\ - gcc -c -x c - >&/dev/null && \ - rm ./-.o && echo yes) + gcc -c -x c - >/dev/null && rm ./-.o && echo yes)From memory the change here (s/>&/>/) was necessary to get the build to succeed - did we ever figure out why that was? I'm not enough of a shell guru to grok the difference.
Quoted 5 lines
Using >& redirects stdout and stderr, whereas > only redirects stdout.
So possibly it doesn't fix anything, but rather lets you see any error emitted by the compiler rather than swallowing it?
Just had to double-check what the problem was.
The bug was that none of the ppc specific files were being built. I'm not entirely sure how, but this fixes it so the altivec and vpermxor files are built.
I'll fix up the commit message and move the vpermxor make defs into the other patch.
Thanks,
Matt
Quoted 1 line
cheers