Re: ckrm-e17

[Shailabh Nagar <nagar@watson.ibm.com>]

Peter Williams wrote:
> Shailabh Nagar wrote:
> 
> 
> At line 3887 of cpu.ckrm-e17.v10.patch you add the line:
> 
>         set_task_cpu(p,this_cpu);
> 
> to the middle of the function wake_up_new_task() resulting in the 
> following code:
> 
>     } else {
>         this_rq = cpu_rq(this_cpu);
> 
>         /*
>          * Not the local CPU - must adjust timestamp. This should
>          * get optimised away in the !CONFIG_SMP case.
>          */
>         p->sdu.ingosched.timestamp = (p->sdu.ingosched.timestamp - 
> this_rq->timestamp_last_tick)
>                     + rq->timestamp_last_tick;
>         set_task_cpu(p,this_cpu);
>         __activate_task(p, rq);
>         if (TASK_PREEMPTS_CURR(p, rq))
>             resched_task(rq->curr);
> 
>         schedstat_inc(rq, wunt_moved);
>         /*
>          * Parent and child are on different CPUs, now get the
>          * parent runqueue to update the parent's 
> ->sdu.ingosched.sleep_avg:
>          */
>         task_rq_unlock(rq, &flags);
>         this_rq = task_rq_lock(current, &flags);
>     }
> 
> where "rq" has been set by the return value of "task_rq_lock(p, 
> &flags)", and the test "(cpu == this_cpu)" has failed with "cpu" set to 
> "task_cpu(p)".  The result of this when the CKRM CPU code is not 
> configured into the build is that "p" will be queued on a runqueue that 
> is not in agreement with "p->thread_info->cpu" which in turn will lead 
> to future use of "task_rq_lock()" locking the wrong run queue and 
> eventually triggering some form of race condition.
> 
> If CKRM CPU is configured into the build the results are less drastic as 
> they only result in "nr_running" being incremented for the wrong run 
> queue.  However, even this will have adverse scheduling effects as it 
> will probably confuse the load balancing code.  Another potentially 
> confusing thing with this code (when CKRM CPU is configured in) is that 
> __activate_task() does NOT queue "p" on "rq" but on the queue found by 
> the call "get_task_lrq(p)".
> 
> The recommended fix for this problem would be to withdraw the:
> 
>         set_task_cpu(p,this_cpu);
> 
> Peter

Thanks for finding that out. Will confirm and fix in the next release.

> PS I reported this to the ckrm-tech list 5 days ago but it was ignored.

Other project priorities prevented us from responding sooner. There's no 
call to jump to conclusions.

-- Shailabh