Re: [kernel.org users] [KORG] Panics on master backend

[Yong Zhang <yong.zhang0@gmail.com>]

On Thu, Aug 25, 2011 at 03:54:29PM +0200, Oleg Nesterov wrote:
> On 08/25, Peter Zijlstra wrote:
> >
> > On Wed, 2011-08-24 at 18:08 +0200, Oleg Nesterov wrote:
> > > >
> > > >  static void ttwu_queue(struct task_struct *p, int cpu)
> > > > @@ -2705,7 +2703,6 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
> > > >  	 * this task as prev, wait until its done referencing the task.
> > > >  	 */
> > > >  	while (p->on_cpu) {
> > > > -#ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW
> > > >  		/*
> > > >  		 * In case the architecture enables interrupts in
> > > >  		 * context_switch(), we cannot busy wait, since that
> > > > @@ -2713,11 +2710,11 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
> > > >  		 * tries to wake up @prev. So bail and do a complete
> > > >  		 * remote wakeup.
> > > >  		 */
> > > > -		if (ttwu_activate_remote(p, wake_flags))
> > > > +		if (cpu == smp_processor_id() &&
> > >
> > > I think this needs "task_cpu(p) == smp_processor_id()". We can't trust
> > > "cpu", task_cpu() was called before ->on_rq check.
> >
> > Isn't us holding ->pi_lock sufficient to stabilize task_cpu()? If its a
> > running task the initial ->state check would have failed,
> 
> Of course it is not TASK_RUNNING, but it can be running or not.

Yup. Before we go beyond ttwu_remote() in ttwu(), 'cpu' is not safe.
For example, wait_event() could be preempted in between.

But after we go beyond ttwu_remote(), ->pi_lock will stabilize it.

So after we take Oleg's suggestion("task_cpu(p) == smp_processor_id()"),
things we left is just how to account stat correctly.

IMHO, we could get cpu in ttwu_remote() to prevent the side effect of
pull_task().

something like below?

Thanks,
Yong

---
diff --git a/kernel/sched.c b/kernel/sched.c
index ccacdbd..4a1d05d 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -2540,7 +2540,7 @@ ttwu_do_activate(struct rq *rq, struct task_struct *p, int wake_flags)
  * since all we need to do is flip p->state to TASK_RUNNING, since
  * the task is still ->on_rq.
  */
-static int ttwu_remote(struct task_struct *p, int wake_flags)
+static int ttwu_remote(struct task_struct *p, int wake_flags, int *cpu)
 {
 	struct rq *rq;
 	int ret = 0;
@@ -2548,6 +2548,7 @@ static int ttwu_remote(struct task_struct *p, int wake_flags)
 	rq = __task_rq_lock(p);
 	if (p->on_rq) {
 		ttwu_do_wakeup(rq, p, wake_flags);
+		*cpu = task_cpu(p);
 		ret = 1;
 	}
 	__task_rq_unlock(rq);
@@ -2696,7 +2697,12 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
 	success = 1; /* we're going to change ->state */
 	cpu = task_cpu(p);
 
-	if (p->on_rq && ttwu_remote(p, wake_flags))
+	/*
+	 * read cpu for another time if ttwu_remote() success,
+	 * just to prevent task migration in between, otherwise
+	 * we maybe account stat incorrectly.
+	 */
+	if (p->on_rq && ttwu_remote(p, wake_flags, &cpu))
 		goto stat;
 
 #ifdef CONFIG_SMP