Re: usertc: small consistency tweaks (was: Re: riscv64 usertc)
> From: Jeremie Courreges-Anglas
> Date: Sun, 25 Jul 2021 04:31:20 +0200
>
> On Sat, Jul 24 2021, Mark Kettenis wrote:
> >> From: Jeremie Courreges-Anglas
> >> 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 -
> >> 1.1
> >> +++ lib/libc/arch/riscv64/gen/usertc.c 24 Jul 2021 17:07:01 -
> >> @@ -1,6 +1,7 @@
> >> /*$OpenBSD: usertc.c,v 1.1 2021/04/29 18:33:36 drahn Exp $
> >> */
> >> /*
> >> * Copyright (c) 2020 Paul Irofti
> >> + * Copyright (c) 2021 Jeremie Courreges-Anglas
> >> *
> >> * 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
> >> #include
> >>
> >> -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 & 0x;
> >
> > The & 0x 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.c15 Jul 2020 22:58:33 -
> 1.2
> +++ lib/libc/arch/aarch64/gen/usertc.c24 Jul 2021 23:45:52 -
> @@ -29,7 +29,7 @@ agtimer_get_timecount(struct timecounter
>*/
> __asm volatile("isb" ::: "memory");
> __asm volatile("mrs %x0, CNTVCT_EL0" : "=r" (val));
> - return (val & 0x);
> + 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 - 1.3
> +++ lib/libc/arch/amd64/gen/usertc.c 24 Jul 2021 23:45:52 -
> @@ -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 - 1.2
> +++ lib/libc/arch/mips64/gen/usertc.c 25 Jul 2021 02:16:21 -
> @@ -23,7 +23,7 @@
usertc: small consistency tweaks (was: Re: riscv64 usertc)
On Sat, Jul 24 2021, Mark Kettenis wrote:
>> From: Jeremie Courreges-Anglas
>> 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 -
>> 1.1
>> +++ lib/libc/arch/riscv64/gen/usertc.c 24 Jul 2021 17:07:01 -
>> @@ -1,6 +1,7 @@
>> /* $OpenBSD: usertc.c,v 1.1 2021/04/29 18:33:36 drahn Exp $*/
>> /*
>> * Copyright (c) 2020 Paul Irofti
>> + * Copyright (c) 2021 Jeremie Courreges-Anglas
>> *
>> * 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
>> #include
>>
>> -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 & 0x;
>
> The & 0x 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.
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 - 1.2
+++ lib/libc/arch/aarch64/gen/usertc.c 24 Jul 2021 23:45:52 -
@@ -29,7 +29,7 @@ agtimer_get_timecount(struct timecounter
*/
__asm volatile("isb" ::: "memory");
__asm volatile("mrs %x0, CNTVCT_EL0" : "=r" (val));
- return (val & 0x);
+ return val;
}
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.c23 Aug 2020 21:38:47 - 1.3
+++ lib/libc/arch/amd64/gen/usertc.c24 Jul 2021 23:45:52 -
@@ -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);
}
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 - 1.2
+++ lib/libc/arch/mips64/gen/usertc.c 25 Jul 2021 02:16:21 -
@@ -23,7 +23,7 @@ get_cp0_count(void)
{
uint32_t count;
- __asm__ volatile (
+ __asm volatile (
" .setpush\n"
" .setmips64r2\n"
" rdhwr %0, $2\n"
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 - 1.2
+++ lib/libc/arch/sparc64/gen/usertc.c 24 Jul 2021 23:45:52 -
@@ -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 v
Re: riscv64 usertc
> From: Jeremie Courreges-Anglas
> 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: sys/arch/riscv64/include/timetc.h
> ===
> RCS file: /cvs/src/sys/arch/riscv64/include/timetc.h,v
> retrieving revision 1.2
> diff -u -p -r1.2 timetc.h
> --- sys/arch/riscv64/include/timetc.h 12 May 2021 01:20:52 - 1.2
> +++ sys/arch/riscv64/include/timetc.h 23 Jul 2021 13:30:08 -
> @@ -19,5 +19,6 @@
> #ifndef _MACHINE_TIMETC_H_
> #define _MACHINE_TIMETC_H_
>
> +#define TC_TB1
>
> #endif /* _MACHINE_TIMETC_H_ */
> Index: sys/arch/riscv64/riscv64/clock.c
> ===
> RCS file: /cvs/src/sys/arch/riscv64/riscv64/clock.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 clock.c
> --- sys/arch/riscv64/riscv64/clock.c 21 Jun 2021 15:19:39 - 1.2
> +++ sys/arch/riscv64/riscv64/clock.c 23 Jul 2021 13:29:42 -
> @@ -47,6 +47,7 @@ static struct timecounter tb_timecounter
> .tc_name = "tb",
> .tc_quality = 0,
> .tc_priv = NULL,
> + .tc_user = TC_TB,
> };
>
> void cpu_startclock(void);
> 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.c29 Apr 2021 18:33:36 -
> 1.1
> +++ lib/libc/arch/riscv64/gen/usertc.c24 Jul 2021 17:07:01 -
> @@ -1,6 +1,7 @@
> /* $OpenBSD: usertc.c,v 1.1 2021/04/29 18:33:36 drahn Exp $*/
> /*
> * Copyright (c) 2020 Paul Irofti
> + * Copyright (c) 2021 Jeremie Courreges-Anglas
> *
> * 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
> #include
>
> -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 voltile?
> + return ret & 0x;
The & 0x isn't really necessary here and the kernel doesn't do
it. So I'd drop that bit and simply return ret.
> +}
> +
> +static int
> +tc_get_timecount(struct timekeep *tk, u_int *tc)
> +{
> + switch (tk->tk_user) {
> + case TC_TB:
> + *tc = rdtime();
> + return 0;
> + }
> +
> + return -1;
> +}
> +
> +int (*const _tc_get_timecount)(struct timekeep *, u_int *) =
> tc_get_timecount;
>
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
>
>
riscv64 usertc
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?
Index: sys/arch/riscv64/include/timetc.h
===
RCS file: /cvs/src/sys/arch/riscv64/include/timetc.h,v
retrieving revision 1.2
diff -u -p -r1.2 timetc.h
--- sys/arch/riscv64/include/timetc.h 12 May 2021 01:20:52 - 1.2
+++ sys/arch/riscv64/include/timetc.h 23 Jul 2021 13:30:08 -
@@ -19,5 +19,6 @@
#ifndef _MACHINE_TIMETC_H_
#define _MACHINE_TIMETC_H_
+#define TC_TB 1
#endif /* _MACHINE_TIMETC_H_ */
Index: sys/arch/riscv64/riscv64/clock.c
===
RCS file: /cvs/src/sys/arch/riscv64/riscv64/clock.c,v
retrieving revision 1.2
diff -u -p -r1.2 clock.c
--- sys/arch/riscv64/riscv64/clock.c21 Jun 2021 15:19:39 - 1.2
+++ sys/arch/riscv64/riscv64/clock.c23 Jul 2021 13:29:42 -
@@ -47,6 +47,7 @@ static struct timecounter tb_timecounter
.tc_name = "tb",
.tc_quality = 0,
.tc_priv = NULL,
+ .tc_user = TC_TB,
};
void cpu_startclock(void);
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 - 1.1
+++ lib/libc/arch/riscv64/gen/usertc.c 24 Jul 2021 17:07:01 -
@@ -1,6 +1,7 @@
/* $OpenBSD: usertc.c,v 1.1 2021/04/29 18:33:36 drahn Exp $*/
/*
* Copyright (c) 2020 Paul Irofti
+ * Copyright (c) 2021 Jeremie Courreges-Anglas
*
* 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
#include
-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));
+ return ret & 0x;
+}
+
+static int
+tc_get_timecount(struct timekeep *tk, u_int *tc)
+{
+ switch (tk->tk_user) {
+ case TC_TB:
+ *tc = rdtime();
+ return 0;
+ }
+
+ return -1;
+}
+
+int (*const _tc_get_timecount)(struct timekeep *, u_int *) = tc_get_timecount;
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
