[PATCH] clean up ret_from_{irq,exception}

[PATCH] clean up ret_from_{irq,exception}

[Franck Bui-Huu]

From: Franck Bui-Huu <fbuihuu@gmail.com>

This patch makes these routines a lot more readable whatever
the value of CONFIG_PREEMPT.

It also moves one branch instruction from ret_from_irq()
to ret_from_exception(). Therefore we favour the return
from irq path which should be more common than the other
one.

Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
---
 arch/mips/kernel/entry.S |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/arch/mips/kernel/entry.S b/arch/mips/kernel/entry.S
index f10b6a1..571029b 100644
--- a/arch/mips/kernel/entry.S
+++ b/arch/mips/kernel/entry.S
@@ -21,23 +21,18 @@
 #endif
 
 #ifndef CONFIG_PREEMPT
-	.macro	preempt_stop
-	local_irq_disable
-	.endm
 #define resume_kernel	restore_all
 #endif
 
 	.text
 	.align	5
-FEXPORT(ret_from_irq)
-	LONG_S	s0, TI_REGS($28)
-#ifdef CONFIG_PREEMPT
-FEXPORT(ret_from_exception)
-#else
-	b	_ret_from_irq
 FEXPORT(ret_from_exception)
-	preempt_stop
+#ifndef CONFIG_PREEMPT
+	local_irq_disable			# preempt stop
 #endif
+	b	_ret_from_irq
+FEXPORT(ret_from_irq)
+	LONG_S	s0, TI_REGS($28)
 FEXPORT(_ret_from_irq)
 	LONG_L	t0, PT_STATUS(sp)		# returning to kernel mode?
 	andi	t0, t0, KU_USER
-- 
1.4.4.3.ge6d4

Re: [PATCH] clean up ret_from_{irq,exception}

[Atsushi Nemoto]

On Tue, 06 Feb 2007 16:53:27 +0100, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> This patch makes these routines a lot more readable whatever
> the value of CONFIG_PREEMPT.
> 
> It also moves one branch instruction from ret_from_irq()
> to ret_from_exception(). Therefore we favour the return
> from irq path which should be more common than the other
> one.

After this patch, entry.S becomes:

FEXPORT(ret_from_exception)
#ifndef CONFIG_PREEMPT
	local_irq_disable			# preempt stop
#endif
	b	_ret_from_irq
FEXPORT(ret_from_irq)
	LONG_S	s0, TI_REGS($28)
FEXPORT(_ret_from_irq)


Apparently your patch add an additional branch in critical path in
CONFIG_PREEMPT=y case.

Maybe this would be better?

#ifdef CONFIG_PREEMPT
FEXPORT(ret_from_irq)
	LONG_S	s0, TI_REGS($28)
FEXPORT(ret_from_exception)
#else
FEXPORT(ret_from_exception)
	local_irq_disable			# preempt stop
	b	_ret_from_irq
FEXPORT(ret_from_irq)
	LONG_S	s0, TI_REGS($28)
#endif
FEXPORT(_ret_from_irq)

---
Atsushi Nemoto

Re: [PATCH] clean up ret_from_{irq,exception}

[Franck Bui-Huu]

Hi Atsushi,

On 2/10/07, Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> Maybe this would be better?
>
> #ifdef CONFIG_PREEMPT
> FEXPORT(ret_from_irq)
>         LONG_S  s0, TI_REGS($28)
> FEXPORT(ret_from_exception)
> #else
> FEXPORT(ret_from_exception)
>         local_irq_disable                       # preempt stop
>         b       _ret_from_irq
> FEXPORT(ret_from_irq)
>         LONG_S  s0, TI_REGS($28)
> #endif
> FEXPORT(_ret_from_irq)
>

well maybe this one would be more readable:

-- >8 --

diff --git a/arch/mips/kernel/entry.S b/arch/mips/kernel/entry.S
index f10b6a1..b5d27d5 100644
--- a/arch/mips/kernel/entry.S
+++ b/arch/mips/kernel/entry.S
@@ -21,23 +21,20 @@
 #endif

 #ifndef CONFIG_PREEMPT
-	.macro	preempt_stop
-	local_irq_disable
-	.endm
 #define resume_kernel	restore_all
+#else
+#define _ret_from_irq	ret_from_exception
 #endif

 	.text
 	.align	5
