Re: git: 6b82130e6c9a - main - clock: Add a long ticks variable, ticksl

On Fri, Jan 10, 2025 at 9:55 AM Jessica Clarke <jrtc27@freebsd.org> wrote:

> On 10 Jan 2025, at 16:37, Jessica Clarke <jrtc27@FreeBSD.org> wrote:
> > 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?
>
> And one more thing: the convention is to use .S, not .s, for assembly
> files, since the former are preprocessed[^1]. I’m surprised this even
> builds when given the lower case suffix, as bsd.suffixes.mk passes
> -x assembler for the .s.o rule.
>
> Jess
>
> [^1]: Unless you know it doesn’t need preprocessing and never will...
>

I had this same comment in a review, but was busy and didn't get to make it.

We *NEVER* have .s except for weird things we might generate, by convention.
We spent a lot of time fixing this a few months ago.

Warner