hotplug thread issues

hotplug thread issues

[Peter Zijlstra]

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(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 10e489c448fe..9787244d43ec 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -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);

Re: hotplug thread issues

[Thomas Gleixner]

On Thu, 6 Nov 2014, Peter Zijlstra wrote:

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

Hmm. Need to have a look.
 
> 
> 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.

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

Thanks,

	tglx

[PATCH]: kthread: Fix memory ordering in __kthread_parkme

[Peter Zijlstra]

On Fri, Nov 07, 2014 at 10:39:48AM +0100, Thomas Gleixner wrote:
> > 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.
> 
> My bad. Can you send a proper patch addressing that issue please? That
> should be tagged stable as well I guess.

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.

Cc: stable@vger.kernel.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/kthread.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 10e489c448fe..9787244d43ec 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -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);

Re: [PATCH]: kthread: Fix memory ordering in __kthread_parkme

[Oleg Nesterov]

On 11/07, Peter Zijlstra wrote:
>
>  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);
>  	}

Perhaps it makses sense to do set_current_state(PARKED) once at the start
of "for (;;)" loop, but this is cosmetic.

What if kthread_unpark() is called right after test_bit(KTHREAD_SHOULD_PARK)
and KTHREAD_IS_PARKED is not set? It seems that __kthread_unpark() should
call wake_up_state() unconditionally ?

Oleg.

Re: [PATCH]: kthread: Fix memory ordering in __kthread_parkme

[Peter Zijlstra]

On Fri, Nov 07, 2014 at 07:41:03PM +0100, Oleg Nesterov wrote:
> On 11/07, Peter Zijlstra wrote:
> >
> >  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);
> >  	}
> 
> Perhaps it makses sense to do set_current_state(PARKED) once at the start
> of "for (;;)" loop, but this is cosmetic.

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

> What if kthread_unpark() is called right after test_bit(KTHREAD_SHOULD_PARK)
> and KTHREAD_IS_PARKED is not set? It seems that __kthread_unpark() should
> call wake_up_state() unconditionally ?

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.