[PATCH 0/4] smpboot/watchdog: Fixes and cleanups

[PATCH 0/4] smpboot/watchdog: Fixes and cleanups

[Frederic Weisbecker]

Hi,

The 2nd patch should fix the strange bug we've seen with Chris.

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
	core/watchdog

HEAD: 5e36d90b3ccb0023ad54ac7a2381c132c6b12280

Thanks,
	Frederic
---

Frederic Weisbecker (4):
      smpboot: Fix memory leak on error handling
      smpboot: Make cleanup to mirror setup
      smpboot: Allow to pass the cpumask on per-cpu thread registration
      watchdog: Simplify housekeeping affinity with the appropriate mask


 include/linux/smpboot.h | 11 ++++++++++-
 kernel/smpboot.c        | 22 +++++++++-------------
 kernel/watchdog.c       | 15 +++++----------
 3 files changed, 24 insertions(+), 24 deletions(-)

[PATCH 1/4] smpboot: Fix memory leak on error handling

[Frederic Weisbecker]

The cpumask is allocated before threads get created. If the latter step
fails, we need to free the cpumask.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Chris Metcalf <cmetcalf@ezchip.com>
Cc: Don Zickus <dzickus@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ulrich Obergfell <uobergfe@redhat.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/smpboot.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/smpboot.c b/kernel/smpboot.c
index 7c434c3..71aa90b 100644
--- a/kernel/smpboot.c
+++ b/kernel/smpboot.c
@@ -301,6 +301,7 @@ int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread)
 		ret = __smpboot_create_thread(plug_thread, cpu);
 		if (ret) {
 			smpboot_destroy_threads(plug_thread);
+			free_cpumask_var(plug_thread->cpumask);
 			goto out;
 		}
 		smpboot_unpark_thread(plug_thread, cpu);
-- 
2.1.4

[PATCH 2/4] smpboot: Make cleanup to mirror setup

[Frederic Weisbecker]

The per-cpu kthread cleanup() callback is the mirror of the setup()
callback. When the per-cpu kthread is started, it first calls setup()
to initialize the ressources which are then released by cleanup() when
the kthread exits.

Now since the introduction of a per-cpu kthread cpumask, the kthreads
excluded by the cpumask on boot may happen to be parked immediately
after their creation without taking the setup() stage, waiting to be
asked to unpark to do so. Then when smpboot_unregister_percpu_thread()
is later called, the kthread is stopped without having ever called
setup().

But this triggers a bug as the kthread unconditionally calls cleanup()
on exit but this doesn't mirror any setup(). Thus the kernel crashes
because we try to free resources that haven't been initialized, as in
the watchdog case:

	[  112.645556] WATCHDOG disable 0
	[  112.648765] WATCHDOG disable 1
	[  112.651891] WATCHDOG disable 2
	[  112.654953] BUG: unable to handle kernel NULL pointer dereference at           (null)
	[  112.662808] IP: [<ffffffff8111ea16>] hrtimer_active+0x26/0x60
	[...]
	[  112.815078] Call Trace:
	[  112.817523]  [<ffffffff8111fe7c>] hrtimer_try_to_cancel+0x1c/0x280
	[  112.823697]  [<ffffffff811200fd>] hrtimer_cancel+0x1d/0x30
	[  112.829172]  [<ffffffff8115d846>] watchdog_disable+0x56/0x70
	[  112.834818]  [<ffffffff8115d86e>] watchdog_cleanup+0xe/0x10
	[  112.840381]  [<ffffffff810ca05c>] smpboot_thread_fn+0x23c/0x2c0
	[  112.846296]  [<ffffffff810c9e20>] ? sort_range+0x30/0x30
	[  112.851596]  [<ffffffff810c6478>] kthread+0xf8/0x110
	[  112.856550]  [<ffffffff810c6380>] ? kthread_create_on_node+0x210/0x210
	[  112.863065]  [<ffffffff8191501f>] ret_from_fork+0x3f/0x70
	[  112.868460]  [<ffffffff810c6380>] ? kthread_create_on_node+0x210/0x210

