Re: [PATCH v5 1/3] riscv: optimized memcpy

[Matteo Croce <mcroce@linux.microsoft.com>]

On Thu, Sep 30, 2021 at 3:25 AM Guo Ren <guoren@kernel.org> wrote:
>
> On Thu, Sep 30, 2021 at 1:22 AM Matteo Croce <mcroce@linux.microsoft.com> wrote:
> >
> > From: Matteo Croce <mcroce@microsoft.com>
> >
> > Write a C version of memcpy() which uses the biggest data size allowed,
> > without generating unaligned accesses.
> >
> > The procedure is made of three steps:
> > First copy data one byte at time until the destination buffer is aligned
> > to a long boundary.
> > Then copy the data one long at time shifting the current and the next u8
> > to compose a long at every cycle.
> > Finally, copy the remainder one byte at time.
> >
> > On a BeagleV, the TCP RX throughput increased by 45%:
> >
> > before:
> >
> > $ iperf3 -c beaglev
> > Connecting to host beaglev, port 5201
> > [  5] local 192.168.85.6 port 44840 connected to 192.168.85.48 port 5201
> > [ ID] Interval           Transfer     Bitrate         Retr  Cwnd
> > [  5]   0.00-1.00   sec  76.4 MBytes   641 Mbits/sec   27    624 KBytes
> > [  5]   1.00-2.00   sec  72.5 MBytes   608 Mbits/sec    0    708 KBytes
> > [  5]   2.00-3.00   sec  73.8 MBytes   619 Mbits/sec   10    451 KBytes
> > [  5]   3.00-4.00   sec  72.5 MBytes   608 Mbits/sec    0    564 KBytes
> > [  5]   4.00-5.00   sec  73.8 MBytes   619 Mbits/sec    0    658 KBytes
> > [  5]   5.00-6.00   sec  73.8 MBytes   619 Mbits/sec   14    522 KBytes
> > [  5]   6.00-7.00   sec  73.8 MBytes   619 Mbits/sec    0    621 KBytes
> > [  5]   7.00-8.00   sec  72.5 MBytes   608 Mbits/sec    0    706 KBytes
> > [  5]   8.00-9.00   sec  73.8 MBytes   619 Mbits/sec   20    580 KBytes
> > [  5]   9.00-10.00  sec  73.8 MBytes   619 Mbits/sec    0    672 KBytes
> > - - - - - - - - - - - - - - - - - - - - - - - - -
> > [ ID] Interval           Transfer     Bitrate         Retr
> > [  5]   0.00-10.00  sec   736 MBytes   618 Mbits/sec   71             sender
> > [  5]   0.00-10.01  sec   733 MBytes   615 Mbits/sec                  receiver
> >
> > after:
> >
> > $ iperf3 -c beaglev
> > Connecting to host beaglev, port 5201
> > [  5] local 192.168.85.6 port 44864 connected to 192.168.85.48 port 5201
> > [ ID] Interval           Transfer     Bitrate         Retr  Cwnd
> > [  5]   0.00-1.00   sec   109 MBytes   912 Mbits/sec   48    559 KBytes
> > [  5]   1.00-2.00   sec   108 MBytes   902 Mbits/sec    0    690 KBytes
> > [  5]   2.00-3.00   sec   106 MBytes   891 Mbits/sec   36    396 KBytes
> > [  5]   3.00-4.00   sec   108 MBytes   902 Mbits/sec    0    567 KBytes
> > [  5]   4.00-5.00   sec   106 MBytes   891 Mbits/sec    0    699 KBytes
> > [  5]   5.00-6.00   sec   106 MBytes   891 Mbits/sec   32    414 KBytes
> > [  5]   6.00-7.00   sec   106 MBytes   891 Mbits/sec    0    583 KBytes
> > [  5]   7.00-8.00   sec   106 MBytes   891 Mbits/sec    0    708 KBytes
> > [  5]   8.00-9.00   sec   106 MBytes   891 Mbits/sec   28    433 KBytes
> > [  5]   9.00-10.00  sec   108 MBytes   902 Mbits/sec    0    591 KBytes
> > - - - - - - - - - - - - - - - - - - - - - - - - -
> > [ ID] Interval           Transfer     Bitrate         Retr
> > [  5]   0.00-10.00  sec  1.04 GBytes   897 Mbits/sec  144             sender
> > [  5]   0.00-10.01  sec  1.04 GBytes   894 Mbits/sec                  receiver
> >
> > And the decreased CPU time of the memcpy() is observable with perf top.
> > This is the `perf top -Ue task-clock` output when doing the test:
> >
> > before:
> >
> > Overhead  Shared O  Symbol
> >   42.22%  [kernel]  [k] memcpy
> >   35.00%  [kernel]  [k] __asm_copy_to_user
> >    3.50%  [kernel]  [k] sifive_l2_flush64_range
> >    2.30%  [kernel]  [k] stmmac_napi_poll_rx
> >    1.11%  [kernel]  [k] memset
> >
> > after:
> >
> > Overhead  Shared O  Symbol
> >   45.69%  [kernel]  [k] __asm_copy_to_user
> >   29.06%  [kernel]  [k] memcpy
> >    4.09%  [kernel]  [k] sifive_l2_flush64_range
> >    2.77%  [kernel]  [k] stmmac_napi_poll_rx
> >    1.24%  [kernel]  [k] memset
> >
> > Signed-off-by: Matteo Croce <mcroce@microsoft.com>
> > Reported-by: kernel test robot <lkp@intel.com>
> > ---
> >  arch/riscv/include/asm/string.h |   8 ++-
> >  arch/riscv/kernel/riscv_ksyms.c |   2 -
> >  arch/riscv/lib/Makefile         |   2 +-
> >  arch/riscv/lib/memcpy.S         | 108 --------------------------------
> >  arch/riscv/lib/string.c         |  90 ++++++++++++++++++++++++++
> >  5 files changed, 97 insertions(+), 113 deletions(-)
> >  delete mode 100644 arch/riscv/lib/memcpy.S
> >  create mode 100644 arch/riscv/lib/string.c
> >
> > diff --git a/arch/riscv/include/asm/string.h b/arch/riscv/include/asm/string.h
> > index 909049366555..6b5d6fc3eab4 100644
> > --- a/arch/riscv/include/asm/string.h
> > +++ b/arch/riscv/include/asm/string.h
> > @@ -12,9 +12,13 @@
> >  #define __HAVE_ARCH_MEMSET
> >  extern asmlinkage void *memset(void *, int, size_t);
> >  extern asmlinkage void *__memset(void *, int, size_t);
> > +
> > +#ifdef CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE
> What's the problem with the -O3 & -Os? If the user uses
> CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE_O3 that will cause bad performance
> for memcpy?
> Seems asm version is more compatible?
>
> >  #define __HAVE_ARCH_MEMCPY
> > -extern asmlinkage void *memcpy(void *, const void *, size_t);
> > -extern asmlinkage void *__memcpy(void *, const void *, size_t);
> > +extern void *memcpy(void *dest, const void *src, size_t count);
> > +extern void *__memcpy(void *dest, const void *src, size_t count);
> > +#endif
> > +
> >  #define __HAVE_ARCH_MEMMOVE
> >  extern asmlinkage void *memmove(void *, const void *, size_t);
> >  extern asmlinkage void *__memmove(void *, const void *, size_t);
> > diff --git a/arch/riscv/kernel/riscv_ksyms.c b/arch/riscv/kernel/riscv_ksyms.c
> > index 5ab1c7e1a6ed..3f6d512a5b97 100644
> > --- a/arch/riscv/kernel/riscv_ksyms.c
> > +++ b/arch/riscv/kernel/riscv_ksyms.c
> > @@ -10,8 +10,6 @@
> >   * Assembly functions that may be used (directly or indirectly) by modules
> >   */
> >  EXPORT_SYMBOL(memset);
> > -EXPORT_SYMBOL(memcpy);
> >  EXPORT_SYMBOL(memmove);
> >  EXPORT_SYMBOL(__memset);
> > -EXPORT_SYMBOL(__memcpy);
> >  EXPORT_SYMBOL(__memmove);
> > diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
> > index 25d5c9664e57..2ffe85d4baee 100644
> > --- a/arch/riscv/lib/Makefile
> > +++ b/arch/riscv/lib/Makefile
> > @@ -1,9 +1,9 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  lib-y                  += delay.o
> > -lib-y                  += memcpy.o
> >  lib-y                  += memset.o
> >  lib-y                  += memmove.o
> >  lib-$(CONFIG_MMU)      += uaccess.o
> >  lib-$(CONFIG_64BIT)    += tishift.o
> > +lib-$(CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE) += string.o
> >
> >  obj-$(CONFIG_FUNCTION_ERROR_INJECTION) += error-inject.o
> > diff --git a/arch/riscv/lib/memcpy.S b/arch/riscv/lib/memcpy.S
> > deleted file mode 100644
> > index 51ab716253fa..000000000000
> > --- a/arch/riscv/lib/memcpy.S
> > +++ /dev/null
> > @@ -1,108 +0,0 @@
> > -/* SPDX-License-Identifier: GPL-2.0-only */
> > -/*
> > - * Copyright (C) 2013 Regents of the University of California
> > - */
> > -
> > -#include <linux/linkage.h>
> > -#include <asm/asm.h>
> > -
> > -/* void *memcpy(void *, const void *, size_t) */
> > -ENTRY(__memcpy)
> > -WEAK(memcpy)
> > -       move t6, a0  /* Preserve return value */
> > -
> > -       /* Defer to byte-oriented copy for small sizes */
> > -       sltiu a3, a2, 128
> > -       bnez a3, 4f
> > -       /* Use word-oriented copy only if low-order bits match */
> > -       andi a3, t6, SZREG-1
> > -       andi a4, a1, SZREG-1
> > -       bne a3, a4, 4f
> > -
> > -       beqz a3, 2f  /* Skip if already aligned */
> > -       /*
> > -        * Round to nearest double word-aligned address
> > -        * greater than or equal to start address
> > -        */
> > -       andi a3, a1, ~(SZREG-1)
> > -       addi a3, a3, SZREG
> > -       /* Handle initial misalignment */
> > -       sub a4, a3, a1
> > -1:
> > -       lb a5, 0(a1)
> > -       addi a1, a1, 1
> > -       sb a5, 0(t6)
> > -       addi t6, t6, 1
> > -       bltu a1, a3, 1b
> > -       sub a2, a2, a4  /* Update count */
> > -
> > -2:
> > -       andi a4, a2, ~((16*SZREG)-1)
> > -       beqz a4, 4f
> > -       add a3, a1, a4
> > -3:
> > -       REG_L a4,       0(a1)
> > -       REG_L a5,   SZREG(a1)
> > -       REG_L a6, 2*SZREG(a1)
> > -       REG_L a7, 3*SZREG(a1)
> > -       REG_L t0, 4*SZREG(a1)
> > -       REG_L t1, 5*SZREG(a1)
> > -       REG_L t2, 6*SZREG(a1)
> > -       REG_L t3, 7*SZREG(a1)
> > -       REG_L t4, 8*SZREG(a1)
> > -       REG_L t5, 9*SZREG(a1)
> > -       REG_S a4,       0(t6)
> > -       REG_S a5,   SZREG(t6)
> > -       REG_S a6, 2*SZREG(t6)
> > -       REG_S a7, 3*SZREG(t6)
> > -       REG_S t0, 4*SZREG(t6)
> > -       REG_S t1, 5*SZREG(t6)
> > -       REG_S t2, 6*SZREG(t6)
> > -       REG_S t3, 7*SZREG(t6)
> > -       REG_S t4, 8*SZREG(t6)
> > -       REG_S t5, 9*SZREG(t6)
> > -       REG_L a4, 10*SZREG(a1)
> > -       REG_L a5, 11*SZREG(a1)
> > -       REG_L a6, 12*SZREG(a1)
> > -       REG_L a7, 13*SZREG(a1)
> > -       REG_L t0, 14*SZREG(a1)
> > -       REG_L t1, 15*SZREG(a1)
> > -       addi a1, a1, 16*SZREG
> > -       REG_S a4, 10*SZREG(t6)
> > -       REG_S a5, 11*SZREG(t6)
> > -       REG_S a6, 12*SZREG(t6)
> > -       REG_S a7, 13*SZREG(t6)
> > -       REG_S t0, 14*SZREG(t6)
> > -       REG_S t1, 15*SZREG(t6)
> > -       addi t6, t6, 16*SZREG
> > -       bltu a1, a3, 3b
> > -       andi a2, a2, (16*SZREG)-1  /* Update count */
> > -
> > -4:
> > -       /* Handle trailing misalignment */
> > -       beqz a2, 6f
> > -       add a3, a1, a2
> > -
> > -       /* Use word-oriented copy if co-aligned to word boundary */
> > -       or a5, a1, t6
> > -       or a5, a5, a3
> > -       andi a5, a5, 3
> > -       bnez a5, 5f
> > -7:
> > -       lw a4, 0(a1)
> > -       addi a1, a1, 4
> > -       sw a4, 0(t6)
> > -       addi t6, t6, 4
> > -       bltu a1, a3, 7b
> > -
> > -       ret
> > -
> > -5:
> > -       lb a4, 0(a1)
> > -       addi a1, a1, 1
> > -       sb a4, 0(t6)
> > -       addi t6, t6, 1
> > -       bltu a1, a3, 5b
> > -6:
> > -       ret
> > -END(__memcpy)
> > diff --git a/arch/riscv/lib/string.c b/arch/riscv/lib/string.c
> > new file mode 100644
> > index 000000000000..bfc912ee23f8
> > --- /dev/null
> > +++ b/arch/riscv/lib/string.c
> > @@ -0,0 +1,90 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * String functions optimized for hardware which doesn't
> > + * handle unaligned memory accesses efficiently.
> > + *
> > + * Copyright (C) 2021 Matteo Croce
> > + */
> > +
> > +#include <linux/types.h>
> > +#include <linux/module.h>
> > +
> > +/* Minimum size for a word copy to be convenient */
> > +#define BYTES_LONG     sizeof(long)
> > +#define WORD_MASK      (BYTES_LONG - 1)
> > +#define MIN_THRESHOLD  (BYTES_LONG * 2)
> > +
> > +/* convenience union to avoid cast between different pointer types */
> > +union types {
> > +       u8 *as_u8;
> > +       unsigned long *as_ulong;
> > +       uintptr_t as_uptr;
> > +};
> > +
> > +union const_types {
> > +       const u8 *as_u8;
> > +       unsigned long *as_ulong;
> > +       uintptr_t as_uptr;
> > +};
> > +
> > +void *__memcpy(void *dest, const void *src, size_t count)
> How about using __attribute__((optimize("-O2"))) here to replace your
> previous "#ifdef CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE"?
>
> > +{
> > +       union const_types s = { .as_u8 = src };
> > +       union types d = { .as_u8 = dest };
> > +       int distance = 0;
> > +
> > +       if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {
> > +               if (count < MIN_THRESHOLD)
> > +                       goto copy_remainder;
> > +
> > +               /* Copy a byte at time until destination is aligned. */
> > +               for (; d.as_uptr & WORD_MASK; count--)
> > +                       *d.as_u8++ = *s.as_u8++;
> > +
> > +               distance = s.as_uptr & WORD_MASK;
> > +       }
> > +
> > +       if (distance) {
> > +               unsigned long last, next;
> > +
> > +               /*
> > +                * s is distance bytes ahead of d, and d just reached
> > +                * the alignment boundary. Move s backward to word align it
> > +                * and shift data to compensate for distance, in order to do
> > +                * word-by-word copy.
> > +                */
> > +               s.as_u8 -= distance;
> > +
> > +               next = s.as_ulong[0];
> > +               for (; count >= BYTES_LONG; count -= BYTES_LONG) {
> > +                       last = next;
> > +                       next = s.as_ulong[1];
> > +
> > +                       d.as_ulong[0] = last >> (distance * 8) |
> > +                                       next << ((BYTES_LONG - distance) * 8);
> > +
> > +                       d.as_ulong++;
> > +                       s.as_ulong++;
> > +               }
> > +
> > +               /* Restore s with the original offset. */
> > +               s.as_u8 += distance;
> > +       } else {
> > +               /*
> > +                * If the source and dest lower bits are the same, do a simple
> > +                * 32/64 bit wide copy.
> > +                */
> > +               for (; count >= BYTES_LONG; count -= BYTES_LONG)
> > +                       *d.as_ulong++ = *s.as_ulong++;
> > +       }
> > +
> > +copy_remainder:
> > +       while (count--)
> > +               *d.as_u8++ = *s.as_u8++;
> > +
> > +       return dest;
> > +}
> > +EXPORT_SYMBOL(__memcpy);
> > +
> > +void *memcpy(void *dest, const void *src, size_t count) __weak __alias(__memcpy);
> > +EXPORT_SYMBOL(memcpy);
> > --
> > 2.31.1
> >
>
>
> --
> Best Regards
>  Guo Ren
>
> ML: https://lore.kernel.org/linux-csky/

If CONFIG_CC_OPTIMIZE_FOR_SIZE I fallback to the generic
implementations, which are very small.
It's just an optimization to save more space when -Os is used.

CC_OPTIMIZE_FOR_PERFORMANCE_O3 is ARC specific.

Regards,
-- 
per aspera ad upstream