small migration thread fix

small migration thread fix

[Erich Focht]

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

Hi,

the small patch fixes a potential problem in the migration thread for
the case that the first CPU in the cpus_allowed mask of a process is
offline. Please consider applying it to your trees.

Thanks!
Regards,
Erich


[-- Attachment #2: migration-fix-2.5.55.patch --]
[-- Type: text/x-diff, Size: 498 bytes --]

diff -urNp linux-2.5.55/kernel/sched.c linux-2.5.55-fix/kernel/sched.c
--- linux-2.5.55/kernel/sched.c	2003-01-09 05:04:22.000000000 +0100
+++ linux-2.5.55-fix/kernel/sched.c	2003-01-10 13:37:40.000000000 +0100
@@ -2108,7 +2108,7 @@ static int migration_thread(void * data)
 		spin_unlock_irqrestore(&rq->lock, flags);
 
 		p = req->task;
-		cpu_dest = __ffs(p->cpus_allowed);
+		cpu_dest = __ffs(p->cpus_allowed & cpu_online_map);
 		rq_dest = cpu_rq(cpu_dest);
 repeat:
 		cpu_src = task_cpu(p);

Re: small migration thread fix

[William Lee Irwin III]

On Fri, Jan 10, 2003 at 01:46:03PM +0100, Erich Focht wrote:
> the small patch fixes a potential problem in the migration thread for
> the case that the first CPU in the cpus_allowed mask of a process is
> offline. Please consider applying it to your trees.

I'm not mingo, but I can say this looks sane. My only question is
whether there are more codepaths that need this kind of check, for
instance, what happens if someone does set_cpus_allowed() to a cpumask
with !(task->cpumask & cpu_online_map) ?


Thanks,
Bill

Re: small migration thread fix

[Erich Focht]

On Friday 10 January 2003 14:11, William Lee Irwin III wrote:
> I'm not mingo, but I can say this looks sane. My only question is
> whether there are more codepaths that need this kind of check, for
> instance, what happens if someone does set_cpus_allowed() to a cpumask
> with !(task->cpumask & cpu_online_map) ?

The piece of code below was intended for that. I agree with Rusty's
comment, BUG() is too strong for that case. 

#if 0 /* FIXME: Grab cpu_lock, return error on this case. --RR */
	new_mask &= cpu_online_map;
	if (!new_mask)
		BUG();
#endif

Anyhow, changing the new_mask in this way is BAD, because the masks
are inherited. So when more CPUs come online, they remain excluded
from the mask of the process and it's children.

The fix suggested in the comments still has to be done...

Regards,
Erich

Re: small migration thread fix

[William Lee Irwin III]

On Friday 10 January 2003 14:11, William Lee Irwin III wrote:
>> I'm not mingo, but I can say this looks sane. My only question is
>> whether there are more codepaths that need this kind of check, for
>> instance, what happens if someone does set_cpus_allowed() to a cpumask
>> with !(task->cpumask & cpu_online_map) ?

On Fri, Jan 10, 2003 at 03:29:33PM +0100, Erich Focht wrote:
> The piece of code below was intended for that. I agree with Rusty's
> comment, BUG() is too strong for that case. 
> #if 0 /* FIXME: Grab cpu_lock, return error on this case. --RR */
> 	new_mask &= cpu_online_map;
> 	if (!new_mask)
> 		BUG();
> #endif
> Anyhow, changing the new_mask in this way is BAD, because the masks
> are inherited. So when more CPUs come online, they remain excluded
> from the mask of the process and it's children.
> The fix suggested in the comments still has to be done...

I don't have much to add but another ack and a "hmm, maybe something
could be done". My prior comments stand. I'd be very much obliged if
you provide a fix for the set_cpus_allowed() issue. I very much rely
upon you now to provide scheduler fixes and optimizations for large
scale and/or NUMA machines these days.


Thanks,
Bill