Re: [PATCH] rc2-mm2: unneeded cli/sti in fix-preemption-race patch

Re: [PATCH] rc2-mm2: unneeded cli/sti in fix-preemption-race patch

[Thomas Gleixner]

On Mon, 2005-01-31 at 16:24 +0300, Oleg Nesterov wrote:
> Hello.
> 
> Thomas Gleixner wrote:
> >
> > The patch prevents this by leaving interrupts disabled and calling
> > a seperate function preempt_schedule_irq().
> 
> sched-fix-preemption-race-core-i386.patch:
> >
> >	+++ 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.

Right. I missed the preempt_stop == cli.

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

> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
> 
> --- 2.6.11-rc2-mm2/arch/i386/kernel/entry.S~	Mon Jan 31 14:09:37 2005
> +++ 2.6.11-rc2-mm2/arch/i386/kernel/entry.S	Mon Jan 31 14:55:25 2005
> @@ -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:
> --- 2.6.11-rc2-mm2/kernel/sched.c~	Mon Jan 31 14:09:54 2005
> +++ 2.6.11-rc2-mm2/kernel/sched.c	Mon Jan 31 14:57:44 2005
> @@ -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

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

[Oleg Nesterov]

Hello.

Thomas Gleixner wrote:
>
> The patch prevents this by leaving interrupts disabled and calling
> a seperate function preempt_schedule_irq().

sched-fix-preemption-race-core-i386.patch:
>
>	+++ 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.

>	+++ 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?

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- 2.6.11-rc2-mm2/arch/i386/kernel/entry.S~	Mon Jan 31 14:09:37 2005
+++ 2.6.11-rc2-mm2/arch/i386/kernel/entry.S	Mon Jan 31 14:55:25 2005
@@ -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:
--- 2.6.11-rc2-mm2/kernel/sched.c~	Mon Jan 31 14:09:54 2005
+++ 2.6.11-rc2-mm2/kernel/sched.c	Mon Jan 31 14:57:44 2005
@@ -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