suspend regression in 4.1-rc1

suspend regression in 4.1-rc1

[Michal Hocko]

Hi,
s2ram broke after 4.1-rc1 for me. The second s2ram simply doesn't wake
up (fans turn on but the screen is off). I have even noticed fans
starting also while suspended in some instances (which was especially
annoying when it happened on the way home from work).
I've tried /sys/power/pm_test and the issue starts at processors mode.
Nothing really interesting shows up in the netconsole but I didn't get
to a more detailed testing there.

I've tried to bisect this as 4.0 works reliably. This was tricky though
because the first bad commit is a merge:

commit 1dcf58d6e6e6eb7ec10e9abc56887b040205b06f
Merge: 80dcc31fbe55 e4b0db72be24
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue Apr 14 16:49:17 2015 -0700

    Merge branch 'akpm' (patches from Andrew)

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

on top of my current Linus tree (4cfceaf0c087f47033f5e61a801f4136d6fb68c6)
and the issue is gone. I have hard time to understand what these 3 could have
to do with suspend path, though.

Then I've tried to bisect the other branch and merge 195daf665a62 during
each step to find out which patch starts failing. This lead to an even
weirder commit a1e12da4796a ("perf tools: Add 'I' event modifier for
exclude_idle bit") but maybe I've just screwed something on the way.

I will continue debugging tomorrow but any hints would be helpful.
-- 
Michal Hocko
SUSE Labs

Re: suspend regression in 4.1-rc1

[Sergey Senozhatsky]

On (05/17/15 20:50), Michal Hocko wrote:
> Hi,
> s2ram broke after 4.1-rc1 for me. The second s2ram simply doesn't wake
> up (fans turn on but the screen is off). I have even noticed fans
> starting also while suspended in some instances (which was especially
> annoying when it happened on the way home from work).
> I've tried /sys/power/pm_test and the issue starts at processors mode.
> Nothing really interesting shows up in the netconsole but I didn't get
> to a more detailed testing there.
> 
> I've tried to bisect this as 4.0 works reliably. This was tricky though
> because the first bad commit is a merge:
> 

Hello,

JFI, I see quite similar behaviour on my laptop (linux-next doesn't boot:
blank screen and fans on). I've tried to bisect yesterday, but it didn't
go well, showing that the first bad commit is '31ccd0e66d41' (nonsense);
it seems that the root cause is somewhere between next-20150505 (ok) and
next-20150506 (boot failure). I'll continue bisecting.

	-ss

> commit 1dcf58d6e6e6eb7ec10e9abc56887b040205b06f
> Merge: 80dcc31fbe55 e4b0db72be24
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Tue Apr 14 16:49:17 2015 -0700
> 
>     Merge branch 'akpm' (patches from Andrew)
> 
> 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. 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")
> 
> on top of my current Linus tree (4cfceaf0c087f47033f5e61a801f4136d6fb68c6)
> and the issue is gone. I have hard time to understand what these 3 could have
> to do with suspend path, though.
> 
> Then I've tried to bisect the other branch and merge 195daf665a62 during
> each step to find out which patch starts failing. This lead to an even
> weirder commit a1e12da4796a ("perf tools: Add 'I' event modifier for
> exclude_idle bit") but maybe I've just screwed something on the way.
> 
> I will continue debugging tomorrow but any hints would be helpful.
> -- 
> Michal Hocko
> SUSE Labs
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: suspend regression in 4.1-rc1

[Linus Torvalds]

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?

               Linus

Re: suspend regression in 4.1-rc1

[Omar Sandoval]

On Sun, May 17, 2015 at 08:50:41PM +0200, Michal Hocko wrote:
> Hi,
> s2ram broke after 4.1-rc1 for me. The second s2ram simply doesn't wake
> up (fans turn on but the screen is off). I have even noticed fans
> starting also while suspended in some instances (which was especially
> annoying when it happened on the way home from work).
> I've tried /sys/power/pm_test and the issue starts at processors mode.
> Nothing really interesting shows up in the netconsole but I didn't get
> to a more detailed testing there.
> 
> I've tried to bisect this as 4.0 works reliably. This was tricky though
> because the first bad commit is a merge:
> 
> commit 1dcf58d6e6e6eb7ec10e9abc56887b040205b06f
> Merge: 80dcc31fbe55 e4b0db72be24
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Tue Apr 14 16:49:17 2015 -0700
> 
>     Merge branch 'akpm' (patches from Andrew)
> 
> 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. 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")
> 
> on top of my current Linus tree (4cfceaf0c087f47033f5e61a801f4136d6fb68c6)
> and the issue is gone. I have hard time to understand what these 3 could have
> to do with suspend path, though.
> 
> Then I've tried to bisect the other branch and merge 195daf665a62 during
> each step to find out which patch starts failing. This lead to an even
> weirder commit a1e12da4796a ("perf tools: Add 'I' event modifier for
> exclude_idle bit") but maybe I've just screwed something on the way.
> 
> I will continue debugging tomorrow but any hints would be helpful.
> -- 
> Michal Hocko
> SUSE Labs

The symptoms here are different, and the bisect indicates that it's
completely unrelated, but just for kicks you could also try:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=533445c6e53368569e50ab3fb712230c03d523f3
-- 
Omar

Re: suspend regression in 4.1-rc1

[Peter Zijlstra]

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; }

Re: suspend regression in 4.1-rc1

[Peter Zijlstra]

On Mon, May 18, 2015 at 09:30:46AM +0200, Peter Zijlstra wrote:
> Let me go see if I can reproduce / test this.. as is the below is
> entirely untested.

I cannot seem to get suspend to fail on my x240 with linus.git.

echo mem > /sys/power/state

makes it sleep, and pressing the power button brings it back to life.

And this is with X loaded and everything up (well, except the wireless,
as it turns out I need a newer firmware version).

So I'll have to ask someone else to test this :/

Re: suspend regression in 4.1-rc1

[Michal Hocko]

On Mon 18-05-15 09:30:46, Peter Zijlstra wrote:
> 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").

Yeah, this is it! b3738d293233 was definitely in the range I was testing
when merging 195daf665 into e95e7f627062..80dcc31fbe55. I must have
screwed something.

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

This doesn't hang anymore. I've just had to move the mutex definition
up to make it compile. So feel free to add my
Reported-and-tested-by: Michal Hocko <mhocko@suse.cz>

Thanks!

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 56aeedb087e3..c398596c35b8 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -604,6 +604,8 @@ static void watchdog_nmi_disable(unsigned int cpu)
 	}
 }
 
