lkml.org 
[lkml]   [2012]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] watchdog: Make sure the watchdog thread gets CPU on loaded system
On Thu, Mar 15, 2012 at 09:02:32AM +0100, Michal Hocko wrote:
> On Wed 14-03-12 16:19:06, Andrew Morton wrote:
> > On Wed, 14 Mar 2012 16:38:45 -0400
> > Don Zickus <dzickus@redhat.com> wrote:
> >
> > > From: Michal Hocko <mhocko@suse.cz>
> >
> > This changelog is awful.

My apologies too, Andrew for not being more diligent.

Some nitpicks below (hopefully it isn't too picky :-( )

>
> Sorry about that, What about this?
>
> If the system is heavy loaded while hotplugging a CPU we might end up
heavily ,

> with a bogus hardlockup detection. This has been seen during LTP pounder
the
> test executed in parallel with hotplug test.
the

>
> Hard lockup detector consist of two parts
> - watchdog_overflow_callback (executed as a perf counter callback
> from NMI) which checks whether per-cpu hrtimer_interrupts changed
> since the last time it run and panics if not
> - watchdog kernel thread which starts watchdog_hrtimer which
> periodically updates hrtimer_interrupts.
>
> The main problem is that watchdog_enable (called when CPU is brought up)
a

> registers perf event but the hrtimer is started later when the watchdog
a

> thread gets a chance to run.

<perhaps an empty line here or merge the paragraphs to make it easier
to read?>

> The watchdog thread starts with a normal priority currently and boosts
^^^^^^^(remove?)

> itself as soon as it gets to a CPU. This might be, however, already too
^^^ replace with 'runs on'

> late as demonstrated with the LTP pounder test executed in parallel with
^^^^ replace with 'by'

> LTP hotplug test. There are zillions of userspace processes sitting in
> the runque in this workload while the number of CPUs gets down to 1 and
> then they are onlined back to the original count.
sounds awkward, how about
"while the number of active CPUS (after
soft-unplugging) is 1. Then all the cpus are soft-plugged back online."

> When we online a CPU and create the watchdog kernel thread it will take
> some time until it gets to a CPU. On the other hand the perf counter
> callback is executed in the timely fashion so we explode the first time
> it finds out there were no changes in the counter.
^^^^^ perhaps "it finds out hrtimer_interrupts was not incremented"

>
> Let's fix this by boosting the watchdog thread priority before we wake it up
> rather than when it's already running.
> This still doesn't handle a case where we have the same amount of high prio
> FIFO tasks but that doesn't seem to be common. The current implementation
> doesn't handle that case anyway so this is not worse at least.
^^^^ "is no worse."??

>
> Unfortunately, we cannot start perf counter from the watchdog thread because we
> could miss a real lock up and also we cannot start the hrtimer watchdog_enable
^ from

> because we there is no way (at least I don't know any) to start a hrtimer from
s/we//

> a different CPU.
>
> [...]
> > > Let's fix this by boosting the watchdog thread priority before we wake it up
> > > rather than when it's already running.
> > > This still doesn't handle a case where we have the same amount of high prio
> > > FIFO tasks but that doesn't seem to be common.
> >
> > Even a single FIFO thread could starve the watchdog() thread.
>
> Only if preemption is off, I guess...

I was going suggest that is a good case for touch_softlockup(), but if the
thread is in userspace that won't work.

>
> > > The current implementation
> > > doesn't handle that case anyway so this is not worse at least.
> >
> > Right. But this isn't specific to the startup case, is it? A spinning
> > SCHED_FIFO thread could cause watchdog() to get starved of CPU for an
> > arbitrarily long time, triggering a false(?) lockup detection? Or did
> > we do something to prevent that case? I assume we did - it would be
> > pretty bad if this were to happen.

Well either the thread should use touch_softlockup() (if possible) or we
need to have a higher priority for the softlockup thread to prevent
userspace from blocking it.

> >
> > > Unfortunately, we cannot start perf counter from the watchdog thread because we
> > > could miss a real lock up and also we cannot start the hrtimer watchdog_enable
> > > because we there is no way (at least I don't know any) to start a hrtimer from
> > > a different CPU.
> > >
> > > [fix compile issue with param -dcz]
> > >
> > > Cc: Ingo Molnar <mingo@elte.hu>
> > > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Mandeep Singh Baines <msb@chromium.org>
> > > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> > > Signed-off-by: Don Zickus <dzickus@redhat.com>
> > > ---
> > > kernel/watchdog.c | 7 +++----
> > > 1 files changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> > > index d117262..6618cde 100644
> > > --- a/kernel/watchdog.c
> > > +++ b/kernel/watchdog.c
> > > @@ -321,11 +321,9 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
> > > */
> > > static int watchdog(void *unused)
> > > {
> > > - struct sched_param param = { .sched_priority = MAX_RT_PRIO-1 };
> > > + struct sched_param param = { .sched_priority = 0 };
> > > struct hrtimer *hrtimer = &__raw_get_cpu_var(watchdog_hrtimer);
> > >
> > > - sched_setscheduler(current, SCHED_FIFO, &param);
> > > -
> > > /* initialize timestamp */
> > > __touch_watchdog();
> > >
> > > @@ -350,7 +348,6 @@ static int watchdog(void *unused)
> > > set_current_state(TASK_INTERRUPTIBLE);
> > > }
> > > __set_current_state(TASK_RUNNING);
> > > - param.sched_priority = 0;
> > > sched_setscheduler(current, SCHED_NORMAL, &param);
> > > return 0;
> > > }
> >
> > Why did watchdog() reset the scheduling policy seven instructions
> > before exiting? Seems pointless.
>
> It has been introduced by Thomas in cba9bd22. To be honest I don't
> understand why it makes a sense?

Yeah I noticed that too. I didn't bother questioning it either when it
went in. I just assumed Thomas and Peter know scheduling a lot better
than I do. :-)

Cheers,
Don


\
 
 \ /
  Last update: 2012-03-15 16:57    [from the cache]
©2003-2011 Jasper Spaans