Re: [PATCH] LoongArch: Add vDSO syscall __vdso_getcpu()

From: Huacai Chen <chenhuacai@kernel.org>
To: WANG Xuerui <kernel@xen0n.name>
Cc: hev <r@hev.cc>, Huacai Chen <chenhuacai@loongson.cn>,
	Arnd Bergmann <arnd@arndb.de>,
	 loongarch@lists.linux.dev,
	linux-arch <linux-arch@vger.kernel.org>,
	 Xuefeng Li <lixuefeng@loongson.cn>, Guo Ren <guoren@kernel.org>,
	 Jiaxun Yang <jiaxun.yang@flygoat.com>
Subject: Re: [PATCH] LoongArch: Add vDSO syscall __vdso_getcpu()
Date: Mon, 20 Jun 2022 12:26:42 +0800	[thread overview]
Message-ID: <CAAhV-H4-=T5hA1UwFq2uN=PU0L9KV9jU9jkDQh9UuyyuUr=zEQ@mail.gmail.com> (raw)
In-Reply-To: <94bebe28-5988-d6b6-bf82-03ef5901cd69@xen0n.name>

Hi, Xuerui,

On Sat, Jun 18, 2022 at 8:56 PM WANG Xuerui <kernel@xen0n.name> wrote:
>
> On 6/18/22 17:10, Huacai Chen wrote:
> > Hi,
> >
> > On Fri, Jun 17, 2022 at 11:35 PM hev <r@hev.cc> wrote:
> >> Hello,
> >>
> >> On Fri, Jun 17, 2022 at 10:57 PM Huacai Chen <chenhuacai@loongson.cn> wrote:
> >>> We test 20 million times of getcpu(), the real syscall version take 25
> >>> seconds, while the vsyscall version take only 2.4 seconds.
> >>>
> >>> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> >>> ---
> >>>   arch/loongarch/include/asm/vdso.h      |  4 +++
> >>>   arch/loongarch/include/asm/vdso/vdso.h | 10 +++++-
> >>>   arch/loongarch/kernel/vdso.c           | 23 +++++++++-----
> >>>   arch/loongarch/vdso/Makefile           |  3 +-
> >>>   arch/loongarch/vdso/vdso.lds.S         |  1 +
> >>>   arch/loongarch/vdso/vgetcpu.c          | 43 ++++++++++++++++++++++++++
> >>>   6 files changed, 74 insertions(+), 10 deletions(-)
> >>>   create mode 100644 arch/loongarch/vdso/vgetcpu.c
> >>>
> >>> diff --git a/arch/loongarch/include/asm/vdso.h b/arch/loongarch/include/asm/vdso.h
> >>> index 8f8a0f9a4953..e76d5e37480d 100644
> >>> --- a/arch/loongarch/include/asm/vdso.h
> >>> +++ b/arch/loongarch/include/asm/vdso.h
> >>> @@ -12,6 +12,10 @@
> >>>
> >>>   #include <asm/barrier.h>
> >>>
> >>> +typedef struct vdso_pcpu_data {
> >>> +       u32 node;
> >>> +} ____cacheline_aligned_in_smp vdso_pcpu_data;
> >>> +
> >>>   /*
> >>>    * struct loongarch_vdso_info - Details of a VDSO image.
> >>>    * @vdso: Pointer to VDSO image (page-aligned).
> >>> diff --git a/arch/loongarch/include/asm/vdso/vdso.h b/arch/loongarch/include/asm/vdso/vdso.h
> >>> index 5a01643a65b3..94055f7c54b7 100644
> >>> --- a/arch/loongarch/include/asm/vdso/vdso.h
> >>> +++ b/arch/loongarch/include/asm/vdso/vdso.h
> >>> @@ -8,6 +8,13 @@
> >>>
> >>>   #include <asm/asm.h>
> >>>   #include <asm/page.h>
> >>> +#include <asm/vdso.h>
> >>> +
> >>> +#if PAGE_SIZE < SZ_16K
> >>> +#define VDSO_DATA_SIZE SZ_16K
> >> Whether we add members to the vdso data structure or extend
> >> SMP_CACHE_BYTES/NR_CPUS, the static VDSO_DATA_SIZE may not match, and
> >> there is no assertion checking to help us catch bugs early. So I
> >> suggest defining VDSO_DATA_SIZE as ALIGN_UP(sizeof (struct vdso_data),
> >> PAGE_SIZE).
> > VSYSCALL usage is very limited (you know, VSYSCALL appears for so many
> > years, but the number nearly doesn't increase until now), so I think
> > 16KB is enough in the future.
>
> I don't think omitting compile-time assertions for *correctness* is
> worth the negligible improvement in brevity and ease of maintenance. In
> fact, static checks for correctness actually *lightens* maintenance
> burden, by explicitly calling out the assumptions so that newcomers
> (i.e. me or some other random linux/arch developer refactoring code)
> would find them very helpful.
>
> So I'm in support for declaring the VDSO_DATA_SIZE explicitly in terms
> of sizeof(struct vdso_data) and PAGE_SIZE.
I'll use hev's method, thank you.

Huacai