Re: [PATCH v4 5/5] sched: add two functions for att(det)aching a task to(from) a cfs_rq

[Byungchul Park]

On Tue, Sep 01, 2015 at 05:03:43PM +0200, Peter Zijlstra wrote:
> On Tue, Sep 01, 2015 at 09:28:49AM +0900, Byungchul Park wrote:
> 
> > check the condition "!(flags & DEQUEUE_SLEEP)" for doing normalizing in
> > dequeue_entity(). i think you have to keep my original comment, or
> > modify your comment to something like below.
> > 
> > before - If it's !queued, sleeping tasks have a normalized vruntime,
> > after - If it's !queued, sleeping tasks have a non-normalize vruntime,
> > 
> > but.. i think it would be better that you keep my original comment..
> 
> The comment we can talk about later, but I think the condition:
> 
> > > -	if (p->state == TASK_RUNNING)
> > > +	if (!p->se.on_rq)
> 
> is important now. Both are broken in different ways.
> 
> 	p->state == TASK_RUNNING
> 
> is broken in this scenario:
> 
> 	CPU0					CPU1
> 
> 	set_current_state(TASK_UNINTERRUPTIBLE);

here, the target task has not been dequeued yet (the task will be 
dequeued within schedule()). so, queued is true when sched_move_task()
is called. sched_move_task()->dequeue_task(flag=0) will normalize as
we expect. that is anyway no problem. however, i also think the 
condition here can make us confused, too.

i think "p->state == TASK_RUNNING" condition can work anyway, which
already exists in original code. the condition is only for checking
if the task is being migrated or in sleep. how about this appoach
below to make code more readable?

> 
> 						sched_move_task()
> 						  task_move_group_fair()
> 						    vruntime_normalized() == true
> 	if (!cond)
> 		schedule();
> 	__set_current_state(TASK_RUNNING);
> 
> 
> Now the proposed replacement:
> 
> 	!p->se.on_rq
> 
> is equally broken, because (as you point out) clearing it isn't
> conditional on DEQUEUE_SLEEP.
> 

i think we must take a task's on_rq into account instead of se's on_rq,
because current code set se's on_rq to 0 even when migrating a task. but
task's on_rq would be set to TASK_ON_RQ_MIGRATING when migrating the
task. there it would be ok if we use task's on_rq to check if it has
a normailzed vruntime or not like below.

---