Re: [PATCH] sched: update cpupri for runqueue when its priority changes

[Hillf Danton <dhillf@gmail.com>]

On Fri, Jun 17, 2011 at 10:56 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 2011-06-17 at 20:59 +0800, Hillf Danton wrote:
>> On Fri, Jun 17, 2011 at 9:50 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> > On Sun, 2011-06-05 at 17:54 +0800, Hillf Danton wrote:
>> >> When the priority of runqueue changes, lower or higer, the info of cpupri
>> >> should be updated, in cases such as pick_next_task_rt() and switched_to_rt().
>> >
>> > Why?
>> >
>> > We do the calculation on queuing and dequeuing the task, we only care
>> > about the highest priority task that is on the queue, not what is
>> > actually running.
>> >
>>
>> It is to capture the changes in CPU priority caused by re-queued task and
>> throttled RQ.
>

Hi Steven,

Thanks for reviewing the patch.

> OK, I talked a little with Peter about this. We don't throttle an rq, we
> throttle a group. A group consists of tasks, not rqs. When a group is
> throttled, we do not migrate tasks, so the cpupri is not a issue here.
>
> For non throttled groups, tasks are enqueued and when they are, the
> cpupri is updated. We *only* care about tasks that are enqueued.
>
> Thus, lets look again at your patch:
>
>
>> diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
>> index 08e9374..9508168 100644
>> --- a/kernel/sched_rt.c
>> +++ b/kernel/sched_rt.c
>> @@ -1158,6 +1158,8 @@ static struct task_struct
>> *pick_next_task_rt(struct rq *rq)
>>          * lock again later if there is no need to push
>>          */
>>         rq->post_schedule = has_pushable_tasks(rq);
>> +
>> +       cpupri_set(&rq->rd->cpupri, rq->cpu, p == NULL ? MAX_RT_PRIO : p->prio);
>
> In pick_next_task_rt(), p is the highes prio that is queued. Thus,
> cpupri is already set to p->prio. If p is NULL, then there is no rt
> tasks queued on this rq, and cpupri is set to MAX_RT_PRIO. Your patch
> here does not change anything.
>

There are two cases that NULL is returned in _pick_next_task_rt(), it is the
second case, after checking rt_rq->rt_nr_running, that is captured, and if
NULL is returned in the second case, the CPU priority does change.

In another scenario that has little with {en, de}queue, as shown by
requeue_task_rt(), the CPU priority will change if other RT tasks exist.


>>  #endif
>>
>>         return p;
>> @@ -1673,6 +1675,8 @@ static void switched_to_rt(struct rq *rq, struct
>> task_struct *p)
>>  {
>>         int check_resched = 1;
>>
>> +       if (!p->on_rq)
>> +               return;
>>         /*
>>          * If we are already running, then there's nothing
>>          * that needs to be done. But if we are not running
>> @@ -1680,7 +1684,7 @@ static void switched_to_rt(struct rq *rq, struct
>> task_struct *p)
>>          * If that current running task is also an RT task
>>          * then see if we can move to another run queue.
>>          */
>> -       if (p->on_rq && rq->curr != p) {
>> +       if (rq->curr != p) {
>>  #ifdef CONFIG_SMP
>>                 if (rq->rt.overloaded && push_rt_task(rq) &&
>>                     /* Don't resched if we changed runqueues */
>> @@ -1690,6 +1694,11 @@ static void switched_to_rt(struct rq *rq,
>> struct task_struct *p)
>>                 if (check_resched && p->prio < rq->curr->prio)
>>                         resched_task(rq->curr);
>>         }
>> +       else {
>> +#ifdef CONFIG_SMP
>> +               cpupri_set(&rq->rd->cpupri, rq->cpu, p->prio);
>> +#endif
>
> switched_to_rt() is called from sched.c's check_class_changed(), which
> is always called after enqueuing the task if p->on_rq was set. Thus, if
> this is running and is the highest priority task, cpupri would have this
> bit set too. Again, your patch does nothing but add more overhead.
>

The patch is overhead at this hunk.

Thanks
	Hillf