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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1308322563.9218.184.camel@gandalf.stny.rr.com>
Date:	Fri, 17 Jun 2011 10:56:03 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Hillf Danton <dhillf@...il.com>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Mike Galbraith <efault@....de>,
	Yong Zhang <yong.zhang0@...il.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH] sched: update cpupri for runqueue when its priority
 changes

On Fri, 2011-06-17 at 20:59 +0800, Hillf Danton wrote:
> On Fri, Jun 17, 2011 at 9:50 AM, Steven Rostedt <rostedt@...dmis.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.

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.

>  #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.

-- Steve


> +       }
>  }
> 
>  /*

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/