+static DEFINE_MUTEX(watchdog_proc_mutex);
+
 void watchdog_nmi_enable_all(void)
 {
 	int cpu;
@@ -752,8 +754,6 @@ static int proc_watchdog_update(void)
 
 }
 
-static DEFINE_MUTEX(watchdog_proc_mutex);
-
 /*
  * common function for watchdog, nmi_watchdog and soft_watchdog parameter
  *

> 
> ---
>  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; }

-- 
Michal Hocko
SUSE Labs

Re: suspend regression in 4.1-rc1

[Peter Zijlstra]

On Mon, May 18, 2015 at 11:03:37AM +0200, Michal Hocko wrote:
> This doesn't hang anymore. I've just had to move the mutex definition
> up to make it compile. So feel free to add my

I've also fixed a lock leak, see goto unlock :-)

> Reported-and-tested-by: Michal Hocko <mhocko@suse.cz>

*blink* that actually fixed it..

That somewhat leaves me at a loss explaining how s2r was failing.

---
Subject: watchdog: Fix merge 'conflict'

Two watchdog changes that came through different trees had a non
conflicting conflict, that is, one changed the semantics of a variable
but no actual code conflict happened. So the merge appeared fine, but
the resulting code did not behave as expected.

Commit 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.

This patch fixes a s2r failure reported by Michal; which I cannot
readily explain. But this does make the code internally consistent
again.

Reported-and-tested-by: Michal Hocko <mhocko@suse.cz>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/watchdog.c |   20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 2316f50..506edcc5 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -41,6 +41,8 @@
 #define NMI_WATCHDOG_ENABLED      (1 << NMI_WATCHDOG_ENABLED_BIT)
 #define SOFT_WATCHDOG_ENABLED     (1 << SOFT_WATCHDOG_ENABLED_BIT)
 
+static DEFINE_MUTEX(watchdog_proc_mutex);
+
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
 static unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED|NMI_WATCHDOG_ENABLED;
 #else
@@ -608,26 +610,36 @@ void watchdog_nmi_enable_all(void)
 {
 	int cpu;
 
-	if (!watchdog_user_enabled)
-		return;
+	mutex_lock(&watchdog_proc_mutex);
+
+	if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
+		goto unlock;
 
 	get_online_cpus();
 	for_each_online_cpu(cpu)
 		watchdog_nmi_enable(cpu);
 	put_online_cpus();
+
+unlock:
+	mutex_lock(&watchdog_proc_mutex);
 }
 
 void watchdog_nmi_disable_all(void)
 {
 	int cpu;
 
+	mutex_lock(&watchdog_proc_mutex);
+
 	if (!watchdog_running)
-		return;
+		goto unlock;
 
 	get_online_cpus();
 	for_each_online_cpu(cpu)
 		watchdog_nmi_disable(cpu);
 	put_online_cpus();
+
+unlock:
+	mutex_unlock(&watchdog_proc_mutex);
 }
 #else
 static int watchdog_nmi_enable(unsigned int cpu) { return 0; }
@@ -744,8 +756,6 @@ static int proc_watchdog_update(void)
 
 }
 
-static DEFINE_MUTEX(watchdog_proc_mutex);
-
 /*
  * common function for watchdog, nmi_watchdog and soft_watchdog parameter
  *

Re: suspend regression in 4.1-rc1

[Ulrich Obergfell]

----- Original Message -----
From: "Michal Hocko" <mhocko@suse.cz>
To: "Peter Zijlstra" <peterz@infradead.org>
[...]
> On Mon 18-05-15 09:30:46, Peter Zijlstra wrote:
>> 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").
>
> Yeah, this is it! b3738d293233 was definitely in the range I was testing
> when merging 195daf665 into e95e7f627062..80dcc31fbe55. I must have
> screwed something.
>
>> 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.
>
> This doesn't hang anymore. I've just had to move the mutex definition
> up to make it compile. So feel free to add my
> Reported-and-tested-by: Michal Hocko <mhocko@suse.cz>
>
> Thanks!
>

Michal,

if I understand you correctly, Peter's patch solves the problem for you.
I would like to make you aware of a patch that Don and I posted in April.

  https://lkml.org/lkml/2015/4/22/306

watchdog_nmi_enable_all() should not use 'watchdog_user_enabled' at all.
It should rather check the NMI_WATCHDOG_ENABLED bit in 'watchdog_enabled'.
The patch is also in Andrew Morton's queue.

  http://ozlabs.org/~akpm/mmots/broken-out/watchdog-fix-watchdog_nmi_enable_all.patch

Peter's patch introduces the same change in watchdog_nmi_enable_all(),
plus some synchronization. However, I'm not sure if we actually need the
synchronization. It is my understanding that {en,dis}able_all() are only
called early during kernel startup via initcall 'fixup_ht_bug':

  kernel_init
  {
    kernel_init_freeable
    {
      lockup_detector_init
      {
        watchdog_enable_all_cpus
          smpboot_register_percpu_thread(&watchdog_threads)
      }

      do_basic_setup
        do_initcalls
          do_initcall_level
            do_one_initcall
              fixup_ht_bug // subsys_initcall(fixup_ht_bug)
              {
                watchdog_nmi_disable_all

                watchdog_nmi_enable_all
              }
    }
  }

Peter,

do we really need the synchronization here?


Regards,

Uli


> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index 56aeedb087e3..c398596c35b8 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -604,6 +604,8 @@ static void watchdog_nmi_disable(unsigned int cpu)
>          }
>  }
>
> +static DEFINE_MUTEX(watchdog_proc_mutex);
> +
> void watchdog_nmi_enable_all(void)
> {
>         int cpu;
> @@ -752,8 +754,6 @@ static int proc_watchdog_update(void)
> 
> }
> 
> -static DEFINE_MUTEX(watchdog_proc_mutex);
> -
> /*
>  * common function for watchdog, nmi_watchdog and soft_watchdog parameter
>  *
>
>>
>> ---
>>  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; }
>
> -- 
> Michal Hocko
> SUSE Labs

Re: suspend regression in 4.1-rc1

[Peter Zijlstra]

Trim emails already.. this seems a spreading disease.

On Mon, May 18, 2015 at 06:10:20AM -0400, Ulrich Obergfell wrote:
> Michal,
> 
> if I understand you correctly, Peter's patch solves the problem for you.
> I would like to make you aware of a patch that Don and I posted in April.
> 
>   https://lkml.org/lkml/2015/4/22/306
> 
> watchdog_nmi_enable_all() should not use 'watchdog_user_enabled' at all.
> It should rather check the NMI_WATCHDOG_ENABLED bit in 'watchdog_enabled'.
> The patch is also in Andrew Morton's queue.
> 
>   http://ozlabs.org/~akpm/mmots/broken-out/watchdog-fix-watchdog_nmi_enable_all.patch
> 
> Peter's patch introduces the same change in watchdog_nmi_enable_all(),
> plus some synchronization. However, I'm not sure if we actually need the
> synchronization. It is my understanding that {en,dis}able_all() are only
> called early during kernel startup via initcall 'fixup_ht_bug':
> 
>   kernel_init
>   {
>     kernel_init_freeable
>     {
>       lockup_detector_init
>       {
>         watchdog_enable_all_cpus
>           smpboot_register_percpu_thread(&watchdog_threads)
>       }
> 
>       do_basic_setup
>         do_initcalls
>           do_initcall_level
>             do_one_initcall
>               fixup_ht_bug // subsys_initcall(fixup_ht_bug)
>               {
>                 watchdog_nmi_disable_all
> 
>                 watchdog_nmi_enable_all
>               }
>     }
>   }
> 
> Peter,
> 
> do we really need the synchronization here?

Well, those are the only current usage sites, but the interface is
exposed and should be fully and correctly implemented, otherwise a next
user might stumble upon sudden unexpected behaviour.

But yes, it appears superfluous for this particular usage.

Re: suspend regression in 4.1-rc1

[Ulrich Obergfell]

Peter,

please see my comments in-line.

Regards,

Uli

----- Original Message -----
From: "Peter Zijlstra" <peterz@infradead.org>
To: "Michal Hocko" <mhocko@suse.cz>
[...]
> On Mon, May 18, 2015 at 11:03:37AM +0200, Michal Hocko wrote:
>> This doesn't hang anymore. I've just had to move the mutex definition
>> up to make it compile. So feel free to add my
>
> I've also fixed a lock leak, see goto unlock :-)
>
>> Reported-and-tested-by: Michal Hocko <mhocko@suse.cz>
>
> *blink* that actually fixed it..
>
> That somewhat leaves me at a loss explaining how s2r was failing.
>
> ---
> Subject: watchdog: Fix merge 'conflict'
>
> Two watchdog changes that came through different trees had a non
> conflicting conflict, that is, one changed the semantics of a variable
> but no actual code conflict happened. So the merge appeared fine, but
> the resulting code did not behave as expected.
>
> Commit 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").

Don and I already posted a patch in April to address this:

https://lkml.org/lkml/2015/4/22/306
http://ozlabs.org/~akpm/mmots/broken-out/watchdog-fix-watchdog_nmi_enable_all.patch

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

As I understand it, the {en,dis}able_all() functions are only called early
at kernel startup, so I do not see how they could be racing with watchdog
code that is executed in the context of write() system calls to parameters
in /proc/sys/kernel. Please see also my earlier reply to Michal for further
details: http://marc.info/?l=linux-pm&m=143194387208250&w=2

Do we really need synchronization here?

> This patch fixes a s2r failure reported by Michal; which I cannot
> readily explain. But this does make the code internally consistent
> again.
>
> Reported-and-tested-by: Michal Hocko <mhocko@suse.cz>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/watchdog.c |   20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index 2316f50..506edcc5 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -41,6 +41,8 @@
>  #define NMI_WATCHDOG_ENABLED      (1 << NMI_WATCHDOG_ENABLED_BIT)
>  #define SOFT_WATCHDOG_ENABLED     (1 << SOFT_WATCHDOG_ENABLED_BIT)
> 
> +static DEFINE_MUTEX(watchdog_proc_mutex);
> +
>  #ifdef CONFIG_HARDLOCKUP_DETECTOR
>  static unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED|NMI_WATCHDOG_ENABLED;
>  #else
> @@ -608,26 +610,36 @@ void watchdog_nmi_enable_all(void)
>  {
>          int cpu;
> 
> -        if (!watchdog_user_enabled)
> -                return;
> +        mutex_lock(&watchdog_proc_mutex);
> +
> +        if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
> +                goto unlock;
> 
>          get_online_cpus();
>          for_each_online_cpu(cpu)
>                  watchdog_nmi_enable(cpu);
>          put_online_cpus();
> +
> +unlock:
> +        mutex_lock(&watchdog_proc_mutex);
>  }
> 
>  void watchdog_nmi_disable_all(void)
>  {
>          int cpu;
> 
> +        mutex_lock(&watchdog_proc_mutex);
> +
>          if (!watchdog_running)
> -                return;
> +                goto unlock;
> 
>          get_online_cpus();
>          for_each_online_cpu(cpu)
>                  watchdog_nmi_disable(cpu);
>          put_online_cpus();
> +
> +unlock:
> +        mutex_unlock(&watchdog_proc_mutex);
>  }
>  #else
>  static int watchdog_nmi_enable(unsigned int cpu) { return 0; }
> @@ -744,8 +756,6 @@ static int proc_watchdog_update(void)
> 
>  }
> 
> -static DEFINE_MUTEX(watchdog_proc_mutex);
> -
>  /*
>   * common function for watchdog, nmi_watchdog and soft_watchdog parameter
>   *

Re: suspend regression in 4.1-rc1

[Peter Zijlstra]

On Mon, May 18, 2015 at 06:56:46AM -0400, Ulrich Obergfell wrote:
> > Subject: watchdog: Fix merge 'conflict'
> >
> > Two watchdog changes that came through different trees had a non
> > conflicting conflict, that is, one changed the semantics of a variable
> > but no actual code conflict happened. So the merge appeared fine, but
> > the resulting code did not behave as expected.
> >
> > Commit 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").
> 
> Don and I already posted a patch in April to address this:
> 
> https://lkml.org/lkml/2015/4/22/306
> http://ozlabs.org/~akpm/mmots/broken-out/watchdog-fix-watchdog_nmi_enable_all.patch

Yeah, but it seems to have gotten lost on its way to Linus.

> > 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.
> 
> As I understand it, the {en,dis}able_all() functions are only called early
> at kernel startup, so I do not see how they could be racing with watchdog
> code that is executed in the context of write() system calls to parameters
> in /proc/sys/kernel. Please see also my earlier reply to Michal for further
> details: http://marc.info/?l=linux-pm&m=143194387208250&w=2
> 
> Do we really need synchronization here?

Same argument as in my previous email; its best to implement exposed
functions fully and correctly, irrespective of their usage sites.

It costs little extra and might safe a few hairs down the lined. None of
this is performance critical.

Re: suspend regression in 4.1-rc1

[Michal Hocko]

On Mon 18-05-15 06:10:20, Ulrich Obergfell wrote:
[...]
> Michal,
> 
> if I understand you correctly, Peter's patch solves the problem for you.
> I would like to make you aware of a patch that Don and I posted in April.
> 
>   https://lkml.org/lkml/2015/4/22/306
> 
> watchdog_nmi_enable_all() should not use 'watchdog_user_enabled' at all.
> It should rather check the NMI_WATCHDOG_ENABLED bit in 'watchdog_enabled'.
> The patch is also in Andrew Morton's queue.
> 
>   http://ozlabs.org/~akpm/mmots/broken-out/watchdog-fix-watchdog_nmi_enable_all.patch

FWIW: This seems to fix my issue as well.
-- 
Michal Hocko
SUSE Labs

Re: suspend regression in 4.1-rc1

[Stephane Eranian]

Hi,

On Mon, May 18, 2015 at 4:05 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, May 18, 2015 at 06:56:46AM -0400, Ulrich Obergfell wrote:
>> > Subject: watchdog: Fix merge 'conflict'
>> >
>> > Two watchdog changes that came through different trees had a non
>> > conflicting conflict, that is, one changed the semantics of a variable
>> > but no actual code conflict happened. So the merge appeared fine, but
>> > the resulting code did not behave as expected.
>> >
>> > Commit 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").
>>
>> Don and I already posted a patch in April to address this:
>>
>> https://lkml.org/lkml/2015/4/22/306
>> http://ozlabs.org/~akpm/mmots/broken-out/watchdog-fix-watchdog_nmi_enable_all.patch
>
> Yeah, but it seems to have gotten lost on its way to Linus.
>
>> > 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.
>>
>> As I understand it, the {en,dis}able_all() functions are only called early
>> at kernel startup, so I do not see how they could be racing with watchdog
>> code that is executed in the context of write() system calls to parameters
>> in /proc/sys/kernel. Please see also my earlier reply to Michal for further
>> details: http://marc.info/?l=linux-pm&m=143194387208250&w=2
>>
>> Do we really need synchronization here?
>
> Same argument as in my previous email; its best to implement exposed
> functions fully and correctly, irrespective of their usage sites.
>
> It costs little extra and might safe a few hairs down the lined. None of
> this is performance critical.

I cannot reproduce this problem on my T430s running tip.git at 4.1-rc3.

The thing about b37609c30e41 is that is introduces a deferred initcall
for perf_events. It adds an subsys_initcall after the default initialization
of perf_events The reason is that the fixup_ht_bug() needs to wait until
cpu topology is setup before proceeding. Thus by the time
watchdog_nmi_disable_all() is called from that function, the kernel
may be multi-cpu already. Thus, there may be a race.




commit b37609c30e41264c4df4acff78abfc894499a49b
Author: Stephane Eranian <eranian@google.com>
Date:   Mon Nov 17 20:07:04 2014 +0100
   perf/x86/intel: Make the HT bug workaround conditional on HT enabled

Re: suspend regression in 4.1-rc1

[Don Zickus]

On Mon, May 18, 2015 at 11:31:50AM +0200, Peter Zijlstra wrote:
> On Mon, May 18, 2015 at 11:03:37AM +0200, Michal Hocko wrote:
> > This doesn't hang anymore. I've just had to move the mutex definition
> > up to make it compile. So feel free to add my
> 
> I've also fixed a lock leak, see goto unlock :-)
> 
> > Reported-and-tested-by: Michal Hocko <mhocko@suse.cz>
> 
> *blink* that actually fixed it..
> 
> That somewhat leaves me at a loss explaining how s2r was failing.
> 
> ---
> Subject: watchdog: Fix merge 'conflict'
> 
> Two watchdog changes that came through different trees had a non
> conflicting conflict, that is, one changed the semantics of a variable
> but no actual code conflict happened. So the merge appeared fine, but
> the resulting code did not behave as expected.
> 
> Commit 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.
> 
> This patch fixes a s2r failure reported by Michal; which I cannot
> readily explain. But this does make the code internally consistent
> again.

I agree with Peter's locking approach.  We need to do better locking the
variables here.  Poking around the code I see too many variables being
exposed sadly.

Thanks Peter!

Andrew, this patch can replace 'watchdog-fix-watchdog_nmi_enable_all.patch'
on your queue.

Acked-by: Don Zickus <dzickus@redhat.com>
> 
> Reported-and-tested-by: Michal Hocko <mhocko@suse.cz>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/watchdog.c |   20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index 2316f50..506edcc5 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -41,6 +41,8 @@
>  #define NMI_WATCHDOG_ENABLED      (1 << NMI_WATCHDOG_ENABLED_BIT)
>  #define SOFT_WATCHDOG_ENABLED     (1 << SOFT_WATCHDOG_ENABLED_BIT)
>  
> +static DEFINE_MUTEX(watchdog_proc_mutex);
> +
>  #ifdef CONFIG_HARDLOCKUP_DETECTOR
>  static unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED|NMI_WATCHDOG_ENABLED;
>  #else
> @@ -608,26 +610,36 @@ void watchdog_nmi_enable_all(void)
>  {
>  	int cpu;
>  
> -	if (!watchdog_user_enabled)
> -		return;
> +	mutex_lock(&watchdog_proc_mutex);
> +
> +	if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
> +		goto unlock;
>  
>  	get_online_cpus();
>  	for_each_online_cpu(cpu)
>  		watchdog_nmi_enable(cpu);
>  	put_online_cpus();
> +
> +unlock:
> +	mutex_lock(&watchdog_proc_mutex);
>  }
>  
>  void watchdog_nmi_disable_all(void)
>  {
>  	int cpu;
>  
> +	mutex_lock(&watchdog_proc_mutex);
> +
>  	if (!watchdog_running)
> -		return;
> +		goto unlock;
>  
>  	get_online_cpus();
>  	for_each_online_cpu(cpu)
>  		watchdog_nmi_disable(cpu);
>  	put_online_cpus();
> +
> +unlock:
> +	mutex_unlock(&watchdog_proc_mutex);
>  }
>  #else
>  static int watchdog_nmi_enable(unsigned int cpu) { return 0; }
> @@ -744,8 +756,6 @@ static int proc_watchdog_update(void)
>  
>  }
>  
> -static DEFINE_MUTEX(watchdog_proc_mutex);
> -
>  /*
>   * common function for watchdog, nmi_watchdog and soft_watchdog parameter
>   *

Re: suspend regression in 4.1-rc1

[Don Zickus]

On Mon, May 18, 2015 at 06:56:46AM -0400, Ulrich Obergfell wrote:
> 
> > 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.
> 
> As I understand it, the {en,dis}able_all() functions are only called early
> at kernel startup, so I do not see how they could be racing with watchdog
> code that is executed in the context of write() system calls to parameters
> in /proc/sys/kernel. Please see also my earlier reply to Michal for further
> details: http://marc.info/?l=linux-pm&m=143194387208250&w=2
> 
> Do we really need synchronization here?

As Peter said we have to focus on doing things correctly and not based on
what is currently.

During s2ram, I believe all the threads get parked and then unparked during
resume.  I am wondering if the race happens there, threads get unparked and
stomp on each other when watchdog_nmi_enable_all() is called.  (or vice
versa on the way down).  I think during boot the cpu bring up is slow enough
that the race doesn't happen, but s2ram is alot quicker.  My guess.

Cheers,
Don

> 
> > This patch fixes a s2r failure reported by Michal; which I cannot
> > readily explain. But this does make the code internally consistent
> > again.
> >
> > Reported-and-tested-by: Michal Hocko <mhocko@suse.cz>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  kernel/watchdog.c |   20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> > index 2316f50..506edcc5 100644
> > --- a/kernel/watchdog.c
> > +++ b/kernel/watchdog.c
> > @@ -41,6 +41,8 @@
> >  #define NMI_WATCHDOG_ENABLED      (1 << NMI_WATCHDOG_ENABLED_BIT)
> >  #define SOFT_WATCHDOG_ENABLED     (1 << SOFT_WATCHDOG_ENABLED_BIT)
> > 
> > +static DEFINE_MUTEX(watchdog_proc_mutex);
> > +
> >  #ifdef CONFIG_HARDLOCKUP_DETECTOR
> >  static unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED|NMI_WATCHDOG_ENABLED;
> >  #else
> > @@ -608,26 +610,36 @@ void watchdog_nmi_enable_all(void)
> >  {
> >          int cpu;
> > 
> > -        if (!watchdog_user_enabled)
> > -                return;
> > +        mutex_lock(&watchdog_proc_mutex);
> > +
> > +        if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
> > +                goto unlock;
> > 
> >          get_online_cpus();
> >          for_each_online_cpu(cpu)
> >                  watchdog_nmi_enable(cpu);
> >          put_online_cpus();
> > +
> > +unlock:
> > +        mutex_lock(&watchdog_proc_mutex);
> >  }
> > 
> >  void watchdog_nmi_disable_all(void)
> >  {
> >          int cpu;
> > 
> > +        mutex_lock(&watchdog_proc_mutex);
> > +
> >          if (!watchdog_running)
> > -                return;
> > +                goto unlock;
> > 
> >          get_online_cpus();
> >          for_each_online_cpu(cpu)
> >                  watchdog_nmi_disable(cpu);
> >          put_online_cpus();
> > +
> > +unlock:
> > +        mutex_unlock(&watchdog_proc_mutex);
> >  }
> >  #else
> >  static int watchdog_nmi_enable(unsigned int cpu) { return 0; }
> > @@ -744,8 +756,6 @@ static int proc_watchdog_update(void)
> > 
> >  }
> > 
> > -static DEFINE_MUTEX(watchdog_proc_mutex);
> > -
> >  /*
> >   * common function for watchdog, nmi_watchdog and soft_watchdog parameter
> >   *

Re: suspend regression in 4.1-rc1

[Michal Hocko]

On Mon 18-05-15 10:26:07, Don Zickus wrote:
> On Mon, May 18, 2015 at 06:56:46AM -0400, Ulrich Obergfell wrote:
> > 
> > > 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.
> > 
> > As I understand it, the {en,dis}able_all() functions are only called early
> > at kernel startup, so I do not see how they could be racing with watchdog
> > code that is executed in the context of write() system calls to parameters
> > in /proc/sys/kernel. Please see also my earlier reply to Michal for further
> > details: http://marc.info/?l=linux-pm&m=143194387208250&w=2
> > 
> > Do we really need synchronization here?
> 
> As Peter said we have to focus on doing things correctly and not based on
> what is currently.
> 
> During s2ram, I believe all the threads get parked and then unparked during
> resume.  I am wondering if the race happens there, threads get unparked and
> stomp on each other when watchdog_nmi_enable_all() is called.

Wouldn't that cause an issue during freezer mode of pm_test? I can see
it much later in the processors mode.
-- 
Michal Hocko
SUSE Labs

Re: suspend regression in 4.1-rc1

[Don Zickus]

On Mon, May 18, 2015 at 04:41:37PM +0200, Michal Hocko wrote:
> On Mon 18-05-15 10:26:07, Don Zickus wrote:
> > On Mon, May 18, 2015 at 06:56:46AM -0400, Ulrich Obergfell wrote:
> > > 
> > > > 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.
> > > 
> > > As I understand it, the {en,dis}able_all() functions are only called early
> > > at kernel startup, so I do not see how they could be racing with watchdog
> > > code that is executed in the context of write() system calls to parameters
> > > in /proc/sys/kernel. Please see also my earlier reply to Michal for further
> > > details: http://marc.info/?l=linux-pm&m=143194387208250&w=2
> > > 
> > > Do we really need synchronization here?
> > 
> > As Peter said we have to focus on doing things correctly and not based on
> > what is currently.
> > 
> > During s2ram, I believe all the threads get parked and then unparked during
> > resume.  I am wondering if the race happens there, threads get unparked and
> > stomp on each other when watchdog_nmi_enable_all() is called.
> 
> Wouldn't that cause an issue during freezer mode of pm_test? I can see
> it much later in the processors mode.

I am not familiar with the freeze mode of pm_test.  But I believe the
race only happens with cpu0.  I would have thought cpu0 is slower to stop in
freezer mode than in s2ram.  Again, this was just my initial guess at the
race problem. :-(

Peter seems to have a patch (and Uli too) that addresses this problem, so
not sure how much time to focus on figuring this out.

Cheers,
Don

Re: suspend regression in 4.1-rc1

[Linus Torvalds]

On Mon, May 18, 2015 at 2:31 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> Subject: watchdog: Fix merge 'conflict'
>
> Two watchdog changes that came through different trees had a non
> conflicting conflict, that is, one changed the semantics of a variable
> but no actual code conflict happened. So the merge appeared fine, but
> the resulting code did not behave as expected.

Ok, I see that people are still discussing this, but I'll apply it
as-is since I want to get rc4 out the door, and I guess people can
tweak this if there's anything else we want to do longer-term.

             Linus

Re: suspend regression in 4.1-rc1

[Michal Hocko]

On Mon 18-05-15 10:10:50, Linus Torvalds wrote:
> On Mon, May 18, 2015 at 2:31 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > Subject: watchdog: Fix merge 'conflict'
> >
> > Two watchdog changes that came through different trees had a non
> > conflicting conflict, that is, one changed the semantics of a variable
> > but no actual code conflict happened. So the merge appeared fine, but
> > the resulting code did not behave as expected.
> 
> Ok, I see that people are still discussing this, but I'll apply it
> as-is since I want to get rc4 out the door, and I guess people can
> tweak this if there's anything else we want to do longer-term.

The final patch from Peter has a typo. Without the following the system
deadlocks obviously:
---
>From 215266efd5e14938d14f0f4258a70fb32f6a455b Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Tue, 19 May 2015 09:07:27 +0200
Subject: [PATCH] watchdog: fix double lock in watchdog_nmi_enable_all

ab992dc38f9a ("watchdog: Fix merge 'conflict'") has introduced an
obvious deadlock because of a typo. watchdog_proc_mutex should be
unlocked on exit.

Thanks to Miroslav Benes who was staring at the code with me and noticed
this.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 kernel/watchdog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 506edcc500c4..581a68a04c64 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -621,7 +621,7 @@ void watchdog_nmi_enable_all(void)
 	put_online_cpus();
 
 unlock:
-	mutex_lock(&watchdog_proc_mutex);
+	mutex_unlock(&watchdog_proc_mutex);
 }
 
 void watchdog_nmi_disable_all(void)
-- 
2.1.4

-- 
Michal Hocko
SUSE Labs

Re: suspend regression in 4.1-rc1

[Peter Zijlstra]

On Tue, May 19, 2015 at 09:12:59AM +0200, Michal Hocko wrote:
> On Mon 18-05-15 10:10:50, Linus Torvalds wrote:
> > On Mon, May 18, 2015 at 2:31 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > > Subject: watchdog: Fix merge 'conflict'
> > >
> > > Two watchdog changes that came through different trees had a non
> > > conflicting conflict, that is, one changed the semantics of a variable
> > > but no actual code conflict happened. So the merge appeared fine, but
> > > the resulting code did not behave as expected.
> > 
> > Ok, I see that people are still discussing this, but I'll apply it
> > as-is since I want to get rc4 out the door, and I guess people can
> > tweak this if there's anything else we want to do longer-term.
> 
> The final patch from Peter has a typo. Without the following the system
> deadlocks obviously:

Duh, sorry about that. :/

Re: suspend regression in 4.1-rc1

[Michal Hocko]

On Mon 18-05-15 11:45:31, Don Zickus wrote:
> On Mon, May 18, 2015 at 04:41:37PM +0200, Michal Hocko wrote:
> > On Mon 18-05-15 10:26:07, Don Zickus wrote:
> > > On Mon, May 18, 2015 at 06:56:46AM -0400, Ulrich Obergfell wrote:
> > > > 
> > > > > 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.
> > > > 
> > > > As I understand it, the {en,dis}able_all() functions are only called early
> > > > at kernel startup, so I do not see how they could be racing with watchdog
> > > > code that is executed in the context of write() system calls to parameters
> > > > in /proc/sys/kernel. Please see also my earlier reply to Michal for further
> > > > details: http://marc.info/?l=linux-pm&m=143194387208250&w=2
> > > > 
> > > > Do we really need synchronization here?
> > > 
> > > As Peter said we have to focus on doing things correctly and not based on
> > > what is currently.
> > > 
> > > During s2ram, I believe all the threads get parked and then unparked during
> > > resume.  I am wondering if the race happens there, threads get unparked and
> > > stomp on each other when watchdog_nmi_enable_all() is called.
> > 
> > Wouldn't that cause an issue during freezer mode of pm_test? I can see
> > it much later in the processors mode.
> 
> I am not familiar with the freeze mode of pm_test. 

AFAIU only suspend_prepare is called for this mode. This will trigger
pm_notifier_call_chain(PM_SUSPEND_PREPARE) and suspend_freeze_processes
which will freeze all the tasks (including kernel threads).

The hang happened with processors mode which means that everything up to
platform mode was OK. Which reduces the area to disable_nonboot_cpus.

I have tried to put some printks around to see where things go wrong but
then I found out that my netconsole doesn't dump them because the
network cards are long suspended. no_console_suspend apparently doesn't
affect netconsole and the laptop doesn't have regular serial console so
I just gave up.

-- 
Michal Hocko
SUSE Labs