[patch] sched: fix SysRq-N (normalize RT tasks)

[patch] sched: fix SysRq-N (normalize RT tasks)

[Ingo Molnar]

From: Ingo Molnar <mingo@elte.hu>
Subject: [patch] sched: fix SysRq-N (normalize RT tasks)

Gene Heskett reported the following problem while testing CFS: SysRq-N 
is not always effective in normalizing tasks back to SCHED_OTHER.

the reason for that turns out to be the following bug: 
normalize_rt_tasks() uses for_each_process() to iterate through all 
tasks in the system. The problem is, this method does not iterate 
through all tasks, it iterates through all thread groups. The proper 
mechanism to enumerate all tasks is to use a do_each_thread() + 
while_each_thread() loop.

obvious bugfix for v2.6.22 inclusion. -stable candidate as well.

Reported-by: Gene Heskett <gene.heskett@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/sched.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Index: linux/kernel/sched.c
===================================================================
--- linux/kernel/sched.c
+++ linux/kernel/sched.c
@@ -7071,12 +7071,13 @@ EXPORT_SYMBOL(__might_sleep);
 void normalize_rt_tasks(void)
 {
 	struct prio_array *array;
-	struct task_struct *p;
+	struct task_struct *g, *p;
 	unsigned long flags;
 	struct rq *rq;
 
 	read_lock_irq(&tasklist_lock);
-	for_each_process(p) {
+
+	do_each_thread(g, p) {
 		if (!rt_task(p))
 			continue;
 
@@ -7094,7 +7095,8 @@ void normalize_rt_tasks(void)
 
 		__task_rq_unlock(rq);
 		spin_unlock_irqrestore(&p->pi_lock, flags);
-	}
+	} while_each_thread(g, p);
+
 	read_unlock_irq(&tasklist_lock);
 }
 

Re: [patch] sched: fix SysRq-N (normalize RT tasks)

[Gene Heskett]

On Thursday 14 June 2007, Ingo Molnar wrote:
>From: Ingo Molnar <mingo@elte.hu>
>Subject: [patch] sched: fix SysRq-N (normalize RT tasks)
>
>Gene Heskett reported the following problem while testing CFS: SysRq-N
>is not always effective in normalizing tasks back to SCHED_OTHER.
>
>the reason for that turns out to be the following bug:
>normalize_rt_tasks() uses for_each_process() to iterate through all
>tasks in the system. The problem is, this method does not iterate
>through all tasks, it iterates through all thread groups. The proper
>mechanism to enumerate all tasks is to use a do_each_thread() +
>while_each_thread() loop.
>
>obvious bugfix for v2.6.22 inclusion. -stable candidate as well.
>
>Reported-by: Gene Heskett <gene.heskett@gmail.com>
>Signed-off-by: Ingo Molnar <mingo@elte.hu>
>---
> kernel/sched.c |    8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
>Index: linux/kernel/sched.c
>===================================================================
>--- linux/kernel/sched.c
>+++ linux/kernel/sched.c
>@@ -7071,12 +7071,13 @@ EXPORT_SYMBOL(__might_sleep);
> void normalize_rt_tasks(void)
> {
> 	struct prio_array *array;
>-	struct task_struct *p;
>+	struct task_struct *g, *p;
> 	unsigned long flags;
> 	struct rq *rq;
>
> 	read_lock_irq(&tasklist_lock);
>-	for_each_process(p) {
>+
>+	do_each_thread(g, p) {
> 		if (!rt_task(p))
> 			continue;
>
>@@ -7094,7 +7095,8 @@ void normalize_rt_tasks(void)
>
> 		__task_rq_unlock(rq);
> 		spin_unlock_irqrestore(&p->pi_lock, flags);
>-	}
>+	} while_each_thread(g, p);
>+
> 	read_unlock_irq(&tasklist_lock);
> }

Ingo:  I appended this patch to the cfs-v16 patch for 2.6.22-rc4, and got this 
while applying it:

patching file kernel/sched.c
Hunk #1 FAILED at 7071.
Hunk #2 FAILED at 7095.
2 out of 2 hunks FAILED -- saving rejects to file kernel/sched.c.rej
done

-- 
Cheers, Gene
"There are four boxes to be used in defense of liberty:
 soap, ballot, jury, and ammo. Please use in that order."
-Ed Howdershelt (Author)
One man tells a falsehood, a hundred repeat it as true.

Re: [patch] sched: fix SysRq-N (normalize RT tasks)

[Ingo Molnar]

* Gene Heskett <gene.heskett@gmail.com> wrote:

> Ingo: I appended this patch to the cfs-v16 patch for 2.6.22-rc4, and 
> got this while applying it:
> 
> patching file kernel/sched.c
> Hunk #1 FAILED at 7071.
> Hunk #2 FAILED at 7095.
> 2 out of 2 hunks FAILED -- saving rejects to file kernel/sched.c.rej

yep, this fix is already included in CFS. (in fact in CFS i also changed 
this function to renormalizing negative-nice tasks to nice-0 - but this 
is not something upstream has to pick up for .22.)

	Ingo

Re: [patch] sched: fix SysRq-N (normalize RT tasks)

[Gene Heskett]

