Skip to contentSkip to conversation

protect call to set_tsk_need_resched() by the rq-lock

RFC PATCH In review Michael Buesch 2 participants 7 messages Dec 6, 2004 – Dec 8, 2004

lore
Share & tools
lore

Michael Buesch

author
#0 lore raw

RFC PATCH 1/1 protect call to set_tsk_need_resched() by the rq-lock

Hi,

The two attached patches (one against vanilla kernel and one against ck patchset) moves the rq-lock a few lines up in scheduler_tick() to also protect set_tsk_need_resched().

Is that neccessary?
If yes, please apply.

Signature · 1 line
Regards Michael Buesch  [ http://www.tuxsoft.de.vu ]

Index: linux-2.5/kernel/sched.c
===================================================================
RCS file: /home/mb/develop/linux/rsync/linux-2.5/kernel/sched.c,v retrieving revision 1.382

kernel/sched.c +2 -2
diff -u -p -r1.382 sched.c
@@ -2318,12 +2318,12 @@ void scheduler_tick(int user_ticks, int 
 		cpustat->user += user_ticks;
 	cpustat->system += sys_ticks;
 
+	spin_lock(&rq->lock);
 	/* Task might have expired already, but not scheduled off yet */
 	if (p->array != rq->active) {
 		set_tsk_need_resched(p);
-		goto out;
+		goto out_unlock;
 	}
-	spin_lock(&rq->lock);
 	/*
 	 * The task was running during this tick - update the
 	 * time slice counter. Note: we do not update a thread's
kernel/sched.c renamed from kernel/sched.c.orig +3 -3
@@ -2147,18 +2147,18 @@
 		cpustat->user += user_ticks;
 	cpustat->system += sys_ticks;
 
+	spin_lock(&rq->lock);
 	/* Task might have expired already, but not scheduled off yet */
 	if (unlikely(!task_queued(p))) {
 		set_tsk_need_resched(p);
-		goto out;
+		goto out_unlock;
 	}
 	/*
 	 * SCHED_FIFO tasks never run out of timeslice.
 	 */
 	if (unlikely(p->policy == SCHED_FIFO))
-		goto out;
+		goto out_unlock;
 
-	spin_lock(&rq->lock);
 	debit = ns_diff(rq->timestamp_last_tick, p->timestamp);
 	p->ns_debit += debit;
 	if (p->ns_debit < NSJIFFY)
#1 lore raw
Quoted 5 lines · Michael Buesch

The two attached patches (one against vanilla kernel and one against ck patchset) moves the rq-lock a few lines up in scheduler_tick() to also protect set_tsk_need_resched().

Is that neccessary?

scheduler_tick() is a special case, 'current' is pinned and cannot go away, nor can it get off the runqueue. So the patch is not needed.

Ingo

author
#2 lore raw

Quoting Ingo Molnar <[email protected]>:

Quoted 7 lines · Michael Buesch
Quoted 5 lines

The two attached patches (one against vanilla kernel and one against ck patchset) moves the rq-lock a few lines up in scheduler_tick() to also protect set_tsk_need_resched().

Quoted 7 lines
Quoted 1 line

Is that neccessary?

scheduler_tick() is a special case,

'current' is pinned and cannot go
away, nor can it get off the runqueue.

Can you explain in short, why this is the case, please? I don't really get behind it.
How are the two things enforced?

Quoted 3 lines

So the patch is not needed.

Ingo

Thanks.

Signature · 1 line
Regards Michael Buesch  [ http://www.tuxsoft.de.vu ]
#3 lore raw
Quoted 11 lines · Michael Buesch
Quoted 8 lines
Quoted 5 lines

The two attached patches (one against vanilla kernel and one against ck patchset) moves the rq-lock a few lines up in scheduler_tick() to also protect set_tsk_need_resched().

Is that neccessary?

scheduler_tick() is a special case, 'current' is pinned and cannot go away, nor can it get off the runqueue.

Can you explain in short, why this is the case, please? I don't really get behind it. How are the two things enforced?

'current' is the currently executing task and as such it wont get moved off the runqueue. The only way to leave the runqueue is to execute schedule() [or to be preempted].

Ingo

author
#4 lore raw

Quoting Ingo Molnar <[email protected]>:

Quoted 12 lines · Michael Buesch
Quoted 10 lines
Quoted 9 lines
Quoted 6 lines

The two attached patches (one against vanilla kernel and one against ck patchset) moves the rq-lock a few lines up in scheduler_tick() to also protect set_tsk_need_resched().

Is that neccessary?

scheduler_tick() is a special case, 'current' is pinned and cannot go away, nor can it get off the runqueue.

Quoted 6 lines
Quoted 2 lines

Can you explain in short, why this is the case, please? I don't really get behind it. How are the two things enforced?

'current' is the currently executing task and as such it wont get moved off the runqueue. The only way to leave the runqueue is to execute schedule() [or to be preempted].

Ok, I understand that.

Someone else said to me:
[quote]
"another runqueue might want to touch your runqueue while you're in scheduler_tick" ...
"that is far more likely to hit with many many cpus and I'd be surprised if you're the first person to get a race there" [/quote]

What about this? Is this possible? Or did she/he/it miss a point?

It's this scenario here:
I frequently get oopses in cpu_idle(). In the two hours before I made the patch, the machine hung twice. Since I'm running a patched scheduler, It did not hang again. I gave em about 15 hours testing.

But maybe that's all pure luck.

Quoted 1 line

Ingo

Unable to handle kernel paging request at virtual address 00099108
 printing eip:
b01010c0
*pde = 00000000
Oops: 0000 [#1]
SMP 
Modules linked in: nfs lockd sunrpc nvidia ipv6 ohci_hcd tuner tvaudio msp3400 bttv video_buf firmware_class btcx_risc ehci_hcd uhci_hcd usbcore intel_agp agpgart evdev
CPU:    0
EIP:    0060:[<b01010c0>]    Tainted: P      VLI
EFLAGS: 00010286   (2.6.10-rc2-ck2-nozeroram-findvmastat)

EIP is at cpu_idle+0x31/0x3f
eax: 00000001 ebx: 00099100 ecx: 00000000 edx: 0000001d
esi: 00000000 edi: b03dff9c ebp: b03dffe4 esp: b03dffe0
ds: 007b es: 007b ss: 0068
Process swapper (pid: 0, threadinfo=b03de000 task=b034db40)

Stack: 00020800 b03dfff8 b03e0898 000000bd b03e0340 b040cb80 0044f007 b0100211 
Call Trace:
 [<b0103c00>] show_stack+0x7a/0x90
 [<b0103d81>] show_registers+0x152/0x1ca
 [<b0103f86>] die+0xf4/0x178
 [<b0114556>] do_page_fault+0x42a/0x645
 [<b01038a7>] error_code+0x2b/0x30
 [<b03e0898>] start_kernel+0x13a/0x151
 [<b0100211>] 0xb0100211
Code: e0 ff ff 21 e3 eb 24 8b 0d 84 c6 40 b0 b8 26 10 10 b0 8b 15 c0 eb 34 b0 85 c9 0f 44 c8 8b 43 10 c1 e0 07 89 90 84 52 41 b0 ff d1 <8b> 43 08 a8 08 74 d5 e8 d8 7f 1f 00 eb f2 55 89 e5 56 53 fb ba 
 <0>Kernel panic - not syncing: Attempted to kill the idle task!

b010108f <cpu_idle>:
b010108f: 55 push %ebp
b0101090: 89 e5 mov %esp,%ebp
b0101092: 53 push %ebx
b0101093: bb 00 e0 ff ff mov $0xffffe000,%ebx
b0101098: 21 e3 and %esp,%ebx
b010109a: eb 24 jmp b01010c0 <cpu_idle+0x31>
b010109c: 8b 0d 84 c6 40 b0 mov 0xb040c684,%ecx
b01010a2: b8 26 10 10 b0 mov $0xb0101026,%eax
b01010a7: 8b 15 c0 eb 34 b0 mov 0xb034ebc0,%edx
b01010ad: 85 c9 test %ecx,%ecx
b01010af: 0f 44 c8 cmove %eax,%ecx
b01010b2: 8b 43 10 mov 0x10(%ebx),%eax
b01010b5: c1 e0 07 shl $0x7,%eax
b01010b8: 89 90 84 52 41 b0 mov %edx,0xb0415284(%eax)
b01010be: ff d1 call *%ecx
b01010c0: 8b 43 08 mov 0x8(%ebx),%eax
^^^^^^^^^^^^^^^^^^ OOPS. This is the check to need_resched().
b01010c3: a8 08 test $0x8,%al
b01010c5: 74 d5 je b010109c <cpu_idle+0xd>
b01010c7: e8 d8 7f 1f 00 call b02f90a4 <schedule>
b01010cc: eb f2 jmp b01010c0 <cpu_idle+0x31>

Ah, and yes, the kernel is tainted. So Nvidia already received a bugreport.

Signature · 1 line
Regards Michael Buesch  [ http://www.tuxsoft.de.vu ]
author
#5 lore raw

Ok, forget it all. It crashed again. :)
Must be nvidia...

Quoting Michael Buesch <[email protected]>:

Quoted 98 lines

Quoting Ingo Molnar <[email protected]>:

Quoted 12 lines · Michael Buesch
Quoted 10 lines
Quoted 9 lines
Quoted 6 lines

The two attached patches (one against vanilla kernel and one against ck patchset) moves the rq-lock a few lines up in scheduler_tick() to also protect set_tsk_need_resched().

Is that neccessary?

scheduler_tick() is a special case, 'current' is pinned and cannot go away, nor can it get off the runqueue.

Quoted 6 lines
Quoted 2 lines

Can you explain in short, why this is the case, please? I don't really get behind it. How are the two things enforced?

'current' is the currently executing task and as such it wont get moved off the runqueue. The only way to leave the runqueue is to execute schedule() [or to be preempted].

Ok, I understand that.

Someone else said to me:
[quote]
"another runqueue might want to touch your runqueue while you're in scheduler_tick" ...
"that is far more likely to hit with many many cpus and I'd be surprised if you're the first person to get a race there" [/quote]

What about this? Is this possible? Or did she/he/it miss a point?

It's this scenario here:
I frequently get oopses in cpu_idle(). In the two hours before I made the patch, the machine hung twice. Since I'm running a patched scheduler, It did not hang again. I gave em about 15 hours testing.

But maybe that's all pure luck.

Quoted 1 line

Ingo

Unable to handle kernel paging request at virtual address 00099108
 printing eip:
b01010c0
*pde = 00000000
Oops: 0000 [#1]
SMP 
Modules linked in: nfs lockd sunrpc nvidia ipv6 ohci_hcd tuner tvaudio msp3400 bttv video_buf firmware_class btcx_risc ehci_hcd uhci_hcd usbcore intel_agp agpgart evdev
CPU:    0
EIP:    0060:[<b01010c0>]    Tainted: P      VLI
EFLAGS: 00010286   (2.6.10-rc2-ck2-nozeroram-findvmastat)

EIP is at cpu_idle+0x31/0x3f
eax: 00000001 ebx: 00099100 ecx: 00000000 edx: 0000001d
esi: 00000000 edi: b03dff9c ebp: b03dffe4 esp: b03dffe0
ds: 007b es: 007b ss: 0068
Process swapper (pid: 0, threadinfo=b03de000 task=b034db40)

Stack: 00020800 b03dfff8 b03e0898 000000bd b03e0340 b040cb80 0044f007 b0100211 
Call Trace:
 [<b0103c00>] show_stack+0x7a/0x90
 [<b0103d81>] show_registers+0x152/0x1ca
 [<b0103f86>] die+0xf4/0x178
 [<b0114556>] do_page_fault+0x42a/0x645
 [<b01038a7>] error_code+0x2b/0x30
 [<b03e0898>] start_kernel+0x13a/0x151
 [<b0100211>] 0xb0100211
Code: e0 ff ff 21 e3 eb 24 8b 0d 84 c6 40 b0 b8 26 10 10 b0 8b 15 c0 eb 34 b0 85 c9 0f 44 c8 8b 43 10 c1 e0 07 89 90 84 52 41 b0 ff d1 <8b> 43 08 a8 08 74 d5 e8 d8 7f 1f 00 eb f2 55 89 e5 56 53 fb ba 
 <0>Kernel panic - not syncing: Attempted to kill the idle task!

b010108f <cpu_idle>:
b010108f: 55 push %ebp
b0101090: 89 e5 mov %esp,%ebp
b0101092: 53 push %ebx
b0101093: bb 00 e0 ff ff mov $0xffffe000,%ebx
b0101098: 21 e3 and %esp,%ebx
b010109a: eb 24 jmp b01010c0 <cpu_idle+0x31>
b010109c: 8b 0d 84 c6 40 b0 mov 0xb040c684,%ecx
b01010a2: b8 26 10 10 b0 mov $0xb0101026,%eax
b01010a7: 8b 15 c0 eb 34 b0 mov 0xb034ebc0,%edx
b01010ad: 85 c9 test %ecx,%ecx
b01010af: 0f 44 c8 cmove %eax,%ecx
b01010b2: 8b 43 10 mov 0x10(%ebx),%eax
b01010b5: c1 e0 07 shl $0x7,%eax
b01010b8: 89 90 84 52 41 b0 mov %edx,0xb0415284(%eax)
b01010be: ff d1 call *%ecx
b01010c0: 8b 43 08 mov 0x8(%ebx),%eax
^^^^^^^^^^^^^^^^^^ OOPS. This is the check to need_resched().
b01010c3: a8 08 test $0x8,%al
b01010c5: 74 d5 je b010109c <cpu_idle+0xd>
b01010c7: e8 d8 7f 1f 00 call b02f90a4 <schedule>
b01010cc: eb f2 jmp b01010c0 <cpu_idle+0x31>

Ah, and yes, the kernel is tainted. So Nvidia already received a bugreport.

Signature · 1 line
Regards Michael Buesch  [ http://www.tuxsoft.de.vu ]
#6 lore raw
In reply to Michael Buesch
Quoted 9 lines · Michael Buesch

Someone else said to me:
[quote]
"another runqueue might want to touch your runqueue while you're in scheduler_tick" ...
"that is far more likely to hit with many many cpus and I'd be surprised if you're the first person to get a race there" [/quote]

What about this? Is this possible? Or did she/he/it miss a point?

it might touch the runqueue but it wont move _your task_ off from under you. So setting need_resched of the current task is perfectly fine.

Ingo