Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1723905 > unrolled thread

[patch 00/29] lockup_detector: Cure hotplug deadlocks and replace duct tape

Back to article view | Back to linux.kernel

Contents

  [patch 00/29] lockup_detector: Cure hotplug deadlocks and replace  duct tape Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 02/29] perf/x86/intel: Sanitize PMU HT bug workaround Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 25/29] lockup_detector: Implement init time detection of perf Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 19/29] lockup_detector: Cleanup header mess Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 26/29] lockup_detector/perf: Implement CPU enable replacement Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 24/29] lockup_detector/perf: Implement init time perf  validation Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
      Re: [patch 24/29] lockup_detector/perf: Implement init time perf  validation Don Zickus <dzickus@redhat.com> - 2017-09-07 18:00 +0200
    [patch 10/29] lockup_detector/perf: Prevent cpu hotplug deadlock Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
      Re: [patch 10/29] lockup_detector/perf: Prevent cpu hotplug deadlock Don Zickus <dzickus@redhat.com> - 2017-09-01 21:10 +0200
        Re: [patch 10/29] lockup_detector/perf: Prevent cpu hotplug  deadlock Thomas Gleixner <tglx@linutronix.de> - 2017-09-01 21:30 +0200
          Re: [patch 10/29] lockup_detector/perf: Prevent cpu hotplug deadlock Don Zickus <dzickus@redhat.com> - 2017-09-05 17:00 +0200
    [patch 21/29] lockup_detector: Cleanup sysctl variable name space Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 18/29] lockup_detector: Further simplify sysctl handling Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 23/29] lockup_detector: Get rid of the racy update loop Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 27/29] lockup_detector: Use new perf CPU enable mechanism Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 13/29] lockup_detector: Cleanup the ifdef maze Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 06/29] lockup_detector: Rework cpu hotplug locking Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 14/29] lockup_detector: Split out cpumask write function Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 11/29] lockup_detector: Remove park_in_progress hackery Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
      Re: [patch 11/29] lockup_detector: Remove park_in_progress hackery Peter Zijlstra <peterz@infradead.org> - 2017-09-04 14:20 +0200
        Re: [patch 11/29] lockup_detector: Remove park_in_progress hackery Don Zickus <dzickus@redhat.com> - 2017-09-05 17:20 +0200
          Re: [patch 11/29] lockup_detector: Remove park_in_progress hackery Thomas Gleixner <tglx@linutronix.de> - 2017-09-05 17:50 +0200
      Re: [patch 11/29] lockup_detector: Remove park_in_progress hackery Thomas Gleixner <tglx@linutronix.de> - 2017-09-05 16:00 +0200
        [patch V2 11/29] lockup_detector: Remove park_in_progress  obfuscation Thomas Gleixner <tglx@linutronix.de> - 2017-09-05 21:20 +0200
    [patch 04/29] parisc: Use lockup_detector_stop() Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 29/29] lockup_detector: Cleanup hotplug locking mess Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 12/29] lockup_detector: Cleanup stub functions Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 03/29] lockup_detector: Provide interface to stop from  poweroff() Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 20/29] lockup_detector/sysctl: Get rid of the ifdeffery Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 08/29] lockup_detector: Mark hardlockup_detector_disable()  __init Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 17/29] lockup_detector: Get rid of the thread teardown/setup  dance Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
      Re: [patch 17/29] lockup_detector: Get rid of the thread  teardown/setup dance Don Zickus <dzickus@redhat.com> - 2017-09-01 21:10 +0200
        Re: [patch 17/29] lockup_detector: Get rid of the thread teardown/setup  dance Thomas Gleixner <tglx@linutronix.de> - 2017-09-01 21:50 +0200
    [patch 15/29] smpboot/threads: Avoid runtime allocation Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 07/29] lockup_detector: Rename watchdog_proc_mutex Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 16/29] lockup_detector: Create new thread handling  infrastructure Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 22/29] lockup_detector: Make watchdog_nmi_reconfigure() two  stage Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
    [patch 01/29] hardlockup_detector: Provide interface to stop/restart  perf events Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:40 +0200
      Re: [patch 01/29] hardlockup_detector: Provide interface to  stop/restart perf events Borislav Petkov <bp@alien8.de> - 2017-09-06 18:20 +0200
    [patch 05/29] lockup_detector: Remove broken suspend/resume interfaces Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 09:50 +0200
    Re: [patch 00/29] lockup_detector: Cure hotplug deadlocks and  replace duct tape Don Zickus <dzickus@redhat.com> - 2017-09-01 00:20 +0200
      Re: [patch 00/29] lockup_detector: Cure hotplug deadlocks and  replace duct tape Nicholas Piggin <npiggin@gmail.com> - 2017-09-01 06:50 +0200
      Re: [patch 00/29] lockup_detector: Cure hotplug deadlocks and replace  duct tape Thomas Gleixner <tglx@linutronix.de> - 2017-09-01 11:20 +0200
    Re: [patch 00/29] lockup_detector: Cure hotplug deadlocks and  replace duct tape Don Zickus <dzickus@redhat.com> - 2017-09-07 18:10 +0200

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1726815 — Re: [patch 11/29] lockup_detector: Remove park_in_progress hackery

On Mon, Sep 04, 2017 at 02:10:50PM +0200, Peter Zijlstra wrote:
> On Mon, Sep 04, 2017 at 01:09:06PM +0200, Ulrich Obergfell wrote:
> 
> > - A thread hogs CPU N (soft lockup) so that watchdog/N is unable to run.
> > - A user re-configures 'watchdog_thresh' on the fly. The reconfiguration
> >   requires parking/unparking of all watchdog threads.
> 
> This is where you fail, its silly to require parking for
> reconfiguration.

Hi Peter,

Ok, please elaborate.  Unless I am misunderstanding, that is what Thomas
requested us do years ago when he implemented the parking/unparking scheme
and what his current patch set is doing now.

The point of parking I believe was to avoid the overhead of tearing down a
thread and restarting it when the code needed to update various lockup
detector settings.

So if we can't depend on parking for reconfiguration, then are the other
options (besides tearing down threads)?

I am not trying to be argumentative here, just trying to fill in the
disconnect between us.


Hi Uli,

I think the race you detailed is solved with Thomas's patches. In the
original design we set the sample period first, then tried parking the
threads, which created the mess.  With this patchset, Thomas properly
parks the threads first, then sets the sample period, thus avoiding the race
I believe.  You should be able to see that in patch 16,
softlockup_reconfigure_threads().

Cheers,
Don

[toc] | [prev] | [next] | [standalone]

#1726824 — Re: [patch 11/29] lockup_detector: Remove park_in_progress hackery

On Tue, 5 Sep 2017, Don Zickus wrote:

> On Mon, Sep 04, 2017 at 02:10:50PM +0200, Peter Zijlstra wrote:
> > On Mon, Sep 04, 2017 at 01:09:06PM +0200, Ulrich Obergfell wrote:
> > 
> > > - A thread hogs CPU N (soft lockup) so that watchdog/N is unable to run.
> > > - A user re-configures 'watchdog_thresh' on the fly. The reconfiguration
> > >   requires parking/unparking of all watchdog threads.
> > 
> > This is where you fail, its silly to require parking for
> > reconfiguration.
> 
> Hi Peter,
> 
> Ok, please elaborate.  Unless I am misunderstanding, that is what Thomas
> requested us do years ago when he implemented the parking/unparking scheme
> and what his current patch set is doing now.
> 
> The point of parking I believe was to avoid the overhead of tearing down a
> thread and restarting it when the code needed to update various lockup
> detector settings.
> 
> So if we can't depend on parking for reconfiguration, then are the other
> options (besides tearing down threads)?

Yes, the park/unpark is what I still use as this was the simplest way to
keep everything in sync.

I pondered to do on the fly reconfiguration as well, but that would add
more code and would not solve the general issue of park/unpark. So I rather
went for a single mechanism which just works, even if it is suboptimal cpu
cycle wise. OTOH that reconfiguration is not something which happens every
5ms, so we can just go for the stupid, but simple mechanism.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1726758 — Re: [patch 11/29] lockup_detector: Remove park_in_progress hackery

On Mon, 4 Sep 2017, Ulrich Obergfell wrote:
>  "b94f51183b06 ("kernel/watchdog: prevent false hardlockup on overloaded
>   system") tries to fix the following issue:
> 
>    watchdog_stop()
>           hrtimer_cancel()
>           perf_nmi_event_stop()
> 
>   If the task gets preempted after canceling the hrtimer or the VM gets
>   scheduled out long enough, then there is a chance that the next NMI will
>   see a stale hrtimer interrupt count and trigger a false positive hard
>   lockup splat."
> 
> This is not exactly the issue that b94f51183b06 is actually supposed to fix.
> For details, please see the accompanying commit log message at:
> 
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/kernel/watchdog.c?id=b94f51183b0617e7b9b4fb4137d4cf1cab7547c2
> 
> For convenience, here is a slightly different description of the scenario
> that b94f51183b06 originally aims to address:
> 
> - A thread hogs CPU N (soft lockup) so that watchdog/N is unable to run.
> - A user re-configures 'watchdog_thresh' on the fly. The reconfiguration
>   requires parking/unparking of all watchdog threads.
> - watchdog_timer_fn() on all CPUs can already pick up the new sample period,
>   but the perf counter frequency cannot be updated on all CPUs yet because
>   watchdog/N is unable to park, so watchdog_overflow_callback() still occurs
>   at a frequency that is based on the old sample period (on CPUs >= N which
>   have not had a chance to park their watchdog threads yet).
> - If 'watchdog_thresh' was increased by the reconfiguration, the interval
>   at which watchdog_timer_fn() is now running could be too large for the
>   watchdog_overflow_callback() function to observe a change of the timer
>   interrupt counter. This can trigger a false positive.

Oh well. You did not fix that at all with that hack.

The real problem is that the watchdog threshold write in proc immediately
updates sample_period, which is evaluated by the running thread.

proc_write()
  set_sample_period()
					<----- Broken starts
  proc_watchdog_update()
    watchdog_enable_all_cpus()
    update_watchdog_all_cpus()
        watchdog_park_threads()
					<----- Broken ends
	  watchdog_park_in_progress = 1

So up to the point where watchdog_park_in_progress is set to 1, the change
to sample_period is visible to all active watchdog threads and will be
picked up by the watchdog threads to rearm their timer. The NMI watchdog
will run with the old frequency until the thread is parked and the watchdog
NMI disarmed.

