hotplug thread issues

Hi Thomas,

So there have been some reports on hitting:

  BUG_ON(td->cpu != smp_processor_id());

in smpboot_thread_fn.

Now I've been staring at this for a wee bit today and I've found two issues, but I'm not sure either are enough to explain the observed.

1) smpboot_register_percpu_thread() seems to lack serialization against hotplug. It has a for_each_online() loop, but no get_online_cpus() -- unlike smpboot_unregister_percpu_thread, which does.

Typical usage like spawn_ksoftirqd() should be fine, they're early init calls and those run before we bring up the other CPUs. Therefore this does not explain the observation that its ksoftirqd/n triggering the BUG.

However, the usage in proc_dowatchdog() is susceptible to this race and its entirely possible to go wrong there.

2) the usage of __set_current_state(TASK_PARKED) in __kthread_parkme() is wrong AFAICT, one should always use set_current_state() for setting !TASK_RUNNING state. The comment with set_current_state() explains why.

This would've allowed the test_bit(KTHREAD_SHOULD_PARK) load to have been satisfied before the store of TASK_PARKED.

In any case, I'm not sure either of these are enough, I'll go stare at it a bit more I suppose.

---
 kernel/kthread.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 10e489c448fe..9787244d43ec 100644
@@ -156,12 +156,12 @@ void *probe_kthread_data(struct task_struct *task)
 
 static void __kthread_parkme(struct kthread *self)
 {
-	__set_current_state(TASK_PARKED);
+	set_current_state(TASK_PARKED);
 	while (test_bit(KTHREAD_SHOULD_PARK, &self->flags)) {
 		if (!test_and_set_bit(KTHREAD_IS_PARKED, &self->flags))
 			complete(&self->parked);
 		schedule();
-		__set_current_state(TASK_PARKED);
+		set_current_state(TASK_PARKED);
 	}
 	clear_bit(KTHREAD_IS_PARKED, &self->flags);
 	__set_current_state(TASK_RUNNING);

Hmm. Need to have a look.

My bad. Can you send a proper patch addressing that issue please? That should be tagged stable as well I guess.

Thanks,

tglx

Sure thing, something like so then?

---
Subject: kthread: Fix memory ordering in __kthread_parkme

One should always use set_current_state() to set !TASK_RUNNING states.

	set_current_state(TASK_*);		cond = true;
	/* mb */				/* wmb */
	if (!cond)				wake_up_state(, TASK_*);
		schedule();

By not having the mb we allow for the cond load to be satisfied before the state store, this can result in:

	if (!cond)
						cond = true;
						wake_up_state(, TASK_*);
	__set_current_state(TASK_*);
		schedule();

Which would block 'forever', since the cond is still false and the wakeup would not have seen the !TASK_RUNNING state.

---
 kernel/kthread.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 10e489c448fe..9787244d43ec 100644
@@ -156,12 +156,12 @@ void *probe_kthread_data(struct task_struct *task)
 
 static void __kthread_parkme(struct kthread *self)
 {
-	__set_current_state(TASK_PARKED);
+	set_current_state(TASK_PARKED);
 	while (test_bit(KTHREAD_SHOULD_PARK, &self->flags)) {
 		if (!test_and_set_bit(KTHREAD_IS_PARKED, &self->flags))
 			complete(&self->parked);
 		schedule();
-		__set_current_state(TASK_PARKED);
+		set_current_state(TASK_PARKED);
 	}
 	clear_bit(KTHREAD_IS_PARKED, &self->flags);
 	__set_current_state(TASK_RUNNING);

Yeah, we should probably clean that up, it looks a bit odd. But I didn't want to do too many changes.

set_current_state(TASK_PARKED)

while (test_bit(KTHREAD_SHOULD_PARK, ..)) {
						clear_bit(KTHREAD_SHOULD_PARK, ..);
						if (test_and_clear_bit(KTHREAD_IS_PARKED, ..) {
							...
							wake_up_state(, TASK_PARKED);
						}
	if (!test_and_set_bit(KTHREAD_IS_PARKED, ..))
		complete(..);
	schedule();

Then yes we'll miss the wakeup, but we also miss the __kthread_bind().

Now I don't think this'll actually happen because kthread_park() waits for the completion under the hotplug and smpboot_threads_lock lock, and we do the unpark under the hotplug lock as well, so its fully serialized

But yes, we should probably clean this up as well.