Re: suspend regression in 4.1-rc1

[Don Zickus <dzickus@redhat.com>]

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