[patch] f83f9ac causes tasks running at MAX_PRIO

[patch] f83f9ac causes tasks running at MAX_PRIO

[Mike Galbraith]

top - 11:33:19 up 1 min, 21 users,  load average: 4.47, 1.44, 0.51
Tasks: 288 total,   1 running, 287 sleeping,   0 stopped,   0 zombie
Cpu(s):  0.2%us,  0.5%sy,  0.0%ni, 99.3%id,  0.0%wa,  0.0%hi,  0.0%si,  0.0%st

  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  P COMMAND
  568 root      20   0     0    0    0 S    1  0.0   0:00.02 3 scsi_eh_1
 4724 root      40   0  358m  22m 4008 S    1  0.7   0:01.69 0 Xorg
 6064 root      40   0  8872 1316  852 R    1  0.0   0:00.07 3 top
    1 root      40   0  1064  388  324 S    0  0.0   0:01.30 2 init
    2 root      40   0     0    0    0 S    0  0.0   0:00.00 1 kthreadd
    3 root      RT   0     0    0    0 S    0  0.0   0:00.00 0 migration/0

WARN_ON(current->normal_prio == MAX_PRIO);

[    0.092016] ------------[ cut here ]------------
[    0.096008] WARNING: at kernel/sched.c:2591 sched_fork+0xe4/0x1ae()
[    0.100002] Hardware name: MS-7502
[    0.104002] Modules linked in:
[    0.108191] Pid: 0, comm: swapper Not tainted 2.6.32-tip-smpx #956
[    0.112002] Call Trace:
[    0.116004]  [<ffffffff810365ab>] ? sched_fork+0xe4/0x1ae
[    0.120004]  [<ffffffff81038eb8>] warn_slowpath_common+0x77/0xa4
[    0.124004]  [<ffffffff81038ef4>] warn_slowpath_null+0xf/0x11
[    0.128003]  [<ffffffff810365ab>] sched_fork+0xe4/0x1ae
[    0.132005]  [<ffffffff81059a9f>] ? monotonic_to_bootbased+0x26/0x34
[    0.136004]  [<ffffffff8103783b>] copy_process+0x4b9/0x10e4
[    0.140006]  [<ffffffff810bd38b>] ? __get_vm_area_node+0x175/0x1bf
[    0.144004]  [<ffffffff810385b2>] do_fork+0x14c/0x304
[    0.148005]  [<ffffffff810200dd>] ? __ioremap_caller+0x292/0x2fb
[    0.152005]  [<ffffffff812838b0>] ? acpi_os_map_memory+0x12/0x1b
[    0.156006]  [<ffffffff810038d2>] kernel_thread+0x82/0xe0
[    0.160006]  [<ffffffff8146c510>] ? kernel_init+0x0/0x1af
[    0.164004]  [<ffffffff81003930>] ? child_rip+0x0/0x20
[    0.168004]  [<ffffffff81281a48>] ? rest_init+0x1c/0x76
[    0.172003]  [<ffffffff8146cc54>] start_kernel+0x351/0x35c
[    0.176004]  [<ffffffff8146c29a>] x86_64_start_reservations+0xaa/0xae
[    0.180004]  [<ffffffff8146c37f>] x86_64_start_kernel+0xe1/0xe8
[    0.184007] ---[ end trace 4eaa2a86a8e2da22 ]---

sched: fix task priority bug.

f83f9ac removed a call to effective_prio() in wake_up_new_task(), which
leads to tasks running at MAX_PRIO.  That call set both the child's prio
and normal_prio fields to normal_prio(child).  Do the same fork time by
setting both to normal_prio(parent).

Signed-off-by: Mike Galbraith <efault@gmx.de>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Peter Williams <pwil3058@bigpond.net.au>
LKML-Reference: <new-submission>

---
 kernel/sched.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/kernel/sched.c
