[PATCH] cpuidle: Fix a watchdog crash in some configurations

[PATCH] cpuidle: Fix a watchdog crash in some configurations

[john.hubbard]

From: John Hubbard <jhubbard@nvidia.com>

Commit 8fcf2cc768acd845c1fed837bf9cfe2d7106336d in linux-next
introduced a regression in some configurations. Specifically,
with CONFIG_NO_HZ_FULL set, and CONFIG_NO_HZ_FULL_ALL *not* set,
the kernel will crash in lockup_detector_init(), due to a
NULL tick_nohz_full_init pointer.

This is because the above commit uses tick_nohz_full_init
(in lockup_detector_init), if CONFIG_NO_HZ_FULL is set, but
tick_nohz_full_init only gets allocated if either:

    a) CONFIG_NO_HZ_FULL_ALL is set, or

    b) Someone passes in nohz_full=<any_value> on the boot
      args line.

To correct this, change the allocation site (case a, above):
allocate tick_nohz_full_init whenever CONFIG_NO_HZ_FULL is set.
Also, change the enclosing function name to more accurately
reflect its current role.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 kernel/time/tick-sched.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 9142591..b21ee28 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -326,11 +326,11 @@ static int tick_nohz_cpu_down_callback(struct notifier_block *nfb,
 	return NOTIFY_OK;
 }
 
-static int tick_nohz_init_all(void)
+static int tick_nohz_full_init(void)
 {
 	int err = -1;
 
-#ifdef CONFIG_NO_HZ_FULL_ALL
+#ifdef CONFIG_NO_HZ_FULL
 	if (!alloc_cpumask_var(&tick_nohz_full_mask, GFP_KERNEL)) {
 		WARN(1, "NO_HZ: Can't allocate full dynticks cpumask\n");
 		return err;
@@ -347,7 +347,7 @@ void __init tick_nohz_init(void)
 	int cpu;
 
 	if (!tick_nohz_full_running) {
-		if (tick_nohz_init_all() < 0)
+		if (tick_nohz_full_init() < 0)
 			return;
 	}
 
-- 
2.3.7

Re: [PATCH] cpuidle: Fix a watchdog crash in some configurations

[John Hubbard]

[-- Attachment #1: Type: text/plain, Size: 2721 bytes --]

Re-sending from a proper text-only email client. Sorry for the extra 
email.

On Mon, 4 May 2015, Chris Metcalf wrote:

> On 5/2/2015 4:10 AM, john.hubbard@gmail.com wrote:
> 
> From: John Hubbard <jhubbard@nvidia.com>
> 
> Commit 8fcf2cc768acd845c1fed837bf9cfe2d7106336d in linux-next
> introduced a regression in some configurations. Specifically,
> with CONFIG_NO_HZ_FULL set, and CONFIG_NO_HZ_FULL_ALL *not* set,
> the kernel will crash in lockup_detector_init(), due to a
> NULL tick_nohz_full_init pointer.
> 
> This is because the above commit uses tick_nohz_full_init
> (in lockup_detector_init), if CONFIG_NO_HZ_FULL is set, but
> tick_nohz_full_init only gets allocated if either:
> 
>     a) CONFIG_NO_HZ_FULL_ALL is set, or
> 
>     b) Someone passes in nohz_full=<any_value> on the boot
>       args line.
> 
> To correct this, change the allocation site (case a, above):
> allocate tick_nohz_full_init whenever CONFIG_NO_HZ_FULL is set.
> Also, change the enclosing function name to more accurately
> reflect its current role.
> 
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> 
> 
> (Note that you say "tick_nohz_full_init" in your text a couple of times
> where you mean "tick_nohz_full_mask".)

Yes, you're correct. I'll fix that.

> 
> This looks plausible to me, but I know that Frederic was thinking of
> doing something deeper by making tick_nohz_full_mask a mask
> that was available in all configurations, like cpu_possible_mask etc:
> 
>   https://lkml.org/lkml/2015/4/14/347
> 
> Another way to fix this would be to fix lockup_detector_init() to
> test dynamically like this:
> 
>         if (tick_nohz_full_enabled()) {
>                 if (!cpumask_empty(tick_nohz_full_mask))
>                         pr_info("Disabling watchdog on nohz_full cores by default\n");
>                 cpumask_andnot(&watchdog_cpumask, cpu_possible_mask,
>                                tick_nohz_full_mask);
>         } else {
>                 cpumask_copy(&watchdog_cpumask, cpu_possible_mask);
>         }
> 
> This would avoid looking at the NULL cpumask pointer and is more
> consistent with how other nohz_full code works, for better or worse.
> 


OK, right, I see now that the NO_HZ code usually does this check:

#ifdef CONFIG_NO_HZ_FULL
    if (tick_nohz_full_enabled())

...and the new code in lockup_detector_init() simply lacked the second 
line, so your fix is probably the better way to go. I just retested and 
all is well with that approach. I'll send an updated fix separately (so 
that whitespace is preserved) in a moment.


> I didn't see this since by default I don't have CPUMASK_OFFLINE.
> -- 
> Chris Metcalf, EZChip Semiconductor
> http://www.ezchip.com
> 
> 

thanks,
John H.