On Thu, 29 Nov 2018 at 18:00, Dave Martin <Dave.Martin@xxxxxxx> wrote:
>
> On Tue, Nov 27, 2018 at 06:08:58PM +0800, Jackie Liu wrote:
> > This is a NEON acceleration method that can improve
> > performance by approximately 20%. I got the following
> > data from the centos 7.5 on Huawei's HISI1616 chip:
> >
> > [ 93.837726] xor: measuring software checksum speed
> > [ 93.874039] 8regs : 7123.200 MB/sec
> > [ 93.914038] 32regs : 7180.300 MB/sec
> > [ 93.954043] arm64_neon: 9856.000 MB/sec
> > [ 93.954047] xor: using function: arm64_neon (9856.000 MB/sec)
> >
> > I believe this code can bring some optimization for
> > all arm64 platform.
> >
> > That is patch version 3. Thanks for Ard Biesheuvel's
> > suggestions.
> >
> > Signed-off-by: Jackie Liu <liuyun01@xxxxxxxxxx>
> > ---
> > arch/arm64/include/asm/Kbuild | 1 -
> > arch/arm64/include/asm/xor.h | 73 +++++++++++++++++
> > arch/arm64/lib/Makefile | 6 ++
> > arch/arm64/lib/xor-neon.c | 184 ++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 263 insertions(+), 1 deletion(-)
> > create mode 100644 arch/arm64/include/asm/xor.h
> > create mode 100644 arch/arm64/lib/xor-neon.c
> >
> > diff --git a/arch/arm64/include/asm/Kbuild b/arch/arm64/include/asm/Kbuild
> > index 6cd5d77..1877f29 100644
> > --- a/arch/arm64/include/asm/Kbuild
> > +++ b/arch/arm64/include/asm/Kbuild
> > @@ -27,4 +27,3 @@ generic-y += trace_clock.h
> > generic-y += unaligned.h
> > generic-y += user.h
> > generic-y += vga.h
> > -generic-y += xor.h
> > diff --git a/arch/arm64/include/asm/xor.h b/arch/arm64/include/asm/xor.h
> > new file mode 100644
> > index 0000000..856386a
> > --- /dev/null
> > +++ b/arch/arm64/include/asm/xor.h
> > @@ -0,0 +1,73 @@
> > +/*
> > + * arch/arm64/include/asm/xor.h
> > + *
> > + * Authors: Jackie Liu <liuyun01@xxxxxxxxxx>
> > + * Copyright (C) 2018,Tianjin KYLIN Information Technology Co., Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/hardirq.h>
> > +#include <asm-generic/xor.h>
> > +#include <asm/hwcap.h>
> > +#include <asm/neon.h>
> > +
> > +#ifdef CONFIG_KERNEL_MODE_NEON
> > +
> > +extern struct xor_block_template const xor_block_inner_neon;
> > +
> > +static void
> > +xor_neon_2(unsigned long bytes, unsigned long *p1, unsigned long *p2)
> > +{
> > + kernel_neon_begin();
> > + xor_block_inner_neon.do_2(bytes, p1, p2);
> > + kernel_neon_end();
> > +}
> > +
> > +static void
> > +xor_neon_3(unsigned long bytes, unsigned long *p1, unsigned long *p2,
> > + unsigned long *p3)
> > +{
> > + kernel_neon_begin();
> > + xor_block_inner_neon.do_3(bytes, p1, p2, p3);
> > + kernel_neon_end();
> > +}
> > +
> > +static void
> > +xor_neon_4(unsigned long bytes, unsigned long *p1, unsigned long *p2,
> > + unsigned long *p3, unsigned long *p4)
> > +{
> > + kernel_neon_begin();
> > + xor_block_inner_neon.do_4(bytes, p1, p2, p3, p4);
> > + kernel_neon_end();
> > +}
> > +
> > +static void
> > +xor_neon_5(unsigned long bytes, unsigned long *p1, unsigned long *p2,
> > + unsigned long *p3, unsigned long *p4, unsigned long *p5)
> > +{
> > + kernel_neon_begin();
> > + xor_block_inner_neon.do_5(bytes, p1, p2, p3, p4, p5);
> > + kernel_neon_end();
> > +}
> > +
> > +static struct xor_block_template xor_block_arm64 = {
> > + .name = "arm64_neon",
> > + .do_2 = xor_neon_2,
> > + .do_3 = xor_neon_3,
> > + .do_4 = xor_neon_4,
> > + .do_5 = xor_neon_5
> > +};
> > +#undef XOR_TRY_TEMPLATES
> > +#define XOR_TRY_TEMPLATES \
> > + do { \
> > + xor_speed(&xor_block_8regs); \
> > + xor_speed(&xor_block_32regs); \
> > + if (cpu_has_neon()) { \
> > + xor_speed(&xor_block_arm64);\
> > + } \
> > + } while (0)
>
> Should there be a may_use_simd() check somewhere?
>
> If we invoke this in a softirq I don't see what prevents us from
> corrupting the task's NEON state.
>
> (The check might be in some surrounding glue code that I missed...)
>
There is no check. This code should simply not be called from
non-process context, same as the RAID56 code.
This is not terribly robust, obviously, but appears to be common
practice in this realm of the kernel.