This bug is currently masked with explicit kthread unparking before
kthread_stop() on smpboot_destroy_threads(). This forces a call to
setup() and then unpark().

We could fix this by unconditionally calling setup() on kthread entry.
But setup() isn't always cheap. In the case of watchdog it launches
hrtimer, perf events, etc... So we may as well like to skip it if there
are chances the kthread will never be used, as in a reduced cpumask
value.

So lets simply do a state machine check before calling cleanup() that
makes sure setup() has been called before mirroring it.

And remove the nasty hack workaround.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Chris Metcalf <cmetcalf@ezchip.com>
Cc: Don Zickus <dzickus@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ulrich Obergfell <uobergfe@redhat.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/smpboot.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/kernel/smpboot.c b/kernel/smpboot.c
index 71aa90b..60aa858 100644
--- a/kernel/smpboot.c
+++ b/kernel/smpboot.c
@@ -113,7 +113,8 @@ static int smpboot_thread_fn(void *data)
 		if (kthread_should_stop()) {
 			__set_current_state(TASK_RUNNING);
 			preempt_enable();
-			if (ht->cleanup)
+			/* cleanup must mirror setup */
+			if (ht->cleanup && td->status != HP_THREAD_NONE)
 				ht->cleanup(td->cpu, cpu_online(td->cpu));
 			kfree(td);
 			return 0;
@@ -259,15 +260,6 @@ static void smpboot_destroy_threads(struct smp_hotplug_thread *ht)
 {
 	unsigned int cpu;
 
-	/* Unpark any threads that were voluntarily parked. */
-	for_each_cpu_not(cpu, ht->cpumask) {
-		if (cpu_online(cpu)) {
-			struct task_struct *tsk = *per_cpu_ptr(ht->store, cpu);
-			if (tsk)
-				kthread_unpark(tsk);
-		}
-	}
-
 	/* We need to destroy also the parked threads of offline cpus */
 	for_each_possible_cpu(cpu) {
 		struct task_struct *tsk = *per_cpu_ptr(ht->store, cpu);
-- 
2.1.4

[PATCH 3/4] smpboot: Allow to pass the cpumask on per-cpu thread registration

[Frederic Weisbecker]

It makes the registration cheaper and simpler for the smpboot per-cpu
kthread users that don't need to always update the cpumask after threads
creation.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Chris Metcalf <cmetcalf@ezchip.com>
Cc: Don Zickus <dzickus@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ulrich Obergfell <uobergfe@redhat.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 include/linux/smpboot.h | 11 ++++++++++-
 kernel/smpboot.c        |  9 ++++++---
 kernel/watchdog.c       |  9 +++------
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/include/linux/smpboot.h b/include/linux/smpboot.h
index da3c593..e6109a6 100644
--- a/include/linux/smpboot.h
+++ b/include/linux/smpboot.h
@@ -48,7 +48,16 @@ struct smp_hotplug_thread {
 	const char			*thread_comm;
 };
 
-int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread);
+int smpboot_register_percpu_thread_cpumask(struct smp_hotplug_thread *plug_thread,
+					   const struct cpumask *cpumask);
+
+static inline int
+smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread)
+{
+	return smpboot_register_percpu_thread_cpumask(plug_thread,
+						      cpu_possible_mask);
+}
+
 void smpboot_unregister_percpu_thread(struct smp_hotplug_thread *plug_thread);
 int smpboot_update_cpumask_percpu_thread(struct smp_hotplug_thread *plug_thread,
 					 const struct cpumask *);
diff --git a/kernel/smpboot.c b/kernel/smpboot.c
index 60aa858..699846d 100644
--- a/kernel/smpboot.c
+++ b/kernel/smpboot.c
@@ -275,17 +275,19 @@ static void smpboot_destroy_threads(struct smp_hotplug_thread *ht)
 /**
  * smpboot_register_percpu_thread - Register a per_cpu thread related to hotplug
  * @plug_thread:	Hotplug thread descriptor
+ * @cpumask:		The cpumask where threads run
  *
  * Creates and starts the threads on all online cpus.
  */
-int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread)
+int smpboot_register_percpu_thread_cpumask(struct smp_hotplug_thread *plug_thread,
+					   const struct cpumask *cpumask)
 {
 	unsigned int cpu;
 	int ret = 0;
 
 	if (!alloc_cpumask_var(&plug_thread->cpumask, GFP_KERNEL))
 		return -ENOMEM;
-	cpumask_copy(plug_thread->cpumask, cpu_possible_mask);
+	cpumask_copy(plug_thread->cpumask, cpumask);
 
 	get_online_cpus();
 	mutex_lock(&smpboot_threads_lock);
@@ -296,7 +298,8 @@ int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread)
 			free_cpumask_var(plug_thread->cpumask);
 			goto out;
 		}
