[PATCH] sched: fix conflict of schedule domain balance in RT scheduling

[PATCH] sched: fix conflict of schedule domain balance in RT scheduling

[Hillf Danton]

SD_BALANCE_WAKE and SD_WAKE_AFFINE are defined to be mutually exclusive, and
are checked in select_task_rq_rt() and find_lowest_rq() respectively.

The target cpu that is elected only from schedule domains of SD_WAKE_AFFINE if
CPU topology concerned, conflicts with the SD_BALANCE_WAKE requirement.

And checking for cache-hot is also added to confirm the comment there.

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

diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
index 88725c9..87c44e6 100644
--- a/kernel/sched_rt.c
+++ b/kernel/sched_rt.c
@@ -1005,7 +1005,7 @@ select_task_rq_rt(struct task_struct *p, int
sd_flag, int flags)
 	struct rq *rq;
 	int cpu;

-	if (sd_flag != SD_BALANCE_WAKE)
+	if (!(sd_flag & SD_BALANCE_WAKE))
 		return smp_processor_id();

 	cpu = task_cpu(p);
@@ -1253,7 +1253,8 @@ static int find_lowest_rq(struct task_struct *task)
 	 * We prioritize the last cpu that the task executed on since
 	 * it is most likely cache-hot in that location.
 	 */
-	if (cpumask_test_cpu(cpu, lowest_mask))
+	if (cpumask_test_cpu(cpu, lowest_mask) &&
+	    task_hot(task, task_rq(task)->clock_task, NULL))
 		return cpu;

 	/*
@@ -1265,7 +1266,7 @@ static int find_lowest_rq(struct task_struct *task)

 	rcu_read_lock();
 	for_each_domain(cpu, sd) {
-		if (sd->flags & SD_WAKE_AFFINE) {
+		if (sd->flags & (SD_WAKE_AFFINE | SD_BALANCE_WAKE)) {
 			int best_cpu;

 			/*

Re: [PATCH] sched: fix conflict of schedule domain balance in RT scheduling

[Peter Zijlstra]

On Wed, 2011-06-01 at 21:58 +0800, Hillf Danton wrote:
> SD_BALANCE_WAKE and SD_WAKE_AFFINE are defined to be mutually
> exclusive

Uhm, no they're not.. both affect placement of a woken task but they're
complementary. WAKE_AFFINE is a check to see if it makes sense to run
the woken task on the same cpu as the wakee, BALANCE_WAKE does a full
load-balance pass, in case its combined with WAKE_AFFINE it does so in
case that test is negative.

Re: [PATCH] sched: fix conflict of schedule domain balance in RT scheduling

[Steven Rostedt]

On Wed, 2011-06-01 at 21:58 +0800, Hillf Danton wrote:

> And checking for cache-hot is also added to confirm the comment there.

Please read that comment.

>  	cpu = task_cpu(p);
> @@ -1253,7 +1253,8 @@ static int find_lowest_rq(struct task_struct *task)
>  	 * We prioritize the last cpu that the task executed on since
>  	 * it is most likely cache-hot in that location.
>  	 */
> -	if (cpumask_test_cpu(cpu, lowest_mask))
> +	if (cpumask_test_cpu(cpu, lowest_mask) &&
> +	    task_hot(task, task_rq(task)->clock_task, NULL))
>  		return cpu;

What task_hot() checks for and what we are assuming are two different
things. In fact, we can disable task_hot() so it always fails. That's
not what we want.

If the task happens to have ran on a CPU that is in the lowest_mask, we
want that CPU. Time may not matter. If we know a task ran on a
particular CPU last, we want to run it there if possible. It may still
have cache lines for it, even if it has been a long time since it last
ran.

-- Steve

Re: [PATCH] sched: fix conflict of schedule domain balance in RT scheduling

[Hillf Danton]

On Fri, Jun 3, 2011 at 10:19 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 2011-06-01 at 21:58 +0800, Hillf Danton wrote:
>
>> And checking for cache-hot is also added to confirm the comment there.
>
> Please read that comment.
>
>>       cpu = task_cpu(p);
>> @@ -1253,7 +1253,8 @@ static int find_lowest_rq(struct task_struct *task)
>>        * We prioritize the last cpu that the task executed on since
>>        * it is most likely cache-hot in that location.
>>        */
>> -     if (cpumask_test_cpu(cpu, lowest_mask))
>> +     if (cpumask_test_cpu(cpu, lowest_mask) &&
>> +         task_hot(task, task_rq(task)->clock_task, NULL))
>>               return cpu;
>
> What task_hot() checks for and what we are assuming are two different
> things. In fact, we can disable task_hot() so it always fails. That's
> not what we want.
>
> If the task happens to have ran on a CPU that is in the lowest_mask, we
> want that CPU. Time may not matter. If we know a task ran on a
> particular CPU last, we want to run it there if possible. It may still
> have cache lines for it, even if it has been a long time since it last
> ran.
>
Hi Steve

I note the comment related to cache-hot, it is fine for woken task even though
cache-hot is considered to be nop for RT task, but what dose it help pushing
RT tasks waiting on RQ?

thanks
           Hillf

Re: [PATCH] sched: fix conflict of schedule domain balance in RT scheduling

[Hillf Danton]

On Thu, Jun 2, 2011 at 7:56 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, 2011-06-01 at 21:58 +0800, Hillf Danton wrote:
>> SD_BALANCE_WAKE and SD_WAKE_AFFINE are defined to be mutually
>> exclusive
>
> Uhm, no they're not.. both affect placement of a woken task but they're
> complementary. WAKE_AFFINE is a check to see if it makes sense to run
> the woken task on the same cpu as the wakee, BALANCE_WAKE does a full
> load-balance pass,

Though I dont understand the following, my question is why WAKE_AFFINE is
eligible for pushing RT tasks off the RQ on which they are waiting.

thanks
           Hillf

>in case its combined with WAKE_AFFINE it does so in
> case that test is negative.
>

Re: [PATCH] sched: fix conflict of schedule domain balance in RT scheduling

[Steven Rostedt]

On Fri, 2011-06-03 at 22:30 +0800, Hillf Danton wrote:

> I note the comment related to cache-hot, it is fine for woken task even though
> cache-hot is considered to be nop for RT task,

I would not say cache-hot is a nop for RT tasks. Although RT tasks
should not depend on cache being hot, we don't want to go out of our way
to hurt RT tasks.


>  but what dose it help pushing
> RT tasks waiting on RQ?

What the lowest_mask returns is a mask of all cpus with the lowest
priority. Due to the double_lock_balance() we could have unlocked the rq
lock, and things could have changed. Thus, the current CPU could have
lowered its priority. Unlikely to be the case, but can happen.

If the lowest_mask has the task's CPU set, why push this RT task
somewhere else? Most likely the current CPU will not be set, but lets
not punish the RT task if it is.

-- Steve

Re: [PATCH] sched: fix conflict of schedule domain balance in RT scheduling

[Hillf Danton]

On Fri, Jun 3, 2011 at 10:52 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 2011-06-03 at 22:30 +0800, Hillf Danton wrote:
>
>> I note the comment related to cache-hot, it is fine for woken task even though
>> cache-hot is considered to be nop for RT task,
>
> I would not say cache-hot is a nop for RT tasks. Although RT tasks
> should not depend on cache being hot, we don't want to go out of our way
> to hurt RT tasks.
>
>
>>  but what dose it help pushing
>> RT tasks waiting on RQ?
>
> What the lowest_mask returns is a mask of all cpus with the lowest
> priority. Due to the double_lock_balance() we could have unlocked the rq
> lock, and things could have changed. Thus, the current CPU could have
> lowered its priority. Unlikely to be the case, but can happen.
>

I am not sure task switch could happen due to RQ unlock.
If no switch, how priority is lowered?

thanks
            Hillf


> If the lowest_mask has the task's CPU set, why push this RT task
> somewhere else? Most likely the current CPU will not be set, but lets
> not punish the RT task if it is.
>
> -- Steve
>
>
>
>