| Message ID | 20230911-optimize_checksum-v4-4-77cc2ad9e9d7@rivosinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: Add fine-tuned checksum functions | expand |
On Mon, Sep 11, 2023 at 03:57:14PM -0700, Charlie Jenkins wrote: > This patch is not ready for merge as vector support in the kernel is > limited. However, the code has been tested in QEMU so the algorithms > do work. This code requires the kernel to be compiled with C vector > support, but that is not yet possible. It is written in assembly > rather than using the GCC vector instrinsics because they did not > provide optimal code. > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > --- > arch/riscv/lib/csum.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 92 insertions(+) > > diff --git a/arch/riscv/lib/csum.c b/arch/riscv/lib/csum.c > index 47d98c51bab2..eb4596fc7f5b 100644 > --- a/arch/riscv/lib/csum.c > +++ b/arch/riscv/lib/csum.c > @@ -12,6 +12,10 @@ > > #include <net/checksum.h> > > +#ifdef CONFIG_RISCV_ISA_V > +#include <riscv_vector.h> What actually includes this header, I don't see it in either Andy's in-kernel vector series or Bjorn's blake2 one. Can you link to the pre-requisites in your cover letter please. Thanks, Conor.
On Thu, Sep 14, 2023 at 01:46:29PM +0100, Conor Dooley wrote: > On Mon, Sep 11, 2023 at 03:57:14PM -0700, Charlie Jenkins wrote: > > This patch is not ready for merge as vector support in the kernel is > > limited. However, the code has been tested in QEMU so the algorithms > > do work. This code requires the kernel to be compiled with C vector > > support, but that is not yet possible. It is written in assembly > > rather than using the GCC vector instrinsics because they did not > > provide optimal code. > > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > > --- > > arch/riscv/lib/csum.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 92 insertions(+) > > > > diff --git a/arch/riscv/lib/csum.c b/arch/riscv/lib/csum.c > > index 47d98c51bab2..eb4596fc7f5b 100644 > > --- a/arch/riscv/lib/csum.c > > +++ b/arch/riscv/lib/csum.c > > @@ -12,6 +12,10 @@ > > > > #include <net/checksum.h> > > > > +#ifdef CONFIG_RISCV_ISA_V > > +#include <riscv_vector.h> > > What actually includes this header, I don't see it in either Andy's > in-kernel vector series or Bjorn's blake2 one. > Can you link to the pre-requisites in your cover letter please. > > Thanks, > Conor. It is defined here: https://github.com/riscv-non-isa/rvv-intrinsic-doc/blob/main/doc/rvv-intrinsic-spec.adoc. The header is for the vector intrinsics that are supported by llvm and gcc. - Charlie
On Thu, Sep 14, 2023 at 12:14:16PM -0400, Charlie Jenkins wrote: > On Thu, Sep 14, 2023 at 01:46:29PM +0100, Conor Dooley wrote: > > On Mon, Sep 11, 2023 at 03:57:14PM -0700, Charlie Jenkins wrote: > > > This patch is not ready for merge as vector support in the kernel is > > > limited. However, the code has been tested in QEMU so the algorithms > > > do work. This code requires the kernel to be compiled with C vector > > > support, but that is not yet possible. It is written in assembly > > > rather than using the GCC vector instrinsics because they did not > > > provide optimal code. > > > > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > > > --- > > > arch/riscv/lib/csum.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 92 insertions(+) > > > > > > diff --git a/arch/riscv/lib/csum.c b/arch/riscv/lib/csum.c > > > index 47d98c51bab2..eb4596fc7f5b 100644 > > > --- a/arch/riscv/lib/csum.c > > > +++ b/arch/riscv/lib/csum.c > > > @@ -12,6 +12,10 @@ > > > > > > #include <net/checksum.h> > > > > > > +#ifdef CONFIG_RISCV_ISA_V > > > +#include <riscv_vector.h> > > > > What actually includes this header, I don't see it in either Andy's > > in-kernel vector series or Bjorn's blake2 one. > > Can you link to the pre-requisites in your cover letter please. > > > > Thanks, > > Conor. > > It is defined here: > https://github.com/riscv-non-isa/rvv-intrinsic-doc/blob/main/doc/rvv-intrinsic-spec.adoc. > The header is for the vector intrinsics that are supported by llvm and > gcc. Well, whatever you're doing with it does not work, producing 3600 or so fatal errors during compilation, all saying: ../arch/riscv/include/asm/checksum.h:14:10: fatal error: riscv_vector.h: No such file or directory Do you have some makefile hack somewhere that's not part of this patchset? Also, I'm dumb, but can you show me where are the actual intrinsics are being used in this patch anyway? I just seem some types & asm. Thanks, Conor.
On Thu, Sep 14, 2023 at 05:29:29PM +0100, Conor Dooley wrote: > On Thu, Sep 14, 2023 at 12:14:16PM -0400, Charlie Jenkins wrote: > > On Thu, Sep 14, 2023 at 01:46:29PM +0100, Conor Dooley wrote: > > > On Mon, Sep 11, 2023 at 03:57:14PM -0700, Charlie Jenkins wrote: > > > > This patch is not ready for merge as vector support in the kernel is > > > > limited. However, the code has been tested in QEMU so the algorithms > > > > do work. This code requires the kernel to be compiled with C vector > > > > support, but that is not yet possible. It is written in assembly > > > > rather than using the GCC vector instrinsics because they did not > > > > provide optimal code. > > > > > > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > > > > --- > > > > arch/riscv/lib/csum.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 92 insertions(+) > > > > > > > > diff --git a/arch/riscv/lib/csum.c b/arch/riscv/lib/csum.c > > > > index 47d98c51bab2..eb4596fc7f5b 100644 > > > > --- a/arch/riscv/lib/csum.c > > > > +++ b/arch/riscv/lib/csum.c > > > > @@ -12,6 +12,10 @@ > > > > > > > > #include <net/checksum.h> > > > > > > > > +#ifdef CONFIG_RISCV_ISA_V > > > > +#include <riscv_vector.h> > > > > > > What actually includes this header, I don't see it in either Andy's > > > in-kernel vector series or Bjorn's blake2 one. > > > Can you link to the pre-requisites in your cover letter please. > > > > > > Thanks, > > > Conor. > > > > It is defined here: > > https://github.com/riscv-non-isa/rvv-intrinsic-doc/blob/main/doc/rvv-intrinsic-spec.adoc. > > The header is for the vector intrinsics that are supported by llvm and > > gcc. > > Well, whatever you're doing with it does not work, producing 3600 or so > fatal errors during compilation, all saying: > ../arch/riscv/include/asm/checksum.h:14:10: fatal error: riscv_vector.h: No such file or directory > > Do you have some makefile hack somewhere that's not part of this > patchset? Also, I'm dumb, but can you show me where are the actual > intrinsics are being used in this patch anyway? I just seem some > types & asm. > > Thanks, > Conor. > Intrinsics are needed for the vector types. Vector types are needed to get the inline asm to select vector registers at compile time. I could manually select vector registers to use but that is not ideal. In order to get this to work, vector has to be enabled in the compiler. This patch will not compile right now, but since people are working on vector I was hoping that it would be possible in the future. Palmer recommended that I just put up this patch for now since I had the code, but only the non-vector versions should be candidates for release for now. - Charlie
On Thu, Sep 14, 2023 at 01:29:18PM -0400, Charlie Jenkins wrote: > On Thu, Sep 14, 2023 at 05:29:29PM +0100, Conor Dooley wrote: > > On Thu, Sep 14, 2023 at 12:14:16PM -0400, Charlie Jenkins wrote: > > > On Thu, Sep 14, 2023 at 01:46:29PM +0100, Conor Dooley wrote: > > > > On Mon, Sep 11, 2023 at 03:57:14PM -0700, Charlie Jenkins wrote: > > > > > This patch is not ready for merge as vector support in the kernel is > > > > > limited. However, the code has been tested in QEMU so the algorithms > > > > > do work. This code requires the kernel to be compiled with C vector > > > > > support, but that is not yet possible. It is written in assembly > > > > > rather than using the GCC vector instrinsics because they did not > > > > > provide optimal code. > > > > > > > > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > > > > > --- > > > > > arch/riscv/lib/csum.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > 1 file changed, 92 insertions(+) > > > > > > > > > > diff --git a/arch/riscv/lib/csum.c b/arch/riscv/lib/csum.c > > > > > index 47d98c51bab2..eb4596fc7f5b 100644 > > > > > --- a/arch/riscv/lib/csum.c > > > > > +++ b/arch/riscv/lib/csum.c > > > > > @@ -12,6 +12,10 @@ > > > > > > > > > > #include <net/checksum.h> > > > > > > > > > > +#ifdef CONFIG_RISCV_ISA_V > > > > > +#include <riscv_vector.h> > > > > > > > > What actually includes this header, I don't see it in either Andy's > > > > in-kernel vector series or Bjorn's blake2 one. > > > > Can you link to the pre-requisites in your cover letter please. > > > > > > > > Thanks, > > > > Conor. > > > > > > It is defined here: > > > https://github.com/riscv-non-isa/rvv-intrinsic-doc/blob/main/doc/rvv-intrinsic-spec.adoc. > > > The header is for the vector intrinsics that are supported by llvm and > > > gcc. > > > > Well, whatever you're doing with it does not work, producing 3600 or so > > fatal errors during compilation, all saying: > > ../arch/riscv/include/asm/checksum.h:14:10: fatal error: riscv_vector.h: No such file or directory > > > > Do you have some makefile hack somewhere that's not part of this > > patchset? Also, I'm dumb, but can you show me where are the actual > > intrinsics are being used in this patch anyway? I just seem some > > types & asm. > > > > Thanks, > > Conor. > > > > Intrinsics are needed for the vector types. Vector types are needed to > get the inline asm to select vector registers at compile time. I could > manually select vector registers to use but that is not ideal. In order > to get this to work, vector has to be enabled in the compiler. This > patch will not compile right now, but since people are working on vector > I was hoping that it would be possible in the future. Palmer recommended > that I just put up this patch for now since I had the code, but only the > non-vector versions should be candidates for release for now. I see. I was pretty unclear to me anyway what the craic was, you should probably note that the build failures from here onwards are known-broken. If you want that header, I guess you probably need to have v set in -march? If so, the in-kernel vector patches that have been posted do not do that. Oh-so-far from an expert on what is a safe way to do these kinda things though, sadly.
On Thu, Sep 14, 2023 at 06:36:42PM +0100, Conor Dooley wrote: > On Thu, Sep 14, 2023 at 01:29:18PM -0400, Charlie Jenkins wrote: > > On Thu, Sep 14, 2023 at 05:29:29PM +0100, Conor Dooley wrote: > > > On Thu, Sep 14, 2023 at 12:14:16PM -0400, Charlie Jenkins wrote: > > > > On Thu, Sep 14, 2023 at 01:46:29PM +0100, Conor Dooley wrote: > > > > > On Mon, Sep 11, 2023 at 03:57:14PM -0700, Charlie Jenkins wrote: > > > > > > This patch is not ready for merge as vector support in the kernel is > > > > > > limited. However, the code has been tested in QEMU so the algorithms > > > > > > do work. This code requires the kernel to be compiled with C vector > > > > > > support, but that is not yet possible. It is written in assembly > > > > > > rather than using the GCC vector instrinsics because they did not > > > > > > provide optimal code. > > > > > > > > > > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > > > > > > --- > > > > > > arch/riscv/lib/csum.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > 1 file changed, 92 insertions(+) > > > > > > > > > > > > diff --git a/arch/riscv/lib/csum.c b/arch/riscv/lib/csum.c > > > > > > index 47d98c51bab2..eb4596fc7f5b 100644 > > > > > > --- a/arch/riscv/lib/csum.c > > > > > > +++ b/arch/riscv/lib/csum.c > > > > > > @@ -12,6 +12,10 @@ > > > > > > > > > > > > #include <net/checksum.h> > > > > > > > > > > > > +#ifdef CONFIG_RISCV_ISA_V > > > > > > +#include <riscv_vector.h> > > > > > > > > > > What actually includes this header, I don't see it in either Andy's > > > > > in-kernel vector series or Bjorn's blake2 one. > > > > > Can you link to the pre-requisites in your cover letter please. > > > > > > > > > > Thanks, > > > > > Conor. > > > > > > > > It is defined here: > > > > https://github.com/riscv-non-isa/rvv-intrinsic-doc/blob/main/doc/rvv-intrinsic-spec.adoc. > > > > The header is for the vector intrinsics that are supported by llvm and > > > > gcc. > > > > > > Well, whatever you're doing with it does not work, producing 3600 or so > > > fatal errors during compilation, all saying: > > > ../arch/riscv/include/asm/checksum.h:14:10: fatal error: riscv_vector.h: No such file or directory > > > > > > Do you have some makefile hack somewhere that's not part of this > > > patchset? Also, I'm dumb, but can you show me where are the actual > > > intrinsics are being used in this patch anyway? I just seem some > > > types & asm. > > > > > > Thanks, > > > Conor. > > > > > > > Intrinsics are needed for the vector types. Vector types are needed to > > get the inline asm to select vector registers at compile time. I could > > manually select vector registers to use but that is not ideal. In order > > to get this to work, vector has to be enabled in the compiler. This > > patch will not compile right now, but since people are working on vector > > I was hoping that it would be possible in the future. Palmer recommended > > that I just put up this patch for now since I had the code, but only the > > non-vector versions should be candidates for release for now. > > I see. I was pretty unclear to me anyway what the craic was, you should > probably note that the build failures from here onwards are > known-broken. If you want that header, I guess you probably need to > have v set in -march? > If so, the in-kernel vector patches that have been posted do not do that. > Oh-so-far from an expert on what is a safe way to do these kinda things > though, sadly. It seems like more than just enabling v in the march will need to be done. Because linux uses -nostdinc the header file won't be included. After doing some research it also seems like llvm and gcc do not share inline asm constraints. llvm is missing "vd" to specify a register that is not a mask register. I think I will drop these vector patches for now since there seems to be more work than I expected to get this functional. - Charlie
diff --git a/arch/riscv/lib/csum.c b/arch/riscv/lib/csum.c index 47d98c51bab2..eb4596fc7f5b 100644 --- a/arch/riscv/lib/csum.c +++ b/arch/riscv/lib/csum.c @@ -12,6 +12,10 @@ #include <net/checksum.h> +#ifdef CONFIG_RISCV_ISA_V +#include <riscv_vector.h> +#endif + /* Default version is sufficient for 32 bit */ #ifndef CONFIG_32BIT __sum16 csum_ipv6_magic(const struct in6_addr *saddr, @@ -115,6 +119,94 @@ unsigned int __no_sanitize_address do_csum(const unsigned char *buff, int len) offset = (csum_t)buff & OFFSET_MASK; kasan_check_read(buff, len); ptr = (const csum_t *)(buff - offset); +#ifdef CONFIG_RISCV_ISA_V + if (!has_vector()) + goto no_vector; + + len += offset; + + vuint64m1_t prev_buffer; + vuint32m1_t curr_buffer; + unsigned int shift, cl, tail_seg; + csum_t vl, csum; + const csum_t *ptr; + +#ifdef CONFIG_32BIT + csum_t high_result, low_result; +#else + csum_t result; +#endif + + // Read the tail segment + tail_seg = len % 4; + csum = 0; + if (tail_seg) { + shift = (4 - tail_seg) * 8; + csum = *(unsigned int *)((const unsigned char *)ptr + len - tail_seg); + csum = ((unsigned int)csum << shift) >> shift; + len -= tail_seg; + } + + unsigned int start_mask = (unsigned int)(~(~0U << offset)); + + kernel_vector_begin(); + asm(".option push \n\ + .option arch, +v \n\ + vsetvli %[vl], %[len], e8, m1, ta, ma \n\ + # clear out mask and vector registers since we switch up sizes \n\ + vmclr.m v0 \n\ + vmclr.m %[prev_buffer] \n\ + vmclr.m %[curr_buffer] \n\ + # Mask out the leading bits of a misaligned address \n\ + vsetivli x0, 1, e64, m1, ta, ma \n\ + vmv.s.x %[prev_buffer], %[csum] \n\ + vmv.s.x v0, %[start_mask] \n\ + vsetvli %[vl], %[len], e8, m1, ta, ma \n\ + vmnot.m v0, v0 \n\ + vle8.v %[curr_buffer], (%[buff]), v0.t \n\ + j 2f \n\ + # Iterate through the buff and sum all words \n\ + 1: \n\ + vsetvli %[vl], %[len], e8, m1, ta, ma \n\ + vle8.v %[curr_buffer], (%[buff]) \n\ + 2: \n\ + vsetvli x0, x0, e32, m1, ta, ma \n\ + vwredsumu.vs %[prev_buffer], %[curr_buffer], %[prev_buffer] \n\t" +#ifdef CONFIG_32BIT + "sub %[len], %[len], %[vl] \n\ + slli %[vl], %[vl], 2 \n\ + add %[buff], %[vl], %[buff] \n\ + bnez %[len], 1b \n\ + vsetvli x0, x0, e64, m1, ta, ma \n\ + vmv.x.s %[result], %[prev_buffer] \n\ + addi %[vl], x0, 32 \n\ + vsrl.vx %[prev_buffer], %[prev_buffer], %[vl] \n\ + vmv.x.s %[high_result], %[prev_buffer] \n\ + .option pop" + : [vl] "=&r"(vl), [prev_buffer] "=&vd"(prev_buffer), + [curr_buffer] "=&vd"(curr_buffer), + [high_result] "=&r"(high_result), [low_result] "=&r"(low_result) + : [buff] "r"(ptr), [len] "r"(len), [start_mask] "r"(start_mask), + [csum] "r"(csum)); + + high_result += low_result; + high_result += high_result < low_result; +#else // !CONFIG_32BIT + "subw %[len], %[len], %[vl] \n\ + slli %[vl], %[vl], 2 \n\ + addw %[buff], %[vl], %[buff] \n\ + bnez %[len], 1b \n\ + vsetvli x0, x0, e64, m1, ta, ma \n\ + vmv.x.s %[result], %[prev_buffer] \n\ + .option pop" + : [vl] "=&r"(vl), [prev_buffer] "=&vd"(prev_buffer), + [curr_buffer] "=&vd"(curr_buffer), [result] "=&r"(result) + : [buff] "r"(ptr), [len] "r"(len), [start_mask] "r"(start_mask), + [csum] "r"(csum)); +#endif // !CONFIG_32BIT + kernel_vector_end(); +no_vector: +#endif // CONFIG_RISCV_ISA_V len = len + offset - sizeof(csum_t); /*
This patch is not ready for merge as vector support in the kernel is limited. However, the code has been tested in QEMU so the algorithms do work. This code requires the kernel to be compiled with C vector support, but that is not yet possible. It is written in assembly rather than using the GCC vector instrinsics because they did not provide optimal code. Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> --- arch/riscv/lib/csum.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+)