| Message ID | 20150518093150.GC21418@twins.programming.kicks-ass.net (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 > * -- 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
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. -- 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
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 -- 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
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 > * -- 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
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 > > * -- 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
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.
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 -- 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
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 -- 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
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.
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 *