Re: [PATCH V4 2/2] rcu: Update jiffies in rcu_cpu_stall_reset()

[Alan Huang]

>> 
>>>>>>>>>>> 
>>>>>>>>>>> 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.
>>>>>>>>>> 
>>>>>>>>>> Would it make sense for there to be a kgdb_cpu_exit()?  In that case,
>>>>>>>>>> the stalls could simply be suppressed at the beginning of the debug
>>>>>>>>>> session and re-enabled upon exit, as is currently done for sysrq output
>>>>>>>>>> via rcu_sysrq_start() and rcu_sysrq_end().
>>>>>>>>> Thank you for your advice, but that doesn't help. Because
>>>>>>>>> rcu_sysrq_start() and rcu_sysrq_end() try to suppress the warnings
>>>>>>>>> during sysrq, but kgdb already has no warnings during kgdb_cpu_enter()
>>>>>>>>> since it is executed in irq disabled context. Instead, this patch
>>>>>>>>> wants to suppress the warnings *after* kgdb_cpu_enter() due to a very
>>>>>>>>> old jiffies value.
>>>>>>>>> 
>>>>>>>> 
>>>>>>>> Hello, Huacai
>>>>>>>> 
>>>>>>>> Is it possible to set  the rcu_cpu_stall_suppress is true in
>>>>>>>> dbg_touch_watchdogs()
>>>>>>>> and reset the rcu_cpu_stall_suppress at the beginning and end of the
>>>>>>>> RCU grace period?
>>>>>>> This is possible but not the best: 1, kgdb is not the only caller of
>>>>>>> rcu_cpu_stall_reset(); 2, it is difficult to find the "end" to reset
>>>>>>> rcu_cpu_stall_suppress.
>>>>>>> 
>>>>>> 
>>>>>> You can replace rcu_state.jiffies_stall update by setting rcu_cpu_stall_suppress
>>>>>> in rcu_cpu_stall_reset(),  and reset rcu_cpu_stall_suppress in rcu_gp_init() and
>>>>>> rcu_gp_cleanup().
>>>>> What's the advantage compared with updating jiffies? Updating jiffies
>>>>> seems more straight forward.
>>>>> 
>>>> 
>>>> In do_update_jiffies_64(), need to acquire jiffies_lock raw spinlock,
>>>> like you said, kgdb is not the only caller of rcu_cpu_stall_reset(),
>>>> the rcu_cpu_stall_reset() maybe invoke in NMI  (arch/x86/platform/uv/uv_nmi.c)
>>> Reset rcu_cpu_stall_suppress in rcu_gp_init()/rcu_gp_cleanup() is
>>> still not so good to me, because it does a useless operation in most
>>> cases. Moreover, the rcu core is refactored again and again, something
>>> may be changed in future.
>>> 
>>> If  do_update_jiffies_64() cannot be used in NMI context, can we
>> 
>> What about updating jiffies in dbg_touch_watchdogs or adding a wrapper which updates
>> both jiffies and jiffies_stall?
> This can solve the kgdb problem, but I found that most callers of
> rcu_cpu_stall_reset() are in irq disabled context so they may meet

The duration of other contexts where interrupts are disabled may not be as long as in the case of kgdb？

> similar problems. Modifying rcu_cpu_stall_reset() can solve all of
> them.
> 
> But due to the NMI issue, from my point of view, setting jiffies_stall
> to jiffies + 300*HZ is the best solution now. :)

If I understand correctly, the NMI issue is the deadlock issue? If so, plus the short duration of other irq disabled 
contexts, it’s ok just update jiffies in dbg_touch_watchdogs.

Please correct me if anything wrong. :)

> 
> Huacai
>> 
>>> consider my old method [1]?
>>> https://lore.kernel.org/rcu/CAAhV-H7j9Y=VvRLm8thLw-EX1PGqBA9YfT4G1AN7ucYS=iP+DQ@xxxxxxxxxxxxxx/T/#t
>>> 
>>> Of course we should set rcu_state.jiffies_stall large enough, so we
>>> can do like this:
>>> 
>>> void rcu_cpu_stall_reset(void)
>>> {
>>> WRITE_ONCE(rcu_state.jiffies_stall,
>>> -   jiffies + rcu_jiffies_till_stall_check());
>>> +   jiffies + 300 * HZ);
>>> }
>>> 
>>> 300s is the largest timeout value, and I think 300s is enough here in practice.
>>> 
>>> Huacai
>>> 
>>>> 
>>>> Thanks
>>>> Zqiang
>>>> 
>>>> 
>>>>> Huacai
>>>>> 
>>>>>> 
>>>>>> Thanks
>>>>>> Zqiang
>>>>>> 
>>>>>>> 
>>>>>>>> or set rcupdate.rcu_cpu_stall_suppress_at_boot=1 in bootargs can
>>>>>>>> suppress RCU stall
>>>>>>>> in booting.
>>>>>>> This is also possible, but it suppresses all kinds of stall warnings,
>>>>>>> which is not what we want.
>>>>>>> 
>>>>>>> Huacai
>>>>>>>> 
>>>>>>>> 
>>>>>>>> Thanks
>>>>>>>> Zqiang
>>>>>>>> 
>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Huacai
>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>>                                                       Thanx, Paul
>>>>>>>>>> 
>>>>>>>>>>> Cc: stable@xxxxxxxxxxxxxxx
>>>>>>>>>>> Fixes: a80be428fbc1f1f3bc9ed924 ("rcu: Do not disable GP stall detection in rcu_cpu_stall_reset()")
>>>>>>>>>>> Reported-off-by: Binbin Zhou <zhoubinbin@xxxxxxxxxxx>
>>>>>>>>>>> Signed-off-by: Huacai Chen <chenhuacai@xxxxxxxxxxx>
>>>>>>>>>>> ---
>>>>>>>>>>> kernel/rcu/tree_stall.h | 1 +
>>>>>>>>>>> 1 file changed, 1 insertion(+)
>>>>>>>>>>> 
>>>>>>>>>>> diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h
>>>>>>>>>>> index b10b8349bb2a..1c7b540985bf 100644
>>>>>>>>>>> --- a/kernel/rcu/tree_stall.h
>>>>>>>>>>> +++ b/kernel/rcu/tree_stall.h
>>>>>>>>>>> @@ -153,6 +153,7 @@ static void panic_on_rcu_stall(void)
>>>>>>>>>>> */
>>>>>>>>>>> void rcu_cpu_stall_reset(void)
>>>>>>>>>>> {
>>>>>>>>>>> +     do_update_jiffies_64(ktime_get());
>>>>>>>>>>>     WRITE_ONCE(rcu_state.jiffies_stall,
>>>>>>>>>>>                jiffies + rcu_jiffies_till_stall_check());
>>>>>>>>>>> }
>>>>>>>>>>> --
>>>>>>>>>>> 2.39.3