Messages in this thread Patch in this message | | | Date | Fri, 26 Aug 2011 14:01:07 +0800 | From | Yong Zhang <> | Subject | Re: [kernel.org users] [KORG] Panics on master backend |
| |
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
| |