Re: [PATCH] watchdog: Make sure the watchdog thread gets CPU on loaded system

[Andrew Morton <akpm@linux-foundation.org>]

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.

> If the system is loaded while hotplugging a CPU we might end up with a bogus
> hardlockup detection. This has been seen during LTP pounder test executed
> in parallel with hotplug test.
> 
> The main problem is that enable_watchdog (called when CPU is brought up)

You mean watchdog_enable().

> registers perf event which periodically checks per-cpu counter
> (hrtimer_interrupts), updated from a hrtimer callback, but the hrtimer is fired

s/fired/started/

> from the kernel thread.

"the kernel thread" being kernel/watchdog.c:watchdog()

> This means that while we already do check for the hard lockup the kernel thread

Who is "we" and where in the kernel does this check occur?

"the kernel thread" is still kernel/watchdog.c:watchdog().

> might be sitting on the runqueue with zillions of tasks

What causes these "zillions of tasks"?  Are they userspace tasks? 
They're preventing the watchdog() function from being called in a
timely fashion, I assume?

> so there is nobody to
> update the value we rely on and so we KABOOM.

Who is "we" and what is "the value"?

etcetera.  It is maddeningly inaccurate, vague and handwavy for someone
who is actually trying to understand what you're trying to tell us.

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

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

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

> @@ -439,6 +436,7 @@ static int watchdog_enable(int cpu)
>  
>  	/* create the watchdog thread */
>  	if (!p) {
> +		struct sched_param param = { .sched_priority = MAX_RT_PRIO-1 };
>  		p = kthread_create_on_node(watchdog, NULL, cpu_to_node(cpu), "watchdog/%d", cpu);
>  		if (IS_ERR(p)) {
>  			printk(KERN_ERR "softlockup watchdog for %i failed\n", cpu);
> @@ -450,6 +448,7 @@ static int watchdog_enable(int cpu)
>  			}
>  			goto out;
>  		}
> +		sched_setscheduler(p, SCHED_FIFO, &param);
>  		kthread_bind(p, cpu);
>  		per_cpu(watchdog_touch_ts, cpu) = 0;
>  		per_cpu(softlockup_watchdog, cpu) = p;

It's pretty silly that kthread_create_on_node() sets the scheduling
policy and priority and then the caller immediately resets it.  There
should be a version of kthread_create_on_node() whcih takes these as
arguments.

Oh well, despite all that the patch looks OK to me, after using
whiteout all over the changelog.