* [PATCH] powerpc: inline current_stack_pointer()
@ 2016-05-23 8:46 Christophe Leroy
2016-05-23 17:17 ` Gabriel Paubert
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Christophe Leroy @ 2016-05-23 8:46 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, Scott Wood
Cc: linux-kernel, linuxppc-dev
current_stack_pointeur() is a single instruction function. it
It is not worth breaking the execution flow with a bl/blr for a
single instruction
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/reg.h | 7 ++++++-
arch/powerpc/kernel/misc.S | 4 ----
arch/powerpc/kernel/ppc_ksyms.c | 2 --
3 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index c1e82e9..7ce6777 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -1301,7 +1301,12 @@ static inline unsigned long mfvtb (void)
#define proc_trap() asm volatile("trap")
-extern unsigned long current_stack_pointer(void);
+static inline unsigned long current_stack_pointer(void)
+{
+ register unsigned long *ptr asm("r1");
+
+ return *ptr;
+}
extern unsigned long scom970_read(unsigned int address);
extern void scom970_write(unsigned int address, unsigned long value);
diff --git a/arch/powerpc/kernel/misc.S b/arch/powerpc/kernel/misc.S
index 0d43219..7ce26d4 100644
--- a/arch/powerpc/kernel/misc.S
+++ b/arch/powerpc/kernel/misc.S
@@ -114,7 +114,3 @@ _GLOBAL(longjmp)
mtlr r0
mr r3,r4
blr
-
-_GLOBAL(current_stack_pointer)
- PPC_LL r3,0(r1)
- blr
diff --git a/arch/powerpc/kernel/ppc_ksyms.c b/arch/powerpc/kernel/ppc_ksyms.c
index 9f01e28..eb5c5dc 100644
--- a/arch/powerpc/kernel/ppc_ksyms.c
+++ b/arch/powerpc/kernel/ppc_ksyms.c
@@ -33,5 +33,3 @@ EXPORT_SYMBOL(store_vr_state);
#ifdef CONFIG_EPAPR_PARAVIRT
EXPORT_SYMBOL(epapr_hypercall_start);
#endif
-
-EXPORT_SYMBOL(current_stack_pointer);
--
2.1.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc: inline current_stack_pointer()
2016-05-23 8:46 [PATCH] powerpc: inline current_stack_pointer() Christophe Leroy
@ 2016-05-23 17:17 ` Gabriel Paubert
2016-05-24 22:21 ` Paul Mackerras
2016-05-23 20:22 ` Segher Boessenkool
2016-05-31 10:05 ` Anton Blanchard
2 siblings, 1 reply; 7+ messages in thread
From: Gabriel Paubert @ 2016-05-23 17:17 UTC (permalink / raw)
To: Christophe Leroy
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Scott Wood, linuxppc-dev, linux-kernel
On Mon, May 23, 2016 at 10:46:02AM +0200, Christophe Leroy wrote:
> current_stack_pointeur() is a single instruction function. it
> It is not worth breaking the execution flow with a bl/blr for a
> single instruction
Are you sure that the result is always the same?
Calling an external function prevents the compiler from considering the
caller of of current_stack_pointer a leaf function, which certainly
does not help the compiler, but in a leaf function the compiler is free
not to establish a new frame.
If the compiler decides to establishes a new frame (typically with
"stwu r1,-frame_size(r1)"), *r1 is the previous stack pointer, or
the caller's stack pointer, or the current function frame pointer if
I remember correctly the ABI definitions.
However, if the compiler decides that it can get away without a frame
for the function, *r1 is the stack pointer of the caller's caller.
Depending on the application, this may or may not be important.
By the way, isn't there a GCC builtin which can perform this task,
for example builtin_frame_address()?
Gabriel
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/include/asm/reg.h | 7 ++++++-
> arch/powerpc/kernel/misc.S | 4 ----
> arch/powerpc/kernel/ppc_ksyms.c | 2 --
> 3 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index c1e82e9..7ce6777 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -1301,7 +1301,12 @@ static inline unsigned long mfvtb (void)
>
> #define proc_trap() asm volatile("trap")
>
> -extern unsigned long current_stack_pointer(void);
> +static inline unsigned long current_stack_pointer(void)
> +{
> + register unsigned long *ptr asm("r1");
> +
> + return *ptr;
> +}
>
> extern unsigned long scom970_read(unsigned int address);
> extern void scom970_write(unsigned int address, unsigned long value);
> diff --git a/arch/powerpc/kernel/misc.S b/arch/powerpc/kernel/misc.S
> index 0d43219..7ce26d4 100644
> --- a/arch/powerpc/kernel/misc.S
> +++ b/arch/powerpc/kernel/misc.S
> @@ -114,7 +114,3 @@ _GLOBAL(longjmp)
> mtlr r0
> mr r3,r4
> blr
> -
> -_GLOBAL(current_stack_pointer)
> - PPC_LL r3,0(r1)
> - blr
> diff --git a/arch/powerpc/kernel/ppc_ksyms.c b/arch/powerpc/kernel/ppc_ksyms.c
> index 9f01e28..eb5c5dc 100644
> --- a/arch/powerpc/kernel/ppc_ksyms.c
> +++ b/arch/powerpc/kernel/ppc_ksyms.c
> @@ -33,5 +33,3 @@ EXPORT_SYMBOL(store_vr_state);
> #ifdef CONFIG_EPAPR_PARAVIRT
> EXPORT_SYMBOL(epapr_hypercall_start);
> #endif
> -
> -EXPORT_SYMBOL(current_stack_pointer);
> --
> 2.1.0
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc: inline current_stack_pointer()
2016-05-23 8:46 [PATCH] powerpc: inline current_stack_pointer() Christophe Leroy
2016-05-23 17:17 ` Gabriel Paubert
@ 2016-05-23 20:22 ` Segher Boessenkool
2016-05-24 5:39 ` Christophe Leroy
2016-05-31 10:05 ` Anton Blanchard
2 siblings, 1 reply; 7+ messages in thread
From: Segher Boessenkool @ 2016-05-23 20:22 UTC (permalink / raw)
To: Christophe Leroy
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Scott Wood, linuxppc-dev, linux-kernel
On Mon, May 23, 2016 at 10:46:02AM +0200, Christophe Leroy wrote:
> +static inline unsigned long current_stack_pointer(void)
> +{
> + register unsigned long *ptr asm("r1");
> +
> + return *ptr;
> +}
Register asm is only guaranteed to work as input to inline asm. NAK.
Segher
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc: inline current_stack_pointer()
2016-05-23 20:22 ` Segher Boessenkool
@ 2016-05-24 5:39 ` Christophe Leroy
2016-05-24 6:08 ` Segher Boessenkool
0 siblings, 1 reply; 7+ messages in thread
From: Christophe Leroy @ 2016-05-24 5:39 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Scott Wood, linuxppc-dev, linux-kernel
Le 23/05/2016 à 22:22, Segher Boessenkool a écrit :
> On Mon, May 23, 2016 at 10:46:02AM +0200, Christophe Leroy wrote:
>> +static inline unsigned long current_stack_pointer(void)
>> +{
>> + register unsigned long *ptr asm("r1");
>> +
>> + return *ptr;
>> +}
> Register asm is only guaranteed to work as input to inline asm. NAK.
>
Does it mean that the following declaration in
arch/powerpc/include/asm/paca.h is wrong too ?
register struct paca_struct *local_paca asm("r13");
Christophe
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc: inline current_stack_pointer()
2016-05-24 5:39 ` Christophe Leroy
@ 2016-05-24 6:08 ` Segher Boessenkool
0 siblings, 0 replies; 7+ messages in thread
From: Segher Boessenkool @ 2016-05-24 6:08 UTC (permalink / raw)
To: Christophe Leroy
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Scott Wood, linuxppc-dev, linux-kernel
On Tue, May 24, 2016 at 07:39:59AM +0200, Christophe Leroy wrote:
> >>+static inline unsigned long current_stack_pointer(void)
> >>+{
> >>+ register unsigned long *ptr asm("r1");
> >>+
> >>+ return *ptr;
> >>+}
> >Register asm is only guaranteed to work as input to inline asm. NAK.
> >
> Does it mean that the following declaration in
> arch/powerpc/include/asm/paca.h is wrong too ?
>
> register struct paca_struct *local_paca asm("r13");
That one is fine, because it is a global var.
https://gcc.gnu.org/onlinedocs/gcc/Explicit-Register-Variables.html
Segher
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc: inline current_stack_pointer()
2016-05-23 17:17 ` Gabriel Paubert
@ 2016-05-24 22:21 ` Paul Mackerras
0 siblings, 0 replies; 7+ messages in thread
From: Paul Mackerras @ 2016-05-24 22:21 UTC (permalink / raw)
To: Gabriel Paubert
Cc: Christophe Leroy, Benjamin Herrenschmidt, Michael Ellerman,
Scott Wood, linuxppc-dev, linux-kernel
On Mon, May 23, 2016 at 07:17:38PM +0200, Gabriel Paubert wrote:
> On Mon, May 23, 2016 at 10:46:02AM +0200, Christophe Leroy wrote:
> > current_stack_pointeur() is a single instruction function. it
> > It is not worth breaking the execution flow with a bl/blr for a
> > single instruction
>
> Are you sure that the result is always the same?
>
> Calling an external function prevents the compiler from considering the
> caller of of current_stack_pointer a leaf function, which certainly
> does not help the compiler, but in a leaf function the compiler is free
> not to establish a new frame.
>
> If the compiler decides to establishes a new frame (typically with
> "stwu r1,-frame_size(r1)"), *r1 is the previous stack pointer, or
> the caller's stack pointer, or the current function frame pointer if
> I remember correctly the ABI definitions.
>
> However, if the compiler decides that it can get away without a frame
> for the function, *r1 is the stack pointer of the caller's caller.
>
> Depending on the application, this may or may not be important.
Right. I think I wrote the original current_stack_pointer()
implementation, and that I deliberately didn't make it an inline
so that the caller would have to establish its own stack frame,
and thus its stack pointer value would be a well-defined thing.
Paul.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc: inline current_stack_pointer()
2016-05-23 8:46 [PATCH] powerpc: inline current_stack_pointer() Christophe Leroy
2016-05-23 17:17 ` Gabriel Paubert
2016-05-23 20:22 ` Segher Boessenkool
@ 2016-05-31 10:05 ` Anton Blanchard
2 siblings, 0 replies; 7+ messages in thread
From: Anton Blanchard @ 2016-05-31 10:05 UTC (permalink / raw)
To: Christophe Leroy
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Scott Wood, linuxppc-dev, linux-kernel
Hi,
> current_stack_pointeur() is a single instruction function. it
> It is not worth breaking the execution flow with a bl/blr for a
> single instruction
Check out bfe9a2cfe91a ("powerpc: Reimplement __get_SP() as a function
not a define") to see why we made it a function.
Anton
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-05-31 10:05 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-23 8:46 [PATCH] powerpc: inline current_stack_pointer() Christophe Leroy
2016-05-23 17:17 ` Gabriel Paubert
2016-05-24 22:21 ` Paul Mackerras
2016-05-23 20:22 ` Segher Boessenkool
2016-05-24 5:39 ` Christophe Leroy
2016-05-24 6:08 ` Segher Boessenkool
2016-05-31 10:05 ` Anton Blanchard
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).