-		smpboot_unpark_thread(plug_thread, cpu);
+		if (cpumask_test_cpu(cpu, cpumask))
+			smpboot_unpark_thread(plug_thread, cpu);
 	}
 	list_add(&plug_thread->list, &hotplug_threads);
 out:
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index a6ffa43..e5bb86f 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -713,15 +713,12 @@ static int watchdog_enable_all_cpus(void)
 	int err = 0;
 
 	if (!watchdog_running) {
-		err = smpboot_register_percpu_thread(&watchdog_threads);
+		err = smpboot_register_percpu_thread_cpumask(&watchdog_threads,
+							     &watchdog_cpumask);
 		if (err)
 			pr_err("Failed to create watchdog threads, disabled\n");
-		else {
-			if (smpboot_update_cpumask_percpu_thread(
-				    &watchdog_threads, &watchdog_cpumask))
-				pr_err("Failed to set cpumask for watchdog threads\n");
+		else
 			watchdog_running = 1;
-		}
 	} else {
 		/*
 		 * Enable/disable the lockup detectors or
-- 
2.1.4

Re: [PATCH 3/4] smpboot: Allow to pass the cpumask on per-cpu thread registration

[Ulrich Obergfell]

Frederic,

since you changed the function name, you may want to adjust the comment header
too ----------------.
                    v
 /**
  * smpboot_register_percpu_thread - Register a per_cpu thread related to hotplug
  * @plug_thread:        Hotplug thread descriptor
+ * @cpumask:                The cpumask where threads run
  *
  * Creates and starts the threads on all online cpus.
  */
-int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread)
+int smpboot_register_percpu_thread_cpumask(struct smp_hotplug_thread *plug_thread,
+                                           const struct cpumask *cpumask)

Regards,

Uli

Re: [PATCH 3/4] smpboot: Allow to pass the cpumask on per-cpu thread registration

[Frederic Weisbecker]

On Sat, Jul 11, 2015 at 11:17:01AM -0400, Ulrich Obergfell wrote:
> 
> Frederic,
> 
> since you changed the function name, you may want to adjust the comment header
> too ----------------.
>                     v

Oops, thanks for pointing out.

>  /**
>   * smpboot_register_percpu_thread - Register a per_cpu thread related to hotplug
>   * @plug_thread:        Hotplug thread descriptor
> + * @cpumask:                The cpumask where threads run
>   *
>   * Creates and starts the threads on all online cpus.
>   */
> -int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread)
> +int smpboot_register_percpu_thread_cpumask(struct smp_hotplug_thread *plug_thread,
> +                                           const struct cpumask *cpumask)
> 
> Regards,
> 
> Uli

[PATCH 4/4] watchdog: Simplify housekeeping affinity with the appropriate mask

[Frederic Weisbecker]

