[PATCH 2.6.12-rc5-mm2] [sched] add allowed CPUs check into find_idlest_group()

[PATCH 2.6.12-rc5-mm2] [sched] add allowed CPUs check into find_idlest_group()

[M.Baris Demiray]

[-- Attachment #1: Type: text/plain, Size: 1744 bytes --]


Hello,
following patch adds check for allowed CPUs into
sched.c:find_idlest_group() -as told in comment line that had
removed-. But, I have several questions about that comment.

Firstly, I've understood it as "Check whether process p is allowed to
run on each CPU of to-be-found idlest group"; is that right?

If so, isn't it more appropriate to do check in find_idlest_cpu()?
Because, we're only interested in CPUs that are in idlest group
but doing a check in find_idlest_group() also checks for CPUs
that are not in idlest group (since we're traversing all the groups
in given domain). Checking this after finding the idlest group
(in find_idlest_cpu() with ordinary call order as in
sched_balance_self()) will save us from extra overhead.

Although I've questions in my mind, I'm sending a patch following
that comment. Any explanation and comment on patch will be
appreciated.

Regards.

Signed-off-by: M.Baris Demiray <baris@labristeknoloji.com>

--- linux-2.6.12-rc5-mm2/kernel/sched.c.orig	2005-06-05 
12:31:04.000000000 +0000
+++ linux-2.6.12-rc5-mm2/kernel/sched.c	2005-06-05 16:49:49.000000000 +0000
@@ -1040,7 +1040,12 @@
  		int i;

  		local_group = cpu_isset(this_cpu, group->cpumask);
-		/* XXX: put a cpus allowed check */
+
+		/* Check whether all CPUs in the group is allowed to run on */
+		for_each_cpu_mask(i, group->cpumask) {
+			if (!cpu_isset(i, p->cpus_allowed))
+				continue;
+		}

  		/* Tally up the load of all CPUs in the group */
  		avg_load = 0;


-- 
"You have to understand, most of these people are not ready to be
unplugged. And many of them are no inert, so hopelessly dependent
on the system, that they will fight to protect it."
                                                          Morpheus




[-- Attachment #2: baris.vcf --]
[-- Type: text/x-vcard, Size: 342 bytes --]

begin:vcard
fn:M.Baris Demiray
n:Demiray;M.Baris
org:Labris Teknoloji
adr:;;Teknokent Silikon Bina No:24 ODTU;Ankara;;06531;Turkey
email;internet:baris@labristeknoloji.com
title:Yazilim Gelistirme Uzmani
tel;work:+903122101490
tel;fax:+903122101492
x-mozilla-html:FALSE
url:http://www.labristeknoloji.com
version:2.1
end:vcard

Re: [PATCH 2.6.12-rc5-mm2] [sched] add allowed CPUs check into find_idlest_group()

[Nick Piggin]

M.Baris Demiray wrote:
> 
> Hello,
> following patch adds check for allowed CPUs into
> sched.c:find_idlest_group() -as told in comment line that had
> removed-. But, I have several questions about that comment.
> 
> Firstly, I've understood it as "Check whether process p is allowed to
> run on each CPU of to-be-found idlest group"; is that right?
> 

Close.

Probably it would be better to take the intersection of
(group->cpumask, p->cpus_allowed), and skip the group if
the intersection is empty.

In addition to that, do a patch for find_idlest_cpu that
skips cpus that aren't allowed. You needn't do the cpumask
check each time round the loop, again just take the
intersection of the group->cpumask and p->cpus_allowed, and
loop over that.

Wanna do a patch for that?


> If so, isn't it more appropriate to do check in find_idlest_cpu()?
> Because, we're only interested in CPUs that are in idlest group
> but doing a check in find_idlest_group() also checks for CPUs
> that are not in idlest group (since we're traversing all the groups
> in given domain). Checking this after finding the idlest group
> (in find_idlest_cpu() with ordinary call order as in
> sched_balance_self()) will save us from extra overhead.
> 
> Although I've questions in my mind, I'm sending a patch following
> that comment. Any explanation and comment on patch will be
> appreciated.
> 

I don't think it does anything ;)

> Regards.
> 
> Signed-off-by: M.Baris Demiray <baris@labristeknoloji.com>
> 
> --- linux-2.6.12-rc5-mm2/kernel/sched.c.orig    2005-06-05 
> 12:31:04.000000000 +0000
> +++ linux-2.6.12-rc5-mm2/kernel/sched.c    2005-06-05 16:49:49.000000000 
> +0000
> @@ -1040,7 +1040,12 @@
>          int i;
> 
>          local_group = cpu_isset(this_cpu, group->cpumask);
> -        /* XXX: put a cpus allowed check */
> +
> +        /* Check whether all CPUs in the group is allowed to run on */
> +        for_each_cpu_mask(i, group->cpumask) {
> +            if (!cpu_isset(i, p->cpus_allowed))
> +                continue;
> +        }
> 
>          /* Tally up the load of all CPUs in the group */
>          avg_load = 0;
> 
> 


-- 
SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH 2.6.12-rc5-mm2] [sched] add allowed CPUs check into find_idlest_group()

[M.Baris Demiray]

[-- Attachment #1: Type: text/plain, Size: 1471 bytes --]


Hello Nick,

Nick Piggin wrote:
> M.Baris Demiray wrote:
> 
> [...]
> Close.
> 
> Probably it would be better to take the intersection of
> (group->cpumask, p->cpus_allowed), and skip the group if
> the intersection is empty.

But, isn't it required for us to be allowed to run on _every_
CPU in that group. If we take intersection and continue if
that's not empty, then there could be CPUs in group that are
not allowed. Since any CPU then can be the idlest in that
group we can be assigned to a CPU that is not allowed.
Missing something?

> In addition to that, do a patch for find_idlest_cpu that
> skips cpus that aren't allowed. You needn't do the cpumask
> check each time round the loop, again just take the
> intersection of the group->cpumask and p->cpus_allowed, and
> loop over that.

Will do it.

> Wanna do a patch for that?

Sure. But I'll wait EOT and do read something more to fill
the gaps.

 > [...]
> 
> I don't think it does anything ;)

LOL. Hope next one'll do.

Meanwhile, what is the problem with that patch? Not traversing
the CPUs correctly or continue;ing is wrong?

	for_each_cpu_mask(i, group->cpumask) {
		if (!cpu_isset(i, p->cpus_allowed))
			continue;
	}

Thanks for comments.

> 

-- 
"You have to understand, most of these people are not ready to be
unplugged. And many of them are no inert, so hopelessly dependent
on the system, that they will fight to protect it."
                                                         Morpheus

[-- Attachment #2: baris.vcf --]
[-- Type: text/x-vcard, Size: 342 bytes --]

begin:vcard
fn:M.Baris Demiray
n:Demiray;M.Baris
org:Labris Teknoloji
adr:;;Teknokent Silikon Bina No:24 ODTU;Ankara;;06531;Turkey
email;internet:baris@labristeknoloji.com
title:Yazilim Gelistirme Uzmani
tel;work:+903122101490
tel;fax:+903122101492
x-mozilla-html:FALSE
url:http://www.labristeknoloji.com
version:2.1
end:vcard

Re: [PATCH 2.6.12-rc5-mm2] [sched] add allowed CPUs check into find_idlest_group()

[Nick Piggin]

M.Baris Demiray wrote:
> 
> Hello Nick,
> 
> Nick Piggin wrote:
> 
>> M.Baris Demiray wrote:
>>
>> [...]
>> Close.
>>
>> Probably it would be better to take the intersection of
>> (group->cpumask, p->cpus_allowed), and skip the group if
>> the intersection is empty.
> 
> 
> But, isn't it required for us to be allowed to run on _every_
> CPU in that group. If we take intersection and continue if
> that's not empty, then there could be CPUs in group that are
> not allowed. Since any CPU then can be the idlest in that
> group we can be assigned to a CPU that is not allowed.
> Missing something?
> 

That should be OK. We basically aren't too interested in
exactly which CPU it should go to, but just that it should
go to that group.

If the group is determined to be the idlest, and there is
a CPU that can run the task, then that's all we need.

>  > [...]
> 
>>
>> I don't think it does anything ;)
> 
> 
> LOL. Hope next one'll do.
> 
> Meanwhile, what is the problem with that patch? Not traversing
> the CPUs correctly or continue;ing is wrong?
> 
>     for_each_cpu_mask(i, group->cpumask) {
>         if (!cpu_isset(i, p->cpus_allowed))
>             continue;
>     }
> 

In Linux, the for_* macros actually *are* for loops. So that is
that loop that your continue continues, and seeing as it is at
the end of that for loop, it does nothing.

Thanks,
Nick

-- 
SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH 2.6.12-rc5-mm2] [sched] add allowed CPUs check into find_idlest_group()

[M.Baris Demiray]

[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]


Nick Piggin wrote:
> M.Baris Demiray wrote:
 > [...]
>>
>> But, isn't it required for us to be allowed to run on _every_
>> CPU in that group. If we take intersection and continue if
>> that's not empty, then there could be CPUs in group that are
>> not allowed. Since any CPU then can be the idlest in that
>> group we can be assigned to a CPU that is not allowed.
>> Missing something?
>>
> 
> That should be OK. We basically aren't too interested in
> exactly which CPU it should go to, but just that it should
> go to that group.
> 
> If the group is determined to be the idlest, and there is
> a CPU that can run the task, then that's all we need.

OK. And also I missed the point that you requested a second check
in find_idlest_cpu() which'll prevent an assignment to an unallowed
CPU. That will solve the problem.

 > [...]
>>
>> Meanwhile, what is the problem with that patch? Not traversing
>> the CPUs correctly or continue;ing is wrong?
>>
>>     for_each_cpu_mask(i, group->cpumask) {
>>         if (!cpu_isset(i, p->cpus_allowed))
>>             continue;
>>     }
>>
> 
> In Linux, the for_* macros actually *are* for loops. So that is
> that loop that your continue continues, and seeing as it is at
> the end of that for loop, it does nothing.

Argh. I'll look at these.

Thanks Nick.

> Thanks,
> Nick
> 

-- 
"You have to understand, most of these people are not ready to be
unplugged. And many of them are no inert, so hopelessly dependent
on the system, that they will fight to protect it."
                                                         Morpheus

[-- Attachment #2: baris.vcf --]
[-- Type: text/x-vcard, Size: 342 bytes --]

begin:vcard
fn:M.Baris Demiray
n:Demiray;M.Baris
org:Labris Teknoloji
adr:;;Teknokent Silikon Bina No:24 ODTU;Ankara;;06531;Turkey
email;internet:baris@labristeknoloji.com
title:Yazilim Gelistirme Uzmani
tel;work:+903122101490
tel;fax:+903122101492
x-mozilla-html:FALSE
url:http://www.labristeknoloji.com
version:2.1
end:vcard