sched: fix SysRq-N (normalize RT tasks)

PATCH In review Ingo Molnar 3 participants 6 messages Jun 14, 2007

lore

Share & tools

lore

Ingo Molnar

author

2007-06-14 09:48 UTC #0 lore raw

PATCH 1/1 sched: fix SysRq-N (normalize RT tasks)

From: Ingo Molnar <[email protected]>
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 <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/sched.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

kernel/sched.c +5 -3

Index: 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);
 }

Gene Heskett

2007-06-14 13:49 UTC #1 lore raw

Quoted 42 lines · Ingo Molnar

From: Ingo Molnar <[email protected]>
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 <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
---
kernel/sched.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
Index: linux/kernel/sched.c
===================================================================
kernel/sched.c +3 -2
@@ -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;
 

kernel/sched.c void normalize_rt_tasks(void) · line 7101

Quoted 8 lines

kernel/sched.c +2 -1

@@ -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

Signature · 5 lines

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.

Ingo Molnar

author

2007-06-14 13:51 UTC #2 lore raw

Quoted 7 lines · Gene Heskett

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

Gene Heskett

2007-06-14 14:38 UTC #3 lore raw

Quoted 9 lines · Ingo Molnar

Quoted 7 lines · Gene Heskett
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

Quoted 5 lines

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.

Signature · 6 lines

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

Gene Heskett

2007-06-14 14:12 UTC #4 lore raw

In reply to Ingo Molnar

Quoted 42 lines · Ingo Molnar

From: Ingo Molnar <[email protected]>
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 <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
---
kernel/sched.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
Index: linux/kernel/sched.c
===================================================================
kernel/sched.c +3 -2
@@ -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;
 

kernel/sched.c void normalize_rt_tasks(void) · line 7101

Quoted 8 lines

kernel/sched.c +2 -1

@@ -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.

Signature · 5 lines

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.

Måns Rullgård

2007-06-14 19:10 UTC #5 lore raw

In reply to Ingo Molnar

Quoted 6 lines · Ingo Molnar

From: Ingo Molnar <[email protected]>
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.

Quoted 6 lines

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?

Signature · 2 lines

Måns Rullgård
[email protected]