On Thursday 14 June 2007, Ingo Molnar wrote:
>* Gene Heskett <gene.heskett@gmail.com> wrote:
>> Ingo: I appended this patch to the cfs-v16 patch for 2.6.22-rc4, and
>> got this while applying it:
>>
>> patching file kernel/sched.c
>> Hunk #1 FAILED at 7071.
>> Hunk #2 FAILED at 7095.
>> 2 out of 2 hunks FAILED -- saving rejects to file kernel/sched.c.rej
>
>yep, this fix is already included in CFS. (in fact in CFS i also changed
>this function to renormalizing negative-nice tasks to nice-0 - but this
>is not something upstream has to pick up for .22.)
>
>	Ingo

I built it without it, but I saw something else that's been mentioned before 
here:

  MODPOST vmlinux
WARNING: arch/i386/kernel/built-in.o(.text+0x844d): Section mismatch: 
reference to .init.text:amd_init_mtrr (between 'mtrr_bp_init' 
and 'mtrr_save_state')
WARNING: arch/i386/kernel/built-in.o(.text+0x8452): Section mismatch: 
reference to .init.text:cyrix_init_mtrr (between 'mtrr_bp_init' 
and 'mtrr_save_state')
WARNING: arch/i386/kernel/built-in.o(.text+0x8457): Section mismatch: 
reference to .init.text:centaur_init_mtrr (between 'mtrr_bp_init' 
and 'mtrr_save_state')
WARNING: arch/i386/kernel/built-in.o(.text+0x9274): Section mismatch: 
reference to .init.text: (between 'get_mtrr_state' and 'generic_get_mtrr')
WARNING: arch/i386/kernel/built-in.o(.text+0x9288): Section mismatch: 
reference to .init.text: (between 'get_mtrr_state' and 'generic_get_mtrr')
WARNING: arch/i386/kernel/built-in.o(.text+0x92ac): Section mismatch: 
reference to .init.text: (between 'get_mtrr_state' and 'generic_get_mtrr')

Which looks a little worrisome.  Its done, so I'll see if it will boot.

It did.  I had several items that failed on the shutdown, NDI why, but 
everything seemed to restart ok.  Now to live with it for a day or so.

-- 
Cheers, Gene
"There are four boxes to be used in defense of liberty:
 soap, ballot, jury, and ammo. Please use in that order."
-Ed Howdershelt (Author)
Netscape is not a newsreader, and probably never shall be.
	-- Tom Christiansen

Re: [patch] sched: fix SysRq-N (normalize RT tasks)

[Gene Heskett]

On Thursday 14 June 2007, Ingo Molnar wrote:
>From: Ingo Molnar <mingo@elte.hu>
>Subject: [patch] sched: fix SysRq-N (normalize RT tasks)
>
>Gene Heskett reported the following problem while testing CFS: SysRq-N
>is not always effective in normalizing tasks back to SCHED_OTHER.
>
>the reason for that turns out to be the following bug:
>normalize_rt_tasks() uses for_each_process() to iterate through all
>tasks in the system. The problem is, this method does not iterate
>through all tasks, it iterates through all thread groups. The proper
>mechanism to enumerate all tasks is to use a do_each_thread() +
>while_each_thread() loop.
>
>obvious bugfix for v2.6.22 inclusion. -stable candidate as well.
>
>Reported-by: Gene Heskett <gene.heskett@gmail.com>
>Signed-off-by: Ingo Molnar <mingo@elte.hu>
>---
> kernel/sched.c |    8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
>Index: linux/kernel/sched.c
>===================================================================
>--- linux/kernel/sched.c
>+++ linux/kernel/sched.c
>@@ -7071,12 +7071,13 @@ EXPORT_SYMBOL(__might_sleep);
> void normalize_rt_tasks(void)
> {
> 	struct prio_array *array;
>-	struct task_struct *p;
>+	struct task_struct *g, *p;
> 	unsigned long flags;
> 	struct rq *rq;
>
> 	read_lock_irq(&tasklist_lock);
>-	for_each_process(p) {
>+
>+	do_each_thread(g, p) {
> 		if (!rt_task(p))
> 			continue;
>
>@@ -7094,7 +7095,8 @@ void normalize_rt_tasks(void)
>
> 		__task_rq_unlock(rq);
> 		spin_unlock_irqrestore(&p->pi_lock, flags);
>-	}
>+	} while_each_thread(g, p);
>+
> 	read_unlock_irq(&tasklist_lock);
> }

When I looked at sched.c, I found it about 1000 lines shorter than the offsets 
listed above.  So I took it back out and will test the plain 
2.6.22-rc4-cfs-v16 shortly.

-- 
Cheers, Gene
"There are four boxes to be used in defense of liberty:
 soap, ballot, jury, and ammo. Please use in that order."
-Ed Howdershelt (Author)
To save a single life is better than to build a seven story pagoda.

Re: [patch] sched: fix SysRq-N (normalize RT tasks)

[Måns Rullgård]

Ingo Molnar <mingo@elte.hu> writes:

> From: Ingo Molnar <mingo@elte.hu>
> Subject: [patch] sched: fix SysRq-N (normalize RT tasks)
>
> Gene Heskett reported the following problem while testing CFS: SysRq-N 
> is not always effective in normalizing tasks back to SCHED_OTHER.
>
> the reason for that turns out to be the following bug: 
> normalize_rt_tasks() uses for_each_process() to iterate through all 
> tasks in the system. The problem is, this method does not iterate 
> through all tasks, it iterates through all thread groups. The proper 
> mechanism to enumerate all tasks is to use a do_each_thread() + 
> while_each_thread() loop.

Was this always a bug or did the meaning of for_each_process() change
since this code was added?

-- 
Måns Rullgård
mans@mansr.com