> From: Jeremie Courreges-Anglas <[email protected]>
> Date: Sun, 25 Jul 2021 04:31:20 +0200
>
> On Sat, Jul 24 2021, Mark Kettenis <[email protected]> wrote:
> >> From: Jeremie Courreges-Anglas <[email protected]>
> >> Date: Sat, 24 Jul 2021 21:22:23 +0200
> >>
> >> hifive /usr/src/regress/sys/kern/gettimeofday$ doas -u build time
> >> obj/gettimeofday
> >> 6.64 real 6.63 user 0.02 sys
> >> hifive /usr/src/regress/sys/kern/gettimeofday$ doas -u build env
> >> LIBC_NOUSERTC=1 time obj/gettimeofday
> >> 6.48 real 0.60 user 5.42 sys
> >>
> >> Initially I thought that a more descriptive name than TC_TB could be
> >> helpful (TC_TIMEBASE?). But since powerpc also uses TC_TB I think it's
> >> fine as a first step. We can change it later easily, it's just a define
> >> name.
> >>
> >> I haven't even built a release with this, not sure it's worth it.
> >> If you have cpu cycles to spare, please say so.
> >>
> >> ok?
> >
> > Two small nits below. With that fixed, ok kettenis@
>
> [...]
>
> >> Index: lib/libc/arch/riscv64/gen/usertc.c
> >> ===================================================================
> >> RCS file: /cvs/src/lib/libc/arch/riscv64/gen/usertc.c,v
> >> retrieving revision 1.1
> >> diff -u -p -r1.1 usertc.c
> >> --- lib/libc/arch/riscv64/gen/usertc.c 29 Apr 2021 18:33:36 -0000
> >> 1.1
> >> +++ lib/libc/arch/riscv64/gen/usertc.c 24 Jul 2021 17:07:01 -0000
> >> @@ -1,6 +1,7 @@
> >> /* $OpenBSD: usertc.c,v 1.1 2021/04/29 18:33:36 drahn Exp $
> >> */
> >> /*
> >> * Copyright (c) 2020 Paul Irofti <[email protected]>
> >> + * Copyright (c) 2021 Jeremie Courreges-Anglas <[email protected]>
> >> *
> >> * Permission to use, copy, modify, and distribute this software for any
> >> * purpose with or without fee is hereby granted, provided that the above
> >> @@ -18,4 +19,24 @@
> >> #include <sys/types.h>
> >> #include <sys/timetc.h>
> >>
> >> -int (*const _tc_get_timecount)(struct timekeep *, u_int *) = NULL;
> >> +static inline u_int
> >> +rdtime(void)
> >> +{
> >> + uint64_t ret;
> >> + asm volatile("rdtime %0" : "=r"(ret));
> >
> > Can you make that __asm vol[a]tile?
>
> Done. I copied that from amd64 usertc.c.
>
> >> + return ret & 0xffffffff;
> >
> > The & 0xffffffff isn't really necessary here and the kernel doesn't do
> > it. So I'd drop that bit and simply return ret.
>
> I thought I would make it explicit to the reader that we only cared
> about the low 32 bits, rather then rely on the implicit truncation.
> Your nit was about consistency, what about trying to make other
> implementations consistent? Or should we make it explicit on other
> archs?
>
> Two changes, only compile-tested on amd64 and sparc64:
> - asm/__asm__ -> __asm
> - val & mask -> val
>
> I can also drop this diff, consistency is good but so is time on our
> hands.
So the whole idea was to minimize the diffs between the kernel and
userland implementation of the tc_get_timecount() functions. On some
of the architectures that isn't entirely feasable so there are some
differences. But it seems you realized this ;).
Masking is in general unnecessary as the generic timecounter code
(kernel and userland) already does the masking. However, there are
exceptions.
> Index: lib/libc/arch/aarch64/gen/usertc.c
> ===================================================================
> RCS file: /d/cvs/src/lib/libc/arch/aarch64/gen/usertc.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 usertc.c
> --- lib/libc/arch/aarch64/gen/usertc.c 15 Jul 2020 22:58:33 -0000
> 1.2
> +++ lib/libc/arch/aarch64/gen/usertc.c 24 Jul 2021 23:45:52 -0000
> @@ -29,7 +29,7 @@ agtimer_get_timecount(struct timecounter
> */
> __asm volatile("isb" ::: "memory");
> __asm volatile("mrs %x0, CNTVCT_EL0" : "=r" (val));
> - return (val & 0xffffffff);
> + return val;
> }
The masking here is deliberate as without the masking the errata
mentioned in the comment comes into play. So please drop this.
> static int
> Index: lib/libc/arch/amd64/gen/usertc.c
> ===================================================================
> RCS file: /d/cvs/src/lib/libc/arch/amd64/gen/usertc.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 usertc.c
> --- lib/libc/arch/amd64/gen/usertc.c 23 Aug 2020 21:38:47 -0000 1.3
> +++ lib/libc/arch/amd64/gen/usertc.c 24 Jul 2021 23:45:52 -0000
> @@ -22,7 +22,7 @@ static inline u_int
> rdtsc_lfence(void)
> {
> uint32_t hi, lo;
> - asm volatile("lfence; rdtsc" : "=a"(lo), "=d"(hi));
> + __asm volatile("lfence; rdtsc" : "=a"(lo), "=d"(hi));
> return ((uint64_t)lo)|(((uint64_t)hi)<<32);
> }
ok kettenis@
> Index: lib/libc/arch/mips64/gen/usertc.c
> ===================================================================
> RCS file: /d/cvs/src/lib/libc/arch/mips64/gen/usertc.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 usertc.c
> --- lib/libc/arch/mips64/gen/usertc.c 18 Jul 2020 08:37:43 -0000 1.2
> +++ lib/libc/arch/mips64/gen/usertc.c 25 Jul 2021 02:16:21 -0000
> @@ -23,7 +23,7 @@ get_cp0_count(void)
> {
> uint32_t count;
>
> - __asm__ volatile (
> + __asm volatile (
> " .set push\n"
> " .set mips64r2\n"
> " rdhwr %0, $2\n"
ok kettenis@
> Index: lib/libc/arch/sparc64/gen/usertc.c
> ===================================================================
> RCS file: /d/cvs/src/lib/libc/arch/sparc64/gen/usertc.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 usertc.c
> --- lib/libc/arch/sparc64/gen/usertc.c 8 Jul 2020 09:20:28 -0000
> 1.2
> +++ lib/libc/arch/sparc64/gen/usertc.c 24 Jul 2021 23:45:52 -0000
> @@ -25,7 +25,7 @@ tick_get_timecount(struct timecounter *t
>
> __asm volatile("rd %%tick, %0" : "=r" (tick));
>
> - return (tick & ~0u);
> + return tick;
> }
>
> static inline u_int
> @@ -35,7 +35,7 @@ sys_tick_get_timecount(struct timecounte
>
> __asm volatile("rd %%sys_tick, %0" : "=r" (tick));
>
> - return (tick & ~0u);
> + return tick;
> }
>
> static int
ok kettenis@
> Index: sys/arch/arm64/dev/agtimer.c
> ===================================================================
> RCS file: /d/cvs/src/sys/arch/arm64/dev/agtimer.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 agtimer.c
> --- sys/arch/arm64/dev/agtimer.c 11 Mar 2021 11:16:56 -0000 1.18
> +++ sys/arch/arm64/dev/agtimer.c 24 Jul 2021 23:45:52 -0000
> @@ -206,7 +206,7 @@ agtimer_get_timecount(struct timecounter
> */
> __asm volatile("isb" ::: "memory");
> __asm volatile("mrs %x0, CNTVCT_EL0" : "=r" (val));
> - return (val & 0xffffffff);
> + return val;
> }
Masking is deliberate here as well.
>
> int
> Index: sys/arch/sparc64/sparc64/clock.c
> ===================================================================
> RCS file: /d/cvs/src/sys/arch/sparc64/sparc64/clock.c,v
> retrieving revision 1.70
> diff -u -p -r1.70 clock.c
> --- sys/arch/sparc64/sparc64/clock.c 11 Mar 2021 11:17:00 -0000 1.70
> +++ sys/arch/sparc64/sparc64/clock.c 24 Jul 2021 23:45:52 -0000
> @@ -931,7 +931,7 @@ tick_get_timecount(struct timecounter *t
>
> __asm volatile("rd %%tick, %0" : "=r" (tick));
>
> - return (tick & ~0u);
> + return tick;
> }
>
> u_int
> @@ -941,5 +941,5 @@ sys_tick_get_timecount(struct timecounte
>
> __asm volatile("rd %%sys_tick, %0" : "=r" (tick));
>
> - return (tick & ~0u);
> + return tick;
> }
ok kettenis@