housekeeping_mask gathers all the CPUs that aren't part of the nohz_full
set. This is exactly what we want the watchdog to be affine to without
the need to use complicated cpumask operations.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Chris Metcalf <cmetcalf@ezchip.com>
Cc: Don Zickus <dzickus@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ulrich Obergfell <uobergfe@redhat.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/watchdog.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index e5bb86f..d18330f 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -929,10 +929,8 @@ void __init lockup_detector_init(void)
 
 #ifdef CONFIG_NO_HZ_FULL
 	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);
+		pr_info("Disabling watchdog on nohz_full cores by default\n");
+		cpumask_copy(&watchdog_cpumask, housekeeping_mask);
 	} else
 		cpumask_copy(&watchdog_cpumask, cpu_possible_mask);
 #else
-- 
2.1.4

Re: [PATCH 0/4] smpboot/watchdog: Fixes and cleanups

[Chris Metcalf]

On 07/09/2015 11:16 AM, Frederic Weisbecker wrote:
> Hi,
>
> The 2nd patch should fix the strange bug we've seen with Chris.
>
> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> 	core/watchdog
>
> HEAD: 5e36d90b3ccb0023ad54ac7a2381c132c6b12280
>
> Thanks,
> 	Frederic
> ---
>
> Frederic Weisbecker (4):
>        smpboot: Fix memory leak on error handling
>        smpboot: Make cleanup to mirror setup
>        smpboot: Allow to pass the cpumask on per-cpu thread registration
>        watchdog: Simplify housekeeping affinity with the appropriate mask
>
>
>   include/linux/smpboot.h | 11 ++++++++++-
>   kernel/smpboot.c        | 22 +++++++++-------------
>   kernel/watchdog.c       | 15 +++++----------
>   3 files changed, 24 insertions(+), 24 deletions(-)

You can add my

Reviewed-by: Chris Metcalf <cmetcalf@ezchip.com>

to all four.  There are a couple of typos in the commit message for
patch 2/4: "ressources" and "lets simply" (should be "let's simply").

Glad you figured out what the bug was.  Curious that in your
environment you got a panic rather than an infinite loop waiting
for a timer subsystem pointer to go non-NULL.

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

Re: [PATCH 0/4] smpboot/watchdog: Fixes and cleanups

[Frederic Weisbecker]

On Thu, Jul 09, 2015 at 04:59:07PM -0400, Chris Metcalf wrote:
> On 07/09/2015 11:16 AM, Frederic Weisbecker wrote:
> >Hi,
> >
> >The 2nd patch should fix the strange bug we've seen with Chris.
> >
> >git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> >	core/watchdog
> >
> >HEAD: 5e36d90b3ccb0023ad54ac7a2381c132c6b12280
> >
> >Thanks,
> >	Frederic
> >---
> >
> >Frederic Weisbecker (4):
> >       smpboot: Fix memory leak on error handling
> >       smpboot: Make cleanup to mirror setup
> >       smpboot: Allow to pass the cpumask on per-cpu thread registration
> >       watchdog: Simplify housekeeping affinity with the appropriate mask
> >
> >
> >  include/linux/smpboot.h | 11 ++++++++++-
> >  kernel/smpboot.c        | 22 +++++++++-------------
> >  kernel/watchdog.c       | 15 +++++----------
> >  3 files changed, 24 insertions(+), 24 deletions(-)
> 
> You can add my
> 
> Reviewed-by: Chris Metcalf <cmetcalf@ezchip.com>
> 
> to all four.  There are a couple of typos in the commit message for
> patch 2/4: "ressources" and "lets simply" (should be "let's simply").

Ok I'm going to respin and ask Andrew to apply them.

> Glad you figured out what the bug was.  Curious that in your
> environment you got a panic rather than an infinite loop waiting
> for a timer subsystem pointer to go non-NULL.

It might be a config difference. In my box it crashes because hrtimer_cancel()
called on an htimer that hasn't been init does a NULL dereference of the cpu base.