Skip to contentSkip to conversation

rc2-mm2: unneeded cli/sti in fix-preemption-race patch

PATCH In review Oleg Nesterov 2 participants 2 messages Jan 31, 2005

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 rc2-mm2: unneeded cli/sti in fix-preemption-race patch

Hello.

Quoted 3 lines · Thomas Gleixner

The patch prevents this by leaving interrupts disabled and calling a seperate function preempt_schedule_irq().

sched-fix-preemption-race-core-i386.patch:

Quoted 7 lines
	+++ 25-akpm/arch/i386/kernel/entry.S
	@@ -189,6 +189,7 @@ ENTRY(resume_userspace)
	 
	 #ifdef CONFIG_PREEMPT
	 ENTRY(resume_kernel)
	+	cli

I think this 'cli' is unneeded. resume_kernel: is used on return path from do_IRQ() which leaves interrupts disabled. irq_desc->handler->end() should not enable interrupts, yes?

And we have preempt_stop==cli in ret_from_exception: case, before ret_from_intr.

Quoted 8 lines
	+++ 25-akpm/kernel/sched.c
	@@ -2872,6 +2872,48 @@ need_resched:
	...
	+asmlinkage void __sched preempt_schedule_irq(void)
	+{
	...
	+	local_irq_enable();
	+	schedule();

It is ok to enter schedule() with interrupts disabled, so this 'sti' seems to be unneeded too.

Am I missed something?

arch/i386/kernel/entry.S renamed from arch/i386/kernel/entry.S~ +0 -1
@@ -189,7 +189,6 @@ ENTRY(resume_userspace)
 
 #ifdef CONFIG_PREEMPT
 ENTRY(resume_kernel)
-	cli
 	cmpl $0,TI_preempt_count(%ebp)	# non-zero preempt_count ?
 	jnz restore_all
 need_resched:
kernel/sched.c renamed from kernel/sched.c~ +0 -1
@@ -2971,7 +2971,6 @@ need_resched:
 	saved_lock_depth = task->lock_depth;
 	task->lock_depth = -1;
 #endif
-	local_irq_enable();
 	schedule();
 	local_irq_disable();
 #ifdef CONFIG_PREEMPT_BKL
#1 lore raw
Quoted 16 lines · Oleg Nesterov

Hello.

Quoted 3 lines · Thomas Gleixner

The patch prevents this by leaving interrupts disabled and calling a seperate function preempt_schedule_irq().

sched-fix-preemption-race-core-i386.patch:

Quoted 7 lines
	+++ 25-akpm/arch/i386/kernel/entry.S
	@@ -189,6 +189,7 @@ ENTRY(resume_userspace)
	 
	 #ifdef CONFIG_PREEMPT
	 ENTRY(resume_kernel)
	+	cli
Quoted 6 lines

I think this 'cli' is unneeded. resume_kernel: is used on return path from do_IRQ() which leaves interrupts disabled. irq_desc->handler->end() should not enable interrupts, yes?

And we have preempt_stop==cli in ret_from_exception: case, before ret_from_intr.

Right. I missed the preempt_stop == cli.

Quoted 11 lines
Quoted 8 lines
	+++ 25-akpm/kernel/sched.c
	@@ -2872,6 +2872,48 @@ need_resched:
	...
	+asmlinkage void __sched preempt_schedule_irq(void)
	+{
	...
	+	local_irq_enable();
	+	schedule();

It is ok to enter schedule() with interrupts disabled, so this 'sti' seems to be unneeded too.

Makes sense.

tglx

Quoted 22 lines
arch/i386/kernel/entry.S renamed from arch/i386/kernel/entry.S~ +0 -1
@@ -189,7 +189,6 @@ ENTRY(resume_userspace)
 
 #ifdef CONFIG_PREEMPT
 ENTRY(resume_kernel)
-	cli
 	cmpl $0,TI_preempt_count(%ebp)	# non-zero preempt_count ?
 	jnz restore_all
 need_resched:
kernel/sched.c renamed from kernel/sched.c~ +0 -1
@@ -2971,7 +2971,6 @@ need_resched:
 	saved_lock_depth = task->lock_depth;
 	task->lock_depth = -1;
 #endif
-	local_irq_enable();
 	schedule();
 	local_irq_disable();
 #ifdef CONFIG_PREEMPT_BKL