Re: kernel: don't jump ticks, jiffies during boot
On Mon, Feb 27, 2023 at 08:48:53PM -0600, Scott Cheloha wrote:
> On Tue, Feb 28, 2023 at 01:01:32PM +1100, Jonathan Gray wrote:
> > On Mon, Feb 27, 2023 at 06:26:00PM -0600, Scott Cheloha wrote:
> > > On Tue, Feb 28, 2023 at 10:18:16AM +1100, Jonathan Gray wrote:
> > > > On Mon, Feb 27, 2023 at 04:57:04PM -0600, Scott Cheloha wrote:
> > > > > ticks and jiffies start at zero. During boot in initclocks(), we
> > > > > reset them:
> > > > >
> > > > > /* sys/kern/kern_clock.c */
> > > > >
> > > > > 89int ticks;
> > > > > 90static int psdiv, pscnt;/* prof => stat
> > > > > divider */
> > > > > 91int psratio;/* ratio: prof
> > > > > / stat */
> > > > > 92
> > > > > 93volatile unsigned long jiffies; /* XXX Linux
> > > > > API for drm(4) */
> > > > > 94
> > > > > 95/*
> > > > > 96 * Initialize clock frequencies and start both clocks
> > > > > running.
> > > > > 97 */
> > > > > 98void
> > > > > 99initclocks(void)
> > > > >100{
> > > > >101ticks = INT_MAX - (15 * 60 * hz);
> > > > >102jiffies = ULONG_MAX - (10 * 60 * hz);
> > > > >103
> > > > >104/* [... ] */
> > > > >
> > > > > The idea here (committed by dlg@) is sound. We reset ticks and
> > > > > jiffies to near-rollover values to catch buggy code misusing them.
> > > > >
> > > > > But! That jump from zero to whatever violates valid assumptions made
> > > > > by correct code, too.
> > > >
> > > > Assumptions made by what code? Does it exist in the tree?
> > >
> > > First, even if the code did not exist, wouldn't it be simpler to not
> > > do the jump? No?
> >
> > There are enough problems to fix without chasing ones that
> > don't exist.
> >
> > > Second, with rare exception, all kernel code using ticks/jiffies
> > > assumes ticks/jiffies does not advance more than once every 1/hz
> > > seconds on average.
> > >
> > > In timeout_add(9), we assign an absolute expiration time relative
> > > to the current value of ticks. Code calling timeout_add(9) before
> > > initclocks() cannot account for the jump in initclocks().
> >
> > What code calling timeout_add() before initclocks()?
>
> I count 8 calls on my laptop:
>
> [...]
>
> Jumping ticks/jiffies as we do in initclocks() violates assumptions
> about how ticks/jiffies work. If we initialize ticks/jiffies to
> values near rollover we can keep the test of correct behavior intended
> by dlg's patch without surprising other code.
>
> ok?
>
> Is there a better place to put "jiffies"?
kettenis@ suggested finding a way to initialize ticks/jiffies without
moving them.
HZ is not visible outside of param.c. If we move the conditional
definition of HZ from param.c into sys/kernel.h (where hz(9) is
extern'd), kern_clock.c can use it. I don't see a more obvious place
to put HZ than sys/kernel.h.
This fixes the problem. The timeouts no longer fire early.
Can I go with this?
Index: kern/kern_clock.c
===
RCS file: /cvs/src/sys/kern/kern_clock.c,v
retrieving revision 1.106
diff -u -p -r1.106 kern_clock.c
--- kern/kern_clock.c 4 Feb 2023 19:33:03 - 1.106
+++ kern/kern_clock.c 1 Mar 2023 16:34:50 -
@@ -86,11 +86,11 @@ int stathz;
intschedhz;
intprofhz;
intprofprocs;
-intticks;
+intticks = INT_MAX - (15 * 60 * HZ);
static int psdiv, pscnt; /* prof => stat divider */
intpsratio;/* ratio: prof / stat */
-volatile unsigned long jiffies;/* XXX Linux API for drm(4) */
+volatile unsigned long jiffies = ULONG_MAX - (10 * 60 * HZ);
/*
* Initialize clock frequencies and start both clocks running.
@@ -98,9 +98,6 @@ volatile unsigned long jiffies; /* XXX
void
initclocks(void)
{
- ticks = INT_MAX - (15 * 60 * hz);
- jiffies = ULONG_MAX - (10 * 60 * hz);
-
/*
* Set divisors to 1 (normal case) and let the machine-specific
* code do its bit.
Index: sys/kernel.h
===
RCS file: /cvs/src/sys/sys/kernel.h,v
retrieving revision 1.25
diff -u -p -r1.25 kernel.h
--- sys/kernel.h13 Jan 2021 16:28:50 - 1.25
+++ sys/kernel.h1 Mar 2023 16:34:50 -
@@ -56,3 +56,7 @@ extern int hz;/* system clock's frequ
extern int stathz; /* statistics clock's frequency */
extern int profhz; /* profiling clock's frequency */
extern int lbolt; /* once a second sleep address */
+
+#ifndef HZ
+#define HZ 100
+#endif
Index: conf/param.c
===
RCS file: /cvs/src/sys/conf/param.c,v
retrieving revision 1.47
diff -u -p -r1.47 param.c
---
Re: kernel: don't jump ticks, jiffies during boot
On Tue, Feb 28, 2023 at 01:01:32PM +1100, Jonathan Gray wrote:
> On Mon, Feb 27, 2023 at 06:26:00PM -0600, Scott Cheloha wrote:
> > On Tue, Feb 28, 2023 at 10:18:16AM +1100, Jonathan Gray wrote:
> > > On Mon, Feb 27, 2023 at 04:57:04PM -0600, Scott Cheloha wrote:
> > > > ticks and jiffies start at zero. During boot in initclocks(), we
> > > > reset them:
> > > >
> > > > /* sys/kern/kern_clock.c */
> > > >
> > > > 89 int ticks;
> > > > 90 static int psdiv, pscnt;/* prof => stat divider
> > > > */
> > > > 91 int psratio;/* ratio: prof / stat */
> > > > 92
> > > > 93 volatile unsigned long jiffies; /* XXX Linux API for
> > > > drm(4) */
> > > > 94
> > > > 95 /*
> > > > 96 * Initialize clock frequencies and start both clocks running.
> > > > 97 */
> > > > 98 void
> > > > 99 initclocks(void)
> > > >100 {
> > > >101 ticks = INT_MAX - (15 * 60 * hz);
> > > >102 jiffies = ULONG_MAX - (10 * 60 * hz);
> > > >103
> > > >104 /* [... ] */
> > > >
> > > > The idea here (committed by dlg@) is sound. We reset ticks and
> > > > jiffies to near-rollover values to catch buggy code misusing them.
> > > >
> > > > But! That jump from zero to whatever violates valid assumptions made
> > > > by correct code, too.
> > >
> > > Assumptions made by what code? Does it exist in the tree?
> >
> > First, even if the code did not exist, wouldn't it be simpler to not
> > do the jump? No?
>
> There are enough problems to fix without chasing ones that
> don't exist.
>
> > Second, with rare exception, all kernel code using ticks/jiffies
> > assumes ticks/jiffies does not advance more than once every 1/hz
> > seconds on average.
> >
> > In timeout_add(9), we assign an absolute expiration time relative
> > to the current value of ticks. Code calling timeout_add(9) before
> > initclocks() cannot account for the jump in initclocks().
>
> What code calling timeout_add() before initclocks()?
I count 8 calls on my laptop:
#0 timeout_add+0x43
#1 random_start+0xb7
#2 main+0x96
#3 longmode_hi+0x9c
#0 timeout_add+0x43
#1 thinkpad_attach+0x1f9
#2 config_attach+0x1f4
#3 acpi_foundhid+0x326
#4 aml_find_node+0x74
#5 aml_find_node+0xa1
#6 aml_find_node+0xa1
#7 aml_find_node+0xa1
#8 aml_find_node+0xa1
#9 aml_find_node+0xa1
#10 acpi_attach_common+0x6f4
#11 config_attach+0x1f4
#12 bios_attach+0x74f
#13 config_attach+0x1f4
#14 mainbus_attach+0x7b
#15 config_attach+0x1f4
#16 config_rootfound+0xd2
#17 cpu_configure+0x2a
#18 main+0x3a8
#0 timeout_add+0x43
#1 acpibat_attach+0x171
#2 config_attach+0x1f4
#3 acpi_foundhid+0x326
#4 aml_find_node+0x74
#5 aml_find_node+0xa1
#6 aml_find_node+0xa1
#7 aml_find_node+0xa1
#8 aml_find_node+0xa1
#9 aml_find_node+0xa1
#10 acpi_attach_common+0x6f4
#11 config_attach+0x1f4
#12 bios_attach+0x74f
#13 config_attach+0x1f4
#14 mainbus_attach+0x7b
#15 config_attach+0x1f4
#16 config_rootfound+0xd2
#17 cpu_configure+0x2a
#18 main+0x3a8
#0 timeout_add+0x43
#1 acpitz_attach+0x559
#2 config_attach+0x1f4
#3 acpi_add_device+0x147
#4 aml_walknodes+0x3b
#5 aml_walknodes+0x61
#6 aml_walknodes+0x61
#7 acpi_attach_common+0x712
#8 config_attach+0x1f4
#9 bios_attach+0x74f
#10 config_attach+0x1f4
#11 mainbus_attach+0x7b
#12 config_attach+0x1f4
#13 config_rootfound+0xd2
#14 cpu_configure+0x2a
#15 main+0x3a8
#0 timeout_add+0x43
#1 if_attachsetup+0x102
#2 if_attach+0x4e
#3 iwm_attach+0xe5a
#4 config_attach+0x1f4
#5 pci_probe_device+0x515
#6 pci_enumerate_bus+0x189
#7 config_attach+0x1f4
#8 ppbattach+0x790
#9 config_attach+0x1f4
#10 pci_probe_device+0x515
#11 pci_enumerate_bus+0x189
#12 config_attach+0x1f4
#13 acpipci_attach_bus+0x1b3
#14 acpipci_attach_busses+0x4d
#15 mainbus_attach+0x1c6
#16 config_attach+0x1f4
#17 config_rootfound+0xd2
#18 cpu_configure+0x2a
#0 timeout_add+0x43
#1 if_attachsetup+0x102
#2 if_attach+0x4e
#3 em_setup_interface+0x1c6
#4 em_attach+0x401
#5 config_attach+0x1f4
#6 pci_probe_device+0x515
#7 pci_enumerate_bus+0x189
#8 config_attach+0x1f4
#9 acpipci_attach_bus+0x1b3
#10 acpipci_attach_busses+0x4d
#11 mainbus_attach+0x1c6
#12 config_attach+0x1f4
#13 config_rootfound+0xd2
#14 cpu_configure+0x2a
#15 main+0x3a8
#0 timeout_add+0x43
#1 pckbdattach+0x172
#2 config_attach+0x1f4
#3 pckbc_attach+0x239
#4 pckbc_isa_attach+0x17c
#5 config_attach+0x1f4
#6 isascan+0x309
#7 config_scan+0xab
#8 config_attach+0x1f4
#9 pcib_callback+0x62
#10 config_process_deferred_children+0x83
#11 config_attach+0x1fc
#12 acpipci_attach_bus+0x1b3
#13 acpipci_attach_busses+0x4d
#14 mainbus_attach+0x1c6
#15 config_attach+0x1f4
#16 config_rootfound+0xd2
#17 cpu_configure+0x2a
#18 main+0x3a8
#0 timeout_add+0x43
#1 azalia_rirb_intr+0x67
#2 azalia_intr+0xe1
#3 intr_handler+0x67
#4 Xintr_ioapic_edge22_untramp+0x18f
#5 Xspllower+0x17
#6 cpu_configure+0x76
#7 main+0x3a8
#0 timeout_add+0x43
#1
Re: kernel: don't jump ticks, jiffies during boot
On Mon, Feb 27, 2023 at 06:26:00PM -0600, Scott Cheloha wrote:
> On Tue, Feb 28, 2023 at 10:18:16AM +1100, Jonathan Gray wrote:
> > On Mon, Feb 27, 2023 at 04:57:04PM -0600, Scott Cheloha wrote:
> > > ticks and jiffies start at zero. During boot in initclocks(), we
> > > reset them:
> > >
> > > /* sys/kern/kern_clock.c */
> > >
> > > 89int ticks;
> > > 90static int psdiv, pscnt;/* prof => stat divider
> > > */
> > > 91int psratio;/* ratio: prof / stat */
> > > 92
> > > 93volatile unsigned long jiffies; /* XXX Linux API for
> > > drm(4) */
> > > 94
> > > 95/*
> > > 96 * Initialize clock frequencies and start both clocks running.
> > > 97 */
> > > 98void
> > > 99initclocks(void)
> > >100{
> > >101ticks = INT_MAX - (15 * 60 * hz);
> > >102jiffies = ULONG_MAX - (10 * 60 * hz);
> > >103
> > >104/* [... ] */
> > >
> > > The idea here (committed by dlg@) is sound. We reset ticks and
> > > jiffies to near-rollover values to catch buggy code misusing them.
> > >
> > > But! That jump from zero to whatever violates valid assumptions made
> > > by correct code, too.
> >
> > Assumptions made by what code? Does it exist in the tree?
>
> First, even if the code did not exist, wouldn't it be simpler to not
> do the jump? No?
There are enough problems to fix without chasing ones that
don't exist.
>
> Second, with rare exception, all kernel code using ticks/jiffies
> assumes ticks/jiffies does not advance more than once every 1/hz
> seconds on average.
>
> In timeout_add(9), we assign an absolute expiration time relative
> to the current value of ticks. Code calling timeout_add(9) before
> initclocks() cannot account for the jump in initclocks().
What code calling timeout_add() before initclocks()?
>
> There is probably equivalent code in drm(4) making the same
> assumption.
The vast majority of drm runs after root is mounted.
>
> Relatedly, in cpu_relax() we increment jiffies if the kernel is
> cold:
>
> sys/dev/pci/drm/include/linux/processor.h
>
> 12 static inline void
> 13 cpu_relax(void)
> 14 {
> 15 CPU_BUSY_CYCLE();
> 16 if (cold) {
> 17 delay(tick);
> 18 jiffies++;
> 19 }
> 20 }
>
Re: kernel: don't jump ticks, jiffies during boot
On Tue, Feb 28, 2023 at 10:18:16AM +1100, Jonathan Gray wrote:
> On Mon, Feb 27, 2023 at 04:57:04PM -0600, Scott Cheloha wrote:
> > ticks and jiffies start at zero. During boot in initclocks(), we
> > reset them:
> >
> > /* sys/kern/kern_clock.c */
> >
> > 89 int ticks;
> > 90 static int psdiv, pscnt;/* prof => stat divider
> > */
> > 91 int psratio;/* ratio: prof / stat */
> > 92
> > 93 volatile unsigned long jiffies; /* XXX Linux API for
> > drm(4) */
> > 94
> > 95 /*
> > 96 * Initialize clock frequencies and start both clocks running.
> > 97 */
> > 98 void
> > 99 initclocks(void)
> >100 {
> >101 ticks = INT_MAX - (15 * 60 * hz);
> >102 jiffies = ULONG_MAX - (10 * 60 * hz);
> >103
> >104 /* [... ] */
> >
> > The idea here (committed by dlg@) is sound. We reset ticks and
> > jiffies to near-rollover values to catch buggy code misusing them.
> >
> > But! That jump from zero to whatever violates valid assumptions made
> > by correct code, too.
>
> Assumptions made by what code? Does it exist in the tree?
First, even if the code did not exist, wouldn't it be simpler to not
do the jump? No?
Second, with rare exception, all kernel code using ticks/jiffies
assumes ticks/jiffies does not advance more than once every 1/hz
seconds on average.
In timeout_add(9), we assign an absolute expiration time relative
to the current value of ticks. Code calling timeout_add(9) before
initclocks() cannot account for the jump in initclocks().
There is probably equivalent code in drm(4) making the same
assumption.
Relatedly, in cpu_relax() we increment jiffies if the kernel is
cold:
sys/dev/pci/drm/include/linux/processor.h
12 static inline void
13 cpu_relax(void)
14 {
15 CPU_BUSY_CYCLE();
16 if (cold) {
17 delay(tick);
18 jiffies++;
19 }
20 }
Re: kernel: don't jump ticks, jiffies during boot
On Mon, Feb 27, 2023 at 04:57:04PM -0600, Scott Cheloha wrote:
> ticks and jiffies start at zero. During boot in initclocks(), we
> reset them:
>
> /* sys/kern/kern_clock.c */
>
> 89int ticks;
> 90static int psdiv, pscnt;/* prof => stat divider
> */
> 91int psratio;/* ratio: prof / stat */
> 92
> 93volatile unsigned long jiffies; /* XXX Linux API for
> drm(4) */
> 94
> 95/*
> 96 * Initialize clock frequencies and start both clocks running.
> 97 */
> 98void
> 99initclocks(void)
>100{
>101ticks = INT_MAX - (15 * 60 * hz);
>102jiffies = ULONG_MAX - (10 * 60 * hz);
>103
>104/* [... ] */
>
> The idea here (committed by dlg@) is sound. We reset ticks and
> jiffies to near-rollover values to catch buggy code misusing them.
>
> But! That jump from zero to whatever violates valid assumptions made
> by correct code, too.
Assumptions made by what code? Does it exist in the tree?
>
> It would be better to just initialize ticks and jiffies to the
> near-rollover values when we declare them. To do this we need to
> move their declarations from sys/kern/kern_clock.c to sys/conf/param.c
> where HZ is visible.
>
> ok?
>
> Index: kern/kern_clock.c
> ===
> RCS file: /cvs/src/sys/kern/kern_clock.c,v
> retrieving revision 1.106
> diff -u -p -r1.106 kern_clock.c
> --- kern/kern_clock.c 4 Feb 2023 19:33:03 - 1.106
> +++ kern/kern_clock.c 27 Feb 2023 22:55:24 -
> @@ -86,21 +86,15 @@ int stathz;
> int schedhz;
> int profhz;
> int profprocs;
> -int ticks;
> static int psdiv, pscnt; /* prof => stat divider */
> int psratio;/* ratio: prof / stat */
>
> -volatile unsigned long jiffies; /* XXX Linux API for drm(4) */
> -
> /*
> * Initialize clock frequencies and start both clocks running.
> */
> void
> initclocks(void)
> {
> - ticks = INT_MAX - (15 * 60 * hz);
> - jiffies = ULONG_MAX - (10 * 60 * hz);
> -
> /*
>* Set divisors to 1 (normal case) and let the machine-specific
>* code do its bit.
> @@ -171,7 +165,8 @@ hardclock(struct clockframe *frame)
>
> tc_ticktock();
> ticks++;
> - jiffies++;
> + extern volatile unsigned long jiffies;
> + jiffies++; /* XXX drm(4) */
>
> /*
>* Update the timeout wheel.
> Index: conf/param.c
> ===
> RCS file: /cvs/src/sys/conf/param.c,v
> retrieving revision 1.47
> diff -u -p -r1.47 param.c
> --- conf/param.c 13 Apr 2022 10:08:10 - 1.47
> +++ conf/param.c 27 Feb 2023 22:55:24 -
> @@ -73,6 +73,8 @@
> #define HZ 100
> #endif
> int hz = HZ;
> +int ticks = INT_MAX - (15 * 60 * HZ);
> +volatile unsigned long jiffies = ULONG_MAX - (10 * 60 * HZ); /* drm(4) */
> int tick = 100 / HZ;
> int tick_nsec = 10 / HZ;
> int utc_offset = 0;
>
