[merged] smpboot-make-cleanup-to-mirror-setup.patch removed from -mm tree

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



The patch titled
     Subject: smpboot: make cleanup to mirror setup
has been removed from the -mm tree.  Its filename was
     smpboot-make-cleanup-to-mirror-setup.patch

This patch was dropped because it was merged into mainline or a subsystem tree

------------------------------------------------------
From: Frederic Weisbecker <fweisbec@xxxxxxxxx>
Subject: smpboot: make cleanup to mirror setup

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 resources 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 let's 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.

Signed-off-by: Frederic Weisbecker <fweisbec@xxxxxxxxx>
Reviewed-by: Chris Metcalf <cmetcalf@xxxxxxxxxx>
Reviewed-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Cc: Chris Metcalf <cmetcalf@xxxxxxxxxx>
Cc: Don Zickus <dzickus@xxxxxxxxxx>
Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
Cc: Ulrich Obergfell <uobergfe@xxxxxxxxxx>
Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
---

 kernel/smpboot.c |   12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff -puN kernel/smpboot.c~smpboot-make-cleanup-to-mirror-setup kernel/smpboot.c
--- a/kernel/smpboot.c~smpboot-make-cleanup-to-mirror-setup
+++ a/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(stru
 {
 	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);
_

Patches currently in -mm which might be from fweisbec@xxxxxxxxx are

kmod-bunch-of-internal-functions-renames.patch
kmod-remove-unecessary-explicit-wide-cpu-affinity-setting.patch
kmod-add-up-to-date-explanations-on-the-purpose-of-each-asynchronous-levels.patch
kmod-use-system_unbound_wq-instead-of-khelper.patch
kmod-handle-umh_wait_proc-from-system-unbound-workqueue.patch

--
To unsubscribe from this list: send the line "unsubscribe mm-commits" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Kernel Newbies FAQ]     [Kernel Archive]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [Bugtraq]     [Photo]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]

  Powered by Linux