Date:	Sun, 02 Nov 2014 20:01:51 +0800
From:	Daniel J Blueman <daniel@...ascale.com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	LKML <linux-kernel@...r.kernel.org>,
	Subbaraman Narayanamurthy <subbaram@...eaurora.org>,
	Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH] kthread: Fix the race condition when kthread is parked

On Thursday, June 26, 2014 8:50:01 AM UTC+8, Thomas Gleixner wrote:
 > On Wed, 25 Jun 2014, Subbaraman Narayanamurthy wrote:
 > > While stressing the CPU hotplug path, sometimes we hit a problem
 > > as shown below.
 > >
 > > [57056.416774] ------------[ cut here ]------------
 > > [57056.489232] ksoftirqd/1 (14): undefined instruction: pc=c01931e8
 > > [57056.489245] Code: e594a000 eb085236 e15a0000 0a000000 (e7f001f2)
 > > [57056.489259] ------------[ cut here ]------------
 > > [57056.492840] kernel BUG at kernel/kernel/smpboot.c:134!
 > > [57056.513236] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
 > > [57056.519055] Modules linked in: wlan(O) mhi(O)
 > > [57056.523394] CPU: 0 PID: 14 Comm: ksoftirqd/1 Tainted: G        W O
 > > 3.10.0-g3677c61-00008-g180c060 #1
 > > [57056.532595] task: f0c8b000 ti: f0e78000 task.ti: f0e78000
 > > [57056.537991] PC is at smpboot_thread_fn+0x124/0x218
 > > [57056.542750] LR is at smpboot_thread_fn+0x11c/0x218
 > > [57056.547528] pc : [<c01931e8>]    lr : [<c01931e0>] psr: 200f0013
 > > [57056.547528] sp : f0e79f30  ip : 00000000  fp : 00000000
 > > [57056.558983] r10: 00000001  r9 : 00000000  r8 : f0e78000
 > > [57056.564192] r7 : 00000001  r6 : c1195758  r5 : f0e78000  r4 : 
f0e5fd00
 > > [57056.570701] r3 : 00000001  r2 : f0e79f20  r1 : 00000000  r0 : 
