В Вт, 17/02/2015 в 13:12 +0100, Peter Zijlstra пишет:
> On Tue, Feb 17, 2015 at 01:47:01PM +0300, Kirill Tkhai wrote:
> >
> > We migrate a task using TASK_ON_RQ_MIGRATING state of on_rq:
> >
> > raw_spin_lock(&old_rq->lock);
> > deactivate_task(old_rq, p, 0);
> > p->on_rq = TASK_ON_RQ_MIGRATING;
> > set_task_cpu(p, new_cpu);
> > raw_spin_unlock(&rq->lock);
> >
> > I.e.:
> >
> > write TASK_ON_RQ_MIGRATING
> > smp_wmb() (in __set_task_cpu)
> > write new_cpu
> >
> > But {,__}task_rq_lock() don't use smp_rmb(), and they may see
> > the cpu and TASK_ON_RQ_MIGRATING in opposite order. In this case
> > {,__}task_rq_lock() lock new_rq before the task is actually queued
> > on it.
>
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index fc12a1d..a42fb88 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -319,8 +319,12 @@ static struct rq *task_rq_lock(struct task_struct *p,
> > unsigned long *flags)
> > raw_spin_lock_irqsave(&p->pi_lock, *flags);
> > rq = task_rq(p);
> > raw_spin_lock(&rq->lock);
> > - if (likely(rq == task_rq(p) && !task_on_rq_migrating(p)))
> > - return rq;
> > + if (likely(rq == task_rq(p))) {
> > + /* Pairs with smp_wmb() in __set_task_cpu() */
>
> That comment really is insufficient; but aside from that:
>
> If we observe the old cpu value we've just acquired the old rq->lock and
> therefore we must observe the new cpu value and retry -- we don't care
> about the migrate value in this case.
>
> If we observe the new cpu value, we've acquired the new rq->lock and its
> ACQUIRE will pair with the WMB to ensure we see the migrate value.
Yes, I warried about new_cpu case.
So, spin_lock() implies smp_rmb(). I used to think it does not do
(I was confused by smp_mb__before_spin_lock(), but it's for STORE).
Thanks for the explanation :)
> So I think the current code is correct; albeit it could use a comment.
>
> > + smp_rmb();
> > + if (likely(!task_on_rq_migrating(p)))
> > + return rq;
> > + }
>
>
> ---
> Subject: sched: Clarify ordering between task_rq_lock() and move_queued_task()
> From: Peter Zijlstra <pet...@infradead.org>
> Date: Tue Feb 17 13:07:38 CET 2015
>
> There was a wee bit of confusion around the exact ordering here;
> clarify things.
>
> Cc: Oleg Nesterov <o...@redhat.com>
> Cc: "Paul E. McKenney" <paul...@linux.vnet.ibm.com>
> Reported-by: Kirill Tkhai <ktk...@parallels.com>
> Signed-off-by: Peter Zijlstra (Intel) <pet...@infradead.org>
> ---
> kernel/sched/core.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -341,6 +341,22 @@ static struct rq *task_rq_lock(struct ta
> raw_spin_lock_irqsave(&p->pi_lock, *flags);
> rq = task_rq(p);
> raw_spin_lock(&rq->lock);
> + /*
> + * move_queued_task() task_rq_lock()
> + *
> + * ACQUIRE (rq->lock)
> + * [S] ->on_rq = MIGRATING [L] rq = task_rq()
> + * WMB (__set_task_cpu()) ACQUIRE (rq->lock);
> + * [S] ->cpu = new_cpu [L] task_rq()
> + * [L] ->on_rq
> + * RELEASE (rq->lock)
> + *
> + * If we observe the old cpu in task_rq_lock, the acquire of
> + * the old rq->lock will fully serialize against the stores.
> + *
> + * If we observe the new cpu in task_rq_lock, the acquire will
> + * pair with the WMB to ensure we must then also see migrating.
> + */
> if (likely(rq == task_rq(p) && !task_on_rq_migrating(p)))
> return rq;
> raw_spin_unlock(&rq->lock);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