===================================================================
--- linux-2.6.orig/kernel/sched.c
+++ linux-2.6/kernel/sched.c
@@ -2609,7 +2609,7 @@ void sched_fork(struct task_struct *p, i
 	/*
 	 * Make sure we do not leak PI boosting priority to the child.
 	 */
-	p->prio = current->normal_prio;
+	p->prio = p->normal_prio = normal_prio(current);
 
 	if (!rt_prio(p->prio))
 		p->sched_class = &fair_sched_class;

Re: [patch] f83f9ac causes tasks running at MAX_PRIO

[Peter Zijlstra]

On Sun, 2009-11-29 at 14:23 +0100, Mike Galbraith wrote:

> sched: fix task priority bug.
> 
> f83f9ac removed a call to effective_prio() in wake_up_new_task(), which
> leads to tasks running at MAX_PRIO.  That call set both the child's prio
> and normal_prio fields to normal_prio(child).  Do the same fork time by
> setting both to normal_prio(parent).
> 
> Signed-off-by: Mike Galbraith <efault@gmx.de>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Peter Williams <pwil3058@bigpond.net.au>
> LKML-Reference: <new-submission>
> 
> ---
>  kernel/sched.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6/kernel/sched.c
> ===================================================================
> --- linux-2.6.orig/kernel/sched.c
> +++ linux-2.6/kernel/sched.c
> @@ -2609,7 +2609,7 @@ void sched_fork(struct task_struct *p, i
>  	/*
>  	 * Make sure we do not leak PI boosting priority to the child.
>  	 */
> -	p->prio = current->normal_prio;
> +	p->prio = p->normal_prio = normal_prio(current);
>  
>  	if (!rt_prio(p->prio))
>  		p->sched_class = &fair_sched_class;
> 

Damn PI stuff makes my head hurt ;-)

So we've got:

 ->prio        - the actual effective priority    [ prio scale ]
 ->normal_prio - the task's normal priority       [ prio scale ]
 ->static_prio - SCHED_OTHER's nice value         [ prio scale ]
 ->rt_priority - SCHED_FIFO/RR prio value  [ sched_param scale ]

[ with prio scale being:

  [0,    MAX_RT_PRIO-1]     [MAX_RT_PRIO, MAX_PRIO-1]
   RT-100, RT-99..RT-1           NICE-20, NICE+19
]

So at sched_fork() we do the

 p->prio = p->normal_prio;

thing, to unboost.

If that results in MAX_PRIO, then our parent's ->normal_prio is stuffed.

Looking at the code I can see that happening because we've got:

init_idle() doing:
  idle->prio = idle->normal_prio = MAX_PRIO;

Which will propagate... like reported.

Now, since the idle-threads usually run on &idle_sched_class, nobody
will actually look at their ->prio, so having that out-of-range might
make sense.

Just needs to get fixed up when we fork a normal thread, which would be
in sched_fork(), now your call to normal_prio() fixes this by setting
everything to ->static_prio for SCHED_OTHER tasks, however

migration_call()
  CPU_DEAD:
    rq->idle->static_prio = MAX_PRIO;

spoils that too, then again, at that point nothing will fork from that
idle thread.

Funny thing though, INIT_TASK() sets everything at MAX_PRIO-20.

Ingo, any particular reason we set idle threads at MAX_PRIO? Can't we
simply do something like below and be done with it?

---
 kernel/sched.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index c0e4e9d..5ad5a66 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -6963,7 +6963,6 @@ void __cpuinit init_idle(struct task_struct *idle,
int cpu)
 	__sched_fork(idle);
 	idle->se.exec_start = sched_clock();
 
-	idle->prio = idle->normal_prio = MAX_PRIO;
 	cpumask_copy(&idle->cpus_allowed, cpumask_of(cpu));
 	__set_task_cpu(idle, cpu);
 
@@ -7667,7 +7666,6 @@ migration_call(struct notifier_block *nfb,
unsigned long action, void *hcpu)
 		spin_lock_irq(&rq->lock);
 		update_rq_clock(rq);
 		deactivate_task(rq, rq->idle, 0);
-		rq->idle->static_prio = MAX_PRIO;
 		__setscheduler(rq, rq->idle, SCHED_NORMAL, 0);
 		rq->idle->sched_class = &idle_sched_class;
 		migrate_dead_tasks(cpu);

Re: [patch] f83f9ac causes tasks running at MAX_PRIO

[Mike Galbraith]

On Wed, 2009-12-02 at 12:46 +0100, Peter Zijlstra wrote:

> init_idle() doing:
>   idle->prio = idle->normal_prio = MAX_PRIO;
> 
> Which will propagate... like reported.
> 
> Now, since the idle-threads usually run on &idle_sched_class, nobody
> will actually look at their ->prio, so having that out-of-range might
> make sense.
> 
> Just needs to get fixed up when we fork a normal thread, which would be
> in sched_fork(), now your call to normal_prio() fixes this by setting
> everything to ->static_prio for SCHED_OTHER tasks, however
> 
> migration_call()
>   CPU_DEAD:
>     rq->idle->static_prio = MAX_PRIO;
> 
> spoils that too..

Darn.

> Ingo, any particular reason we set idle threads at MAX_PRIO? Can't we
> simply do something like below and be done with it?

Hysterical reasons?  That might have been a doorstop conversion kit for
O(1), but boots fine with CFS, and prio 40 tasks are history.

> ---
>  kernel/sched.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched.c b/kernel/sched.c
> index c0e4e9d..5ad5a66 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -6963,7 +6963,6 @@ void __cpuinit init_idle(struct task_struct *idle,
> int cpu)
>  	__sched_fork(idle);
>  	idle->se.exec_start = sched_clock();
>  
> -	idle->prio = idle->normal_prio = MAX_PRIO;
>  	cpumask_copy(&idle->cpus_allowed, cpumask_of(cpu));
>  	__set_task_cpu(idle, cpu);
>  
> @@ -7667,7 +7666,6 @@ migration_call(struct notifier_block *nfb,
> unsigned long action, void *hcpu)
>  		spin_lock_irq(&rq->lock);
>  		update_rq_clock(rq);
>  		deactivate_task(rq, rq->idle, 0);
> -		rq->idle->static_prio = MAX_PRIO;
>  		__setscheduler(rq, rq->idle, SCHED_NORMAL, 0);
>  		rq->idle->sched_class = &idle_sched_class;
>  		migrate_dead_tasks(cpu);
> 

Re: [patch] f83f9ac causes tasks running at MAX_PRIO

[Peter Williams]

On 02/12/09 22:49, Mike Galbraith wrote:
> Hysterical reasons?  That might have been a doorstop conversion kit for
> O(1), but boots fine with CFS, and prio 40 tasks are history.

In my (not so) humble opinion, there is still a lot of unnecessary cruft 
in sched.c that should have been removed as the final (clean up) stage 
of implementing CFS (i.e. stuff that was needed for O(1) but no longer 
serves a useful purpose).  I know that the extra overhead of this code 
is probably inconsequential (and the compiler may even optimize some of 
it away) but it looks untidy and makes the code harder to understand.

Recent patches that I've submitted were intended as a start to removing 
some of this cruft and I had intended to send more patches after they 
were accepted.  I figured that it was better to do it as a number of 
small changes rather than one big one.  Should I continue?

Peter

Re: [patch] f83f9ac causes tasks running at MAX_PRIO

[Mike Galbraith]

On Thu, 2009-12-03 at 11:25 +1000, Peter Williams wrote:
> On 02/12/09 22:49, Mike Galbraith wrote:
> > Hysterical reasons?  That might have been a doorstop conversion kit for
> > O(1), but boots fine with CFS, and prio 40 tasks are history.
> 
> In my (not so) humble opinion, there is still a lot of unnecessary cruft 
> in sched.c that should have been removed as the final (clean up) stage 
> of implementing CFS (i.e. stuff that was needed for O(1) but no longer 
> serves a useful purpose).  I know that the extra overhead of this code 
> is probably inconsequential (and the compiler may even optimize some of 
> it away) but it looks untidy and makes the code harder to understand.
> 
> Recent patches that I've submitted were intended as a start to removing 
> some of this cruft and I had intended to send more patches after they 
> were accepted.  I figured that it was better to do it as a number of 
> small changes rather than one big one.  Should I continue?

Certainly.

	-Mike

Re: [patch] f83f9ac causes tasks running at MAX_PRIO

[Peter Zijlstra]

On Thu, 2009-12-03 at 11:25 +1000, Peter Williams wrote:

> Recent patches that I've submitted were intended as a start to removing 
> some of this cruft and I had intended to send more patches after they 
> were accepted.  I figured that it was better to do it as a number of 
> small changes rather than one big one.  Should I continue?

Please, feel free to clean that up.

Re: [patch] f83f9ac causes tasks running at MAX_PRIO

[Steven Rostedt]

On Wed, 2009-12-02 at 12:46 +0100, Peter Zijlstra wrote:
> On Sun, 2009-11-29 at 14:23 +0100, Mike Galbraith wrote:
> 
> > sched: fix task priority bug.
> > 
> > f83f9ac removed a call to effective_prio() in wake_up_new_task(), which
> > leads to tasks running at MAX_PRIO.  That call set both the child's prio
> > and normal_prio fields to normal_prio(child).  Do the same fork time by
> > setting both to normal_prio(parent).
> > 
> > Signed-off-by: Mike Galbraith <efault@gmx.de>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Cc: Peter Williams <pwil3058@bigpond.net.au>
> > LKML-Reference: <new-submission>
> > 
> > ---
> >  kernel/sched.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > Index: linux-2.6/kernel/sched.c
> > ===================================================================
> > --- linux-2.6.orig/kernel/sched.c
> > +++ linux-2.6/kernel/sched.c
> > @@ -2609,7 +2609,7 @@ void sched_fork(struct task_struct *p, i
> >  	/*
> >  	 * Make sure we do not leak PI boosting priority to the child.
> >  	 */
> > -	p->prio = current->normal_prio;
> > +	p->prio = p->normal_prio = normal_prio(current);
> >  
> >  	if (!rt_prio(p->prio))
> >  		p->sched_class = &fair_sched_class;
> > 
> 
> Damn PI stuff makes my head hurt ;-)

I recommend Advil

> 
> So we've got:
> 
>  ->prio        - the actual effective priority    [ prio scale ]
>  ->normal_prio - the task's normal priority       [ prio scale ]
>  ->static_prio - SCHED_OTHER's nice value         [ prio scale ]
>  ->rt_priority - SCHED_FIFO/RR prio value  [ sched_param scale ]
> 
> [ with prio scale being:
> 
>   [0,    MAX_RT_PRIO-1]     [MAX_RT_PRIO, MAX_PRIO-1]
>    RT-100, RT-99..RT-1           NICE-20, NICE+19
> ]
> 
> So at sched_fork() we do the
> 
>  p->prio = p->normal_prio;
> 
> thing, to unboost.
> 
> If that results in MAX_PRIO, then our parent's ->normal_prio is stuffed.
> 
> Looking at the code I can see that happening because we've got:
> 
> init_idle() doing:
>   idle->prio = idle->normal_prio = MAX_PRIO;

But this is only called on the idle task, which should never fork.


> 
> Which will propagate... like reported.
> 
> Now, since the idle-threads usually run on &idle_sched_class, nobody
> will actually look at their ->prio, so having that out-of-range might
> make sense.
> 
> Just needs to get fixed up when we fork a normal thread, which would be
> in sched_fork(), now your call to normal_prio() fixes this by setting
> everything to ->static_prio for SCHED_OTHER tasks, however
> 
> migration_call()
>   CPU_DEAD:
>     rq->idle->static_prio = MAX_PRIO;
> 
> spoils that too, then again, at that point nothing will fork from that
> idle thread.

Well, that is when the CPU is dead, right?

> 
> Funny thing though, INIT_TASK() sets everything at MAX_PRIO-20.

Right, because the init task will fork. But once a task becomes idle, it
should never do anything (but service interrupts).

> 
> Ingo, any particular reason we set idle threads at MAX_PRIO? Can't we
> simply do something like below and be done with it?

There probably isn't any reason this can't be done, but I'm thinking we
may be papering over a bug instead of solving one.

-- Steve

> 
> ---
>  kernel/sched.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched.c b/kernel/sched.c
> index c0e4e9d..5ad5a66 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -6963,7 +6963,6 @@ void __cpuinit init_idle(struct task_struct *idle,
> int cpu)
>  	__sched_fork(idle);
>  	idle->se.exec_start = sched_clock();
>  
> -	idle->prio = idle->normal_prio = MAX_PRIO;
>  	cpumask_copy(&idle->cpus_allowed, cpumask_of(cpu));
>  	__set_task_cpu(idle, cpu);
>  
> @@ -7667,7 +7666,6 @@ migration_call(struct notifier_block *nfb,
> unsigned long action, void *hcpu)
>  		spin_lock_irq(&rq->lock);
>  		update_rq_clock(rq);
>  		deactivate_task(rq, rq->idle, 0);
> -		rq->idle->static_prio = MAX_PRIO;
>  		__setscheduler(rq, rq->idle, SCHED_NORMAL, 0);
>  		rq->idle->sched_class = &idle_sched_class;
>  		migrate_dead_tasks(cpu);
> 
> 

Re: [patch] f83f9ac causes tasks running at MAX_PRIO

[Mike Galbraith]

On Wed, 2009-12-02 at 09:03 -0500, Steven Rostedt wrote:
> On Wed, 2009-12-02 at 12:46 +0100, Peter Zijlstra wrote:

> > Funny thing though, INIT_TASK() sets everything at MAX_PRIO-20.
> 
> Right, because the init task will fork. But once a task becomes idle, it
> should never do anything (but service interrupts).


Seems it forks _as_ the idle thread..

        /*
         * Make us the idle thread. Technically, schedule() should not be
         * called from this thread, however somewhere below it might be,
         * but because we are the idle thread, we just pick up running again
         * when this runqueue becomes "idle".
         */
        init_idle(current, smp_processor_id());

        calc_load_update = jiffies + LOAD_FREQ;

        /*
         * During early bootup we pretend to be a normal task:
         */
        current->sched_class = &fair_sched_class;

..init task becomes the idle thread in sched_init().. gets to
rest_init(), forks with prio and normal_prio now at MAX_PRIO, but
static_prio still at NICE_0.

Peter's change looks OK unless I'm missing something.  Though...

static void pull_task(struct rq *src_rq, struct task_struct *p,
                      struct rq *this_rq, int this_cpu)
{
        deactivate_task(src_rq, p, 0);
        set_task_cpu(p, this_cpu);
        activate_task(this_rq, p, 0);
        /*
         * Note that idle threads have a prio of MAX_PRIO, for this test
         * to be always true for them.
         */
        check_preempt_curr(this_rq, p, 0);
}

..we have some O(1) comments for future generations to ponder.

	-Mike

[tip:sched/urgent] sched: Fix task priority bug

[tip-bot for Peter Zijlstra]

Commit-ID:  57785df5ac53c70da9fb53696130f3c551bfe1f9
Gitweb:     http://git.kernel.org/tip/57785df5ac53c70da9fb53696130f3c551bfe1f9
Author:     Peter Zijlstra <a.p.zijlstra@chello.nl>
AuthorDate: Fri, 4 Dec 2009 09:59:02 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 9 Dec 2009 10:03:10 +0100

sched: Fix task priority bug

83f9ac removed a call to effective_prio() in wake_up_new_task(), which
leads to tasks running at MAX_PRIO.

This is caused by the idle thread being set to MAX_PRIO before forking
off init. O(1) used that to make sure idle was always preempted, CFS
uses check_preempt_curr_idle() for that so we can savely remove this bit
of legacy code.

Reported-by: Mike Galbraith <efault@gmx.de>
Tested-by: Mike Galbraith <efault@gmx.de>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <1259754383.4003.610.camel@laptop>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/sched.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index 71eb062..3878f50 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -3158,10 +3158,6 @@ static void pull_task(struct rq *src_rq, struct task_struct *p,
 	deactivate_task(src_rq, p, 0);
 	set_task_cpu(p, this_cpu);
 	activate_task(this_rq, p, 0);
-	/*
-	 * Note that idle threads have a prio of MAX_PRIO, for this test
-	 * to be always true for them.
-	 */
 	check_preempt_curr(this_rq, p, 0);
 }
 
@@ -6992,7 +6988,6 @@ void __cpuinit init_idle(struct task_struct *idle, int cpu)
 	__sched_fork(idle);
 	idle->se.exec_start = sched_clock();
 
-	idle->prio = idle->normal_prio = MAX_PRIO;
 	cpumask_copy(&idle->cpus_allowed, cpumask_of(cpu));
 	__set_task_cpu(idle, cpu);
 
@@ -7696,7 +7691,6 @@ migration_call(struct notifier_block *nfb, unsigned long action, void *hcpu)
 		spin_lock_irq(&rq->lock);
 		update_rq_clock(rq);
 		deactivate_task(rq, rq->idle, 0);
-		rq->idle->static_prio = MAX_PRIO;
 		__setscheduler(rq, rq->idle, SCHED_NORMAL, 0);
 		rq->idle->sched_class = &idle_sched_class;
 		migrate_dead_tasks(cpu);