* Loading handle_arch_irq with a PC relative load
@ 2012-07-13 19:05 Gilles Chanteperdrix
2012-07-13 19:40 ` Nicolas Pitre
0 siblings, 1 reply; 6+ messages in thread
From: Gilles Chanteperdrix @ 2012-07-13 19:05 UTC (permalink / raw)
To: linux-arm-kernel
I do not know if it is really useful, but it seems it would be possible
to reduce the number of memory accesses to just one in the irq_handler
macro in the case where CONFIG_MULTI_IRQ_HANDLER is enabled, by using a
PC relative load, with something like the following patch:
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index 0d1851c..48ee46a 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -37,10 +37,9 @@
*/
.macro irq_handler
#ifdef CONFIG_MULTI_IRQ_HANDLER
- ldr r1, =handle_arch_irq
mov r0, sp
adr lr, BSYM(9997f)
- ldr pc, [r1]
+ ldr pc, handle_arch_irq
#else
arch_irq_handler_default
#endif
@@ -325,6 +324,12 @@ ENDPROC(__pabt_svc)
#endif
.LCfp:
.word fp_enter
+#ifdef CONFIG_MULTI_IRQ_HANDLER
+ .globl handle_arch_irq
+handle_arch_irq:
+ .space 4
+#endif
+
/*
* User mode handlers
@@ -1151,9 +1156,3 @@ cr_alignment:
.space 4
cr_no_alignment:
.space 4
-
-#ifdef CONFIG_MULTI_IRQ_HANDLER
- .globl handle_arch_irq
-handle_arch_irq:
- .space 4
-#endif
--
Gilles.
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Loading handle_arch_irq with a PC relative load
2012-07-13 19:05 Loading handle_arch_irq with a PC relative load Gilles Chanteperdrix
@ 2012-07-13 19:40 ` Nicolas Pitre
2012-07-13 19:51 ` Gilles Chanteperdrix
0 siblings, 1 reply; 6+ messages in thread
From: Nicolas Pitre @ 2012-07-13 19:40 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 13 Jul 2012, Gilles Chanteperdrix wrote:
>
> I do not know if it is really useful, but it seems it would be possible
> to reduce the number of memory accesses to just one in the irq_handler
> macro in the case where CONFIG_MULTI_IRQ_HANDLER is enabled, by using a
> PC relative load, with something like the following patch:
To be strict with ccode sections, you can't do this. The
handle_arch_irq symbol identifies a variable and with your patch you're
moving it from the .data section to the .text section. The .text
section is meant to be read only, and this is even more true when using
a XIP kernel where .text is in ROM, or if we could make the access
protection of the kernel ro.
> diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
> index 0d1851c..48ee46a 100644
> --- a/arch/arm/kernel/entry-armv.S
> +++ b/arch/arm/kernel/entry-armv.S
> @@ -37,10 +37,9 @@
> */
> .macro irq_handler
> #ifdef CONFIG_MULTI_IRQ_HANDLER
> - ldr r1, =handle_arch_irq
> mov r0, sp
> adr lr, BSYM(9997f)
> - ldr pc, [r1]
> + ldr pc, handle_arch_irq
> #else
> arch_irq_handler_default
> #endif
> @@ -325,6 +324,12 @@ ENDPROC(__pabt_svc)
> #endif
> .LCfp:
> .word fp_enter
> +#ifdef CONFIG_MULTI_IRQ_HANDLER
> + .globl handle_arch_irq
> +handle_arch_irq:
> + .space 4
> +#endif
> +
>
> /*
> * User mode handlers
> @@ -1151,9 +1156,3 @@ cr_alignment:
> .space 4
> cr_no_alignment:
> .space 4
> -
> -#ifdef CONFIG_MULTI_IRQ_HANDLER
> - .globl handle_arch_irq
> -handle_arch_irq:
> - .space 4
> -#endif
>
>
> --
> Gilles.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Loading handle_arch_irq with a PC relative load
2012-07-13 19:40 ` Nicolas Pitre
@ 2012-07-13 19:51 ` Gilles Chanteperdrix
2012-07-13 20:09 ` Nicolas Pitre
0 siblings, 1 reply; 6+ messages in thread
From: Gilles Chanteperdrix @ 2012-07-13 19:51 UTC (permalink / raw)
To: linux-arm-kernel
On 07/13/2012 09:40 PM, Nicolas Pitre wrote:
> On Fri, 13 Jul 2012, Gilles Chanteperdrix wrote:
>
>>
>> I do not know if it is really useful, but it seems it would be possible
>> to reduce the number of memory accesses to just one in the irq_handler
>> macro in the case where CONFIG_MULTI_IRQ_HANDLER is enabled, by using a
>> PC relative load, with something like the following patch:
>
> To be strict with ccode sections, you can't do this. The
> handle_arch_irq symbol identifies a variable and with your patch you're
> moving it from the .data section to the .text section. The .text
> section is meant to be read only, and this is even more true when using
> a XIP kernel where .text is in ROM, or if we could make the access
> protection of the kernel ro.
I understand that but, XIP kernel aside, the handle_arch_irq variable is
set only once very early during the boot process, so, almost read-only.
Is not Linux using self-modifying code in some cases anyway (booting an
SMP kernel on an UP processor for instance).
--
Gilles.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Loading handle_arch_irq with a PC relative load
2012-07-13 19:51 ` Gilles Chanteperdrix
@ 2012-07-13 20:09 ` Nicolas Pitre
2012-07-13 20:13 ` Gilles Chanteperdrix
2012-07-14 10:39 ` Gilles Chanteperdrix
0 siblings, 2 replies; 6+ messages in thread
From: Nicolas Pitre @ 2012-07-13 20:09 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 13 Jul 2012, Gilles Chanteperdrix wrote:
> On 07/13/2012 09:40 PM, Nicolas Pitre wrote:
> > On Fri, 13 Jul 2012, Gilles Chanteperdrix wrote:
> >
> >>
> >> I do not know if it is really useful, but it seems it would be possible
> >> to reduce the number of memory accesses to just one in the irq_handler
> >> macro in the case where CONFIG_MULTI_IRQ_HANDLER is enabled, by using a
> >> PC relative load, with something like the following patch:
> >
> > To be strict with ccode sections, you can't do this. The
> > handle_arch_irq symbol identifies a variable and with your patch you're
> > moving it from the .data section to the .text section. The .text
> > section is meant to be read only, and this is even more true when using
> > a XIP kernel where .text is in ROM, or if we could make the access
> > protection of the kernel ro.
>
> I understand that but, XIP kernel aside, the handle_arch_irq variable is
> set only once very early during the boot process, so, almost read-only.
> Is not Linux using self-modifying code in some cases anyway (booting an
> SMP kernel on an UP processor for instance).
There are limits to which such tricks should be applied. In the SMP on
UP case this is a matter of making the kernel boot at all which is a
rather strong reason.
Do you have performance numbers like interrupt latency that show this
patch being worth it? Without concrete justifications I don't think we
should go down that path.
Nicolas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Loading handle_arch_irq with a PC relative load
2012-07-13 20:09 ` Nicolas Pitre
@ 2012-07-13 20:13 ` Gilles Chanteperdrix
2012-07-14 10:39 ` Gilles Chanteperdrix
1 sibling, 0 replies; 6+ messages in thread
From: Gilles Chanteperdrix @ 2012-07-13 20:13 UTC (permalink / raw)
To: linux-arm-kernel
On 07/13/2012 10:09 PM, Nicolas Pitre wrote:
> On Fri, 13 Jul 2012, Gilles Chanteperdrix wrote:
>
>> On 07/13/2012 09:40 PM, Nicolas Pitre wrote:
>>> On Fri, 13 Jul 2012, Gilles Chanteperdrix wrote:
>>>
>>>>
>>>> I do not know if it is really useful, but it seems it would be possible
>>>> to reduce the number of memory accesses to just one in the irq_handler
>>>> macro in the case where CONFIG_MULTI_IRQ_HANDLER is enabled, by using a
>>>> PC relative load, with something like the following patch:
>>>
>>> To be strict with ccode sections, you can't do this. The
>>> handle_arch_irq symbol identifies a variable and with your patch you're
>>> moving it from the .data section to the .text section. The .text
>>> section is meant to be read only, and this is even more true when using
>>> a XIP kernel where .text is in ROM, or if we could make the access
>>> protection of the kernel ro.
>>
>> I understand that but, XIP kernel aside, the handle_arch_irq variable is
>> set only once very early during the boot process, so, almost read-only.
>> Is not Linux using self-modifying code in some cases anyway (booting an
>> SMP kernel on an UP processor for instance).
>
> There are limits to which such tricks should be applied. In the SMP on
> UP case this is a matter of making the kernel boot at all which is a
> rather strong reason.
>
> Do you have performance numbers like interrupt latency that show this
> patch being worth it? Without concrete justifications I don't think we
> should go down that path.
I intend to do some interrupt latency measurements soon. But I suspect
CONFIG_MULTI_IRQ_HANDLER will cause more differences due to the fact
that the irq handlers are now fat C compiled code instead of carefully
optimized assembly code, than because of these two memory accesses.
And in fact, chances are that I will observe nothing at all since the
low end platforms I have are AT91 which are not using
CONFIG_MULTI_IRQ_HANDLER yet.
--
Gilles.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Loading handle_arch_irq with a PC relative load
2012-07-13 20:09 ` Nicolas Pitre
2012-07-13 20:13 ` Gilles Chanteperdrix
@ 2012-07-14 10:39 ` Gilles Chanteperdrix
1 sibling, 0 replies; 6+ messages in thread
From: Gilles Chanteperdrix @ 2012-07-14 10:39 UTC (permalink / raw)
To: linux-arm-kernel
On 07/13/2012 10:09 PM, Nicolas Pitre wrote:
> On Fri, 13 Jul 2012, Gilles Chanteperdrix wrote:
>
>> On 07/13/2012 09:40 PM, Nicolas Pitre wrote:
>>> On Fri, 13 Jul 2012, Gilles Chanteperdrix wrote:
>>>
>>>>
>>>> I do not know if it is really useful, but it seems it would be possible
>>>> to reduce the number of memory accesses to just one in the irq_handler
>>>> macro in the case where CONFIG_MULTI_IRQ_HANDLER is enabled, by using a
>>>> PC relative load, with something like the following patch:
>>>
>>> To be strict with ccode sections, you can't do this. The
>>> handle_arch_irq symbol identifies a variable and with your patch you're
>>> moving it from the .data section to the .text section. The .text
>>> section is meant to be read only, and this is even more true when using
>>> a XIP kernel where .text is in ROM, or if we could make the access
>>> protection of the kernel ro.
>>
>> I understand that but, XIP kernel aside, the handle_arch_irq variable is
>> set only once very early during the boot process, so, almost read-only.
>> Is not Linux using self-modifying code in some cases anyway (booting an
>> SMP kernel on an UP processor for instance).
>
> There are limits to which such tricks should be applied. In the SMP on
> UP case this is a matter of making the kernel boot at all which is a
> rather strong reason.
>
> Do you have performance numbers like interrupt latency that show this
> patch being worth it? Without concrete justifications I don't think we
> should go down that path.
So, I ran a few tests on at91rm9200, where I expected the differences to
be most visible.
First I enabled CONFIG_MULTI_IRQ_HANDLER and wrote the irq decoding
handler in plain C. This increases the irq latency of 1.2us (measured
with the average irq latency on an idle system).
I rewrote this irq decoding handler in assembly, using the macros in
entry-macro.S. This decreases the irq latency of 600ns.
Then I try the trick at the beginning of this thread, and... could not
measure any difference, so, you were right.
Anyway, given that on at91rm9200 worst case irq latencies are in the
80us range, all these optimizations are pointless.
--
Gilles.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-07-14 10:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-07-13 19:05 Loading handle_arch_irq with a PC relative load Gilles Chanteperdrix
2012-07-13 19:40 ` Nicolas Pitre
2012-07-13 19:51 ` Gilles Chanteperdrix
2012-07-13 20:09 ` Nicolas Pitre
2012-07-13 20:13 ` Gilles Chanteperdrix
2012-07-14 10:39 ` Gilles Chanteperdrix
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).