Date: Fri, 10 Jan 2025 16:37:08 +0000 From: Jessica Clarke <jrtc27@freebsd.org> To: Mark Johnston <markj@FreeBSD.org> Cc: "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: Re: git: 6b82130e6c9a - main - clock: Add a long ticks variable, ticksl Message-ID: <9B1B709F-5828-489C-81B5-74ED9E4502FC@freebsd.org> In-Reply-To: <202501101600.50AG0jk6062308@gitrepo.freebsd.org> References: <202501101600.50AG0jk6062308@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 10 Jan 2025, at 16:00, Mark Johnston <markj@FreeBSD.org> wrote: > > The branch main has been updated by markj: > > URL: https://cgit.FreeBSD.org/src/commit/?id=6b82130e6c9add4a8892ca897df5a0ec04663ea2 > > commit 6b82130e6c9add4a8892ca897df5a0ec04663ea2 > Author: Mark Johnston <markj@FreeBSD.org> > AuthorDate: 2025-01-10 15:37:07 +0000 > Commit: Mark Johnston <markj@FreeBSD.org> > CommitDate: 2025-01-10 15:42:59 +0000 > > clock: Add a long ticks variable, ticksl > > For compatibility with Linux, it's useful to have a tick counter of > width sizeof(long), but our tick counter is an int. Currently the > linuxkpi tries paper over this difference, but this cannot really be > done reliably, so it's desirable to have a wider tick counter. This > change introduces ticksl, keeping the existing ticks variable. > > Follow a suggestion from kib to avoid having to maintain two separate > counters and to avoid converting existing code to use ticksl: change > hardclock() to update ticksl instead of ticks, and then use assembler > directives to make ticks and ticksl overlap such that loading ticks > gives the bottom 32 bits. This makes it possible to use ticksl in the > linuxkpi without having to convert any native code, and without making > hardclock() more complicated or expensive. Then, the linuxkpi can be > modified to use ticksl instead of ticks. > > Reviewed by: olce, kib, emaste > MFC after: 1 month > Differential Revision: https://reviews.freebsd.org/D48383 > --- > sys/conf/files | 1 + > sys/kern/kern_clock.c | 26 ++++++++++++++------------ > sys/kern/kern_tc.c | 4 ++-- > sys/kern/subr_param.c | 2 +- > sys/kern/subr_ticks.s | 44 ++++++++++++++++++++++++++++++++++++++++++++ > sys/sys/kernel.h | 9 +++++++++ > sys/sys/timetc.h | 2 +- > 7 files changed, 72 insertions(+), 16 deletions(-) > > diff --git a/sys/conf/files b/sys/conf/files > index d358737c5613..a630d9dd72bc 100644 > --- a/sys/conf/files > +++ b/sys/conf/files > @@ -3932,6 +3932,7 @@ kern/subr_stack.c optional ddb | stack | ktr > kern/subr_stats.c optional stats > kern/subr_taskqueue.c standard > kern/subr_terminal.c optional vt > +kern/subr_ticks.s standard > kern/subr_trap.c standard > kern/subr_turnstile.c standard > kern/subr_uio.c standard > diff --git a/sys/kern/kern_clock.c b/sys/kern/kern_clock.c > index 6fa2272ed54a..b11c0d235139 100644 > --- a/sys/kern/kern_clock.c > +++ b/sys/kern/kern_clock.c > @@ -323,7 +323,7 @@ read_cpu_time(long *cp_time) > > #include <sys/watchdog.h> > > -static int watchdog_ticks; > +static long watchdog_ticks; > static int watchdog_enabled; > static void watchdog_fire(void); > static void watchdog_config(void *, u_int, int *); > @@ -369,10 +369,9 @@ watchdog_attach(void) > int stathz; > int profhz; > int profprocs; > -volatile int ticks; > int psratio; > > -DPCPU_DEFINE_STATIC(int, pcputicks); /* Per-CPU version of ticks. */ > +DPCPU_DEFINE_STATIC(long, pcputicks); /* Per-CPU version of ticks. */ > #ifdef DEVICE_POLLING > static int devpoll_run = 0; > #endif > @@ -480,14 +479,14 @@ hardclock(int cnt, int usermode) > struct pstats *pstats; > struct thread *td = curthread; > struct proc *p = td->td_proc; > - int *t = DPCPU_PTR(pcputicks); > - int global, i, newticks; > + long global, newticks, *t; > > /* > * Update per-CPU and possibly global ticks values. > */ > + t = DPCPU_PTR(pcputicks); > *t += cnt; > - global = ticks; > + global = atomic_load_long(&ticksl); > do { > newticks = *t - global; > if (newticks <= 0) { > @@ -496,7 +495,7 @@ hardclock(int cnt, int usermode) > newticks = 0; > break; > } > - } while (!atomic_fcmpset_int(&ticks, &global, *t)); > + } while (!atomic_fcmpset_long(&ticksl, &global, *t)); > > /* > * Run current process's virtual and profile time, as needed. > @@ -525,8 +524,10 @@ hardclock(int cnt, int usermode) > } > #endif /* DEVICE_POLLING */ > if (watchdog_enabled > 0) { > - i = atomic_fetchadd_int(&watchdog_ticks, -newticks); > - if (i > 0 && i <= newticks) > + long left; > + > + left = atomic_fetchadd_long(&watchdog_ticks, -newticks); > + if (left > 0 && left <= newticks) > watchdog_fire(); > } > intr_event_handle(clk_intr_event, NULL); > @@ -540,11 +541,12 @@ hardclock(int cnt, int usermode) > void > hardclock_sync(int cpu) > { > - int *t; > + long *t; > + > KASSERT(!CPU_ABSENT(cpu), ("Absent CPU %d", cpu)); > - t = DPCPU_ID_PTR(cpu, pcputicks); > > - *t = ticks; > + t = DPCPU_ID_PTR(cpu, pcputicks); > + *t = ticksl; > } > > /* > diff --git a/sys/kern/kern_tc.c b/sys/kern/kern_tc.c > index 26f09cb60260..a797a101bf6f 100644 > --- a/sys/kern/kern_tc.c > +++ b/sys/kern/kern_tc.c > @@ -1916,9 +1916,9 @@ SYSCTL_INT(_kern_timecounter, OID_AUTO, tick, CTLFLAG_RD, &tc_tick, 0, > "Approximate number of hardclock ticks in a millisecond"); > > void > -tc_ticktock(int cnt) > +tc_ticktock(long cnt) > { > - static int count; > + static long count; > > if (mtx_trylock_spin(&tc_setclock_mtx)) { > count += cnt; > diff --git a/sys/kern/subr_param.c b/sys/kern/subr_param.c > index 19169ba63061..f4359efec466 100644 > --- a/sys/kern/subr_param.c > +++ b/sys/kern/subr_param.c > @@ -197,7 +197,7 @@ init_param1(void) > * Arrange for ticks to wrap 10 minutes after boot to help catch > * sign problems sooner. > */ > - ticks = INT_MAX - (hz * 10 * 60); > + ticksl = INT_MAX - (hz * 10 * 60); > > vn_lock_pair_pause_max = hz / 100; > if (vn_lock_pair_pause_max == 0) > diff --git a/sys/kern/subr_ticks.s b/sys/kern/subr_ticks.s > new file mode 100644 > index 000000000000..6565ba424137 > --- /dev/null > +++ b/sys/kern/subr_ticks.s > @@ -0,0 +1,44 @@ > +/*- > + * SPDX-License-Identifier: BSD-2-Clause > + * > + * Copyright (c) 2025 Mark Johnston <markj@FreeBSD.org> > + */ > + > +/* > + * Define the "ticks" and "ticksl" variables. The former is overlaid onto the > + * low bits of the latter. > + */ > + > +#if defined(__aarch64__) > +#include <sys/elf_common.h> > +#include <machine/asm.h> > + > +GNU_PROPERTY_AARCH64_FEATURE_1_NOTE(GNU_PROPERTY_AARCH64_FEATURE_1_VAL) > +#endif > + > +#ifdef _ILP32 > +#define SIZEOF_TICKSL 4 > +#define TICKSL_INIT .long 0 > +#else > +#define SIZEOF_TICKSL 8 > +#define TICKSL_INIT .quad 0 > +#endif > + > +#if defined(_ILP32) || __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ > +#define TICKS_OFFSET 0 > +#else > +#define TICKS_OFFSET 4 > +#endif > + > + .data > + > + .global ticksl > + .type ticksl, %object > + .align SIZEOF_TICKSL > +ticksl: TICKSL_INIT > + .size ticksl, SIZEOF_TICKSL > + > + .global ticks > + .type ticks, %object > +ticks =ticksl + TICKS_OFFSET > + .size ticks, 4 This can be simplified to: #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ #define TICKS_OFFSET 0 #else #define TICKS_OFFSET (__SIZEOF_LONG__ - __SIZEOF_INT__) #endif .data .global ticksl .type ticksl, %object .align __SIZEOF_LONG__ ticksl: .zero __SIZEOF_LONG__ .size ticksl, __SIZEOF_LONG__ .global ticks .type ticks, %object ticks =ticksl + TICKS_OFFSET .size ticks, __SIZEOF_INT__ (excuse my mail client stripping the tabs...) No need to check the ABI beyond endianness. Also, shouldn’t these both be in .bss? Jess
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?9B1B709F-5828-489C-81B5-74ED9E4502FC>