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.