cc'ing cheloha@ since he probably cares about this.
On Sun, Jul 25 2021, Mark Kettenis <[email protected]> wrote: >> 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. I'm not talking about the masking done by the timecounter API. The *_timecount() kernel functions (and their userland counterparts) return a u_int, not a uint64_t. Before reaching the timecounter internals, masking off the 32 high bits of a 64 bit value has already been done by an implicit truncation. Thus, > 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. ... even if a bug makes the high bits of this timer unreliable, the masking seems superfluous to me. I would expect the same optimized machine code to be generated with or without the mask (as happens on amd64, sparc64 and riscv64, I don't have an arm64 box). So, regarding those functions that return a 64 bits unsigned integer typecasted as an u_int, I think there are three easy options: - we remove the explicit masking from all the affected *_timecount(), except for this timer because it's *special* (but note that there already is a comment to warn about the errata) - we remove the explicit masking from all the affected *_timecount() functions (sparc64) - we add explicit masking to all said functions to hilight that we only care about the low 32 bits All this with the hope that the next folk that adds a usertc implementation doesn't waste time on those small differences. I won't suggest widening the return type of those functions, it probably brings no improvement and brings more questions. Thanks a lot for the feedback! For now I'll just commit the asm/__asm__ -> __asm bits. >> 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@ -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
