[PATCH] sched: SCHED_RESET_ON_FORK to recalculate load weights

[PATCH] sched: SCHED_RESET_ON_FORK to recalculate load weights

[Linus Walleij]

I noticed the following phonomena: a process elevated to SCHED_RR
forks with SCHED_RESET_ON_FORK set, and the child is indeed
SCHED_OTHER, and the niceval is indeed reset to 0. However the
weight is still something enormous like 177522.

So we always need to call set_load_weight(), not just if the
niceval was changed, because the scheduler gives
SCHED_RR/SCHED_FIFO processes very high weights.

Cc: Lennart Poettering <lennart@poettering.net>
Cc: stable@kernel.org
Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
This patch solves the problem for me, albeit on kernel 2.6.34 but
it seems to me like the bug is as relevant in the HEAD.

If I'm not mistaken the weights is what is actually controlling
the CPU alottment, so this nasty bug makes the schuduling class
and niceval *look* correct, but if you inspect the actual weight
it's totally wrong.

I found this when playing around with RTKit patches and showing
processes in CGFreak where I display processes in piecharts,
using the actual weights.

If this fix is correct it should probably go into the stable
series as well.
---
 kernel/sched.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index 91c19db..9ed647f 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -2537,9 +2537,10 @@ void sched_fork(struct task_struct *p, int clone_flags)
 		if (PRIO_TO_NICE(p->static_prio) < 0) {
 			p->static_prio = NICE_TO_PRIO(0);
 			p->normal_prio = p->static_prio;
-			set_load_weight(p);
 		}
 
+		set_load_weight(p);
+
 		/*
 		 * We don't need the reset flag anymore after the fork. It has
 		 * fulfilled its duty:
-- 
1.7.2.3

Re: [PATCH] sched: SCHED_RESET_ON_FORK to recalculate load weights

[Peter Zijlstra]

On Sat, 2010-10-09 at 10:16 +0200, Linus Walleij wrote:
> 
> So we always need to call set_load_weight(), not just if the
> niceval was changed, because the scheduler gives
> SCHED_RR/SCHED_FIFO processes very high weights. 

SCHED_RR/FIFO never uses that weight, we should remove all that cruft..

Re: [PATCH] sched: SCHED_RESET_ON_FORK to recalculate load weights

[Linus Walleij]

2010/10/9 Peter Zijlstra <peterz@infradead.org>:

> On Sat, 2010-10-09 at 10:16 +0200, Linus Walleij wrote:
>>
>> So we always need to call set_load_weight(), not just if the
>> niceval was changed, because the scheduler gives
>> SCHED_RR/SCHED_FIFO processes very high weights.
>
> SCHED_RR/FIFO never uses that weight, we should remove all that cruft..

Hm I wonder if that is an ACK or "please throughly rewrite the
scheduler" request ;-)

Anyway I also saw you have started to get rid of RT weights it in
commit e51fd5e2, so in set_load_weight():

       if (task_has_rt_policy(p)) {
                p->se.load.weight = prio_to_weight[0] * 2;
                p->se.load.inv_weight = prio_to_wmult[0] >> 1;
                return;
        }

is now replaced by this:

        if (task_has_rt_policy(p)) {
                p->se.load.weight = 0;
                p->se.load.inv_weight = WMULT_CONST;
                return;
        }

I backported that commit onto 2.6.34 (bah, just patch -p1)
and tested. The problem persists, but mutates:

Whereas before this commit the problem was that processes came
back with enormous weights after forking of an RT process flagged
with SCHED_RESET_ON_FORK, the problem is now the reverse:
the process comes back with load weight zero making the forked
process totally numb (when it has enormous weights it would atlest
respond), so this patch is still needed to bring the weight back in
balance AFAICT.

Yours,
Linus Walleij

Re: [PATCH] sched: SCHED_RESET_ON_FORK to recalculate load weights

[Peter Zijlstra]

On Mon, 2010-10-11 at 01:05 +0200, Linus Walleij wrote:
> 2010/10/9 Peter Zijlstra <peterz@infradead.org>:
> 
> > On Sat, 2010-10-09 at 10:16 +0200, Linus Walleij wrote:
> >>
> >> So we always need to call set_load_weight(), not just if the
> >> niceval was changed, because the scheduler gives
> >> SCHED_RR/SCHED_FIFO processes very high weights.
> >
> > SCHED_RR/FIFO never uses that weight, we should remove all that cruft..
> 
> Hm I wonder if that is an ACK or "please throughly rewrite the
> scheduler" request ;-)

Nah, its an SCHED_FIFO/RR shouldn't care about p->se.load at all
statement, any patch that mentions that relation cannot be right ;-)

> Anyway I also saw you have started to get rid of RT weights it in
> commit e51fd5e2, so in set_load_weight():
> 
>        if (task_has_rt_policy(p)) {
>                 p->se.load.weight = prio_to_weight[0] * 2;
>                 p->se.load.inv_weight = prio_to_wmult[0] >> 1;
>                 return;
>         }
> 
> is now replaced by this:
> 
>         if (task_has_rt_policy(p)) {
>                 p->se.load.weight = 0;
>                 p->se.load.inv_weight = WMULT_CONST;
>                 return;
>         }

Right, that was to catch anybody relying on RR/FIFO tasks having a
sensible weight, I think we can now simply remove that whole clause.

> I backported that commit onto 2.6.34 (bah, just patch -p1)
> and tested. The problem persists, but mutates:

/me fails to see the relevance to .34 (or for that matter remember
what .34 looked like).

> Whereas before this commit the problem was that processes came
> back with enormous weights after forking of an RT process flagged
> with SCHED_RESET_ON_FORK, the problem is now the reverse:
> the process comes back with load weight zero making the forked
> process totally numb (when it has enormous weights it would atlest
> respond), so this patch is still needed to bring the weight back in
> balance AFAICT.

OK, so the problem is that if a RR/FIFO task does s fork() and it has
SCHED_RESET_ON_FORK set, the child normalization fails to properly set
the weight?

Does (as Mike just suggested) removing that whole RT clause in
set_load_weight() work for you?

Re: [PATCH] sched: SCHED_RESET_ON_FORK to recalculate load weights

[Linus Walleij]

2010/10/11 Peter Zijlstra <peterz@infradead.org>:

> OK, so the problem is that if a RR/FIFO task does s fork() and it has
> SCHED_RESET_ON_FORK set, the child normalization fails to properly set
> the weight?
>
> Does (as Mike just suggested) removing that whole RT clause in
> set_load_weight() work for you?

Yep it sure does! Time to drop some weight.
Sent a new patch for this...

Yours,
Linus Walleij