Re: git: 6b82130e6c9a - main - clock: Add a long ticks variable, ticksl
Date: Fri, 10 Jan 2025 16:37:08 UTC
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