The underlying problem is the non synchronized update of sample_period and
this band aid hack is not solving that at all.
 
> The above scenario should be rare in practice, so it seemed reasonable to
> come up with a simple low-risk fix that addresses merely this particular
> scenario (watchdog_timer_fn() and watchdog_overflow_callback() now return
> immediately while park is in progress / no lockup detection is performed
> during that window of time).

That thing is neither reasonable nor a fix. It's mindless hackery which
papers over the problem instead of fixing the root cause. It neither saw
the problem in the park/unpark in general which is again a valid source for
false positives.

> In relation to:
> 
>  "That commit added a complete abstrusity with a atomic variable telling the
>   NMI watchdog function that stop is in progress. This is so stupid, that
>   it's not funny anymore."
> 
> I cannot see any 'abstrusity' or 'stupidity' in the pragmatic approach that
> was taken in b94f51183b06 to fix an issue that should be rare in practice
> as described above.
> 
> Please change the commit log of [patch 11/29] since it is inconsistent with
> the commit log of b94f51183b06.

Yes, I will change it to include the above reasoning why this is complete
crap and keep the extra findings for reference.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1726948 — [patch V2 11/29] lockup_detector: Remove park_in_progress obfuscation

b94f51183b06 ("kernel/watchdog: prevent false hardlockup on overloaded
system") tries to fix the following issue:

proc_write()
   set_sample_period()    <--- New sample period becoms visible
 			  <----- Broken starts
   proc_watchdog_update()
     watchdog_enable_all_cpus() 	watchdog_hrtimer_fn()
     update_watchdog_all_cpus()		   restart_timer(sample_period)
        watchdog_park_threads()

					thread->park()
					  disable_nmi()
			  <----- Broken ends

The reason why this is broken is that the update of the watchdog threshold
becomes immediately effective and visible for the hrtimer function which
uses that value to rearm the timer. But the NMI/perf side still uses the
old value up to the point where it is disabled. If the rate has been
lowered then the NMI can run fast enough to 'detect' a hard lockup because
the timer has not fired due to the longer period.

The patch 'fixed' this by adding a variable:

proc_write()
   set_sample_period()
 					<----- Broken starts
   proc_watchdog_update()
     watchdog_enable_all_cpus()		watchdog_hrtimer_fn()
     update_watchdog_all_cpus()		   restart_timer(sample_period)
         watchdog_park_threads()
 	  park_in_progress = 1
 					<----- Broken ends
	  			        nmi_watchdog()
					  if (park_in_progress)
					     return;

The only effect of this variable was to make the window where the breakage
can hit small enough that it was not longer observable in testing. From a
correctness point of view it is a pointless bandaid which merily papers
over the root cause: the unsychronized update of the variable.

Looking deeper into the related code pathes unearthed similar problems in
the watchdog_start()/stop() functions.

 watchdog_start()
	perf_nmi_event_start()
	hrtimer_start()

 watchdog_stop()
	hrtimer_cancel()
	perf_nmi_event_stop()

In both cases the call order is wrong because if the tasks gets preempted
or the VM gets scheduled out long enough after the first call, then there is
a chance that the next NMI will see a stale hrtimer interrupt count and
trigger a false positive hard lockup splat.

Get rid of park_in_progress so the code can be gradually deobfuscated and
pruned from several layers of duct tape papering over the root cause,
which has been either ignored or not understood at all.

Once this is removed the underlying problem will be fixed by rewriting the
proc interface to do a proper synchronized update.

Address the start/stop() ordering problem as well by reverting the call
order, so this part is at least correct now.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---

V2: Make the changelog technically and politically correct.

 include/linux/nmi.h   |    1 -
 kernel/watchdog.c     |   39 ++++++++++++++++++---------------------
 kernel/watchdog_hld.c |    7 ++-----
 3 files changed, 20 insertions(+), 27 deletions(-)

--- a/include/linux/nmi.h
+++ b/include/linux/nmi.h
@@ -27,7 +27,6 @@ extern void touch_softlockup_watchdog_sy
 extern void touch_all_softlockup_watchdogs(void);
 extern unsigned int  softlockup_panic;
 extern int soft_watchdog_enabled;
-extern atomic_t watchdog_park_in_progress;
 #else
 static inline void touch_softlockup_watchdog_sched(void)
 {
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -134,8 +134,6 @@ void __weak watchdog_nmi_reconfigure(voi
 #define for_each_watchdog_cpu(cpu) \
 	for_each_cpu_and((cpu), cpu_online_mask, &watchdog_cpumask)
 
-atomic_t watchdog_park_in_progress = ATOMIC_INIT(0);
-
 static u64 __read_mostly sample_period;
 
 static DEFINE_PER_CPU(unsigned long, watchdog_touch_ts);
@@ -320,8 +318,7 @@ static enum hrtimer_restart watchdog_tim
 	int duration;
 	int softlockup_all_cpu_backtrace = sysctl_softlockup_all_cpu_backtrace;
 
-	if (!watchdog_enabled ||
-	    atomic_read(&watchdog_park_in_progress) != 0)
+	if (!watchdog_enabled)
 		return HRTIMER_NORESTART;
 
 	/* kick the hardlockup detector */
@@ -435,33 +432,38 @@ static void watchdog_set_prio(unsigned i
 
 static void watchdog_enable(unsigned int cpu)
 {
-	struct hrtimer *hrtimer = raw_cpu_ptr(&watchdog_hrtimer);
+	struct hrtimer *hrtimer = this_cpu_ptr(&watchdog_hrtimer);
 
-	/* kick off the timer for the hardlockup detector */
+	/*
+	 * Start the timer first to prevent the NMI watchdog triggering
+	 * before the timer has a chance to fire.
+	 */
 	hrtimer_init(hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	hrtimer->function = watchdog_timer_fn;
+	hrtimer_start(hrtimer, ns_to_ktime(sample_period),
+		      HRTIMER_MODE_REL_PINNED);
 
+	/* Initialize timestamp */
+	__touch_watchdog();
 	/* Enable the perf event */
 	watchdog_nmi_enable(cpu);
 
-	/* done here because hrtimer_start can only pin to smp_processor_id() */
-	hrtimer_start(hrtimer, ns_to_ktime(sample_period),
-		      HRTIMER_MODE_REL_PINNED);
-
-	/* initialize timestamp */
 	watchdog_set_prio(SCHED_FIFO, MAX_RT_PRIO - 1);
-	__touch_watchdog();
 }
 
 static void watchdog_disable(unsigned int cpu)
 {
-	struct hrtimer *hrtimer = raw_cpu_ptr(&watchdog_hrtimer);
+	struct hrtimer *hrtimer = this_cpu_ptr(&watchdog_hrtimer);
 
 	watchdog_set_prio(SCHED_NORMAL, 0);
-	hrtimer_cancel(hrtimer);
-	/* disable the perf event */
-	watchdog_nmi_disable(cpu);
+	/*
+	 * Disable the perf event first. That prevents that a large delay
+	 * between disabling the timer and disabling the perf event causes
+	 * the perf NMI to detect a false positive.
+	 */
 	hardlockup_detector_perf_disable();
+	watchdog_nmi_disable(cpu);
+	hrtimer_cancel(hrtimer);
 }
 
 static void watchdog_cleanup(unsigned int cpu, bool online)
@@ -517,16 +519,11 @@ static int watchdog_park_threads(void)
 {
 	int cpu, ret = 0;
 
-	atomic_set(&watchdog_park_in_progress, 1);
-
 	for_each_watchdog_cpu(cpu) {
 		ret = kthread_park(per_cpu(softlockup_watchdog, cpu));
 		if (ret)
 			break;
 	}
-
-	atomic_set(&watchdog_park_in_progress, 0);
-
 	return ret;
 }
 
--- a/kernel/watchdog_hld.c
+++ b/kernel/watchdog_hld.c
@@ -106,15 +106,12 @@ static struct perf_event_attr wd_hw_attr
 
 /* Callback function for perf event subsystem */
 static void watchdog_overflow_callback(struct perf_event *event,
-		 struct perf_sample_data *data,
-		 struct pt_regs *regs)
+				       struct perf_sample_data *data,
+				       struct pt_regs *regs)
 {
 	/* Ensure the watchdog never gets throttled */
 	event->hw.interrupts = 0;
 
-	if (atomic_read(&watchdog_park_in_progress) != 0)
-		return;
-
 	if (__this_cpu_read(watchdog_nmi_touch) == true) {
 		__this_cpu_write(watchdog_nmi_touch, false);
 		return;

[toc] | [prev] | [next] | [standalone]

#1723925 — [patch 04/29] parisc: Use lockup_detector_stop()

The broken lockup_detector_suspend/resume() interface is going away. Use
the new lockup_detector_soft_poweroff() interface to stop the watchdog from
the busy looping power off routine.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Helge Deller <deller@gmx.de>
Cc: linux-parisc@vger.kernel.org
---
 arch/parisc/kernel/process.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/arch/parisc/kernel/process.c
+++ b/arch/parisc/kernel/process.c
@@ -146,7 +146,7 @@ void machine_power_off(void)
 
 	/* prevent soft lockup/stalled CPU messages for endless loop. */
 	rcu_sysrq_start();
-	lockup_detector_suspend();
+	lockup_detector_soft_poweroff();
 	for (;;);
 }
 

[toc] | [prev] | [next] | [standalone]

#1723926 — [patch 29/29] lockup_detector: Cleanup hotplug locking mess

All watchdog thread related functions are delegated to the smpboot thread
infrastructure, which handles serialization against CPU hotplug correctly.

The sysctl interface is completely decoupled from anything which requires
CPU hotplug protection.

No need to protect the sysctl writes against cpu hotplug anymore. Remove it
and add the now required protection to the powerpc arch_nmi_watchdog
implementation.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: linuxppc-dev@lists.ozlabs.org
---
 arch/powerpc/kernel/watchdog.c |    2 ++
 kernel/watchdog.c              |    6 ------
 2 files changed, 2 insertions(+), 6 deletions(-)

--- a/arch/powerpc/kernel/watchdog.c
+++ b/arch/powerpc/kernel/watchdog.c
@@ -356,6 +356,7 @@ void watchdog_nmi_reconfigure(bool stop)
 {
 	int cpu;
 
+	cpus_read_lock();
 	if (stop) {
 		for_each_cpu(cpu, &wd_cpus_enabled)
 			stop_wd_on_cpu(cpu);
@@ -364,6 +365,7 @@ void watchdog_nmi_reconfigure(bool stop)
 		for_each_cpu_and(cpu, cpu_online_mask, &watchdog_cpumask)
 			start_wd_on_cpu(cpu);
 	}
+	cpus_read_unlock();
 }
 
 /*
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -652,7 +652,6 @@ static int proc_watchdog_common(int whic
 {
 	int err, old, *param = table->data;
 
-	cpu_hotplug_disable();
 	mutex_lock(&watchdog_mutex);
 
 	if (!write) {
@@ -669,7 +668,6 @@ static int proc_watchdog_common(int whic
 			proc_watchdog_update();
 	}
 	mutex_unlock(&watchdog_mutex);
-	cpu_hotplug_enable();
 	return err;
 }
 
@@ -713,7 +711,6 @@ int proc_watchdog_thresh(struct ctl_tabl
 {
 	int err, old;
 
-	cpu_hotplug_disable();
 	mutex_lock(&watchdog_mutex);
 
 	old = READ_ONCE(watchdog_thresh);
@@ -723,7 +720,6 @@ int proc_watchdog_thresh(struct ctl_tabl
 		proc_watchdog_update();
 
 	mutex_unlock(&watchdog_mutex);
-	cpu_hotplug_enable();
 	return err;
 }
 
@@ -738,7 +734,6 @@ int proc_watchdog_cpumask(struct ctl_tab
 {
 	int err;
 
-	cpu_hotplug_disable();
 	mutex_lock(&watchdog_mutex);
 
 	err = proc_do_large_bitmap(table, write, buffer, lenp, ppos);
@@ -746,7 +741,6 @@ int proc_watchdog_cpumask(struct ctl_tab
 		proc_watchdog_update();
 
 	mutex_unlock(&watchdog_mutex);
-	cpu_hotplug_enable();
 	return err;
 }
 #endif /* CONFIG_SYSCTL */

[toc] | [prev] | [next] | [standalone]

#1723927 — [patch 12/29] lockup_detector: Cleanup stub functions

Having stub functions which take a full page is not helping the
readablility of code.

Condense them and move the doubled #ifdef variant into the SYSFS section.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/watchdog.c |   77 ++++++++++++++++--------------------------------------
 1 file changed, 24 insertions(+), 53 deletions(-)

--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -105,13 +105,8 @@ static int __read_mostly watchdog_runnin
  * softlockup watchdog threads start and stop. The arch must select the
  * SOFTLOCKUP_DETECTOR Kconfig.
  */
-int __weak watchdog_nmi_enable(unsigned int cpu)
-{
-	return 0;
-}
-void __weak watchdog_nmi_disable(unsigned int cpu)
-{
-}
+int __weak watchdog_nmi_enable(unsigned int cpu) { return 0; }
+void __weak watchdog_nmi_disable(unsigned int cpu) { }
 
 /*
  * watchdog_nmi_reconfigure can be implemented to be notified after any
@@ -123,10 +118,7 @@ void __weak watchdog_nmi_disable(unsigne
  * - sysctl_hardlockup_all_cpu_backtrace
  * - hardlockup_panic
  */
-void __weak watchdog_nmi_reconfigure(void)
-{
-}
-
+void __weak watchdog_nmi_reconfigure(void) { }
 
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
 
@@ -134,6 +126,11 @@ void __weak watchdog_nmi_reconfigure(voi
 #define for_each_watchdog_cpu(cpu) \
 	for_each_cpu_and((cpu), cpu_online_mask, &watchdog_cpumask)
 
+/* Global variables, exported for sysctl */
+unsigned int __read_mostly softlockup_panic =
+			CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE;
+int __read_mostly soft_watchdog_enabled;
+
 static u64 __read_mostly sample_period;
 
 static DEFINE_PER_CPU(unsigned long, watchdog_touch_ts);
@@ -147,13 +144,9 @@ static DEFINE_PER_CPU(struct task_struct
 static DEFINE_PER_CPU(unsigned long, hrtimer_interrupts_saved);
 static unsigned long soft_lockup_nmi_warn;
 
-unsigned int __read_mostly softlockup_panic =
-			CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE;
-
 static int __init softlockup_panic_setup(char *str)
 {
 	softlockup_panic = simple_strtoul(str, NULL, 0);
-
 	return 1;
 }
 __setup("softlockup_panic=", softlockup_panic_setup);
@@ -592,44 +585,13 @@ static void watchdog_disable_all_cpus(vo
 	}
 }
 
-#ifdef CONFIG_SYSCTL
-static int watchdog_update_cpus(void)
-{
-	return smpboot_update_cpumask_percpu_thread(
-		    &watchdog_threads, &watchdog_cpumask);
-}
-#endif
-
-#else /* SOFTLOCKUP */
-static int watchdog_park_threads(void)
-{
-	return 0;
-}
-
-static void watchdog_unpark_threads(void)
-{
-}
-
-static int watchdog_enable_all_cpus(void)
-{
-	return 0;
-}
-
-static void watchdog_disable_all_cpus(void)
-{
-}
-
-#ifdef CONFIG_SYSCTL
-static int watchdog_update_cpus(void)
-{
-	return 0;
-}
-#endif
-
-static void set_sample_period(void)
-{
-}
-#endif /* SOFTLOCKUP */
+#else /* CONFIG_SOFTLOCKUP_DETECTOR */
+static inline int watchdog_park_threads(void) { return 0; }
+static inline void watchdog_unpark_threads(void) { }
+static inline int watchdog_enable_all_cpus(void) { return 0; }
+static inline void watchdog_disable_all_cpus(void) { }
+static inline void set_sample_period(void) { }
+#endif /* !CONFIG_SOFTLOCKUP_DETECTOR */
 
 static void __lockup_detector_cleanup(void)
 {
@@ -826,6 +788,15 @@ int proc_watchdog_thresh(struct ctl_tabl
 	return err;
 }
 
+static int watchdog_update_cpus(void)
+{
+	if (IS_ENABLED(CONFIG_SOFTLOCKUP_DETECTOR)) {
+		return smpboot_update_cpumask_percpu_thread(&watchdog_threads,
+							    &watchdog_cpumask);
+	}
+	return 0;
+}
+
 /*
  * The cpumask is the mask of possible cpus that the watchdog can run
  * on, not the mask of cpus it is actually running on.  This allows the

[toc] | [prev] | [next] | [standalone]

#1723928 — [patch 03/29] lockup_detector: Provide interface to stop from poweroff()

PARISC has a a busy looping power off routine. If the watchdog is enabled
the watchdog timer will still fire, but the thread is not running, which
causes the softlockup watchdog to trigger.

Provide a interface which allows to turn the watchdog off.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Helge Deller <deller@gmx.de>
Cc: linux-parisc@vger.kernel.org
---
 include/linux/nmi.h |    6 +++---
 kernel/watchdog.c   |   14 +++++++++++++-
 2 files changed, 16 insertions(+), 4 deletions(-)

--- a/include/linux/nmi.h
+++ b/include/linux/nmi.h
@@ -12,10 +12,10 @@
 
 #ifdef CONFIG_LOCKUP_DETECTOR
 void lockup_detector_init(void);
+void lockup_detector_soft_poweroff(void);
 #else
-static inline void lockup_detector_init(void)
-{
-}
+static inline void lockup_detector_init(void) { }
+static inline void lockup_detector_soft_poweroff(void) { }
 #endif
 
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -333,7 +333,8 @@ static enum hrtimer_restart watchdog_tim
 	int duration;
 	int softlockup_all_cpu_backtrace = sysctl_softlockup_all_cpu_backtrace;
 
-	if (atomic_read(&watchdog_park_in_progress) != 0)
+	if (!watchdog_enabled ||
+	    atomic_read(&watchdog_park_in_progress) != 0)
 		return HRTIMER_NORESTART;
 
 	/* kick the hardlockup detector */
@@ -660,6 +661,17 @@ static void set_sample_period(void)
 }
 #endif /* SOFTLOCKUP */
 
+/**
+ * lockup_detector_soft_poweroff - Interface to stop lockup detector(s)
+ *
+ * Special interface for parisc. It prevents lockup detector warnings from
+ * the default pm_poweroff() function which busy loops forever.
+ */
+void lockup_detector_soft_poweroff(void)
+{
+	watchdog_enabled = 0;
+}
+
 /*
  * Suspend the hard and soft lockup detector by parking the watchdog threads.
  */

[toc] | [prev] | [next] | [standalone]

#1723930 — [patch 20/29] lockup_detector/sysctl: Get rid of the ifdeffery

The sysctl of the nmi_watchdog file prevents writes by setting
    min = max = 0

if none of the users is enabled. That involves ifdeffery and is competely
non obvious.

If none of the facilities is enabeld, then the file can simply be made read
only. Move the ifdeffery into the header and use a constant for file
permissions.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/nmi.h |    6 ++++++
 kernel/sysctl.c     |    6 +-----
 2 files changed, 7 insertions(+), 5 deletions(-)

--- a/include/linux/nmi.h
+++ b/include/linux/nmi.h
@@ -81,6 +81,12 @@ extern unsigned int hardlockup_panic;
 static inline void hardlockup_detector_disable(void) {}
 #endif
 
+#if defined(CONFIG_HAVE_NMI_WATCHDOG) || defined(CONFIG_HARDLOCKUP_DETECTOR)
+# define NMI_WATCHDOG_SYSCTL_PERM	0644
+#else
+# define NMI_WATCHDOG_SYSCTL_PERM	0444
+#endif
+
 #if defined(CONFIG_HARDLOCKUP_DETECTOR_PERF)
 extern void arch_touch_nmi_watchdog(void);
 extern void hardlockup_detector_perf_stop(void);
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -891,14 +891,10 @@ static struct ctl_table kern_table[] = {
 		.procname       = "nmi_watchdog",
 		.data           = &nmi_watchdog_enabled,
 		.maxlen         = sizeof (int),
-		.mode           = 0644,
+		.mode		= NMI_WATCHDOG_SYSCTL_PERM,
 		.proc_handler   = proc_nmi_watchdog,
 		.extra1		= &zero,
-#if defined(CONFIG_HAVE_NMI_WATCHDOG) || defined(CONFIG_HARDLOCKUP_DETECTOR)
 		.extra2		= &one,
-#else
-		.extra2		= &zero,
-#endif
 	},
 	{
 		.procname	= "watchdog_cpumask",

[toc] | [prev] | [next] | [standalone]

#1723931 — [patch 08/29] lockup_detector: Mark hardlockup_detector_disable() __init

The function is only used by the KVM init code. Mark it __init to prevent
creative abuse.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/watchdog.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -55,7 +55,7 @@ unsigned int __read_mostly hardlockup_pa
  * kernel command line parameters are parsed, because otherwise it is not
  * possible to override this in hardlockup_panic_setup().
  */
-void hardlockup_detector_disable(void)
+void __init hardlockup_detector_disable(void)
 {
 	watchdog_enabled &= ~NMI_WATCHDOG_ENABLED;
 }

[toc] | [prev] | [next] | [standalone]

#1723932 — [patch 17/29] lockup_detector: Get rid of the thread teardown/setup dance

The lockup detector reconfiguration tears down all watchdog threads when
the watchdog is disabled and sets them up again when its enabled.

That's a pointless exercise. The watchdog threads are not consuming an
insane amount of resources, so it's enough to set them up at init time and
keep them in parked position when the watchdog is disabled and unpark them
when it is reenabled. The smpboot thread infrastructure takes care of
keeping the force parked threads in place even across cpu hotplug.

Aside of that the code implements the park/unpark facility of smp hotplug
threads on its own, which is even more pointless. We have functionality in
the smpboot thread code to do so.

Use the new thread management functions and get rid of the unholy mess.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/watchdog.c |  190 +++++-------------------------------------------------
 1 file changed, 19 insertions(+), 171 deletions(-)

--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -92,13 +92,6 @@ struct cpumask watchdog_cpumask __read_m
 unsigned long *watchdog_cpumask_bits = cpumask_bits(&watchdog_cpumask);
 
 /*
- * The 'watchdog_running' variable is set to 1 when the watchdog threads
- * are registered/started and is set to 0 when the watchdog threads are
- * unregistered/stopped, so it is an indicator whether the threads exist.
- */
-static int __read_mostly watchdog_running;
-
-/*
  * These functions can be overridden if an architecture implements its
  * own hardlockup detector.
  *
@@ -123,10 +116,6 @@ void __weak watchdog_nmi_reconfigure(voi
 
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
 
-/* Helper for online, unparked cpus. */
-#define for_each_watchdog_cpu(cpu) \
-	for_each_cpu_and((cpu), cpu_online_mask, &watchdog_cpumask)
-
 /* Global variables, exported for sysctl */
 unsigned int __read_mostly softlockup_panic =
 			CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE;
@@ -252,11 +241,15 @@ void touch_all_softlockup_watchdogs(void
 	int cpu;
 
 	/*
-	 * this is done lockless
-	 * do we care if a 0 races with a timestamp?
-	 * all it means is the softlock check starts one cycle later
+	 * watchdog_mutex cannpt be taken here, as this might be called
+	 * from (soft)interrupt context, so the access to
+	 * watchdog_allowed_cpumask might race with a concurrent update.
+	 *
+	 * The watchdog time stamp can race against a concurrent real
+	 * update as well, the only side effect might be a cycle delay for
+	 * the softlockup check.
 	 */
-	for_each_watchdog_cpu(cpu)
+	for_each_cpu(cpu, &watchdog_allowed_mask)
 		per_cpu(watchdog_touch_ts, cpu) = 0;
 	wq_watchdog_touch(-1);
 }
@@ -296,9 +289,6 @@ static void watchdog_interrupt_count(voi
 	__this_cpu_inc(hrtimer_interrupts);
 }
 
-static int watchdog_enable_all_cpus(void);
-static void watchdog_disable_all_cpus(void);
-
 /* watchdog kicker functions */
 static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
 {
@@ -492,95 +482,6 @@ static struct smp_hotplug_thread watchdo
 	.unpark			= watchdog_enable,
 };
 
-/*
- * park all watchdog threads that are specified in 'watchdog_cpumask'
- *
- * This function returns an error if kthread_park() of a watchdog thread
- * fails. In this situation, the watchdog threads of some CPUs can already
- * be parked and the watchdog threads of other CPUs can still be runnable.
- * Callers are expected to handle this special condition as appropriate in
- * their context.
- *
- * This function may only be called in a context that is protected against
- * races with CPU hotplug - for example, via get_online_cpus().
- */
-static int watchdog_park_threads(void)
-{
-	int cpu, ret = 0;
-
-	for_each_watchdog_cpu(cpu) {
-		ret = kthread_park(per_cpu(softlockup_watchdog, cpu));
-		if (ret)
-			break;
-	}
-	return ret;
-}
-
-/*
- * unpark all watchdog threads that are specified in 'watchdog_cpumask'
- *
- * This function may only be called in a context that is protected against
- * races with CPU hotplug - for example, via get_online_cpus().
- */
-static void watchdog_unpark_threads(void)
-{
-	int cpu;
-
-	for_each_watchdog_cpu(cpu)
-		kthread_unpark(per_cpu(softlockup_watchdog, cpu));
-}
-
-static int update_watchdog_all_cpus(void)
-{
-	int ret;
-
-	ret = watchdog_park_threads();
-	if (ret)
-		return ret;
-
-	watchdog_unpark_threads();
-
-	return 0;
-}
-
-static int watchdog_enable_all_cpus(void)
-{
-	int err = 0;
-
-	if (!watchdog_running) {
-		err = smpboot_register_percpu_thread_cpumask(&watchdog_threads,
-							     &watchdog_cpumask);
-		if (err)
-			pr_err("Failed to create watchdog threads, disabled\n");
-		else
-			watchdog_running = 1;
-	} else {
-		/*
-		 * Enable/disable the lockup detectors or
-		 * change the sample period 'on the fly'.
-		 */
-		err = update_watchdog_all_cpus();
-
-		if (err) {
-			watchdog_disable_all_cpus();
-			pr_err("Failed to update lockup detectors, disabled\n");
-		}
-	}
-
-	if (err)
-		watchdog_enabled = 0;
-
-	return err;
-}
-
-static void watchdog_disable_all_cpus(void)
-{
-	if (watchdog_running) {
-		watchdog_running = 0;
-		smpboot_unregister_percpu_thread(&watchdog_threads);
-	}
-}
-
 static void softlockup_update_smpboot_threads(void)
 {
 	lockdep_assert_held(&watchdog_mutex);
@@ -655,7 +556,6 @@ static inline int watchdog_park_threads(
 static inline void watchdog_unpark_threads(void) { }
 static inline int watchdog_enable_all_cpus(void) { return 0; }
 static inline void watchdog_disable_all_cpus(void) { }
-static inline void set_sample_period(void) { }
 static inline void softlockup_init_threads(void) { }
 static inline void softlockup_update_threads(void) { }
 static inline void softlockup_reconfigure_threads(bool enabled) { }
@@ -695,28 +595,10 @@ void lockup_detector_soft_poweroff(void)
 /*
  * Update the run state of the lockup detectors.
  */
-static int proc_watchdog_update(void)
+static void proc_watchdog_update(void)
 {
-	int err = 0;
-
-	/*
-	 * Watchdog threads won't be started if they are already active.
-	 * The 'watchdog_running' variable in watchdog_*_all_cpus() takes
-	 * care of this. If those threads are already active, the sample
-	 * period will be updated and the lockup detectors will be enabled
-	 * or disabled 'on the fly'.
-	 */
-	if (watchdog_enabled && watchdog_thresh)
-		err = watchdog_enable_all_cpus();
-	else
-		watchdog_disable_all_cpus();
-
+	softlockup_reconfigure_threads(watchdog_enabled && watchdog_thresh);
 	watchdog_nmi_reconfigure();
-
-	__lockup_detector_cleanup();
-
-	return err;
-
 }
 
 /*
@@ -772,17 +654,8 @@ static int proc_watchdog_common(int whic
 				new = old & ~which;
 		} while (cmpxchg(&watchdog_enabled, old, new) != old);
 
-		/*
-		 * Update the run state of the lockup detectors. There is _no_
-		 * need to check the value returned by proc_watchdog_update()
-		 * and to restore the previous value of 'watchdog_enabled' as
-		 * both lockup detectors are disabled if proc_watchdog_update()
-		 * returns an error.
-		 */
-		if (old == new)
-			goto out;
-
-		err = proc_watchdog_update();
+		if (old != new)
+			proc_watchdog_update();
 	}
 out:
 	mutex_unlock(&watchdog_mutex);
@@ -826,50 +699,28 @@ int proc_soft_watchdog(struct ctl_table
 int proc_watchdog_thresh(struct ctl_table *table, int write,
 			 void __user *buffer, size_t *lenp, loff_t *ppos)
 {
-	int err, old, new;
+	int err, old;
 
 	cpu_hotplug_disable();
 	mutex_lock(&watchdog_mutex);
 
-	old = ACCESS_ONCE(watchdog_thresh);
+	old = READ_ONCE(watchdog_thresh);
 	err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
 
-	if (err || !write)
-		goto out;
+	if (!err && write && old != READ_ONCE(watchdog_thresh))
+		proc_watchdog_update();
 
-	/*
-	 * Update the sample period. Restore on failure.
-	 */
-	new = ACCESS_ONCE(watchdog_thresh);
-	if (old == new)
-		goto out;
-
-	set_sample_period();
-	err = proc_watchdog_update();
-	if (err) {
-		watchdog_thresh = old;
-		set_sample_period();
-	}
-out:
 	mutex_unlock(&watchdog_mutex);
 	cpu_hotplug_enable();
 	return err;
 }
 
-static void watchdog_update_cpus(void)
-{
-	if (IS_ENABLED(CONFIG_SOFTLOCKUP_DETECTOR) && watchdog_running) {
-		smpboot_update_cpumask_percpu_thread(&watchdog_threads,
-						     &watchdog_cpumask);
-		__lockup_detector_cleanup();
-	}
-}
-
 static void proc_watchdog_cpumask_update(void)
 {
 	/* Remove impossible cpus to keep sysctl output clean. */
 	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
-	watchdog_update_cpus();
+
+	softlockup_update_threads();
 	watchdog_nmi_reconfigure();
 }
 
@@ -899,8 +750,6 @@ int proc_watchdog_cpumask(struct ctl_tab
 
 void __init lockup_detector_init(void)
 {
-	set_sample_period();
-
 #ifdef CONFIG_NO_HZ_FULL
 	if (tick_nohz_full_enabled()) {
 		pr_info("Disabling watchdog on nohz_full cores by default\n");
@@ -911,6 +760,5 @@ void __init lockup_detector_init(void)
 	cpumask_copy(&watchdog_cpumask, cpu_possible_mask);
 #endif
 
-	if (watchdog_enabled)
-		watchdog_enable_all_cpus();
+	softlockup_init_threads();
 }

[toc] | [prev] | [next] | [standalone]

#1725292 — Re: [patch 17/29] lockup_detector: Get rid of the thread teardown/setup dance

On Thu, Aug 31, 2017 at 09:16:15AM +0200, Thomas Gleixner wrote:
> The lockup detector reconfiguration tears down all watchdog threads when
> the watchdog is disabled and sets them up again when its enabled.
> 
> That's a pointless exercise. The watchdog threads are not consuming an
> insane amount of resources, so it's enough to set them up at init time and
> keep them in parked position when the watchdog is disabled and unpark them
> when it is reenabled. The smpboot thread infrastructure takes care of
> keeping the force parked threads in place even across cpu hotplug.

The original reasoning for implementing this complexity was we were worried
about catching kthread_[un]park errors.  It was thought in the rare case if
it failed, we would hang.  Perhaps we misunderstood the kthread_[un]park
code.

Cheers,
Don

> 
> Aside of that the code implements the park/unpark facility of smp hotplug
> threads on its own, which is even more pointless. We have functionality in
> the smpboot thread code to do so.
> 
> Use the new thread management functions and get rid of the unholy mess.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  kernel/watchdog.c |  190 +++++-------------------------------------------------
>  1 file changed, 19 insertions(+), 171 deletions(-)
> 
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -92,13 +92,6 @@ struct cpumask watchdog_cpumask __read_m
>  unsigned long *watchdog_cpumask_bits = cpumask_bits(&watchdog_cpumask);
>  
>  /*
> - * The 'watchdog_running' variable is set to 1 when the watchdog threads
> - * are registered/started and is set to 0 when the watchdog threads are
> - * unregistered/stopped, so it is an indicator whether the threads exist.
> - */
> -static int __read_mostly watchdog_running;
> -
> -/*
>   * These functions can be overridden if an architecture implements its
>   * own hardlockup detector.
>   *
> @@ -123,10 +116,6 @@ void __weak watchdog_nmi_reconfigure(voi
>  
>  #ifdef CONFIG_SOFTLOCKUP_DETECTOR
>  
> -/* Helper for online, unparked cpus. */
> -#define for_each_watchdog_cpu(cpu) \
> -	for_each_cpu_and((cpu), cpu_online_mask, &watchdog_cpumask)
> -
>  /* Global variables, exported for sysctl */
>  unsigned int __read_mostly softlockup_panic =
>  			CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE;
> @@ -252,11 +241,15 @@ void touch_all_softlockup_watchdogs(void
>  	int cpu;
>  
>  	/*
> -	 * this is done lockless
> -	 * do we care if a 0 races with a timestamp?
> -	 * all it means is the softlock check starts one cycle later
> +	 * watchdog_mutex cannpt be taken here, as this might be called
> +	 * from (soft)interrupt context, so the access to
> +	 * watchdog_allowed_cpumask might race with a concurrent update.
> +	 *
> +	 * The watchdog time stamp can race against a concurrent real
> +	 * update as well, the only side effect might be a cycle delay for
> +	 * the softlockup check.
>  	 */
> -	for_each_watchdog_cpu(cpu)
> +	for_each_cpu(cpu, &watchdog_allowed_mask)
>  		per_cpu(watchdog_touch_ts, cpu) = 0;
>  	wq_watchdog_touch(-1);
>  }
> @@ -296,9 +289,6 @@ static void watchdog_interrupt_count(voi
>  	__this_cpu_inc(hrtimer_interrupts);
>  }
>  
> -static int watchdog_enable_all_cpus(void);
> -static void watchdog_disable_all_cpus(void);
> -
>  /* watchdog kicker functions */
>  static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
>  {
> @@ -492,95 +482,6 @@ static struct smp_hotplug_thread watchdo
>  	.unpark			= watchdog_enable,
>  };
>  
> -/*
> - * park all watchdog threads that are specified in 'watchdog_cpumask'
> - *
> - * This function returns an error if kthread_park() of a watchdog thread
> - * fails. In this situation, the watchdog threads of some CPUs can already
> - * be parked and the watchdog threads of other CPUs can still be runnable.
> - * Callers are expected to handle this special condition as appropriate in
> - * their context.
> - *
> - * This function may only be called in a context that is protected against
> - * races with CPU hotplug - for example, via get_online_cpus().
> - */
> -static int watchdog_park_threads(void)
> -{
> -	int cpu, ret = 0;
> -
> -	for_each_watchdog_cpu(cpu) {
> -		ret = kthread_park(per_cpu(softlockup_watchdog, cpu));
> -		if (ret)
> -			break;
> -	}
> -	return ret;
> -}
> -
> -/*
> - * unpark all watchdog threads that are specified in 'watchdog_cpumask'
> - *
> - * This function may only be called in a context that is protected against
> - * races with CPU hotplug - for example, via get_online_cpus().
> - */
> -static void watchdog_unpark_threads(void)
> -{
> -	int cpu;
> -
> -	for_each_watchdog_cpu(cpu)
> -		kthread_unpark(per_cpu(softlockup_watchdog, cpu));
> -}
> -
> -static int update_watchdog_all_cpus(void)
> -{
> -	int ret;
> -
> -	ret = watchdog_park_threads();
> -	if (ret)
> -		return ret;
> -
> -	watchdog_unpark_threads();
> -
> -	return 0;
> -}
> -
> -static int watchdog_enable_all_cpus(void)
> -{
> -	int err = 0;
> -
> -	if (!watchdog_running) {
> -		err = smpboot_register_percpu_thread_cpumask(&watchdog_threads,
> -							     &watchdog_cpumask);
> -		if (err)
> -			pr_err("Failed to create watchdog threads, disabled\n");
> -		else
> -			watchdog_running = 1;
> -	} else {
> -		/*
> -		 * Enable/disable the lockup detectors or
> -		 * change the sample period 'on the fly'.
> -		 */
> -		err = update_watchdog_all_cpus();
> -
> -		if (err) {
> -			watchdog_disable_all_cpus();
> -			pr_err("Failed to update lockup detectors, disabled\n");
> -		}
> -	}
> -
> -	if (err)
> -		watchdog_enabled = 0;
> -
> -	return err;
> -}
> -
> -static void watchdog_disable_all_cpus(void)
> -{
> -	if (watchdog_running) {
> -		watchdog_running = 0;
> -		smpboot_unregister_percpu_thread(&watchdog_threads);
> -	}
> -}
> -
>  static void softlockup_update_smpboot_threads(void)
>  {
>  	lockdep_assert_held(&watchdog_mutex);
> @@ -655,7 +556,6 @@ static inline int watchdog_park_threads(
>  static inline void watchdog_unpark_threads(void) { }
>  static inline int watchdog_enable_all_cpus(void) { return 0; }
>  static inline void watchdog_disable_all_cpus(void) { }
> -static inline void set_sample_period(void) { }
>  static inline void softlockup_init_threads(void) { }
>  static inline void softlockup_update_threads(void) { }
>  static inline void softlockup_reconfigure_threads(bool enabled) { }
> @@ -695,28 +595,10 @@ void lockup_detector_soft_poweroff(void)
>  /*
>   * Update the run state of the lockup detectors.
>   */
> -static int proc_watchdog_update(void)
> +static void proc_watchdog_update(void)
>  {
> -	int err = 0;
> -
> -	/*
> -	 * Watchdog threads won't be started if they are already active.
> -	 * The 'watchdog_running' variable in watchdog_*_all_cpus() takes
> -	 * care of this. If those threads are already active, the sample
> -	 * period will be updated and the lockup detectors will be enabled
> -	 * or disabled 'on the fly'.
> -	 */
> -	if (watchdog_enabled && watchdog_thresh)
> -		err = watchdog_enable_all_cpus();
> -	else
> -		watchdog_disable_all_cpus();
> -
> +	softlockup_reconfigure_threads(watchdog_enabled && watchdog_thresh);
>  	watchdog_nmi_reconfigure();
> -
> -	__lockup_detector_cleanup();
> -
> -	return err;
> -
>  }
>  
>  /*
> @@ -772,17 +654,8 @@ static int proc_watchdog_common(int whic
>  				new = old & ~which;
>  		} while (cmpxchg(&watchdog_enabled, old, new) != old);
>  
> -		/*
> -		 * Update the run state of the lockup detectors. There is _no_
> -		 * need to check the value returned by proc_watchdog_update()
> -		 * and to restore the previous value of 'watchdog_enabled' as
> -		 * both lockup detectors are disabled if proc_watchdog_update()
> -		 * returns an error.
> -		 */
> -		if (old == new)
> -			goto out;
> -
> -		err = proc_watchdog_update();
> +		if (old != new)
> +			proc_watchdog_update();
>  	}
>  out:
>  	mutex_unlock(&watchdog_mutex);
> @@ -826,50 +699,28 @@ int proc_soft_watchdog(struct ctl_table
>  int proc_watchdog_thresh(struct ctl_table *table, int write,
>  			 void __user *buffer, size_t *lenp, loff_t *ppos)
>  {
> -	int err, old, new;
> +	int err, old;
>  
>  	cpu_hotplug_disable();
>  	mutex_lock(&watchdog_mutex);
>  
> -	old = ACCESS_ONCE(watchdog_thresh);
> +	old = READ_ONCE(watchdog_thresh);
>  	err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
>  
> -	if (err || !write)
> -		goto out;
> +	if (!err && write && old != READ_ONCE(watchdog_thresh))
> +		proc_watchdog_update();
>  
> -	/*
> -	 * Update the sample period. Restore on failure.
> -	 */
> -	new = ACCESS_ONCE(watchdog_thresh);
> -	if (old == new)
> -		goto out;
> -
> -	set_sample_period();
> -	err = proc_watchdog_update();
> -	if (err) {
> -		watchdog_thresh = old;
> -		set_sample_period();
> -	}
> -out:
>  	mutex_unlock(&watchdog_mutex);
>  	cpu_hotplug_enable();
>  	return err;
>  }
>  
> -static void watchdog_update_cpus(void)
> -{
> -	if (IS_ENABLED(CONFIG_SOFTLOCKUP_DETECTOR) && watchdog_running) {
> -		smpboot_update_cpumask_percpu_thread(&watchdog_threads,
> -						     &watchdog_cpumask);
> -		__lockup_detector_cleanup();
> -	}
> -}
> -
>  static void proc_watchdog_cpumask_update(void)
>  {
>  	/* Remove impossible cpus to keep sysctl output clean. */
>  	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
> -	watchdog_update_cpus();
> +
> +	softlockup_update_threads();
>  	watchdog_nmi_reconfigure();
>  }
>  
> @@ -899,8 +750,6 @@ int proc_watchdog_cpumask(struct ctl_tab
>  
>  void __init lockup_detector_init(void)
>  {
> -	set_sample_period();
> -
>  #ifdef CONFIG_NO_HZ_FULL
>  	if (tick_nohz_full_enabled()) {
>  		pr_info("Disabling watchdog on nohz_full cores by default\n");
> @@ -911,6 +760,5 @@ void __init lockup_detector_init(void)
>  	cpumask_copy(&watchdog_cpumask, cpu_possible_mask);
>  #endif
>  
> -	if (watchdog_enabled)
> -		watchdog_enable_all_cpus();
> +	softlockup_init_threads();
>  }
> 
> 

[toc] | [prev] | [next] | [standalone]

#1725319 — Re: [patch 17/29] lockup_detector: Get rid of the thread teardown/setup dance

On Fri, 1 Sep 2017, Don Zickus wrote:
> On Thu, Aug 31, 2017 at 09:16:15AM +0200, Thomas Gleixner wrote:
> > The lockup detector reconfiguration tears down all watchdog threads when
> > the watchdog is disabled and sets them up again when its enabled.
> > 
> > That's a pointless exercise. The watchdog threads are not consuming an
> > insane amount of resources, so it's enough to set them up at init time and
> > keep them in parked position when the watchdog is disabled and unpark them
> > when it is reenabled. The smpboot thread infrastructure takes care of
> > keeping the force parked threads in place even across cpu hotplug.
> 
> The original reasoning for implementing this complexity was we were worried
> about catching kthread_[un]park errors.  It was thought in the rare case if
> it failed, we would hang.  Perhaps we misunderstood the kthread_[un]park
> code.

park is waiting for the thread to reach park position. So if this fails,
then the thread which invoked the parking will be stuck forever.

the unpark is async, i.e. it just kicks the thread out of park position and
let it run.

So you cannot catch anything with your open coding as it just uses the same
mechanisms as the smpboot infrastructure does.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1723933 — [patch 15/29] smpboot/threads: Avoid runtime allocation

smpboot_update_cpumask_threads_percpu() allocates a temporary cpumask at
runtime. This is suboptimal because the call site needs more code size for
proper error handling than a statically allocated temporary mask requires
data size.

Add static temporary cpumask. The function is globaly serialized, so no
further protection required.

Remove the half baken error handling in the watchdog code and get rid of
the export as there are no in tree modular users of that function.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/smpboot.h |    4 ++--
 kernel/smpboot.c        |   22 +++++++---------------
 kernel/watchdog.c       |   21 +++++----------------
 3 files changed, 14 insertions(+), 33 deletions(-)

--- a/include/linux/smpboot.h
+++ b/include/linux/smpboot.h
@@ -55,7 +55,7 @@ smpboot_register_percpu_thread(struct sm
 }
 
 void smpboot_unregister_percpu_thread(struct smp_hotplug_thread *plug_thread);
-int smpboot_update_cpumask_percpu_thread(struct smp_hotplug_thread *plug_thread,
-					 const struct cpumask *);
+void smpboot_update_cpumask_percpu_thread(struct smp_hotplug_thread *plug_thread,
+					  const struct cpumask *);
 
 #endif
--- a/kernel/smpboot.c
+++ b/kernel/smpboot.c
@@ -344,39 +344,31 @@ EXPORT_SYMBOL_GPL(smpboot_unregister_per
  * by the client, but only by calling this function.
  * This function can only be called on a registered smp_hotplug_thread.
  */
-int smpboot_update_cpumask_percpu_thread(struct smp_hotplug_thread *plug_thread,
-					 const struct cpumask *new)
+void smpboot_update_cpumask_percpu_thread(struct smp_hotplug_thread *plug_thread,
+					  const struct cpumask *new)
 {
 	struct cpumask *old = plug_thread->cpumask;
-	cpumask_var_t tmp;
+	static struct cpumask tmp;
 	unsigned int cpu;
 
-	if (!alloc_cpumask_var(&tmp, GFP_KERNEL))
-		return -ENOMEM;
-
 	get_online_cpus();
 	mutex_lock(&smpboot_threads_lock);
 
 	/* Park threads that were exclusively enabled on the old mask. */
-	cpumask_andnot(tmp, old, new);
-	for_each_cpu_and(cpu, tmp, cpu_online_mask)
+	cpumask_andnot(&tmp, old, new);
+	for_each_cpu_and(cpu, &tmp, cpu_online_mask)
 		smpboot_park_thread(plug_thread, cpu);
 
 	/* Unpark threads that are exclusively enabled on the new mask. */
-	cpumask_andnot(tmp, new, old);
-	for_each_cpu_and(cpu, tmp, cpu_online_mask)
+	cpumask_andnot(&tmp, new, old);
+	for_each_cpu_and(cpu, &tmp, cpu_online_mask)
 		smpboot_unpark_thread(plug_thread, cpu);
 
 	cpumask_copy(old, new);
 
 	mutex_unlock(&smpboot_threads_lock);
 	put_online_cpus();
-
-	free_cpumask_var(tmp);
-
-	return 0;
 }
-EXPORT_SYMBOL_GPL(smpboot_update_cpumask_percpu_thread);
 
 static DEFINE_PER_CPU(atomic_t, cpu_hotplug_state) = ATOMIC_INIT(CPU_POST_DEAD);
 
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -781,31 +781,20 @@ int proc_watchdog_thresh(struct ctl_tabl
 	return err;
 }
 
-static int watchdog_update_cpus(void)
+static void watchdog_update_cpus(void)
 {
-	if (IS_ENABLED(CONFIG_SOFTLOCKUP_DETECTOR)) {
-		return smpboot_update_cpumask_percpu_thread(&watchdog_threads,
-							    &watchdog_cpumask);
+	if (IS_ENABLED(CONFIG_SOFTLOCKUP_DETECTOR) && watchdog_running) {
+		smpboot_update_cpumask_percpu_thread(&watchdog_threads,
+						     &watchdog_cpumask);
 		__lockup_detector_cleanup();
 	}
-	return 0;
 }
 
 static void proc_watchdog_cpumask_update(void)
 {
 	/* Remove impossible cpus to keep sysctl output clean. */
 	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
-
-	if (watchdog_running) {
-		/*
-		 * Failure would be due to being unable to allocate a
-		 * temporary cpumask, so we are likely not in a position to
-		 * do much else to make things better.
-		 */
-		if (watchdog_update_cpus() != 0)
-			pr_err("cpumask update failed\n");
-	}
-
+	watchdog_update_cpus();
 	watchdog_nmi_reconfigure();
 }
 

[toc] | [prev] | [next] | [standalone]

#1723934 — [patch 07/29] lockup_detector: Rename watchdog_proc_mutex

Following patches will use the mutex for other purposes as well. Rename it
as it is not longer a proc specific thing.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/watchdog.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -29,8 +29,7 @@
 #include <linux/kvm_para.h>
 #include <linux/kthread.h>
 
-/* Watchdog configuration */
-static DEFINE_MUTEX(watchdog_proc_mutex);
+static DEFINE_MUTEX(watchdog_mutex);
 
 int __read_mostly nmi_watchdog_enabled;
 
@@ -704,7 +703,7 @@ static int proc_watchdog_common(int whic
 	int *watchdog_param = (int *)table->data;
 
 	cpu_hotplug_disable();
-	mutex_lock(&watchdog_proc_mutex);
+	mutex_lock(&watchdog_mutex);
 
 	/*
 	 * If the parameter is being read return the state of the corresponding
@@ -751,7 +750,7 @@ static int proc_watchdog_common(int whic
 		err = proc_watchdog_update();
 	}
 out:
-	mutex_unlock(&watchdog_proc_mutex);
+	mutex_unlock(&watchdog_mutex);
 	cpu_hotplug_enable();
 	return err;
 }
@@ -795,7 +794,7 @@ int proc_watchdog_thresh(struct ctl_tabl
 	int err, old, new;
 
 	cpu_hotplug_disable();
-	mutex_lock(&watchdog_proc_mutex);
+	mutex_lock(&watchdog_mutex);
 
 	old = ACCESS_ONCE(watchdog_thresh);
 	err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
@@ -817,7 +816,7 @@ int proc_watchdog_thresh(struct ctl_tabl
 		set_sample_period();
 	}
 out:
-	mutex_unlock(&watchdog_proc_mutex);
+	mutex_unlock(&watchdog_mutex);
 	cpu_hotplug_enable();
 	return err;
 }
@@ -834,7 +833,7 @@ int proc_watchdog_cpumask(struct ctl_tab
 	int err;
 
 	cpu_hotplug_disable();
-	mutex_lock(&watchdog_proc_mutex);
+	mutex_lock(&watchdog_mutex);
 
 	err = proc_do_large_bitmap(table, write, buffer, lenp, ppos);
 	if (!err && write) {
@@ -855,7 +854,7 @@ int proc_watchdog_cpumask(struct ctl_tab
 		watchdog_nmi_reconfigure();
 	}
 
-	mutex_unlock(&watchdog_proc_mutex);
+	mutex_unlock(&watchdog_mutex);
 	cpu_hotplug_enable();
 	return err;
 }

[toc] | [prev] | [next] | [standalone]

#1723935 — [patch 16/29] lockup_detector: Create new thread handling infrastructure

The lockup detector reconfiguration tears down all watchdog threads when
the watchdog is disabled and sets them up again when its enabled.

That's a pointless exercise. The watchdog threads are not consuming an
insane amount of resources, so it's enough to set them up at init time and
keep them in parked position when the watchdog is disabled and unpark them
when it is reenabled. The smpboot thread infrastructure takes care of
keeping the force parked threads in place even across cpu hotplug.

Another horrible mechanism are the open coded park/unpark loops which are
used for reconfiguration of the watchdog. The smpboot infrastructure allows
exactly the same via smpboot_update_cpumask_thread_percpu(), which is cpu
hotplug safe. Using that instead of the open coded loops allows to get rid
of the hotplug locking mess in the watchdog code.

Implement a clean infrastructure which allows to replace the open coded
nonsense.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/watchdog.c |   75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -132,6 +132,9 @@ unsigned int __read_mostly softlockup_pa
 			CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE;
 int __read_mostly soft_watchdog_enabled;
 
+struct cpumask watchdog_allowed_mask __read_mostly;
+static bool softlockup_threads_initialized __read_mostly;
+
 static u64 __read_mostly sample_period;
 
 static DEFINE_PER_CPU(unsigned long, watchdog_touch_ts);
@@ -578,12 +581,84 @@ static void watchdog_disable_all_cpus(vo
 	}
 }
 
+static void softlockup_update_smpboot_threads(void)
+{
+	lockdep_assert_held(&watchdog_mutex);
+
+	if (!softlockup_threads_initialized)
+		return;
+
+	smpboot_update_cpumask_percpu_thread(&watchdog_threads,
+					     &watchdog_allowed_mask);
+	__lockup_detector_cleanup();
+}
+
+/* Temporarily park all watchdog threads */
+static void softlockup_park_all_threads(void)
+{
+	cpumask_clear(&watchdog_allowed_mask);
+	softlockup_update_smpboot_threads();
+}
+
+/*
+ * Park threads which are not longer enabled and unpark threads which have
+ * been newly enabled.
+ */
+static void softlockup_update_threads(void)
+{
+	cpumask_copy(&watchdog_allowed_mask, &watchdog_cpumask);
+	softlockup_update_smpboot_threads();
+}
+
+static void softlockup_reconfigure_threads(bool enabled)
+{
+	softlockup_park_all_threads();
+	set_sample_period();
+	if (enabled)
+		softlockup_update_threads();
+}
+
+/*
+ * Create the watchdog thread infrastructure.
+ *
+ * The threads are not unparked as watchdog_allowed_mask is empty.  When
+ * the threads are sucessfully initialized, take the proper locks and
+ * unpark the threads in the watchdog_cpumask if the watchdog is enabled.
+ */
+static __init void softlockup_init_threads(void)
+{
+	int ret;
+
+	/*
+	 * If sysctl is off and watchdog got disabled on the command line,
+	 * nothing to do here.
+	 */
+	if (!IS_ENABLED(CONFIG_SYSCTL) &&
+	    !(watchdog_enabled && watchdog_thresh))
+		return;
+
+	ret = smpboot_register_percpu_thread_cpumask(&watchdog_threads,
+						     &watchdog_allowed_mask);
+	if (ret) {
+		pr_err("Failed to initialize soft lockup detector threads\n");
+		return;
+	}
+
+	mutex_lock(&watchdog_mutex);
+	softlockup_threads_initialized = true;
+	softlockup_reconfigure_threads(watchdog_enabled && watchdog_thresh);
+	mutex_unlock(&watchdog_mutex);
+}
+
 #else /* CONFIG_SOFTLOCKUP_DETECTOR */
 static inline int watchdog_park_threads(void) { return 0; }
 static inline void watchdog_unpark_threads(void) { }
 static inline int watchdog_enable_all_cpus(void) { return 0; }
 static inline void watchdog_disable_all_cpus(void) { }
 static inline void set_sample_period(void) { }
+static inline void softlockup_init_threads(void) { }
+static inline void softlockup_update_threads(void) { }
+static inline void softlockup_reconfigure_threads(bool enabled) { }
 #endif /* !CONFIG_SOFTLOCKUP_DETECTOR */
 
 static void __lockup_detector_cleanup(void)

[toc] | [prev] | [next] | [standalone]

#1723936 — [patch 22/29] lockup_detector: Make watchdog_nmi_reconfigure() two stage

Both the perf reconfiguration and the powerpc watchdog_nmi_reconfigure()
need to be done in two steps.

     1) Stop all NMIs
     2) Read the new parameters and start NMIs

Right now watchdog_nmi_reconfigure() is a combination of both. To allow a
clean reconfiguration add a 'run' argument and split the functionality in
powerpc.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: linuxppc-dev@lists.ozlabs.org
---
 arch/powerpc/kernel/watchdog.c |   17 +++++++++--------
 include/linux/nmi.h            |    2 ++
 kernel/watchdog.c              |   31 ++++++++++++++++++++++---------
 3 files changed, 33 insertions(+), 17 deletions(-)

--- a/arch/powerpc/kernel/watchdog.c
+++ b/arch/powerpc/kernel/watchdog.c
@@ -352,17 +352,18 @@ static void watchdog_calc_timeouts(void)
 	wd_timer_period_ms = watchdog_thresh * 1000 * 2 / 5;
 }
 
-void watchdog_nmi_reconfigure(void)
+void watchdog_nmi_reconfigure(bool stop)
 {
 	int cpu;
 
-	watchdog_calc_timeouts();
-
-	for_each_cpu(cpu, &wd_cpus_enabled)
-		stop_wd_on_cpu(cpu);
-
-	for_each_cpu_and(cpu, cpu_online_mask, &watchdog_cpumask)
-		start_wd_on_cpu(cpu);
+	if (stop) {
+		for_each_cpu(cpu, &wd_cpus_enabled)
+			stop_wd_on_cpu(cpu);
+	} else {
+		watchdog_calc_timeouts();
+		for_each_cpu_and(cpu, cpu_online_mask, &watchdog_cpumask)
+			start_wd_on_cpu(cpu);
+	}
 }
 
 /*
--- a/include/linux/nmi.h
+++ b/include/linux/nmi.h
@@ -103,6 +103,8 @@ static inline void arch_touch_nmi_watchd
 #endif
 #endif
 
+void watchdog_nmi_reconfigure(bool run);
+
 /**
  * touch_nmi_watchdog - restart NMI watchdog timeout.
  *
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -105,17 +105,25 @@ static int __init hardlockup_all_cpu_bac
 int __weak watchdog_nmi_enable(unsigned int cpu) { return 0; }
 void __weak watchdog_nmi_disable(unsigned int cpu) { }
 
-/*
- * watchdog_nmi_reconfigure can be implemented to be notified after any
- * watchdog configuration change. The arch hardlockup watchdog should
- * respond to the following variables:
+/**
+ * watchdog_nmi_reconfigure - Optional function to reconfigure NMI watchdogs
+ * @stop:	If true stop the watchdogs on all enabled CPUs
+ *		If false start the watchdogs on all enabled CPUs
+ *
+ * The core call order is:
+ * watchdog_nmi_reconfigure(true);
+ * update_variables();
+ * watchdog_nmi_reconfigure(false);
+ *
+ * The second call which starts the watchdogs again guarantees that the
+ * following variables are stable across the call.
  * - watchdog_enabled
  * - watchdog_thresh
  * - watchdog_cpumask
- * - sysctl_hardlockup_all_cpu_backtrace
- * - hardlockup_panic
+ *
+ * After the call the variables can be changed again.
  */
-void __weak watchdog_nmi_reconfigure(void) { }
+void __weak watchdog_nmi_reconfigure(bool stop) { }
 
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
 
@@ -509,10 +517,12 @@ static void softlockup_unpark_threads(vo
 
 static void softlockup_reconfigure_threads(bool enabled)
 {
+	watchdog_nmi_reconfigure(false);
 	softlockup_park_all_threads();
 	set_sample_period();
 	if (enabled)
 		softlockup_unpark_threads();
+	watchdog_nmi_reconfigure(true);
 }
 
 /*
@@ -553,7 +563,11 @@ static inline void watchdog_unpark_threa
 static inline int watchdog_enable_all_cpus(void) { return 0; }
 static inline void watchdog_disable_all_cpus(void) { }
 static inline void softlockup_init_threads(void) { }
-static inline void softlockup_reconfigure_threads(bool enabled) { }
+static void softlockup_reconfigure_threads(bool enabled)
+{
+	watchdog_nmi_reconfigure(false);
+	watchdog_nmi_reconfigure(true);
+}
 #endif /* !CONFIG_SOFTLOCKUP_DETECTOR */
 
 static void __lockup_detector_cleanup(void)
@@ -593,7 +607,6 @@ static void proc_watchdog_update(void)
 	/* Remove impossible cpus to keep sysctl output clean. */
 	cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
 	softlockup_reconfigure_threads(watchdog_enabled && watchdog_thresh);
-	watchdog_nmi_reconfigure();
 }
 
 /*

[toc] | [prev] | [next] | [standalone]

#1723937 — [patch 01/29] hardlockup_detector: Provide interface to stop/restart perf events

From: Peter Zijlstra <peterz@infradead.org>

Provide a interface to stop and restart perf NMI watchdog events on all
CPUs. This is only useable during init and especially for handling the perf
HT bug on Intel machines. It's safe to use it this way as nothing can
start/stop the NMI watchdog in parallel.

Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/nmi.h   |    4 ++++
 kernel/watchdog_hld.c |   41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

--- a/include/linux/nmi.h
+++ b/include/linux/nmi.h
@@ -75,7 +75,11 @@ static inline void hardlockup_detector_d
 
 #if defined(CONFIG_HARDLOCKUP_DETECTOR_PERF)
 extern void arch_touch_nmi_watchdog(void);
+extern void hardlockup_detector_perf_stop(void);
+extern void hardlockup_detector_perf_restart(void);
 #else
+static inline void hardlockup_detector_perf_stop(void) { }
+static inline void hardlockup_detector_perf_restart(void) { }
 #if !defined(CONFIG_HAVE_NMI_WATCHDOG)
 static inline void arch_touch_nmi_watchdog(void) {}
 #endif
--- a/kernel/watchdog_hld.c
+++ b/kernel/watchdog_hld.c
@@ -261,3 +261,44 @@ void watchdog_nmi_disable(unsigned int c
 			firstcpu_err = 0;
 	}
 }
+
+/**
+ * hardlockup_detector_perf_stop - Globally stop watchdog events
+ *
+ * Special interface for x86 to handle the perf HT bug.
+ */
+void __init hardlockup_detector_perf_stop(void)
+{
+	int cpu;
+
+	lockdep_assert_cpus_held();
+
+	for_each_online_cpu(cpu) {
+		struct perf_event *event = per_cpu(watchdog_ev, cpu);
+
+		if (event)
+			perf_event_disable(event);
+	}
+}
+
+/**
+ * hardlockup_detector_perf_restart - Globally restart watchdog events
+ *
+ * Special interface for x86 to handle the perf HT bug.
+ */
+void __init hardlockup_detector_perf_restart(void)
+{
+	int cpu;
+
+	lockdep_assert_cpus_held();
+
+	if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
+		return;
+
+	for_each_online_cpu(cpu) {
+		struct perf_event *event = per_cpu(watchdog_ev, cpu);
+
+		if (event)
+			perf_event_enable(event);
+	}
+}

[toc] | [prev] | [next] | [standalone]

#1727636 — Re: [patch 01/29] hardlockup_detector: Provide interface to stop/restart perf events

On Thu, Aug 31, 2017 at 09:15:59AM +0200, Thomas Gleixner wrote:
> From: Peter Zijlstra <peterz@infradead.org>
> 
> Provide a interface to stop and restart perf NMI watchdog events on all

"an interface"

> CPUs. This is only useable during init and especially for handling the perf

"usable"

> HT bug on Intel machines. It's safe to use it this way as nothing can
> start/stop the NMI watchdog in parallel.
> 
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  include/linux/nmi.h   |    4 ++++
>  kernel/watchdog_hld.c |   41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> --- a/include/linux/nmi.h
> +++ b/include/linux/nmi.h
> @@ -75,7 +75,11 @@ static inline void hardlockup_detector_d
>  
>  #if defined(CONFIG_HARDLOCKUP_DETECTOR_PERF)
>  extern void arch_touch_nmi_watchdog(void);
> +extern void hardlockup_detector_perf_stop(void);
> +extern void hardlockup_detector_perf_restart(void);
>  #else
> +static inline void hardlockup_detector_perf_stop(void) { }
> +static inline void hardlockup_detector_perf_restart(void) { }
>  #if !defined(CONFIG_HAVE_NMI_WATCHDOG)
>  static inline void arch_touch_nmi_watchdog(void) {}
>  #endif
> --- a/kernel/watchdog_hld.c
> +++ b/kernel/watchdog_hld.c
> @@ -261,3 +261,44 @@ void watchdog_nmi_disable(unsigned int c
>  			firstcpu_err = 0;
>  	}
>  }
> +
> +/**
> + * hardlockup_detector_perf_stop - Globally stop watchdog events
> + *
> + * Special interface for x86 to handle the perf HT bug.
> + */
> +void __init hardlockup_detector_perf_stop(void)

Right, can we make those naming prefixes shorter? Something like
hld_perf_stop() and hld_perf_restart()?

File has already "hld" in the name so ...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1723939 — [patch 05/29] lockup_detector: Remove broken suspend/resume interfaces

This interface has several issues:

 - It's causing recursive locking of the hotplug lock.

 - It's complete overkill to teardown all threads and then recreate them

The same can be achieved with the simple hardlockup_detector_perf_stop /
restart() interfaces. The abuse from the busy looping poweroff() loop of
PARISC has been solved as well.

Remove the cruft.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/powerpc/kernel/watchdog.c |    3 -
 include/linux/nmi.h            |   12 -----
 kernel/watchdog.c              |   89 -----------------------------------------
 3 files changed, 1 insertion(+), 103 deletions(-)

--- a/arch/powerpc/kernel/watchdog.c
+++ b/arch/powerpc/kernel/watchdog.c
@@ -307,9 +307,6 @@ static int start_wd_on_cpu(unsigned int
 	if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
 		return 0;
 
-	if (watchdog_suspended)
-		return 0;
-
 	if (!cpumask_test_cpu(cpu, &watchdog_cpumask))
 		return 0;
 
--- a/include/linux/nmi.h
+++ b/include/linux/nmi.h
@@ -164,7 +164,6 @@ extern int watchdog_thresh;
 extern unsigned long watchdog_enabled;
 extern struct cpumask watchdog_cpumask;
 extern unsigned long *watchdog_cpumask_bits;
-extern int __read_mostly watchdog_suspended;
 #ifdef CONFIG_SMP
 extern int sysctl_softlockup_all_cpu_backtrace;
 extern int sysctl_hardlockup_all_cpu_backtrace;
@@ -192,17 +191,6 @@ extern int proc_watchdog_thresh(struct c
 				void __user *, size_t *, loff_t *);
 extern int proc_watchdog_cpumask(struct ctl_table *, int,
 				 void __user *, size_t *, loff_t *);
-extern int lockup_detector_suspend(void);
-extern void lockup_detector_resume(void);
-#else
-static inline int lockup_detector_suspend(void)
-{
-	return 0;
-}
-
-static inline void lockup_detector_resume(void)
-{
-}
 #endif
 
 #ifdef CONFIG_HAVE_ACPI_APEI_NMI
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -97,19 +97,6 @@ unsigned long *watchdog_cpumask_bits = c
  * unregistered/stopped, so it is an indicator whether the threads exist.
  */
 static int __read_mostly watchdog_running;
-/*
- * If a subsystem has a need to deactivate the watchdog temporarily, it
- * can use the suspend/resume interface to achieve this. The content of
- * the 'watchdog_suspended' variable reflects this state. Existing threads
- * are parked/unparked by the lockup_detector_{suspend|resume} functions
- * (see comment blocks pertaining to those functions for further details).
- *
- * 'watchdog_suspended' also prevents threads from being registered/started
- * or unregistered/stopped via parameters in /proc/sys/kernel, so the state
- * of 'watchdog_running' cannot change while the watchdog is deactivated
- * temporarily (see related code in 'proc' handlers).
- */
-int __read_mostly watchdog_suspended;
 
 /*
  * These functions can be overridden if an architecture implements its
@@ -136,7 +123,6 @@ void __weak watchdog_nmi_disable(unsigne
  * - watchdog_cpumask
  * - sysctl_hardlockup_all_cpu_backtrace
  * - hardlockup_panic
- * - watchdog_suspended
  */
 void __weak watchdog_nmi_reconfigure(void)
 {
@@ -672,61 +658,6 @@ void lockup_detector_soft_poweroff(void)
 	watchdog_enabled = 0;
 }
 
-/*
- * Suspend the hard and soft lockup detector by parking the watchdog threads.
- */
-int lockup_detector_suspend(void)
-{
-	int ret = 0;
-
-	get_online_cpus();
-	mutex_lock(&watchdog_proc_mutex);
-	/*
-	 * Multiple suspend requests can be active in parallel (counted by
-	 * the 'watchdog_suspended' variable). If the watchdog threads are
-	 * running, the first caller takes care that they will be parked.
-	 * The state of 'watchdog_running' cannot change while a suspend
-	 * request is active (see related code in 'proc' handlers).
-	 */
-	if (watchdog_running && !watchdog_suspended)
-		ret = watchdog_park_threads();
-
-	if (ret == 0)
-		watchdog_suspended++;
-	else {
-		watchdog_disable_all_cpus();
-		pr_err("Failed to suspend lockup detectors, disabled\n");
-		watchdog_enabled = 0;
-	}
-
-	watchdog_nmi_reconfigure();
-
-	mutex_unlock(&watchdog_proc_mutex);
-
-	return ret;
-}
-
-/*
- * Resume the hard and soft lockup detector by unparking the watchdog threads.
- */
-void lockup_detector_resume(void)
-{
-	mutex_lock(&watchdog_proc_mutex);
-
-	watchdog_suspended--;
-	/*
-	 * The watchdog threads are unparked if they were previously running
-	 * and if there is no more active suspend request.
-	 */
-	if (watchdog_running && !watchdog_suspended)
-		watchdog_unpark_threads();
-
-	watchdog_nmi_reconfigure();
-
-	mutex_unlock(&watchdog_proc_mutex);
-	put_online_cpus();
-}
-
 #ifdef CONFIG_SYSCTL
 
 /*
@@ -775,12 +706,6 @@ static int proc_watchdog_common(int whic
 	get_online_cpus();
 	mutex_lock(&watchdog_proc_mutex);
 
-	if (watchdog_suspended) {
-		/* no parameter changes allowed while watchdog is suspended */
-		err = -EAGAIN;
-		goto out;
-	}
-
 	/*
 	 * If the parameter is being read return the state of the corresponding
 	 * bit(s) in 'watchdog_enabled', else update 'watchdog_enabled' and the
@@ -872,12 +797,6 @@ int proc_watchdog_thresh(struct ctl_tabl
 	get_online_cpus();
 	mutex_lock(&watchdog_proc_mutex);
 
-	if (watchdog_suspended) {
-		/* no parameter changes allowed while watchdog is suspended */
-		err = -EAGAIN;
-		goto out;
-	}
-
 	old = ACCESS_ONCE(watchdog_thresh);
 	err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
 
@@ -917,12 +836,6 @@ int proc_watchdog_cpumask(struct ctl_tab
 	get_online_cpus();
 	mutex_lock(&watchdog_proc_mutex);
 
-	if (watchdog_suspended) {
-		/* no parameter changes allowed while watchdog is suspended */
-		err = -EAGAIN;
-		goto out;
-	}
-
 	err = proc_do_large_bitmap(table, write, buffer, lenp, ppos);
 	if (!err && write) {
 		/* Remove impossible cpus to keep sysctl output cleaner. */
@@ -941,7 +854,7 @@ int proc_watchdog_cpumask(struct ctl_tab
 
 		watchdog_nmi_reconfigure();
 	}
-out:
+
 	mutex_unlock(&watchdog_proc_mutex);
 	put_online_cpus();
 	return err;

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web