-FEXPORT(ret_from_irq)
-	LONG_S	s0, TI_REGS($28)
-#ifdef CONFIG_PREEMPT
+#ifndef CONFIG_PREEMPT
 FEXPORT(ret_from_exception)
-#else
+	local_irq_disable			# preempt stop
 	b	_ret_from_irq
-FEXPORT(ret_from_exception)
-	preempt_stop
 #endif
+FEXPORT(ret_from_irq)
+	LONG_S	s0, TI_REGS($28)
 FEXPORT(_ret_from_irq)
 	LONG_L	t0, PT_STATUS(sp)		# returning to kernel mode?
 	andi	t0, t0, KU_USER



-- 
               Franck

Re: [PATCH] clean up ret_from_{irq,exception}

[Atsushi Nemoto]

On Mon, 12 Feb 2007 09:44:33 +0100, "Franck Bui-Huu" <vagabon.xyz@gmail.com> wrote:
> well maybe this one would be more readable:
> 
> -- >8 --
> 
> diff --git a/arch/mips/kernel/entry.S b/arch/mips/kernel/entry.S
> index f10b6a1..b5d27d5 100644
> --- a/arch/mips/kernel/entry.S
> +++ b/arch/mips/kernel/entry.S
> @@ -21,23 +21,20 @@
>  #endif
> 
>  #ifndef CONFIG_PREEMPT
> -	.macro	preempt_stop
> -	local_irq_disable
> -	.endm
>  #define resume_kernel	restore_all
> +#else
> +#define _ret_from_irq	ret_from_exception
>  #endif

_ret_from_irq is used by dec/int-handler.S directly, so you should not
remove it (though decstation_defconfig disables CONFIG_PREEMPT).

But looking at dec/int-handler.S again, I can not see why it uses
_ret_from_irq, and why it manipulates TI_REGS($28) in handle_it ...

It seems dec/int-handler.S has been broken for a while.  I'll send a
patch to fix it.  If it was ACKed, I ACK your patch.

---
Atsushi Nemoto

Re: [PATCH] clean up ret_from_{irq,exception}

[Franck Bui-Huu]

On 2/12/07, Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
>
> _ret_from_irq is used by dec/int-handler.S directly, so you should not
> remove it (though decstation_defconfig disables CONFIG_PREEMPT).
>

woah, you're right. the name '_ret_from_irq' looks so private to me
that I didn't think that some other code could use it...

> But looking at dec/int-handler.S again, I can not see why it uses
> _ret_from_irq, and why it manipulates TI_REGS($28) in handle_it ...
>
> It seems dec/int-handler.S has been broken for a while.  I'll send a
> patch to fix it.  If it was ACKed, I ACK your patch.
>

ok I'm waiting...
-- 
               Franck

Re: [PATCH] clean up ret_from_{irq,exception}

[Maciej W. Rozycki]

On Mon, 12 Feb 2007, Atsushi Nemoto wrote:

> _ret_from_irq is used by dec/int-handler.S directly, so you should not
> remove it (though decstation_defconfig disables CONFIG_PREEMPT).
> 
> But looking at dec/int-handler.S again, I can not see why it uses
> _ret_from_irq, and why it manipulates TI_REGS($28) in handle_it ...
> 
> It seems dec/int-handler.S has been broken for a while.  I'll send a
> patch to fix it.  If it was ACKed, I ACK your patch.

 It works for me with 2.6.18 -- if it needs an update due to more recent 
changes, then I have not got there yet. ;-)  I plan to rewrite this bit in 
C if feasible like other platforms did too.

  Maciej

Re: [PATCH] clean up ret_from_{irq,exception}

[Atsushi Nemoto]

On Mon, 12 Feb 2007 16:51:57 +0000 (GMT), "Maciej W. Rozycki" <macro@linux-mips.org> wrote:
> > It seems dec/int-handler.S has been broken for a while.  I'll send a
> > patch to fix it.  If it was ACKed, I ACK your patch.
> 
>  It works for me with 2.6.18 -- if it needs an update due to more recent 
> changes, then I have not got there yet. ;-)  I plan to rewrite this bit in 
> C if feasible like other platforms did too.

The breakage happened during 2.6.19 development cycle (large pt_regs
argument removal).

---
Atsushi Nemoto