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 <[email protected]>
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > ---
> > 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-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