00000000
 > >
 > > This issue was always seen in the context of "ksoftirqd". It seems to
 > > be happening because of a potential race condition in __kthread_parkme
 > > where just after completing the parked completion, before the
 > > ksoftirqd task has been scheduled again, it can go into running state.
 >
 > This explanation does not make any sense. You completely fail to
 > explain the details of the race. And your patch does not make any
 > sense either, because the real issue is this:
 >
 > Task   CPU 0				CPU 1
 >
 > T1     unplug cpu1
 >        kthread_park(T2)
 >        set_bit(KTHREAD_SHOULD_PARK);
 > 	  wait_for_completion()
 > T2					parkme(X)
 > 				   	  __set_current_state(TASK_PARKED);
 > 				   	  while (test_bit(KTHREAD_SHOULD_PARK)) {
 > 				     	    if (!test_and_set_bit(KTHREAD_IS_PARKED))
 > 				              complete();
 > 			             	    schedule();
 > T1   plug cpu1
 >
 > --> premature wakeup of T2, i.e. before unpark, so T2 gets scheduled on
 >     CPU 0
 >
 > T2    __set_current_state(TASK_PARKED);
 >
 > --> Preemption by the plug thread
 >
 > T1     thread_unpark(T2)
 >          clear_bit(KTHREAD_SHOULD_PARK);
 >
 > --> Preemption by the softirq thread which breaks out of the
 >     while(test_bit(KTHREAD_SHOULD_PARK)) loop because
 >     KTHREAD_SHOULD_PARK is not longer set.
 >
 > T2   }
 >     clear_bit(KTHREAD_IS_PARKED);
 >
 > --> Now T2 happily continues to run on CPU0 which rightfully casues
 >     the BUG to trigger.
 >
 > T1
 > 	__kthread_bind(T2)
 >
 > --> Too late ....
 >
 > So the real issue is that the park/unpark code is not able to handle
 > the premature wakeup of T2 and that needs to be fixed.
 >
 > Your changelog says:
 >
 > > It seems to be happening because of a potential race condition in
 >
 > Potential is wrong to begin with. A race condition is either real and
 > explainable or it does not exist.
 >
 > > __kthread_parkme where just after completing the parked completion,
 > > before the ksoftirqd task has been scheduled again, it can go into
 > > running state.
 >
 > What exactly has this to do with state RUNNING or PARKED?
 >
 >   Nothing, the task state is completely irrelevant as the real issue
 >   is the task->*PARK flags state.
 >
 > So what is your patch solving here ?
 >
 >   You put a wait for task->state == TASK_PARKED after the
 >   wait_for_completion. What does it solve? Actually nothing. It just
 >   changes the propability of that issue. Go and apply it between any
 >   step of the above and figure out what it solves. Nothing, really.
 >
 >   Now just as an extra thought experiment assume that you have only
 >   two cpus and T1 is a SCHED_FIFO task and T2 is SCHED_OTHER ....
 >
 > Please do not misunderstand me, but "fixing" races without proper
 > understanding them is plain wrong. Providing a vague changelog which
 > does neither explain what the issue is and why the fix works is even
 > more wrong.
 >
 > The next time you hit something like this, please take the time and
 > sit down, get out the old fashioned piece of paper and a pencil and
 > draw the picture so you can actually understand what the root cause of
 > the observed issue is before sending out halfarsed duct tape fixes
 > which just paper over the root cause. If you cannot figure it out,
 > send a proper bug report.
 >
 > Thanks,
 >
 > 	tglx
 > ------------------>
 >
 > Subject: kthread: Plug park/ unplug race
 > From: Thomas Gleixner <tglx@...utronix.de>
 > Date: Thu, 26 Jun 2014 01:24:36 +0200
 >
 > The kthread park/unpark logic has the following issue:
 >
 > Task   CPU 0				CPU 1
 >
 > T1     unplug cpu1
 >        kthread_park(T2)
 >        set_bit(KTHREAD_SHOULD_PARK);
 > 	  wait_for_completion()
 > T2					parkme(X)
 > 				   	  __set_current_state(TASK_PARKED);
 > 				   	  while (test_bit(KTHREAD_SHOULD_PARK)) {
 > 				     	    if (!test_and_set_bit(KTHREAD_IS_PARKED))
 > 				              complete();
 > 			             	    schedule();
 > T1   plug cpu1
 >
 > --> premature wakeup of T2, i.e. before unpark, so T2 gets scheduled on
 >     CPU 0
 >
 > T2    __set_current_state(TASK_PARKED);
 >
 > --> Preemption by the plug thread
 >
 > T1     thread_unpark(T2)
 >          clear_bit(KTHREAD_SHOULD_PARK);
 >
 > --> Preemption by the softirq thread which breaks out of the
 >     while(test_bit(KTHREAD_SHOULD_PARK)) loop because
 >     KTHREAD_SHOULD_PARK is not longer set.
 >
 > T2   }
 >     clear_bit(KTHREAD_IS_PARKED);
 >
 > --> Now T2 happily continues to run on CPU0 which rightfully causes
 >     the BUG_ON(T2->cpu != smp_processor_id()) to trigger.
 >
 > T1
 > 	__kthread_bind(T2)
 >
 > --> Too late ....
 >
 > Reorder the logic so that the unplug code binds the thread to the
 > target cpu before clearing the KTHREAD_SHOULD_PARK bit.
 >
 > Reported-by: Subbaraman Narayanamurthy <subbaram@...eaurora.org>
 > Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
 > Cc: stable@...r.kernel.org
 >
 > ---
 >  kernel/kthread.c |   14 ++++++++++----
 >  1 file changed, 10 insertions(+), 4 deletions(-)
 >
 > Index: linux/kernel/kthread.c
 > ===================================================================
 > --- linux.orig/kernel/kthread.c
 > +++ linux/kernel/kthread.c
 > @@ -382,6 +382,15 @@ struct task_struct *kthread_create_on_cp
 >
 >  static void __kthread_unpark(struct task_struct *k, struct kthread 
*kthread)
 >  {
 > +	/*
 > +	 * Rebind the thread to the target cpu first if it is a per
 > +	 * cpu thread unconditionally because it must be bound to the
 > +	 * target cpu before it can observe the KTHREAD_SHOULD_PARK
 > +	 * bit cleared.
 > +	 */
 > +	if (test_bit(KTHREAD_IS_PER_CPU, &kthread->flags))
 > +		__kthread_bind(k, kthread->cpu, TASK_PARKED);
 > +
 >  	clear_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
 >  	/*
 >  	 * We clear the IS_PARKED bit here as we don't wait
 > @@ -389,11 +398,8 @@ static void __kthread_unpark(struct task
 >  	 * park before that happens we'd see the IS_PARKED bit
 >  	 * which might be about to be cleared.
 >  	 */
 > -	if (test_and_clear_bit(KTHREAD_IS_PARKED, &kthread->flags)) {
 > -		if (test_bit(KTHREAD_IS_PER_CPU, &kthread->flags))
 > -			__kthread_bind(k, kthread->cpu, TASK_PARKED);
 > +	if (test_and_clear_bit(KTHREAD_IS_PARKED, &kthread->flags))
 >  		wake_up_state(k, TASK_PARKED);
 > -	}
 >  }
 >
 >  /**

I just got a window to test this, and it reliably addresses the 
boot-time core onlining race that we've seen occasionally on a 2000-core 
customer system. Splendid work, Thomas.

Tested-by: Daniel J Blueman <daniel@...ascale.com>

Many thanks,
   Daniel
-- 
Daniel J Blueman
Principal Software Engineer, Numascale
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists