lkml.org 
[lkml]   [2006]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: -mm seems significanty slower than mainline on kernbench
Peter Williams wrote:
> Con Kolivas wrote:
>
>> On Wednesday 11 January 2006 23:24, Peter Williams wrote:
>>
>>> Martin J. Bligh wrote:
>>>
>>>> That seems broken to me ?
>>>
>>>
>>> But, yes, given that the problem goes away when the patch is removed
>>> (which we're still waiting to see) it's broken. I think the problem is
>>> probably due to the changed metric (i.e. biased load instead of simple
>>> load) causing idle_balance() to fail more often (i.e. it decides to not
>>> bother moving any tasks more often than it otherwise would) which would
>>> explain the increased idle time being seen. This means that the fix
>>> would be to review the criteria for deciding whether to move tasks in
>>> idle_balance().
>>
>>
>>
>> Look back on my implementation. The problem as I saw it was that one
>> task alone with a biased load would suddenly make a runqueue look much
>> busier than it was supposed to so I special cased the runqueue that
>> had precisely one task.
>
>
> OK. I'll look at that.

OK. I agree that this mechanism increases the chances that a queue with
only one runnable task is selected as the target for stealing tasks
from. The attached patch addresses this issue in two ways:

1. in find_busiest_group(), only groups that have at least one queue
with more than one task running are considered, and
2. in find_busiest_queue(), only queues with more than one runnable task
are considered.

As I see it, this patch is a bit iffy as it is effected by race
conditions in two ways:

1. just because there's more than one task runnable when these checks
are made there's no guarantee that this will be the case when you try to
move some of them, and
2. just because there's only one task runnable when these checks are
made it's possible that there will be more than one when you attempt the
move.

I don't think that this patch makes case 1 any worse than it already is
but case 2 could cause potential moves to be missed that otherwise
wouldn't be and I assume this is the reason why there is no similar code
in the original. Whether the increased probability of choosing queues
with only one runnable tasks changes this reasoning is up for debate.

Signed-off-by: Peter Williams <pwil3058@bigpond.com.au>

Peter
--
Peter Williams pwil3058@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
-- Ambrose Bierce
Index: MM-2.6.X/kernel/sched.c
===================================================================
--- MM-2.6.X.orig/kernel/sched.c 2006-01-12 10:44:50.000000000 +1100
+++ MM-2.6.X/kernel/sched.c 2006-01-12 10:47:01.000000000 +1100
@@ -2052,6 +2052,7 @@ find_busiest_group(struct sched_domain *
unsigned long load;
int local_group;
int i;
+ unsigned int eligible_qs = 0;

local_group = cpu_isset(this_cpu, group->cpumask);

@@ -2065,8 +2066,11 @@ find_busiest_group(struct sched_domain *
/* Bias balancing toward cpus of our domain */
if (local_group)
load = target_load(i, load_idx);
- else
+ else {
load = source_load(i, load_idx);
+ if (cpu_rq(i)->nr_running > 1)
+ ++eligible_qs;
+ }

avg_load += load;
}
@@ -2080,7 +2084,7 @@ find_busiest_group(struct sched_domain *
if (local_group) {
this_load = avg_load;
this = group;
- } else if (avg_load > max_load) {
+ } else if (avg_load > max_load && eligible_qs) {
max_load = avg_load;
busiest = group;
}
@@ -2181,8 +2185,12 @@ static runqueue_t *find_busiest_queue(st
load = source_load(i, 0);

if (load > max_load) {
- max_load = load;
- busiest = cpu_rq(i);
+ runqueue_t *tmprq = cpu_rq(i);
+
+ if (tmprq->nr_running > 1) {
+ max_load = load;
+ busiest = tmprq;
+ }
}
}
\
 
 \ /
  Last update: 2006-01-12 02:28    [W:0.209 / U:0.292 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site