Messages in this thread Patch in this message | | | Date | Sat, 18 Jun 2011 22:54:42 +0800 | Subject | Re: [PATCH] sched: update cpupri for runqueue when its priority changes | From | Hillf Danton <> |
| |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |