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

Reply: Warner Losh : "Re: git: 6b82130e6c9a - main - clock: Add a long ticks variable, ticksl"
In reply to: Jessica Clarke : "Re: git: 6b82130e6c9a - main - clock: Add a long ticks variable, ticksl"
Go to: [ bottom of page ] [ top of archives ] [ this month ]
From: Jessica Clarke <jrtc27_at_freebsd.org>
Date: Fri, 10 Jan 2025 16:55:28 UTC
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...