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

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

[Hillf Danton]

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().

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---
 kernel/sched_rt.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

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);
 #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
+	}
 }

 /*

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

[Steven Rostedt]

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.

-- Steve

> 
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
>  kernel/sched_rt.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
> 
> 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);
>  #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
> +	}
>  }
> 
>  /*

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

[Hillf Danton]

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.

Hillf

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

[Steven Rostedt]

On Fri, 2011-06-17 at 20:59 +0800, Hillf Danton wrote:

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

If we throttled the rq, then the fix up should happen in the
(en,de)queue code, and not scattered around.

I'll look deeper into how throttling affects the cpupri.

-- Steve

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

[Steven Rostedt]

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.

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


> +       }
>  }
> 
>  /*

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

[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

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

[Steven Rostedt]

On Sat, 2011-06-18 at 22:54 +0800, Hillf Danton wrote:

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

The two cases are:

1) no rt task exists
2) the runqueue is throttled.

We already talked about the throttled case. The case where no rt task
exists means that the last rt task has been dequeued. When that happens,
the cpupri is updated then. I don't see any bug. There's no need to
update cpupri at this point.

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

The requeue_task_rt() does not change the priority of the CPU. It just
updates the task in its order of placement in the queue of other tasks
of the same priority.

-- Steve

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

[Hillf Danton]

On Sun, Jun 19, 2011 at 1:11 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> 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.
>
> The two cases are:
>
> 1) no rt task exists
> 2) the runqueue is throttled.
>
> We already talked about the throttled case. The case where no rt task
> exists means that the last rt task has been dequeued. When that happens,
> the cpupri is updated then. I don't see any bug. There's no need to
> update cpupri at this point.
>

In case 2) , the CPU will be held then by non-RT task, and we have to
update priority.

I understand 1) and 2) to be, there has RT tasks on runqueue but they
could not run at the moment.

If throttled RQ == no RT tasks, please drop this patch directly.

>>
>> 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.
>>
>
> The requeue_task_rt() does not change the priority of the CPU. It just
> updates the task in its order of placement in the queue of other tasks
> of the same priority.
>

You are right, updating CPU priority for requeue is unnecessary.

thanks
           Hillf

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

[Steven Rostedt]

On Sun, 2011-06-19 at 16:33 +0800, Hillf Danton wrote:
> On Sun, Jun 19, 2011 at 1:11 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> >> 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.
> >
> > The two cases are:
> >
> > 1) no rt task exists
> > 2) the runqueue is throttled.
> >
> > We already talked about the throttled case. The case where no rt task
> > exists means that the last rt task has been dequeued. When that happens,
> > the cpupri is updated then. I don't see any bug. There's no need to
> > update cpupri at this point.
> >
> 
> In case 2) , the CPU will be held then by non-RT task, and we have to
> update priority.
> 
> I understand 1) and 2) to be, there has RT tasks on runqueue but they
> could not run at the moment.
> 
> If throttled RQ == no RT tasks, please drop this patch directly.

Yes a throttled RQ means that there are no RT tasks on it. That's
because when we throttle an RQ, we dequeue the RT tasks from it. Which
means the cpupri will be updated at that time.

-- Steve