+ watchdog-fix-barriers-when-printing-backtraces-from-all-cpus.patch added to -mm tree

+ watchdog-fix-barriers-when-printing-backtraces-from-all-cpus.patch added to -mm tree

[akpm]

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

Re: + watchdog-fix-barriers-when-printing-backtraces-from-all-cpus.patch added to -mm tree

[Peter Zijlstra]

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().

Re: + watchdog-fix-barriers-when-printing-backtraces-from-all-cpus.patch added to -mm tree

[Petr Mladek]

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

[PATCH] watchdog: Use bit lock operations to prevent multiple soft-lockup reports

[Petr Mladek]

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

Re: [PATCH] watchdog: Use bit lock operations to prevent multiple soft-lockup reports

[Peter Zijlstra]

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>

Re: [PATCH] watchdog: Use bit lock operations to prevent multiple soft-lockup reports

[Andrew Morton]

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);
_

Re: [PATCH] watchdog: Use bit lock operations to prevent multiple soft-lockup reports

[Petr Mladek]

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);
> _
>