Re: suspend regression in 4.1-rc1

[Peter Zijlstra <peterz@infradead.org>]

On Sun, May 17, 2015 at 09:33:56PM -0700, Linus Torvalds wrote:
> On Sun, May 17, 2015 at 11:50 AM, Michal Hocko <mhocko@suse.cz> wrote:
> >
> > The merge commit is empty and both 80dcc31fbe55 and e4b0db72be24 work
> > properly but the merge is bad. So it seems like some of the commits in
> > either branch has a side effect which needs other branch in order to
> > reproduce.
> >
> > So've tried to bisect ^80dcc31fbe55 e4b0db72be24 and merged 80dcc31fbe55
> > in each step.
> 
> Good extra work! Thanks.
> 
> > This lead to:
> >
> > commit 195daf665a6299de98a4da3843fed2dd9de19d3a
> > Author: Ulrich Obergfell <uobergfe@redhat.com>
> > Date:   Tue Apr 14 15:44:13 2015 -0700
> >
> >     watchdog: enable the new user interface of the watchdog mechanism
> >
> > The patch doesn't revert because of follow up changes so I have reverted
> > all three:
> > 692297d8f968 ("watchdog: introduce the hardlockup_detector_disable() function")
> > b2f57c3a0df9 ("watchdog: clean up some function names and arguments")
> > 195daf665a62 ("watchdog: enable the new user interface of the watchdog mechanism")
> 
> Hmm. I guess we should just revert those three then. Unless somebody
> can see what the subtle interaction is.
> 
> Actually, looking closer, on the *other* side of the merge, the only
> commit that looks like it might be conflicting is
> 
>     b3738d293233 "watchdog: Add watchdog enable/disable all functions"
> 
> which is then used by
> 
>     b37609c30e41 "perf/x86/intel: Make the HT bug workaround
> conditional on HT enabled"
> 
> Does the problem go away if you revert *those* two commits instead?
> 
> At least that would tell is what the exact bad interaction is.
> 
> Adding Stephane (author of those watchdog/perf patches) to the Cc. And
> PeterZ, who signed them off (Ingo also did, but was already on the
> participants list).
> 
> Anybody see it?

The 'obvious' discrepancy is that 195daf665a62 ("watchdog: enable the
new user interface of the watchdog mechanism") changes the semantics of
watchdog_user_enabled, which thereafter is only used by the functions
introduced by b3738d293233 ("watchdog: Add watchdog enable/disable all
functions").

There further appears to be a distinct lack of serialization between
setting and using watchdog_enabled, so perhaps we should wrap the
{en,dis}able_all() things in watchdog_proc_mutex.

Let me go see if I can reproduce / test this.. as is the below is
entirely untested.

---
 kernel/watchdog.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 2316f50b07a4..56aeedb087e3 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -608,19 +608,25 @@ void watchdog_nmi_enable_all(void)
 {
 	int cpu;
 
-	if (!watchdog_user_enabled)
+	mutex_lock(&watchdog_proc_mutex);
+
+	if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
 		return;
 
 	get_online_cpus();
 	for_each_online_cpu(cpu)
 		watchdog_nmi_enable(cpu);
 	put_online_cpus();
+
+	mutex_unlock(&watchdog_proc_mutex);
 }
 
 void watchdog_nmi_disable_all(void)
 {
 	int cpu;
 
+	mutex_lock(&watchdog_proc_mutex);
+
 	if (!watchdog_running)
 		return;
 
@@ -628,6 +634,8 @@ void watchdog_nmi_disable_all(void)
 	for_each_online_cpu(cpu)
 		watchdog_nmi_disable(cpu);
 	put_online_cpus();
+
+	mutex_unlock(&watchdog_proc_mutex);
 }
 #else
 static int watchdog_nmi_enable(unsigned int cpu) { return 0; }