Re: [PATCH v4 3/3] sched: optimize migration by forcing rmb() and updating to be called once

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Nov 17, 2015 at 09:44:16AM +0900, Byungchul Park wrote:

> > So currently, set_task_cpu() is serialized by:
> > 
> >  - p->pi_lock; on wakeup
> >  - rq->lock; otherwise
> > 
> > (see the #ifdef CONFIG_LOCKDEP comment in set_task_cpu())
> 
> I already read the comment.. Then do you mean the comment above
> migrate_task_rq_fair() is wrong and should be fixed? 

Looks that way, I'm not sure we always hold pi_lock there. But I'm low
on sleep, so I could have overlooked something.

See for example move_queued_task(), we call set_task_cpu() with rq->lock
held, but no pi_lock.

> I thought the comment above migrate_task_rq_fair() is correct rather
> than CONFIG_LOCKDEP comment in set_task_cpu(), when I read it. I think
> these two comments are conflict each other a little bit, so one of
> those should be fixed.

Agreed.

> * the comment above migrate_task_rq_fair() describes it like,
> Caller SHOULD HOLD (&p->pi_lock)
> 
> * the CONFIG_LOCKDEP comment in set_task_cpu() describes it like,
> Caller SHOULD HOLD (&p->pi_lock || &rq->lock)

Indeed.

> > 
> > This means that sched_class::migrate_task() cannot indeed rely on
> > rq->lock for full serialization, however it still means that
> > task_rq_lock() will fully serialize against the thing.
> 
> Yes I also think this is true.
> 
> > 
> > By changing this, it no longer will.
> 
> ???

I meant, if you call __set_task_cpu() before
sched_class::migrate_task_rq(), in that case task_rq_lock() will no
longer fully serialize against set_task_cpu().

Because once you've called __set_task_cpu(), task_rq_lock() will acquire
the _other_ rq->lock. And we cannot rely on our rq->lock to serialize
things.