The KGDB initial breakpoint gets an rcu stall warning after commit a80be428fbc1f1f3bc9ed924 ("rcu: Do not disable GP stall detection in rcu_cpu_stall_reset()").
[ 53.452051] rcu: INFO: rcu_preempt self-detected stall on CPU [ 53.487950] rcu: 3-...0: (1 ticks this GP) idle=0e2c/1/0x4000000000000000 softirq=375/375 fqs=8 [ 53.528243] rcu: (t=12297 jiffies g=-995 q=1 ncpus=4) [ 53.564840] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 6.5.0-rc2+ #4848 [ 53.603005] Hardware name: Loongson Loongson-3A5000-HV-7A2000-1w-V0.1-CRB/Loongson-LS3A5000-7A2000-1w-CRB-V1.21, BIOS Loongson-UDK2018-V2.0.05099-beta8 08 [ 53.682062] pc 9000000000332100 ra 90000000003320f4 tp 90000001000a0000 sp 90000001000a3710 [ 53.724934] a0 9000000001d4b488 a1 0000000000000000 a2 0000000000000001 a3 0000000000000000 [ 53.768179] a4 9000000001d526c8 a5 90000001000a38f0 a6 000000000000002c a7 0000000000000000 [ 53.810751] t0 00000000000002b0 t1 0000000000000004 t2 900000000131c9c0 t3 fffffffffffffffa [ 53.853249] t4 0000000000000080 t5 90000001002ac190 t6 0000000000000004 t7 9000000001912d58 [ 53.895684] t8 0000000000000000 u0 90000000013141a0 s9 0000000000000028 s0 9000000001d512f0 [ 53.937633] s1 9000000001d51278 s2 90000001000a3798 s3 90000000019fc410 s4 9000000001d4b488 [ 53.979486] s5 9000000001d512f0 s6 90000000013141a0 s7 0000000000000078 s8 9000000001d4b450 [ 54.021175] ra: 90000000003320f4 kgdb_cpu_enter+0x534/0x640 [ 54.060150] ERA: 9000000000332100 kgdb_cpu_enter+0x540/0x640 [ 54.098347] CRMD: 000000b0 (PLV0 -IE -DA +PG DACF=CC DACM=CC -WE) [ 54.136621] PRMD: 0000000c (PPLV0 +PIE +PWE) [ 54.172192] EUEN: 00000000 (-FPE -SXE -ASXE -BTE) [ 54.207838] ECFG: 00071c1c (LIE=2-4,10-12 VS=7) [ 54.242503] ESTAT: 00000800 [INT] (IS=11 ECode=0 EsubCode=0) [ 54.277996] PRID: 0014c011 (Loongson-64bit, Loongson-3A5000-HV) [ 54.313544] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 6.5.0-rc2+ #4848 [ 54.430170] Stack : 0072617764726148 0000000000000000 9000000000223504 90000001000a0000 [ 54.472308] 9000000100073a90 9000000100073a98 0000000000000000 9000000100073bd8 [ 54.514413] 9000000100073bd0 9000000100073bd0 9000000100073a00 0000000000000001 [ 54.556018] 0000000000000001 9000000100073a98 99828271f24e961a 90000001002810c0 [ 54.596924] 0000000000000001 0000000000010003 0000000000000000 0000000000000001 [ 54.637115] ffff8000337cdb80 0000000000000001 0000000006360000 900000000131c9c0 [ 54.677049] 0000000000000000 0000000000000000 90000000017b4c98 9000000001912000 [ 54.716394] 9000000001912f68 9000000001913000 9000000001912f70 00000000000002b0 [ 54.754880] 90000000014a8840 0000000000000000 900000000022351c 0000000000000000 [ 54.792372] 00000000000002b0 000000000000000c 0000000000000000 0000000000071c1c [ 54.829302] ... [ 54.859163] Call Trace: [ 54.859165] [<900000000022351c>] show_stack+0x5c/0x180 [ 54.918298] [<90000000012f6100>] dump_stack_lvl+0x60/0x88 [ 54.949251] [<90000000012dd5d8>] rcu_dump_cpu_stacks+0xf0/0x148 [ 54.981116] [<90000000002d2fb8>] rcu_sched_clock_irq+0xb78/0xe60 [ 55.012744] [<90000000002e47cc>] update_process_times+0x6c/0xc0 [ 55.044169] [<90000000002f65d4>] tick_sched_timer+0x54/0x100 [ 55.075488] [<90000000002e5174>] __hrtimer_run_queues+0x154/0x240 [ 55.107347] [<90000000002e6288>] hrtimer_interrupt+0x108/0x2a0 [ 55.139112] [<9000000000226418>] constant_timer_interrupt+0x38/0x60 [ 55.170749] [<90000000002b3010>] __handle_irq_event_percpu+0x50/0x160 [ 55.203141] [<90000000002b3138>] handle_irq_event_percpu+0x18/0x80 [ 55.235064] [<90000000002b9d54>] handle_percpu_irq+0x54/0xa0 [ 55.266241] [<90000000002b2168>] generic_handle_domain_irq+0x28/0x40 [ 55.298466] [<9000000000aba95c>] handle_cpu_irq+0x5c/0xa0 [ 55.329749] [<90000000012f7270>] handle_loongarch_irq+0x30/0x60 [ 55.361476] [<90000000012f733c>] do_vint+0x9c/0x100 [ 55.391737] [<9000000000332100>] kgdb_cpu_enter+0x540/0x640 [ 55.422440] [<9000000000332b64>] kgdb_handle_exception+0x104/0x180 [ 55.452911] [<9000000000232478>] kgdb_loongarch_notify+0x38/0xa0 [ 55.481964] [<900000000026b4d4>] notify_die+0x94/0x100 [ 55.509184] [<90000000012f685c>] do_bp+0x21c/0x340 [ 55.562475] [<90000000003315b8>] kgdb_compiled_break+0x0/0x28 [ 55.590319] [<9000000000332e80>] kgdb_register_io_module+0x160/0x1c0 [ 55.618901] [<9000000000c0f514>] configure_kgdboc+0x154/0x1c0 [ 55.647034] [<9000000000c0f5e0>] kgdboc_probe+0x60/0x80 [ 55.674647] [<9000000000c96da8>] platform_probe+0x68/0x100 [ 55.702613] [<9000000000c938e0>] really_probe+0xc0/0x340 [ 55.730528] [<9000000000c93be4>] __driver_probe_device+0x84/0x140 [ 55.759615] [<9000000000c93cdc>] driver_probe_device+0x3c/0x120 [ 55.787990] [<9000000000c93e8c>] __device_attach_driver+0xcc/0x160 [ 55.817145] [<9000000000c91290>] bus_for_each_drv+0x90/0x100 [ 55.845654] [<9000000000c94328>] __device_attach+0xa8/0x1a0 [ 55.874145] [<9000000000c925f0>] bus_probe_device+0xb0/0xe0 [ 55.902572] [<9000000000c8ec7c>] device_add+0x65c/0x860 [ 55.930635] [<9000000000c96704>] platform_device_add+0x124/0x2c0 [ 55.959669] [<9000000001452b38>] init_kgdboc+0x58/0xa0 [ 55.987677] [<900000000022015c>] do_one_initcall+0x7c/0x1e0 [ 56.016134] [<9000000001420f1c>] kernel_init_freeable+0x22c/0x2a0 [ 56.045128] [<90000000012f923c>] kernel_init+0x20/0x124
Currently rcu_cpu_stall_reset() set rcu_state.jiffies_stall to one check period later, i.e. jiffies + rcu_jiffies_till_stall_check(). But jiffies is only updated in the timer interrupt, so when kgdb_cpu_enter() begins to run there may already be nearly one rcu check period after jiffies. Since all interrupts are disabled during kgdb_cpu_enter(), jiffies will not be updated. When kgdb_cpu_enter() returns, rcu_state.jiffies_stall maybe already gets timeout.
We can set rcu_state.jiffies_stall to two rcu check periods later, e.g. jiffies + (rcu_jiffies_till_stall_check() * 2) in rcu_cpu_stall_reset() to avoid this problem. But this isn't a complete solution because kgdb may take a very long time in irq disabled context.
Instead, update jiffies at the beginning of rcu_cpu_stall_reset() can solve all kinds of problems [1]. But this causes a new problem because updating jiffies is not NMI safe while rcu_cpu_stall_reset() may be used in NMI context.
So we don't update the global jiffies, but only add the time 'delta' to jiffies locally at the beginning of rcu_cpu_stall_reset() which has the same effect.
[1] https://lore.kernel.org/rcu/20230814020045.51950-1-chenhuacai@loongson.cn/T/...
Cc: stable@vger.kernel.org Fixes: a80be428fbc1f1f3bc9ed924 ("rcu: Do not disable GP stall detection in rcu_cpu_stall_reset()") Reported-off-by: Binbin Zhou zhoubinbin@loongson.cn Signed-off-by: Huacai Chen chenhuacai@loongson.cn --- V2: Use NMI safe functions. V3: Add comments to explain why.
kernel/rcu/tree_stall.h | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h index b10b8349bb2a..e4e53113d062 100644 --- a/kernel/rcu/tree_stall.h +++ b/kernel/rcu/tree_stall.h @@ -150,11 +150,26 @@ static void panic_on_rcu_stall(void) * rcu_cpu_stall_reset - restart stall-warning timeout for current grace period * * The caller must disable hard irqs. + * + * The jiffies updating may be delayed for a very long time due to tickless and + * irq disabled, especially in the KGDB case, so we need to add the delayed time + * (delta) to rcu_state.jiffies_stall. + * + * This function may be called in NMI context, so we cannot use ktime_get_ns() + * and ktime_get_coarse_ns(). Instead, we use their inaccurate but safe friends + * ktime_get_mono_fast_ns() and ktime_get_seconds() which will cause rcu_state. + * jiffies_stall to be a little large than expected (harmless and safer). */ void rcu_cpu_stall_reset(void) { + u64 curr, last, delta; + + curr = ktime_get_mono_fast_ns(); + last = ktime_get_seconds() * NSEC_PER_SEC; + delta = nsecs_to_jiffies(curr - last); + WRITE_ONCE(rcu_state.jiffies_stall, - jiffies + rcu_jiffies_till_stall_check()); + jiffies + delta + rcu_jiffies_till_stall_check()); }
//////////////////////////////////////////////////////////////////////////////
On Tue, Aug 22, 2023 at 12:02:48PM +0800, Huacai Chen wrote:
The KGDB initial breakpoint gets an rcu stall warning after commit a80be428fbc1f1f3bc9ed924 ("rcu: Do not disable GP stall detection in rcu_cpu_stall_reset()").
[ 53.452051] rcu: INFO: rcu_preempt self-detected stall on CPU
[...]
[1] https://lore.kernel.org/rcu/20230814020045.51950-1-chenhuacai@loongson.cn/T/...
Cc: stable@vger.kernel.org Fixes: a80be428fbc1f1f3bc9ed924 ("rcu: Do not disable GP stall detection in rcu_cpu_stall_reset()") Reported-off-by: Binbin Zhou zhoubinbin@loongson.cn Signed-off-by: Huacai Chen chenhuacai@loongson.cn
V2: Use NMI safe functions. V3: Add comments to explain why.
kernel/rcu/tree_stall.h | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h index b10b8349bb2a..e4e53113d062 100644 --- a/kernel/rcu/tree_stall.h +++ b/kernel/rcu/tree_stall.h @@ -150,11 +150,26 @@ static void panic_on_rcu_stall(void)
- rcu_cpu_stall_reset - restart stall-warning timeout for current grace period
- The caller must disable hard irqs.
- The jiffies updating may be delayed for a very long time due to tickless and
- irq disabled, especially in the KGDB case, so we need to add the delayed time
- (delta) to rcu_state.jiffies_stall.
- This function may be called in NMI context, so we cannot use ktime_get_ns()
- and ktime_get_coarse_ns(). Instead, we use their inaccurate but safe friends
- ktime_get_mono_fast_ns() and ktime_get_seconds() which will cause rcu_state.
*/
- jiffies_stall to be a little large than expected (harmless and safer).
void rcu_cpu_stall_reset(void) {
- u64 curr, last, delta;
- curr = ktime_get_mono_fast_ns();
- last = ktime_get_seconds() * NSEC_PER_SEC;
- delta = nsecs_to_jiffies(curr - last);
- WRITE_ONCE(rcu_state.jiffies_stall,
jiffies + rcu_jiffies_till_stall_check());
jiffies + delta + rcu_jiffies_till_stall_check());}
I prefer the following diff on top of your patch to take advantage of UBSAN detecting overflows.
If you take my diff, feel free to add: Reviewed-by: Joel Fernandes (Google) joel@joelfernandes.org
---8<-----------------------
diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h index 5e9e4779bdf1..3398cf2d19c5 100644 --- a/kernel/rcu/tree_stall.h +++ b/kernel/rcu/tree_stall.h @@ -162,14 +162,15 @@ static void panic_on_rcu_stall(void) */ void rcu_cpu_stall_reset(void) { - u64 curr, last, delta; + ktime_t last, delta_ns; + u64 delta_jiff;
- curr = ktime_get_mono_fast_ns(); last = ktime_get_seconds() * NSEC_PER_SEC; - delta = nsecs_to_jiffies(curr - last); + delta_ns = ktime_sub(ktime_get_mono_fast_ns(), last); + delta_jiff = nsecs_to_jiffies(delta_ns);
WRITE_ONCE(rcu_state.jiffies_stall, - jiffies + delta + rcu_jiffies_till_stall_check()); + jiffies + delta_jiff + rcu_jiffies_till_stall_check()); }
//////////////////////////////////////////////////////////////////////////////
Hi, Joel,
On Tue, Aug 22, 2023 at 11:34 PM Joel Fernandes joel@joelfernandes.org wrote:
On Tue, Aug 22, 2023 at 12:02:48PM +0800, Huacai Chen wrote:
The KGDB initial breakpoint gets an rcu stall warning after commit a80be428fbc1f1f3bc9ed924 ("rcu: Do not disable GP stall detection in rcu_cpu_stall_reset()").
[ 53.452051] rcu: INFO: rcu_preempt self-detected stall on CPU
[...]
[1] https://lore.kernel.org/rcu/20230814020045.51950-1-chenhuacai@loongson.cn/T/...
Cc: stable@vger.kernel.org Fixes: a80be428fbc1f1f3bc9ed924 ("rcu: Do not disable GP stall detection in rcu_cpu_stall_reset()") Reported-off-by: Binbin Zhou zhoubinbin@loongson.cn Signed-off-by: Huacai Chen chenhuacai@loongson.cn
V2: Use NMI safe functions. V3: Add comments to explain why.
kernel/rcu/tree_stall.h | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h index b10b8349bb2a..e4e53113d062 100644 --- a/kernel/rcu/tree_stall.h +++ b/kernel/rcu/tree_stall.h @@ -150,11 +150,26 @@ static void panic_on_rcu_stall(void)
- rcu_cpu_stall_reset - restart stall-warning timeout for current grace period
- The caller must disable hard irqs.
- The jiffies updating may be delayed for a very long time due to tickless and
- irq disabled, especially in the KGDB case, so we need to add the delayed time
- (delta) to rcu_state.jiffies_stall.
- This function may be called in NMI context, so we cannot use ktime_get_ns()
- and ktime_get_coarse_ns(). Instead, we use their inaccurate but safe friends
- ktime_get_mono_fast_ns() and ktime_get_seconds() which will cause rcu_state.
*/
- jiffies_stall to be a little large than expected (harmless and safer).
void rcu_cpu_stall_reset(void) {
u64 curr, last, delta;curr = ktime_get_mono_fast_ns();last = ktime_get_seconds() * NSEC_PER_SEC;delta = nsecs_to_jiffies(curr - last);WRITE_ONCE(rcu_state.jiffies_stall,
jiffies + rcu_jiffies_till_stall_check());
jiffies + delta + rcu_jiffies_till_stall_check());}
I prefer the following diff on top of your patch to take advantage of UBSAN detecting overflows.
If you take my diff, feel free to add: Reviewed-by: Joel Fernandes (Google) joel@joelfernandes.org
---8<-----------------------
diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h index 5e9e4779bdf1..3398cf2d19c5 100644 --- a/kernel/rcu/tree_stall.h +++ b/kernel/rcu/tree_stall.h @@ -162,14 +162,15 @@ static void panic_on_rcu_stall(void) */ void rcu_cpu_stall_reset(void) {
u64 curr, last, delta;
ktime_t last, delta_ns;u64 delta_jiff;
curr = ktime_get_mono_fast_ns(); last = ktime_get_seconds() * NSEC_PER_SEC;delta = nsecs_to_jiffies(curr - last);
delta_ns = ktime_sub(ktime_get_mono_fast_ns(), last);
Though ktime_t is the same as s64/u64 now, but I think we'd better to not mix them. Then, ktime_get() and ktime_get_coarse() return ktime_t; ktime_get_ns(), ktime_get_coarse_ns() and ktime_get_mono_fast_ns() return s64/u64 (means nanoseconds); ktime_get_seconds() returns seconds but ktime_get_seconds() * NSEC_PER_SEC is also nanoseconds. So, the type definition in your diff is not suitable, ktime_sub(ktime_get_mono_fast_ns(), last) is not suitable too.
Huacai
delta_jiff = nsecs_to_jiffies(delta_ns); WRITE_ONCE(rcu_state.jiffies_stall,
jiffies + delta + rcu_jiffies_till_stall_check());
jiffies + delta_jiff + rcu_jiffies_till_stall_check());}
//////////////////////////////////////////////////////////////////////////////
On Tue, Aug 22, 2023 at 11:48 AM Huacai Chen chenhuacai@kernel.org wrote:
Hi, Joel,
On Tue, Aug 22, 2023 at 11:34 PM Joel Fernandes joel@joelfernandes.org wrote:
On Tue, Aug 22, 2023 at 12:02:48PM +0800, Huacai Chen wrote:
The KGDB initial breakpoint gets an rcu stall warning after commit a80be428fbc1f1f3bc9ed924 ("rcu: Do not disable GP stall detection in rcu_cpu_stall_reset()").
[ 53.452051] rcu: INFO: rcu_preempt self-detected stall on CPU
[...]
[1] https://lore.kernel.org/rcu/20230814020045.51950-1-chenhuacai@loongson.cn/T/...
Cc: stable@vger.kernel.org Fixes: a80be428fbc1f1f3bc9ed924 ("rcu: Do not disable GP stall detection in rcu_cpu_stall_reset()") Reported-off-by: Binbin Zhou zhoubinbin@loongson.cn Signed-off-by: Huacai Chen chenhuacai@loongson.cn
V2: Use NMI safe functions. V3: Add comments to explain why.
kernel/rcu/tree_stall.h | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h index b10b8349bb2a..e4e53113d062 100644 --- a/kernel/rcu/tree_stall.h +++ b/kernel/rcu/tree_stall.h @@ -150,11 +150,26 @@ static void panic_on_rcu_stall(void)
- rcu_cpu_stall_reset - restart stall-warning timeout for current grace period
- The caller must disable hard irqs.
- The jiffies updating may be delayed for a very long time due to tickless and
- irq disabled, especially in the KGDB case, so we need to add the delayed time
- (delta) to rcu_state.jiffies_stall.
- This function may be called in NMI context, so we cannot use ktime_get_ns()
- and ktime_get_coarse_ns(). Instead, we use their inaccurate but safe friends
- ktime_get_mono_fast_ns() and ktime_get_seconds() which will cause rcu_state.
*/
- jiffies_stall to be a little large than expected (harmless and safer).
void rcu_cpu_stall_reset(void) {
u64 curr, last, delta;curr = ktime_get_mono_fast_ns();last = ktime_get_seconds() * NSEC_PER_SEC;delta = nsecs_to_jiffies(curr - last);WRITE_ONCE(rcu_state.jiffies_stall,
jiffies + rcu_jiffies_till_stall_check());
jiffies + delta + rcu_jiffies_till_stall_check());}
I prefer the following diff on top of your patch to take advantage of UBSAN detecting overflows.
If you take my diff, feel free to add: Reviewed-by: Joel Fernandes (Google) joel@joelfernandes.org
---8<-----------------------
diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h index 5e9e4779bdf1..3398cf2d19c5 100644 --- a/kernel/rcu/tree_stall.h +++ b/kernel/rcu/tree_stall.h @@ -162,14 +162,15 @@ static void panic_on_rcu_stall(void) */ void rcu_cpu_stall_reset(void) {
u64 curr, last, delta;
ktime_t last, delta_ns;u64 delta_jiff;
curr = ktime_get_mono_fast_ns(); last = ktime_get_seconds() * NSEC_PER_SEC;delta = nsecs_to_jiffies(curr - last);
delta_ns = ktime_sub(ktime_get_mono_fast_ns(), last);Though ktime_t is the same as s64/u64 now,
No, you are incorrect. ktime_t is a signed int not u64. I prefer to use types designed for a specific purpose where possible. It makes the code safer.
but I think we'd better to not mix them. Then, ktime_get() and ktime_get_coarse() return ktime_t; ktime_get_ns(), ktime_get_coarse_ns() and ktime_get_mono_fast_ns() return s64/u64 (means nanoseconds); ktime_get_seconds() returns seconds but ktime_get_seconds() * NSEC_PER_SEC is also nanoseconds. So, the type definition in your diff is not suitable, ktime_sub(ktime_get_mono_fast_ns(), last) is not suitable too.
Is there a technical reason for your concern? ktime_get_mono_fast_ns() is assigned to ktime_t in other places. I am not seeing what the problem is.
Thanks.
On Tue, Aug 22, 2023 at 11:58 PM Joel Fernandes joel@joelfernandes.org wrote:
On Tue, Aug 22, 2023 at 11:48 AM Huacai Chen chenhuacai@kernel.org wrote:
Hi, Joel,
On Tue, Aug 22, 2023 at 11:34 PM Joel Fernandes joel@joelfernandes.org wrote:
On Tue, Aug 22, 2023 at 12:02:48PM +0800, Huacai Chen wrote:
The KGDB initial breakpoint gets an rcu stall warning after commit a80be428fbc1f1f3bc9ed924 ("rcu: Do not disable GP stall detection in rcu_cpu_stall_reset()").
[ 53.452051] rcu: INFO: rcu_preempt self-detected stall on CPU
[...]
[1] https://lore.kernel.org/rcu/20230814020045.51950-1-chenhuacai@loongson.cn/T/...
Cc: stable@vger.kernel.org Fixes: a80be428fbc1f1f3bc9ed924 ("rcu: Do not disable GP stall detection in rcu_cpu_stall_reset()") Reported-off-by: Binbin Zhou zhoubinbin@loongson.cn Signed-off-by: Huacai Chen chenhuacai@loongson.cn
V2: Use NMI safe functions. V3: Add comments to explain why.
kernel/rcu/tree_stall.h | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h index b10b8349bb2a..e4e53113d062 100644 --- a/kernel/rcu/tree_stall.h +++ b/kernel/rcu/tree_stall.h @@ -150,11 +150,26 @@ static void panic_on_rcu_stall(void)
- rcu_cpu_stall_reset - restart stall-warning timeout for current grace period
- The caller must disable hard irqs.
- The jiffies updating may be delayed for a very long time due to tickless and
- irq disabled, especially in the KGDB case, so we need to add the delayed time
- (delta) to rcu_state.jiffies_stall.
- This function may be called in NMI context, so we cannot use ktime_get_ns()
- and ktime_get_coarse_ns(). Instead, we use their inaccurate but safe friends
- ktime_get_mono_fast_ns() and ktime_get_seconds() which will cause rcu_state.
*/
- jiffies_stall to be a little large than expected (harmless and safer).
void rcu_cpu_stall_reset(void) {
u64 curr, last, delta;curr = ktime_get_mono_fast_ns();last = ktime_get_seconds() * NSEC_PER_SEC;delta = nsecs_to_jiffies(curr - last);WRITE_ONCE(rcu_state.jiffies_stall,
jiffies + rcu_jiffies_till_stall_check());
jiffies + delta + rcu_jiffies_till_stall_check());}
I prefer the following diff on top of your patch to take advantage of UBSAN detecting overflows.
If you take my diff, feel free to add: Reviewed-by: Joel Fernandes (Google) joel@joelfernandes.org
---8<-----------------------
diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h index 5e9e4779bdf1..3398cf2d19c5 100644 --- a/kernel/rcu/tree_stall.h +++ b/kernel/rcu/tree_stall.h @@ -162,14 +162,15 @@ static void panic_on_rcu_stall(void) */ void rcu_cpu_stall_reset(void) {
u64 curr, last, delta;
ktime_t last, delta_ns;u64 delta_jiff;
curr = ktime_get_mono_fast_ns(); last = ktime_get_seconds() * NSEC_PER_SEC;delta = nsecs_to_jiffies(curr - last);
delta_ns = ktime_sub(ktime_get_mono_fast_ns(), last);Though ktime_t is the same as s64/u64 now,
No, you are incorrect. ktime_t is a signed int not u64. I prefer to use types designed for a specific purpose where possible. It makes the code safer.
Yes ktime_t is s64 now, I said s64/u64 because there is some existing code to mix them (of course not very good).
but I think we'd better to not mix them. Then, ktime_get() and ktime_get_coarse() return ktime_t; ktime_get_ns(), ktime_get_coarse_ns() and ktime_get_mono_fast_ns() return s64/u64 (means nanoseconds); ktime_get_seconds() returns seconds but ktime_get_seconds() * NSEC_PER_SEC is also nanoseconds. So, the type definition in your diff is not suitable, ktime_sub(ktime_get_mono_fast_ns(), last) is not suitable too.
Is there a technical reason for your concern? ktime_get_mono_fast_ns() is assigned to ktime_t in other places. I am not seeing what the problem is.
My concern is ktime_t may not always be s64 in future, so there are ktime_to_ns() and some other similar APIs to convert from one to another.
Huacai
Thanks.
On Tue, Aug 22 2023 at 12:02, Huacai Chen wrote:
- This function may be called in NMI context, so we cannot use ktime_get_ns()
- and ktime_get_coarse_ns(). Instead, we use their inaccurate but safe friends
- ktime_get_mono_fast_ns() and ktime_get_seconds() which will cause rcu_state.
- jiffies_stall to be a little large than expected (harmless and safer).
What's inaccurate about ktime_get_mono_fast_ns()? Bogus comments are even worse than no comments.
*/ void rcu_cpu_stall_reset(void) {
- u64 curr, last, delta;
- curr = ktime_get_mono_fast_ns();
- last = ktime_get_seconds() * NSEC_PER_SEC;
So this will trigger a warning when someone debugs suspend with KGDB.
It seems the approach taken here seems to be to throw stuff at the wall and see what sticks.
Thanks,
tglx
Hi, Thomas,
On Thu, Aug 24, 2023 at 5:37 PM Thomas Gleixner tglx@linutronix.de wrote:
On Tue, Aug 22 2023 at 12:02, Huacai Chen wrote:
- This function may be called in NMI context, so we cannot use ktime_get_ns()
- and ktime_get_coarse_ns(). Instead, we use their inaccurate but safe friends
- ktime_get_mono_fast_ns() and ktime_get_seconds() which will cause rcu_state.
- jiffies_stall to be a little large than expected (harmless and safer).
What's inaccurate about ktime_get_mono_fast_ns()? Bogus comments are even worse than no comments.
ktime_get_mono_fast_ns() is not as accurate as ktime_get_ns(), I get this conclusion from:
* So if the update lowers the slope, readers who are forced to the * not yet updated second array are still using the old steeper slope.
*/ void rcu_cpu_stall_reset(void) {
u64 curr, last, delta;curr = ktime_get_mono_fast_ns();last = ktime_get_seconds() * NSEC_PER_SEC;So this will trigger a warning when someone debugs suspend with KGDB.
Yes, ktime_get_seconds() may cause a warning, I haven't noticed this before.
It seems the approach taken here seems to be to throw stuff at the wall and see what sticks.
I don't understand what's meaning, but I believe your advice in another thread is the best solution, so let me try.
Huacai
Thanks,
tglx
linux-stable-mirror@lists.linaro.org
-
Huacai Chen -
Huacai Chen -
Joel Fernandes -
Thomas Gleixner