* + watchdog-fix-barriers-when-printing-backtraces-from-all-cpus.patch added to -mm tree
@ 2021-03-11 21:22 akpm
2021-03-16 11:48 ` Peter Zijlstra
0 siblings, 1 reply; 7+ messages in thread
From: akpm @ 2021-03-11 21:22 UTC (permalink / raw)
To: loberman, mhocko, mingo, mm-commits, peterz, pmladek, tglx,
vincent.whitchurch
The 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 <pmladek@suse.com>
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.
Link: https://lkml.kernel.org/r/20210311122130.6788-6-pmladek@suse.com
Signed-off-by: Petr Mladek <pmladek@suse.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Laurence Oberman <loberman@redhat.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vincent Whitchurch <vincent.whitchurch@axis.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
kernel/watchdog.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
--- a/kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus
+++ a/kernel/watchdog.c
@@ -409,12 +409,18 @@ static enum hrtimer_restart watchdog_tim
if (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_tim
dump_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 pmladek@suse.com 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: + watchdog-fix-barriers-when-printing-backtraces-from-all-cpus.patch added to -mm tree
2021-03-11 21:22 + watchdog-fix-barriers-when-printing-backtraces-from-all-cpus.patch added to -mm tree akpm
@ 2021-03-16 11:48 ` Peter Zijlstra
2021-03-17 9:14 ` Petr Mladek
0 siblings, 1 reply; 7+ messages in thread
From: Peter Zijlstra @ 2021-03-16 11:48 UTC (permalink / raw)
To: akpm
Cc: loberman, mhocko, mingo, mm-commits, pmladek, tglx, vincent.whitchurch
On Thu, Mar 11, 2021 at 01:22:07PM -0800, akpm@linux-foundation.org wrote:
> --- a/kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus
> +++ a/kernel/watchdog.c
> @@ -409,12 +409,18 @@ static enum hrtimer_restart watchdog_tim
> if (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();
test_and_set_bit() is a value returning atomic and therefore already
implies a full smp_mb() on both ends.
> }
>
> /* Start period for the next softlockup warning. */
> @@ -431,14 +437,14 @@ static enum hrtimer_restart watchdog_tim
> dump_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.
> + */
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.
> + 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().
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: + watchdog-fix-barriers-when-printing-backtraces-from-all-cpus.patch added to -mm tree
2021-03-16 11:48 ` Peter Zijlstra
@ 2021-03-17 9:14 ` Petr Mladek
2021-03-19 12:14 ` [PATCH] watchdog: Use bit lock operations to prevent multiple soft-lockup reports Petr Mladek
0 siblings, 1 reply; 7+ messages in thread
From: Petr Mladek @ 2021-03-17 9:14 UTC (permalink / raw)
To: Peter Zijlstra, akpm
Cc: loberman, mhocko, mingo, mm-commits, tglx, vincent.whitchurch
On Tue 2021-03-16 12:48:45, Peter Zijlstra wrote:
> On Thu, Mar 11, 2021 at 01:22:07PM -0800, akpm@linux-foundation.org wrote:
> > --- a/kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus
> > +++ a/kernel/watchdog.c
> > @@ -409,12 +409,18 @@ static enum hrtimer_restart watchdog_tim
> > if (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();
>
> test_and_set_bit() is a value returning atomic and therefore already
> implies a full smp_mb() on both ends.
Still learning.
> > }
> >
> > /* Start period for the next softlockup warning. */
> > @@ -431,14 +437,14 @@ static enum hrtimer_restart watchdog_tim
> > dump_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.
> > + */
>
> 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.
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().
> > + 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.
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().
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] watchdog: Use bit lock operations to prevent multiple soft-lockup reports
2021-03-17 9:14 ` Petr Mladek
@ 2021-03-19 12:14 ` Petr Mladek
2021-03-19 12:29 ` Peter Zijlstra
2021-03-20 16:32 ` Andrew Morton
0 siblings, 2 replies; 7+ messages in thread
From: Petr Mladek @ 2021-03-19 12:14 UTC (permalink / raw)
To: Peter Zijlstra, akpm
Cc: loberman, mhocko, mingo, mm-commits, tglx, vincent.whitchurch
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.
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
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(-)
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index c050323fcd33..090b6bc4de79 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -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);
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] watchdog: Use bit lock operations to prevent multiple soft-lockup reports
2021-03-19 12:14 ` [PATCH] watchdog: Use bit lock operations to prevent multiple soft-lockup reports Petr Mladek
@ 2021-03-19 12:29 ` Peter Zijlstra
2021-03-20 16:32 ` Andrew Morton
1 sibling, 0 replies; 7+ messages in thread
From: Peter Zijlstra @ 2021-03-19 12:29 UTC (permalink / raw)
To: Petr Mladek
Cc: akpm, loberman, mhocko, mingo, mm-commits, tglx, vincent.whitchurch
On Fri, Mar 19, 2021 at 01:14:40PM +0100, Petr Mladek wrote:
> 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.
>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
Thanks!
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] watchdog: Use bit lock operations to prevent multiple soft-lockup reports
2021-03-19 12:14 ` [PATCH] watchdog: Use bit lock operations to prevent multiple soft-lockup reports Petr Mladek
2021-03-19 12:29 ` Peter Zijlstra
@ 2021-03-20 16:32 ` Andrew Morton
2021-03-22 13:41 ` Petr Mladek
1 sibling, 1 reply; 7+ messages in thread
From: Andrew Morton @ 2021-03-20 16:32 UTC (permalink / raw)
To: Petr Mladek
Cc: Peter Zijlstra, loberman, mhocko, mingo, mm-commits, tglx,
vincent.whitchurch
On Fri, 19 Mar 2021 13:14:40 +0100 Petr Mladek <pmladek@suse.com> 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 <pmladek@suse.com>
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.
Link: https://lkml.kernel.org/r/20210311122130.6788-6-pmladek@suse.com
Signed-off-by: Petr Mladek <pmladek@suse.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Laurence Oberman <loberman@redhat.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vincent Whitchurch <vincent.whitchurch@axis.com>
kernel/watchdog.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
--- a/kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus
+++ a/kernel/watchdog.c
@@ -409,11 +409,12 @@ static enum hrtimer_restart watchdog_tim
if (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_tim
dump_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);
_
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] watchdog: Use bit lock operations to prevent multiple soft-lockup reports
2021-03-20 16:32 ` Andrew Morton
@ 2021-03-22 13:41 ` Petr Mladek
0 siblings, 0 replies; 7+ messages in thread
From: Petr Mladek @ 2021-03-22 13:41 UTC (permalink / raw)
To: Andrew Morton
Cc: Peter Zijlstra, loberman, mhocko, mingo, mm-commits, tglx,
vincent.whitchurch
On Sat 2021-03-20 09:32:43, Andrew Morton wrote:
> On Fri, 19 Mar 2021 13:14:40 +0100 Petr Mladek <pmladek@suse.com> 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 <pmladek@suse.com>
> 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.
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
>
> Link: https://lkml.kernel.org/r/20210311122130.6788-6-pmladek@suse.com
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Laurence Oberman <loberman@redhat.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Vincent Whitchurch <vincent.whitchurch@axis.com>
>
> kernel/watchdog.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> --- a/kernel/watchdog.c~watchdog-fix-barriers-when-printing-backtraces-from-all-cpus
> +++ a/kernel/watchdog.c
> @@ -409,11 +409,12 @@ static enum hrtimer_restart watchdog_tim
> if (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_tim
> dump_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);
> _
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-03-22 13:42 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-11 21:22 + watchdog-fix-barriers-when-printing-backtraces-from-all-cpus.patch added to -mm tree akpm
2021-03-16 11:48 ` Peter Zijlstra
2021-03-17 9:14 ` Petr Mladek
2021-03-19 12:14 ` [PATCH] watchdog: Use bit lock operations to prevent multiple soft-lockup reports Petr Mladek
2021-03-19 12:29 ` Peter Zijlstra
2021-03-20 16:32 ` Andrew Morton
2021-03-22 13:41 ` Petr Mladek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).