Fix preemption race [1/3] (Core)

This patch adjusts the ARM entry code to use the fixed up preempt_schedule() handling in 2.6.10-mm2

Signed-off-by Benedikt Spranger <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
 entry-armv.S |    4 +---
 1 files changed, 1 insertion(+), 3 deletions(-)

---
Index: 2.6.10-mm1/arch/ppc/kernel/entry.S
===================================================================

arch/ppc/kernel/entry.S +2 -3

@@ -624,12 +624,12 @@
 	beq+	restore
 	andi.	r0,r3,MSR_EE	/* interrupts off? */
 	beq	restore		/* don't schedule if so */
-1:	lis	r0,PREEMPT_ACTIVE@h
+1:	li	r0,1
 	stw	r0,TI_PREEMPT(r9)
 	ori	r10,r10,MSR_EE
 	SYNC
 	MTMSRD(r10)		/* hard-enable interrupts */
-	bl	schedule
+	bl	entry_preempt_schedule
 	LOAD_MSR_KERNEL(r10,MSR_KERNEL)
 	SYNC
 	MTMSRD(r10)		/* disable interrupts */
 
 
-

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 2 lines · Thomas Gleixner

This patch adjusts the ARM entry code to use the fixed up preempt_schedule() handling in 2.6.10-mm2

Looks PPCish to me.

This patch adjusts the ARM entry code to use the fixed up
preempt_schedule() handling in 2.6.10-mm2

...
Index: 2.6.10-mm1/arch/arm/kernel/entry.S

Quoted 6 lines · Thomas Gleixner

This patch adjusts the ARM entry code to use the fixed up
preempt_schedule() handling in 2.6.10-mm2

...
Index: 2.6.10-mm1/arch/arm/kernel/entry.S

There's no such file. I assumed you meant entry-armv.S and ended up with the below.

arch/arm/kernel/entry-armv.S renamed from arch/arm/kernel/entry-armv.S~use-the-new-preemption-code-arm +1 -3

@@ -136,10 +136,8 @@ svc_preempt:	teq	r9, #0				@ was preempt
 		ldr	r1, [r6, #8]			@ local_bh_count
 		adds	r0, r0, r1
 		movne	pc, lr
-		mov	r7, #PREEMPT_ACTIVE
-		str	r7, [r8, #TI_PREEMPT]		@ set PREEMPT_ACTIVE
 1:		enable_irq r2				@ enable IRQs
-		bl	schedule
+               bl      entry_preempt_schedule
 		disable_irq r0				@ disable IRQs
 		ldr	r0, [r8, #TI_FLAGS]		@ get new tasks TI_FLAGS
 		tst	r0, #_TIF_NEED_RESCHED

_

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 5 lines · Russell King

Quoted 2 lines · Thomas Gleixner

This patch adjusts the ARM entry code to use the fixed up preempt_schedule() handling in 2.6.10-mm2

Looks PPCish to me.

Sorry I messed that up. Call me the idiot of today.

This patch adjusts the ARM entry code to use the fixed up preempt_schedule() handling in 2.6.10-mm2

Quoted 7 lines · Thomas Gleixner

Quoted 5 lines · Russell King

Quoted 2 lines · Thomas Gleixner

This patch adjusts the ARM entry code to use the fixed up preempt_schedule() handling in 2.6.10-mm2

Looks PPCish to me.

Quoted 4 lines

Sorry I messed that up. Call me the idiot of today.

This patch adjusts the ARM entry code to use the fixed up preempt_schedule() handling in 2.6.10-mm2

Are you sure ARM suffers from this race condition? It sets preempt count before enabling IRQs and doesn't use preempt_schedule().

Quoted 2 lines

Are you sure ARM suffers from this race condition? It sets preempt count before enabling IRQs and doesn't use preempt_schedule().

There is no race for arm, but using the preempt_schedule() interface is the approach which Ingo suggested for common usage, but his x86 implementation was racy, so I fixed this first before modifying arm to use the interface. Ingo pointed out that he will change it to preempt_schedule_irq, but I'm not religious about the name.

i wouldnt raise this issue if it was the name only, but there's more to preempt_schedule_irq() than its name: it gets called with irqs off and the scheduler returns with irqs off and with a guarantee that there is no (irq-generated) pending preemption request for this task right now. I.e. the checks for need_resched can be skipped, and interrupts dont have to be disabled to do a safe return-to-usermode (as done on some architectures).

as far as i can see do_preempt_schedule() doesnt have these properties:
what it guarantees is that it avoids the preemption recursion via the lowlevel code doing the PREEMPT_ACTIVE setting.

lets agree upon a single, common approach. I went for splitting up preempt_schedule() into two variants: the 'synchronous' one (called preempt_schedule()) is only called from syscall level and has no repeat-preemption and hence stack-recursion worries. The 'asynchronous' one (called preempt_schedule_irq()) is called from asynchronous contexts (hardirq events) and is fully ready to deal with all the reentrancy situations that may occur. It's careful about not re-enabling interrupts, etc.

This patch adjusts the PPC entry code to use the fixed up preempt_schedule() handling in 2.6.10-mm2

Signed-off-by Thomas Gleixner <[email protected]>

---
 entry.S |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

---
Index: 2.6.10-mm1/arch/ppc/kernel/entry.S
===================================================================

Quoted 15 lines

This patch adjusts the PPC entry code to use the fixed up preempt_schedule() handling in 2.6.10-mm2

Signed-off-by Thomas Gleixner <[email protected]>

---
 entry.S |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

---
Index: 2.6.10-mm1/arch/ppc/kernel/entry.S
===================================================================

arch/ppc/kernel/entry.S (working copy) renamed from arch/ppc/kernel/entry.S (revision 141) +0 -0

@@ -624,12 +624,12 @@

arch/ppc/kernel/entry.S (working copy) line 627

Quoted 5 lines

arch/ppc/kernel/entry.S (working copy) renamed from arch/ppc/kernel/entry.S (revision 141) +1 -1

        beq+    restore
        andi.   r0,r3,MSR_EE    /* interrupts off? */
        beq     restore         /* don't schedule if so */
-1:     lis     r0,PREEMPT_ACTIVE@h
+1:     li      r0,1

Perhaps I just don't have enough context, but is there good reason to use a magic constant instead of a define ?

The idle-thread-preemption-fix.patch introduced a race, which is not critical, but might give us an extra turn through the scheduler. When interrupts are reenabled in entry.c and an interrupt occures before we reach the add_preempt_schedule() in preempt_schedule we get rescheduled again in the return from interrupt path.

Quoted 5 lines · * [email protected] <[email protected]> wrote:

The idle-thread-preemption-fix.patch introduced a race, which is not critical, but might give us an extra turn through the scheduler. When interrupts are reenabled in entry.c and an interrupt occures before we reach the add_preempt_schedule() in preempt_schedule we get rescheduled again in the return from interrupt path.

i agree that there's a race. I solved this in the -RT patchset a couple of weeks ago, but in a different wasy. I introduced the preempt_schedule_irq() function and this solves the problem via keeping the whole IRQ-preemption path irqs-off. This has the advantage that if an IRQ signals preemption of a task and the kernel is immediately preemptable then we are able to hit that task atomically without re-enabling IRQs again. I'll split out this patch - can you see any problems with the preempt_schedule_irq() approach?