+ watchdog-fix-barriers-when-printing-backtraces-from-all-cpus.patch added to -mm tree
[email protected]
tree noticeThe patch titled
Subject: watchdog: fix barriers when printing backtraces from all CPUs
has been added to the -mm tree. Its filename is
watchdog-fix-barriers-when-printing-backtraces-from-all-cpus.patch
This patch should soon appear at
https://ozlabs.org/~akpm/mmots/broken-out/watchdog-fix-barriers-when-printing-backtraces-from-all-cpus.patch
and later at
https://ozlabs.org/~akpm/mmotm/broken-out/watchdog-fix-barriers-when-printing-backtraces-from-all-cpus.patch
Before you just go and hit "reply", please:
a) Consider who else should be cc'ed
b) Prefer to cc a suitable mailing list as well
c) Ideally: find the original patch on the mailing list and do a reply-to-all to that, adding suitable additional cc's
*** Remember to use Documentation/process/submit-checklist.rst when testing your code ***
The -mm tree is included into linux-next and is updated there every 3-4 working days
------------------------------------------------------
From: Petr Mladek <[email protected]>
Subject: watchdog: fix barriers when printing backtraces from all CPUs
Any parallel softlockup reports are skipped when one CPU is already printing backtraces from all CPUs.
The exclusive rights are synchronized using one bit in soft_lockup_nmi_warn. There is also one memory barrier that does not make much sense.
Use two barriers on the right location to prevent mixing two reports.
---
kernel/watchdog.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)
kernel/watchdog.c renamed from kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus +15 -9
@@ -409,12 +409,18 @@ static enum hrtimer_restart watchdog_timif (kvm_check_and_clear_guest_paused ())return HRTIMER_RESTART ; +/* +* Prevent multiple soft-lockup reports if one cpu is already +* engaged in dumping all cpu back traces. +*/ if (softlockup_all_cpu_backtrace) { -/* Prevent multiple soft-lockup reports if one cpu is already -* engaged in dumping cpu back traces -*/ if (test_and_set_bit (0 , &soft_lockup_nmi_warn))return HRTIMER_RESTART ; +/* +* Make sure that reports are serialized. Start +* printing after getting the exclusive rights. +*/ +smp_mb__after_atomic (); }/* Start period for the next softlockup warning. */
@@ -431,14 +437,14 @@ static enum hrtimer_restart watchdog_timdump_stack ();if (softlockup_all_cpu_backtrace) { -/* Avoid generating two back traces for current -* given that one is already made above -*/ trigger_allbutself_cpu_backtrace (); - +/* +* Make sure that everything is printed before +* another CPU is allowed to report lockup again. +*/ +smp_mb__before_atomic (); +/* Allow a further report. */ clear_bit (0 , &soft_lockup_nmi_warn); -/* Barrier to sync with other cpus */ -smp_mb__after_atomic (); }add_taint (TAINT_SOFTLOCKUP ,LOCKDEP_STILL_OK );
_
Patches currently in -mm which might be from [email protected] are
watchdog-rename-__touch_watchdog-to-a-better-descriptive-name.patch watchdog-explicitly-update-timestamp-when-reporting-softlockup.patch watchdog-softlockup-report-the-overall-time-of-softlockups.patch watchdog-softlockup-remove-logic-that-tried-to-prevent-repeated-reports.patch watchdog-fix-barriers-when-printing-backtraces-from-all-cpus.patch watchdog-cleanup-handling-of-false-positives.patch
Quoted 11 lines · On Thu, Mar 11, 2021 at 01:22:07PM -0800, [email protected] wrote:
kernel/watchdog.c renamed from kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus +4
-0@@ -409,12 +409,18 @@ static enum hrtimer_restart watchdog_timif (kvm_check_and_clear_guest_paused ())return HRTIMER_RESTART ; +/* +* Prevent multiple soft-lockup reports if one cpu is already +* engaged in dumping all cpu back traces. +*/ if (softlockup_all_cpu_backtrace) {
kernel/watchdog.c static enum hrtimer_restart watchdog_tim · line 423
Quoted 10 lines
kernel/watchdog.c renamed from kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus +5
-3-/* Prevent multiple soft-lockup reports if one cpu is already -* engaged in dumping cpu back traces -*/ if (test_and_set_bit (0 , &soft_lockup_nmi_warn))return HRTIMER_RESTART ; +/* +* Make sure that reports are serialized. Start +* printing after getting the exclusive rights. +*/ +smp_mb__after_atomic ();
test_and_set_bit() is a value returning atomic and therefore already implies a full smp_mb() on both ends.
Quoted 6 lines
kernel/watchdog.c renamed from kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus +0
-0}/* Start period for the next softlockup warning. */ @@ -431,14 +437,14 @@ static enum hrtimer_restart watchdog_timdump_stack ();
kernel/watchdog.c static enum hrtimer_restart watchdog_tim · line 444
Quoted 10 lines
kernel/watchdog.c renamed from kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus +4
-4if (softlockup_all_cpu_backtrace) { -/* Avoid generating two back traces for current -* given that one is already made above -*/ trigger_allbutself_cpu_backtrace (); - +/* +* Make sure that everything is printed before +* another CPU is allowed to report lockup again. +*/
This is not something smp_mb() ensures. smp_mb() is only concerned with memory access ordering, not completion, and certainly not some random IO.
IOW, that comment is complete bollocks.
kernel/watchdog.c static enum hrtimer_restart watchdog_tim · line 447
Quoted 3 lines
kernel/watchdog.c renamed from kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus +2
-0+smp_mb__before_atomic (); +/* Allow a further report. */ clear_bit (0 , &soft_lockup_nmi_warn);
That said, this looks like a test-and-set lock and clear_bit unlock like situation. In fact it doesn't seem to use anything other than bit0 of that word.
FWIW we have test_and_set_bit_lock() / clear_bit_unlock().
Quoted 17 lines · Peter Zijlstra
Quoted 16 lines · On Thu, Mar 11, 2021 at 01:22:07PM -0800, [email protected] wrote:
kernel/watchdog.c renamed from kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus +4
-3@@ -409,12 +409,18 @@ static enum hrtimer_restart watchdog_timif (kvm_check_and_clear_guest_paused ())return HRTIMER_RESTART ; +/* +* Prevent multiple soft-lockup reports if one cpu is already +* engaged in dumping all cpu back traces. +*/ if (softlockup_all_cpu_backtrace) { -/* Prevent multiple soft-lockup reports if one cpu is already -* engaged in dumping cpu back traces -*/ if (test_and_set_bit (0 , &soft_lockup_nmi_warn))return HRTIMER_RESTART ;
Quoted 8 lines
kernel/watchdog.c static enum hrtimer_restart watchdog_tim · line 423
Quoted 5 lines
kernel/watchdog.c renamed from kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus +5
-0+/* +* Make sure that reports are serialized. Start +* printing after getting the exclusive rights. +*/ +smp_mb__after_atomic ();
test_and_set_bit()is a value returning atomic and therefore already implies a fullsmp_mb()on both ends.
Still learning.
Quoted 21 lines
Quoted 6 lines
kernel/watchdog.c renamed from kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus +0
-0}/* Start period for the next softlockup warning. */ @@ -431,14 +437,14 @@ static enum hrtimer_restart watchdog_timdump_stack ();kernel/watchdog.c static enum hrtimer_restart watchdog_tim · line 444
Quoted 10 lines
kernel/watchdog.c renamed from kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus +4
-4if (softlockup_all_cpu_backtrace) { -/* Avoid generating two back traces for current -* given that one is already made above -*/ trigger_allbutself_cpu_backtrace (); - +/* +* Make sure that everything is printed before +* another CPU is allowed to report lockup again. +*/ This is not something
smp_mb()ensures.smp_mb()is only concerned with memory access ordering, not completion, and certainly not some random IO.
Quoted 1 line
IOW, that comment is complete bollocks.
The problem is the comment.
trigger_allbutself_cpu_backtrace() waits until all the NMI handlers confirm that they printed the backtrace. The generic implemetation even does clear_bit_unlock(0, &backtrace_flag), see nmi_trigger_cpumask_backtrace().
Quoted 8 lines
kernel/watchdog.c static enum hrtimer_restart watchdog_tim · line 447
Quoted 3 lines
kernel/watchdog.c renamed from kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus +2
-0+smp_mb__before_atomic (); +/* Allow a further report. */ clear_bit (0 , &soft_lockup_nmi_warn);That said, this looks like a test-and-set lock and
clear_bitunlock like situation. In fact it doesn't seem to use anything other than bit0 of that word.
Exactly, they prevent mixing two reports. It consists of more pieces. The simplified would code looks like:
if (test_and_set_bit_lock())
return;
pr_emerg("BUG: soft lockup - CPU#%d stuck for %us! [%s:%d]\n",...)
print_modules();
print_irqtrace_events(current);
if (regs)
show_regs(regs);
else
dump_stack();
trigger_allbutself_cpu_backtrace();
clear_bit(0, &soft_lockup_nmi_warn);
/* Barrier to sync with other cpus */
smp_mb__after_atomic();
}
clear_bit_unlock().Quoted 1 line
FWIW we have
test_and_set_bit_lock()/clear_bit_unlock().
Thanks for hint. I am going to update the patch.
IMHO, the best solution would be to use test_and_set_bit_lock() and remove the confusing comment.
Andrew, should I send the entire patchset or just fixup of this patch?
Best Regards,
Petr
PATCH 1/1 watchdog: Use bit lock operations to prevent multiple soft-lockup reports
Use bit_lock operation to prevent multiple soft-lockups reports when one CPU already triggered dumping backtraces from all CPUs.
It allows to remove the explicit memory barriers and misleading comments.
---
Andrew,
this patch can be put on top of the patchset fixing/cleaning softlockup watchdog code. Feel free to squash it into the other patch fixing the barriers.
Or should I resend the entire patchset again, please?
Best Regards,
Petr
kernel/watchdog.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-)
kernel/watchdog.c +2 -13
@@ -410,13 +410,8 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)* engaged in dumping all cpu back traces. */ if (softlockup_all_cpu_backtrace) { -if (test_and_set_bit (0 , &soft_lockup_nmi_warn)) +if (test_and_set_bit_lock (0 , &soft_lockup_nmi_warn))return HRTIMER_RESTART ; -/* -* Make sure that reports are serialized. Start -* printing after getting the exclusive rights. -*/ -smp_mb__after_atomic (); }/* Start period for the next softlockup warning. */
@@ -434,13 +429,7 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)if (softlockup_all_cpu_backtrace) {trigger_allbutself_cpu_backtrace (); -/* -* Make sure that everything is printed before -* another CPU is allowed to report lockup again. -*/ -smp_mb__before_atomic (); -/* Allow a further report. */ -clear_bit (0 , &soft_lockup_nmi_warn); +clear_bit_unlock (0 , &soft_lockup_nmi_warn); }add_taint (TAINT_SOFTLOCKUP ,LOCKDEP_STILL_OK );
Signature · 1 line
2.26.2
Re: [PATCH] watchdog: Use bit lock operations to prevent multiple soft-lockup reports
Quoted 8 lines · Petr Mladek
Use
bit_lockoperation to prevent multiple soft-lockups reports when one CPU already triggered dumping backtraces from all CPUs.It allows to remove the explicit memory barriers and misleading comments.
Suggested-by Peter Zijlstra <[email protected]>Signed-off-by Petr Mladek <[email protected]>
Thanks!
Re: [PATCH] watchdog: Use bit lock operations to prevent multiple soft-lockup reports
In reply to Petr MladekQuoted 5 lines · On Fri, 19 Mar 2021 13:14:40 +0100 Petr Mladek <[email protected]> wrote:
this patch can be put on top of the patchset fixing/cleaning softlockup watchdog code. Feel free to squash it into the other patch fixing the barriers.
Or should I resend the entire patchset again, please?
That's OK - I'll queue this as a fix against "watchdog: fix barriers when printing backtraces from all CPUs".
However once the two patches are merged into one for upstreaming, the changelog doesn't seem to be fully correct and complete. Please check and suggest a replacement if needed? The joined patches presently look like this:
From: Petr Mladek <[email protected]>
Subject: watchdog: fix barriers when printing backtraces from all CPUs
Any parallel softlockup reports are skipped when one CPU is already printing backtraces from all CPUs.
The exclusive rights are synchronized using one bit in soft_lockup_nmi_warn. There is also one memory barrier that does not make much sense.
Use two barriers on the right location to prevent mixing two reports.
kernel/watchdog.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
kernel/watchdog.c renamed from kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus +6 -11
@@ -409,11 +409,12 @@ static enum hrtimer_restart watchdog_timif (kvm_check_and_clear_guest_paused ())return HRTIMER_RESTART ; +/* +* Prevent multiple soft-lockup reports if one cpu is already +* engaged in dumping all cpu back traces. +*/ if (softlockup_all_cpu_backtrace) { -/* Prevent multiple soft-lockup reports if one cpu is already -* engaged in dumping cpu back traces -*/ -if (test_and_set_bit (0 , &soft_lockup_nmi_warn)) +if (test_and_set_bit_lock (0 , &soft_lockup_nmi_warn))return HRTIMER_RESTART ; }
@@ -431,14 +432,8 @@ static enum hrtimer_restart watchdog_timdump_stack ();if (softlockup_all_cpu_backtrace) { -/* Avoid generating two back traces for current -* given that one is already made above -*/ trigger_allbutself_cpu_backtrace (); - -clear_bit (0 , &soft_lockup_nmi_warn); -/* Barrier to sync with other cpus */ -smp_mb__after_atomic (); +clear_bit_unlock (0 , &soft_lockup_nmi_warn); }add_taint (TAINT_SOFTLOCKUP ,LOCKDEP_STILL_OK );
_
Re: [PATCH] watchdog: Use bit lock operations to prevent multiple soft-lockup reports
Quoted 19 lines · Andrew Morton
Quoted 5 lines · On Fri, 19 Mar 2021 13:14:40 +0100 Petr Mladek <[email protected]> wrote:
this patch can be put on top of the patchset fixing/cleaning softlockup watchdog code. Feel free to squash it into the other patch fixing the barriers.
Or should I resend the entire patchset again, please?
That's OK - I'll queue this as a fix against "watchdog: fix barriers when printing backtraces from all CPUs".
However once the two patches are merged into one for upstreaming, the changelog doesn't seem to be fully correct and complete. Please check and suggest a replacement if needed? The joined patches presently look like this:
From: Petr Mladek <[email protected]>
Subject: watchdog: fix barriers when printing backtraces from all CPUs
Quoted 8 lines
Any parallel softlockup reports are skipped when one CPU is already printing backtraces from all CPUs.
The exclusive rights are synchronized using one bit in
soft_lockup_nmi_warn. There is also one memory barrier that does not make much sense.Use two barriers on the right location to prevent mixing two reports.
Please, use the following:
<proposal>
Subject: watchdog: Clean up locking when printing backtraces from all CPUs
Any parallel softlockup reports are skipped when one CPU is already printing backtraces from all CPUs.
The exclusive rights are synchronized using one bit in soft_lockup_nmi_warn. There is one explicit barrier. The other is implicitly provided by test_and_set_bit().
Use *_bit_lock() API to better express the intention. It provides the needed barriers out of box.
</proposal>
Thanks a lot for taking care of it.
Best Regards,
Petr
Quoted 50 lines
Signed-off-by Petr Mladek <[email protected]>Cc Ingo Molnar <[email protected]>Cc Laurence Oberman <[email protected]>Cc Michal Hocko <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Vincent Whitchurch <[email protected]>kernel/watchdog.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)kernel/watchdog.c renamed from kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus +6
-11@@ -409,11 +409,12 @@ static enum hrtimer_restart watchdog_timif (kvm_check_and_clear_guest_paused ())return HRTIMER_RESTART ; +/* +* Prevent multiple soft-lockup reports if one cpu is already +* engaged in dumping all cpu back traces. +*/ if (softlockup_all_cpu_backtrace) { -/* Prevent multiple soft-lockup reports if one cpu is already -* engaged in dumping cpu back traces -*/ -if (test_and_set_bit (0 , &soft_lockup_nmi_warn)) +if (test_and_set_bit_lock (0 , &soft_lockup_nmi_warn))return HRTIMER_RESTART ; }@@ -431,14 +432,8 @@ static enum hrtimer_restart watchdog_timdump_stack ();if (softlockup_all_cpu_backtrace) { -/* Avoid generating two back traces for current -* given that one is already made above -*/ trigger_allbutself_cpu_backtrace (); - -clear_bit (0 , &soft_lockup_nmi_warn); -/* Barrier to sync with other cpus */ -smp_mb__after_atomic (); +clear_bit_unlock (0 , &soft_lockup_nmi_warn); }add_taint (TAINT_SOFTLOCKUP ,LOCKDEP_STILL_OK );_