From: Alastair D'Silva <[email protected]>
When building an LTO kernel, the existing code generates warnings:
./arch/powerpc/include/asm/paca.h:37:30: warning: register of
‘local_paca’ used for multiple global register variables
register struct paca_struct *local_paca asm("r13");
^
./arch/powerpc/include/asm/paca.h:37:30: note: conflicts with
‘local_paca’
This patch reworks local_paca into an inline getter & setter function,
which addresses the warning.
Generated ASM from this patch is broadly similar (addresses have
changed and the compiler uses different GPRs in some places).
Signed-off-by: Alastair D'Silva <[email protected]>
---
arch/powerpc/include/asm/paca.h | 44 +++++++++++++++++++++++----------
arch/powerpc/kernel/paca.c | 2 +-
2 files changed, 32 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index e843bc5d1a0f..9c9e2dea0f9b 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -34,19 +34,6 @@
#include <asm/cpuidle.h>
#include <asm/atomic.h>
-register struct paca_struct *local_paca asm("r13");
-
-#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
-extern unsigned int debug_smp_processor_id(void); /* from linux/smp.h */
-/*
- * Add standard checks that preemption cannot occur when using get_paca():
- * otherwise the paca_struct it points to may be the wrong one just after.
- */
-#define get_paca() ((void) debug_smp_processor_id(), local_paca)
-#else
-#define get_paca() local_paca
-#endif
-
#ifdef CONFIG_PPC_PSERIES
#define get_lppaca() (get_paca()->lppaca_ptr)
#endif
@@ -266,6 +253,37 @@ struct paca_struct {
#endif
} ____cacheline_aligned;
+#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
+extern unsigned int debug_smp_processor_id(void); /* from linux/smp.h */
+#endif
+
+static inline struct paca_struct *get_paca_no_preempt_check(void)
+{
+ register struct paca_struct *paca asm("r13");
+ return paca;
+}
+
+static inline struct paca_struct *get_paca(void)
+{
+#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
+ /*
+ * Add standard checks that preemption cannot occur when using get_paca():
+ * otherwise the paca_struct it points to may be the wrong one just after.
+ */
+ debug_smp_processor_id();
+#endif
+ return get_paca_no_preempt_check();
+}
+
+#define local_paca get_paca_no_preempt_check()
+
+static inline void set_paca(struct paca_struct *new)
+{
+ register struct paca_struct *paca asm("r13");
+ paca = new;
+}
+
+
extern void copy_mm_to_paca(struct mm_struct *mm);
extern struct paca_struct **paca_ptrs;
extern void initialise_paca(struct paca_struct *new_paca, int cpu);
diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
index 913bfca09c4f..ae5c243f9d5a 100644
--- a/arch/powerpc/kernel/paca.c
+++ b/arch/powerpc/kernel/paca.c
@@ -172,7 +172,7 @@ void __init initialise_paca(struct paca_struct *new_paca, int cpu)
void setup_paca(struct paca_struct *new_paca)
{
/* Setup r13 */
- local_paca = new_paca;
+ set_paca(new_paca);
#ifdef CONFIG_PPC_BOOK3E
/* On Book3E, initialize the TLB miss exception frames */
--
2.20.1
Hello,
Le 13/03/2019 à 04:42, Alastair D'Silva a écrit :
> From: Alastair D'Silva <[email protected]>
>
> When building an LTO kernel, the existing code generates warnings:
> ./arch/powerpc/include/asm/paca.h:37:30: warning: register of
> ‘local_paca’ used for multiple global register variables
> register struct paca_struct *local_paca asm("r13");
> ^
> ./arch/powerpc/include/asm/paca.h:37:30: note: conflicts with
> ‘local_paca’
How do you build a LTO kernel ?
>
> This patch reworks local_paca into an inline getter & setter function,
> which addresses the warning.
This patch adds sparse warnings, see
https://patchwork.ozlabs.org/patch/1055875/
>
> Generated ASM from this patch is broadly similar (addresses have
> changed and the compiler uses different GPRs in some places).
Your text might be confusion. When I read it the first time I thought
you were saying that the compiler was now using another GPR than r13.
>
> Signed-off-by: Alastair D'Silva <[email protected]>
I guess the same has to be done for current, see
arch/powerpc/include/asm/current.h :
/*
* We keep `current' in r2 for speed.
*/
register struct task_struct *current asm ("r2");
> ---
> arch/powerpc/include/asm/paca.h | 44 +++++++++++++++++++++++----------
> arch/powerpc/kernel/paca.c | 2 +-
> 2 files changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
> index e843bc5d1a0f..9c9e2dea0f9b 100644
> --- a/arch/powerpc/include/asm/paca.h
> +++ b/arch/powerpc/include/asm/paca.h
> @@ -34,19 +34,6 @@
> #include <asm/cpuidle.h>
> #include <asm/atomic.h>
>
> -register struct paca_struct *local_paca asm("r13");
> -
> -#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> -extern unsigned int debug_smp_processor_id(void); /* from linux/smp.h */
> -/*
> - * Add standard checks that preemption cannot occur when using get_paca():
> - * otherwise the paca_struct it points to may be the wrong one just after.
> - */
> -#define get_paca() ((void) debug_smp_processor_id(), local_paca)
> -#else
> -#define get_paca() local_paca
> -#endif
> -
> #ifdef CONFIG_PPC_PSERIES
> #define get_lppaca() (get_paca()->lppaca_ptr)
> #endif
> @@ -266,6 +253,37 @@ struct paca_struct {
> #endif
> } ____cacheline_aligned;
>
> +#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> +extern unsigned int debug_smp_processor_id(void); /* from linux/smp.h */
> +#endif
Why moving this down, why not leaving at the same place as before ?
If you really need to move it, you should remove the 'extern' at the
same time to make checkpatch happy.
> +
> +static inline struct paca_struct *get_paca_no_preempt_check(void)
> +{
> + register struct paca_struct *paca asm("r13");
Should be a blank line there.
> + return paca;
> +}
> +
> +static inline struct paca_struct *get_paca(void)
> +{
> +#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> + /*
> + * Add standard checks that preemption cannot occur when using get_paca():
> + * otherwise the paca_struct it points to may be the wrong one just after.
> + */
> + debug_smp_processor_id();
> +#endif
> + return get_paca_no_preempt_check();
> +}
> +
> +#define local_paca get_paca_no_preempt_check()
> +
> +static inline void set_paca(struct paca_struct *new)
> +{
> + register struct paca_struct *paca asm("r13");
Blank line should be added here.
> + paca = new;
> +}
> +
> +
> extern void copy_mm_to_paca(struct mm_struct *mm);
> extern struct paca_struct **paca_ptrs;
> extern void initialise_paca(struct paca_struct *new_paca, int cpu);
> diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
> index 913bfca09c4f..ae5c243f9d5a 100644
> --- a/arch/powerpc/kernel/paca.c
> +++ b/arch/powerpc/kernel/paca.c
> @@ -172,7 +172,7 @@ void __init initialise_paca(struct paca_struct *new_paca, int cpu)
> void setup_paca(struct paca_struct *new_paca)
> {
> /* Setup r13 */
> - local_paca = new_paca;
> + set_paca(new_paca);
>
> #ifdef CONFIG_PPC_BOOK3E
> /* On Book3E, initialize the TLB miss exception frames */
>
Christophe
"Alastair D'Silva" <[email protected]> writes:
> From: Alastair D'Silva <[email protected]>
>
> When building an LTO kernel, the existing code generates warnings:
> ./arch/powerpc/include/asm/paca.h:37:30: warning: register of
> ‘local_paca’ used for multiple global register variables
> register struct paca_struct *local_paca asm("r13");
> ^
> ./arch/powerpc/include/asm/paca.h:37:30: note: conflicts with
> ‘local_paca’
>
> This patch reworks local_paca into an inline getter & setter function,
> which addresses the warning.
>
> Generated ASM from this patch is broadly similar (addresses have
> changed and the compiler uses different GPRs in some places).
Ditto to Christophe's comment; I'd love to know how to build this so I
can actually see the differences. Perhaps you could bundle up all the
required changes and send it as a patch series with a cover letter
explaining this?
> +static inline struct paca_struct *get_paca_no_preempt_check(void)
> +{
> + register struct paca_struct *paca asm("r13");
> + return paca;
> +}
Isn't the convention to have the { on the same line as the function, or
am I horrible mis-remembering things?
Should these functions be __always_inline?
Regards,
Daniel
> +
> +static inline struct paca_struct *get_paca(void)
> +{
> +#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> + /*
> + * Add standard checks that preemption cannot occur when using get_paca():
> + * otherwise the paca_struct it points to may be the wrong one just after.
> + */
> + debug_smp_processor_id();
> +#endif
> + return get_paca_no_preempt_check();
> +}
> +
> +#define local_paca get_paca_no_preempt_check()
> +
> +static inline void set_paca(struct paca_struct *new)
> +{
> + register struct paca_struct *paca asm("r13");
> + paca = new;
> +}
> +
> +
> extern void copy_mm_to_paca(struct mm_struct *mm);
> extern struct paca_struct **paca_ptrs;
> extern void initialise_paca(struct paca_struct *new_paca, int cpu);
> diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
> index 913bfca09c4f..ae5c243f9d5a 100644
> --- a/arch/powerpc/kernel/paca.c
> +++ b/arch/powerpc/kernel/paca.c
> @@ -172,7 +172,7 @@ void __init initialise_paca(struct paca_struct *new_paca, int cpu)
> void setup_paca(struct paca_struct *new_paca)
> {
> /* Setup r13 */
> - local_paca = new_paca;
> + set_paca(new_paca);
>
> #ifdef CONFIG_PPC_BOOK3E
> /* On Book3E, initialize the TLB miss exception frames */
> --
> 2.20.1
On 14/3/19 10:54 am, Daniel Axtens wrote:
>> +static inline struct paca_struct *get_paca_no_preempt_check(void)
>> +{
>> + register struct paca_struct *paca asm("r13");
>> + return paca;
>> +}
>
> Isn't the convention to have the { on the same line as the function, or
> am I horrible mis-remembering things?
However, there is one special case, namely functions: they have the
opening brace at the beginning of the next line, thus:
int function(int x)
{
body of function
}
Heretic people all over the world have claimed that this inconsistency
is ... well ... inconsistent, but all right-thinking people know that
(a) K&R are **right** and (b) K&R are right. Besides, functions are
special anyway (you can't nest them in C).
--
Andrew Donnellan OzLabs, ADL Canberra
[email protected] IBM Australia Limited
On Wed, 2019-03-13 at 10:06 +0100, Christophe Leroy wrote:
> Hello,
Thanks for reviewing :)
>
> Le 13/03/2019 à 04:42, Alastair D'Silva a écrit :
> > From: Alastair D'Silva <[email protected]>
> >
> > When building an LTO kernel, the existing code generates warnings:
> > ./arch/powerpc/include/asm/paca.h:37:30: warning: register of
> > ‘local_paca’ used for multiple global register variables
> > register struct paca_struct *local_paca asm("r13");
> > ^
> > ./arch/powerpc/include/asm/paca.h:37:30: note: conflicts with
> > ‘local_paca’
>
> How do you build a LTO kernel ?
I'm using Andi Kleen's LTO tree:
https://github.com/andikleen/linux-misc/tree/lto-420-1
with a few other patches:
https://github.com/andikleen/linux-misc/pull/27
You'll need to add the following to your .config:
CONFIG_LTO_MENU=y
CONFIG_LTO=y
>
> > This patch reworks local_paca into an inline getter & setter
> > function,
> > which addresses the warning.
>
> This patch adds sparse warnings, see
> https://patchwork.ozlabs.org/patch/1055875/
These warnings are bogus, they replace warnings that flagged against
spinlock.h.
> > Generated ASM from this patch is broadly similar (addresses have
> > changed and the compiler uses different GPRs in some places).
>
> Your text might be confusion. When I read it the first time I
> thought
> you were saying that the compiler was now using another GPR than r13.
>
I'll see if I can improve it.
> > Signed-off-by: Alastair D'Silva <[email protected]>
>
> I guess the same has to be done for current, see
> arch/powerpc/include/asm/current.h :
>
> /*
> * We keep `current' in r2 for speed.
> */
> register struct task_struct *current asm ("r2");
Hmm, I didn't see problems on PPC64 as that already uses an inline
function. I'll address this in another patch for the PPC32 case.
> > ---
> > arch/powerpc/include/asm/paca.h | 44 +++++++++++++++++++++++-----
> > -----
> > arch/powerpc/kernel/paca.c | 2 +-
> > 2 files changed, 32 insertions(+), 14 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/paca.h
> > b/arch/powerpc/include/asm/paca.h
> > index e843bc5d1a0f..9c9e2dea0f9b 100644
> > --- a/arch/powerpc/include/asm/paca.h
> > +++ b/arch/powerpc/include/asm/paca.h
> > @@ -34,19 +34,6 @@
> > #include <asm/cpuidle.h>
> > #include <asm/atomic.h>
> >
> > -register struct paca_struct *local_paca asm("r13");
> > -
> > -#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> > -extern unsigned int debug_smp_processor_id(void); /* from
> > linux/smp.h */
> > -/*
> > - * Add standard checks that preemption cannot occur when using
> > get_paca():
> > - * otherwise the paca_struct it points to may be the wrong one
> > just after.
> > - */
> > -#define get_paca() ((void) debug_smp_processor_id(), local_paca)
> > -#else
> > -#define get_paca() local_paca
> > -#endif
> > -
> > #ifdef CONFIG_PPC_PSERIES
> > #define get_lppaca() (get_paca()->lppaca_ptr)
> > #endif
> > @@ -266,6 +253,37 @@ struct paca_struct {
> > #endif
> > } ____cacheline_aligned;
> >
> > +#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> > +extern unsigned int debug_smp_processor_id(void); /* from
> > linux/smp.h */
> > +#endif
>
> Why moving this down, why not leaving at the same place as before ?
>
> If you really need to move it, you should remove the 'extern' at the
> same time to make checkpatch happy.
I moved it to keep it close to the usage of it.
I suppose the new implementation should be in the same place though.
> > +
> > +static inline struct paca_struct *get_paca_no_preempt_check(void)
> > +{
> > + register struct paca_struct *paca asm("r13");
>
> Should be a blank line there.
Whoops, I thought I ran checkpatch, but clearly, I forgot. I'll
resubmit.
> > + return paca;
> > +}
> > +
> > +static inline struct paca_struct *get_paca(void)
> > +{
> > +#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> > + /*
> > + * Add standard checks that preemption cannot occur when using
> > get_paca():
> > + * otherwise the paca_struct it points to may be the wrong one
> > just after.
> > + */
> > + debug_smp_processor_id();
> > +#endif
> > + return get_paca_no_preempt_check();
> > +}
> > +
> > +#define local_paca get_paca_no_preempt_check()
> > +
> > +static inline void set_paca(struct paca_struct *new)
> > +{
> > + register struct paca_struct *paca asm("r13");
>
> Blank line should be added here.
>
> > + paca = new;
> > +}
> > +
> > +
> > extern void copy_mm_to_paca(struct mm_struct *mm);
> > extern struct paca_struct **paca_ptrs;
> > extern void initialise_paca(struct paca_struct *new_paca, int
> > cpu);
> > diff --git a/arch/powerpc/kernel/paca.c
> > b/arch/powerpc/kernel/paca.c
> > index 913bfca09c4f..ae5c243f9d5a 100644
> > --- a/arch/powerpc/kernel/paca.c
> > +++ b/arch/powerpc/kernel/paca.c
> > @@ -172,7 +172,7 @@ void __init initialise_paca(struct paca_struct
> > *new_paca, int cpu)
> > void setup_paca(struct paca_struct *new_paca)
> > {
> > /* Setup r13 */
> > - local_paca = new_paca;
> > + set_paca(new_paca);
> >
> > #ifdef CONFIG_PPC_BOOK3E
> > /* On Book3E, initialize the TLB miss exception frames */
> >
>
> Christophe
>
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
From: Alastair D'Silva <[email protected]>
When building an LTO kernel, the existing code generates warnings:
./arch/powerpc/include/asm/paca.h:37:30: warning: register of
‘local_paca’ used for multiple global register variables
register struct paca_struct *local_paca asm("r13");
^
./arch/powerpc/include/asm/paca.h:37:30: note: conflicts with
‘local_paca’
This patch reworks local_paca into an inline getter & setter function,
which addresses the warning.
Changelog:
V2
- Address whitespace issues
- keep new implementation close to where the old implementation was
Signed-off-by: Alastair D'Silva <[email protected]>
---
arch/powerpc/include/asm/paca.h | 37 +++++++++++++++++++++++++--------
arch/powerpc/kernel/paca.c | 2 +-
2 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index e843bc5d1a0f..2fa0b43357c9 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -34,19 +34,38 @@
#include <asm/cpuidle.h>
#include <asm/atomic.h>
-register struct paca_struct *local_paca asm("r13");
-
#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
extern unsigned int debug_smp_processor_id(void); /* from linux/smp.h */
-/*
- * Add standard checks that preemption cannot occur when using get_paca():
- * otherwise the paca_struct it points to may be the wrong one just after.
- */
-#define get_paca() ((void) debug_smp_processor_id(), local_paca)
-#else
-#define get_paca() local_paca
#endif
+static inline struct paca_struct *get_paca_no_preempt_check(void)
+{
+ register struct paca_struct *paca asm("r13");
+
+ return paca;
+}
+
+static inline struct paca_struct *get_paca(void)
+{
+#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
+ /*
+ * Add standard checks that preemption cannot occur when using get_paca():
+ * otherwise the paca_struct it points to may be the wrong one just after.
+ */
+ debug_smp_processor_id();
+#endif
+ return get_paca_no_preempt_check();
+}
+
+#define local_paca get_paca_no_preempt_check()
+
+static inline void set_paca(struct paca_struct *new)
+{
+ register struct paca_struct *paca asm("r13");
+
+ paca = new;
+}
+
#ifdef CONFIG_PPC_PSERIES
#define get_lppaca() (get_paca()->lppaca_ptr)
#endif
diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
index 913bfca09c4f..ae5c243f9d5a 100644
--- a/arch/powerpc/kernel/paca.c
+++ b/arch/powerpc/kernel/paca.c
@@ -172,7 +172,7 @@ void __init initialise_paca(struct paca_struct *new_paca, int cpu)
void setup_paca(struct paca_struct *new_paca)
{
/* Setup r13 */
- local_paca = new_paca;
+ set_paca(new_paca);
#ifdef CONFIG_PPC_BOOK3E
/* On Book3E, initialize the TLB miss exception frames */
--
2.20.1
On Thu, 2019-03-14 at 10:54 +1100, Daniel Axtens wrote:
> "Alastair D'Silva" <[email protected]> writes:
>
> > From: Alastair D'Silva <[email protected]>
> >
> > When building an LTO kernel, the existing code generates warnings:
> > ./arch/powerpc/include/asm/paca.h:37:30: warning: register of
> > ‘local_paca’ used for multiple global register variables
> > register struct paca_struct *local_paca asm("r13");
> > ^
> > ./arch/powerpc/include/asm/paca.h:37:30: note: conflicts with
> > ‘local_paca’
> >
> > This patch reworks local_paca into an inline getter & setter
> > function,
> > which addresses the warning.
> >
> > Generated ASM from this patch is broadly similar (addresses have
> > changed and the compiler uses different GPRs in some places).
>
> Ditto to Christophe's comment; I'd love to know how to build this so
> I
> can actually see the differences. Perhaps you could bundle up all the
> required changes and send it as a patch series with a cover letter
> explaining this?
The differences are visible in a normal build, but if you want to play
with LTO, see my comments to Christophe.
>
> > +static inline struct paca_struct *get_paca_no_preempt_check(void)
> > +{
> > + register struct paca_struct *paca asm("r13");
> > + return paca;
> > +}
>
> Isn't the convention to have the { on the same line as the function,
> or
> am I horrible mis-remembering things?
>
You are :)
> Should these functions be __always_inline?
>
Yes, they should, I'll add that to V3.
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
Le 14/03/2019 à 03:31, Alastair D'Silva a écrit :
> From: Alastair D'Silva <[email protected]>
>
> When building an LTO kernel, the existing code generates warnings:
> ./arch/powerpc/include/asm/paca.h:37:30: warning: register of
> ‘local_paca’ used for multiple global register variables
> register struct paca_struct *local_paca asm("r13");
> ^
> ./arch/powerpc/include/asm/paca.h:37:30: note: conflicts with
> ‘local_paca’
>
> This patch reworks local_paca into an inline getter & setter function,
> which addresses the warning.
>
> Changelog:
> V2
> - Address whitespace issues
> - keep new implementation close to where the old implementation was
>
> Signed-off-by: Alastair D'Silva <[email protected]>
Reviewed-by: Christophe Leroy <[email protected]>
> ---
> arch/powerpc/include/asm/paca.h | 37 +++++++++++++++++++++++++--------
> arch/powerpc/kernel/paca.c | 2 +-
> 2 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
> index e843bc5d1a0f..2fa0b43357c9 100644
> --- a/arch/powerpc/include/asm/paca.h
> +++ b/arch/powerpc/include/asm/paca.h
> @@ -34,19 +34,38 @@
> #include <asm/cpuidle.h>
> #include <asm/atomic.h>
>
> -register struct paca_struct *local_paca asm("r13");
> -
> #if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> extern unsigned int debug_smp_processor_id(void); /* from linux/smp.h */
> -/*
> - * Add standard checks that preemption cannot occur when using get_paca():
> - * otherwise the paca_struct it points to may be the wrong one just after.
> - */
> -#define get_paca() ((void) debug_smp_processor_id(), local_paca)
> -#else
> -#define get_paca() local_paca
> #endif
>
> +static inline struct paca_struct *get_paca_no_preempt_check(void)
> +{
> + register struct paca_struct *paca asm("r13");
> +
> + return paca;
> +}
> +
> +static inline struct paca_struct *get_paca(void)
> +{
> +#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> + /*
> + * Add standard checks that preemption cannot occur when using get_paca():
> + * otherwise the paca_struct it points to may be the wrong one just after.
> + */
> + debug_smp_processor_id();
> +#endif
> + return get_paca_no_preempt_check();
> +}
> +
> +#define local_paca get_paca_no_preempt_check()
> +
> +static inline void set_paca(struct paca_struct *new)
> +{
> + register struct paca_struct *paca asm("r13");
> +
> + paca = new;
> +}
> +
> #ifdef CONFIG_PPC_PSERIES
> #define get_lppaca() (get_paca()->lppaca_ptr)
> #endif
> diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
> index 913bfca09c4f..ae5c243f9d5a 100644
> --- a/arch/powerpc/kernel/paca.c
> +++ b/arch/powerpc/kernel/paca.c
> @@ -172,7 +172,7 @@ void __init initialise_paca(struct paca_struct *new_paca, int cpu)
> void setup_paca(struct paca_struct *new_paca)
> {
> /* Setup r13 */
> - local_paca = new_paca;
> + set_paca(new_paca);
>
> #ifdef CONFIG_PPC_BOOK3E
> /* On Book3E, initialize the TLB miss exception frames */
>
Alastair D'Silva's on March 14, 2019 12:31 pm:
> From: Alastair D'Silva <[email protected]>
>
> When building an LTO kernel, the existing code generates warnings:
> ./arch/powerpc/include/asm/paca.h:37:30: warning: register of
> ‘local_paca’ used for multiple global register variables
> register struct paca_struct *local_paca asm("r13");
> ^
> ./arch/powerpc/include/asm/paca.h:37:30: note: conflicts with
> ‘local_paca’
Isn't this a bogus warning? It doesn't look like there's a way to
define it any other way.
>
> This patch reworks local_paca into an inline getter & setter function,
> which addresses the warning.
>
> Changelog:
> V2
> - Address whitespace issues
> - keep new implementation close to where the old implementation was
>
> Signed-off-by: Alastair D'Silva <[email protected]>
> ---
> arch/powerpc/include/asm/paca.h | 37 +++++++++++++++++++++++++--------
> arch/powerpc/kernel/paca.c | 2 +-
> 2 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
> index e843bc5d1a0f..2fa0b43357c9 100644
> --- a/arch/powerpc/include/asm/paca.h
> +++ b/arch/powerpc/include/asm/paca.h
> @@ -34,19 +34,38 @@
> #include <asm/cpuidle.h>
> #include <asm/atomic.h>
>
> -register struct paca_struct *local_paca asm("r13");
> -
> #if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> extern unsigned int debug_smp_processor_id(void); /* from linux/smp.h */
> -/*
> - * Add standard checks that preemption cannot occur when using get_paca():
> - * otherwise the paca_struct it points to may be the wrong one just after.
> - */
> -#define get_paca() ((void) debug_smp_processor_id(), local_paca)
> -#else
> -#define get_paca() local_paca
> #endif
>
> +static inline struct paca_struct *get_paca_no_preempt_check(void)
> +{
> + register struct paca_struct *paca asm("r13");
> +
> + return paca;
> +}
Problem is it now changes the global register variable to a local
register variable. The compiler would presumably be within its rights
to "cache" that return value or use another register for it, which
is not really what we want.
Thanks,
Nick
On Tue, 2019-03-26 at 15:58 +1000, Nicholas Piggin wrote:
> Alastair D'Silva's on March 14, 2019 12:31 pm:
> > From: Alastair D'Silva <[email protected]>
> >
> > When building an LTO kernel, the existing code generates warnings:
> > ./arch/powerpc/include/asm/paca.h:37:30: warning: register of
> > ‘local_paca’ used for multiple global register variables
> > register struct paca_struct *local_paca asm("r13");
> > ^
> > ./arch/powerpc/include/asm/paca.h:37:30: note: conflicts with
> > ‘local_paca’
>
> Isn't this a bogus warning? It doesn't look like there's a way to
> define it any other way.
There isn't any other way to define it as a global. However, the
warning is legitimate.
The compiler sees that there are multiple global register variables,
all pointing at the same register.
The compiler can only determine this when LTO is used, as otherwise it
only sees the one in the current compilation unit, whicd disappears by
the time the kernel is linked.
>
> > This patch reworks local_paca into an inline getter & setter
> > function,
> > which addresses the warning.
> >
> > Changelog:
> > V2
> > - Address whitespace issues
> > - keep new implementation close to where the old implementation
> > was
> >
> > Signed-off-by: Alastair D'Silva <[email protected]>
> > ---
> > arch/powerpc/include/asm/paca.h | 37 +++++++++++++++++++++++++--
> > ------
> > arch/powerpc/kernel/paca.c | 2 +-
> > 2 files changed, 29 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/paca.h
> > b/arch/powerpc/include/asm/paca.h
> > index e843bc5d1a0f..2fa0b43357c9 100644
> > --- a/arch/powerpc/include/asm/paca.h
> > +++ b/arch/powerpc/include/asm/paca.h
> > @@ -34,19 +34,38 @@
> > #include <asm/cpuidle.h>
> > #include <asm/atomic.h>
> >
> > -register struct paca_struct *local_paca asm("r13");
> > -
> > #if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> > extern unsigned int debug_smp_processor_id(void); /* from
> > linux/smp.h */
> > -/*
> > - * Add standard checks that preemption cannot occur when using
> > get_paca():
> > - * otherwise the paca_struct it points to may be the wrong one
> > just after.
> > - */
> > -#define get_paca() ((void) debug_smp_processor_id(), local_paca)
> > -#else
> > -#define get_paca() local_paca
> > #endif
> >
> > +static inline struct paca_struct *get_paca_no_preempt_check(void)
> > +{
> > + register struct paca_struct *paca asm("r13");
> > +
> > + return paca;
> > +}
>
> Problem is it now changes the global register variable to a local
> register variable. The compiler would presumably be within its rights
> to "cache" that return value or use another register for it, which
> is not really what we want.
>
>
I've confirmed that at least with GCC 8.2.0, the generated assembler is
similar, but yes, the compiler may be free to take a copy into another
register (although that would be a terrible optimisation), and then
operate on that value.
Subsequent uses would still have to call the function (ie. fetch the
data from r13) regardless, so I believe this scenario is safe.
Can you think of a scenario where this is a problem?
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
Alastair D'Silva's on March 27, 2019 2:37 pm:
> On Tue, 2019-03-26 at 15:58 +1000, Nicholas Piggin wrote:
>> Alastair D'Silva's on March 14, 2019 12:31 pm:
>> > From: Alastair D'Silva <[email protected]>
>> >
>> > When building an LTO kernel, the existing code generates warnings:
>> > ./arch/powerpc/include/asm/paca.h:37:30: warning: register of
>> > ‘local_paca’ used for multiple global register variables
>> > register struct paca_struct *local_paca asm("r13");
>> > ^
>> > ./arch/powerpc/include/asm/paca.h:37:30: note: conflicts with
>> > ‘local_paca’
>>
>> Isn't this a bogus warning? It doesn't look like there's a way to
>> define it any other way.
>
> There isn't any other way to define it as a global. However, the
> warning is legitimate.
>
> The compiler sees that there are multiple global register variables,
> all pointing at the same register.
It's one variable though, so it's not a legitimate warning (unless
there is a way to declare a "reference" to it that we are not doing).
>
> The compiler can only determine this when LTO is used, as otherwise it
> only sees the one in the current compilation unit, whicd disappears by
> the time the kernel is linked.
>
>>
>> > This patch reworks local_paca into an inline getter & setter
>> > function,
>> > which addresses the warning.
>> >
>> > Changelog:
>> > V2
>> > - Address whitespace issues
>> > - keep new implementation close to where the old implementation
>> > was
>> >
>> > Signed-off-by: Alastair D'Silva <[email protected]>
>> > ---
>> > arch/powerpc/include/asm/paca.h | 37 +++++++++++++++++++++++++--
>> > ------
>> > arch/powerpc/kernel/paca.c | 2 +-
>> > 2 files changed, 29 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/arch/powerpc/include/asm/paca.h
>> > b/arch/powerpc/include/asm/paca.h
>> > index e843bc5d1a0f..2fa0b43357c9 100644
>> > --- a/arch/powerpc/include/asm/paca.h
>> > +++ b/arch/powerpc/include/asm/paca.h
>> > @@ -34,19 +34,38 @@
>> > #include <asm/cpuidle.h>
>> > #include <asm/atomic.h>
>> >
>> > -register struct paca_struct *local_paca asm("r13");
>> > -
>> > #if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
>> > extern unsigned int debug_smp_processor_id(void); /* from
>> > linux/smp.h */
>> > -/*
>> > - * Add standard checks that preemption cannot occur when using
>> > get_paca():
>> > - * otherwise the paca_struct it points to may be the wrong one
>> > just after.
>> > - */
>> > -#define get_paca() ((void) debug_smp_processor_id(), local_paca)
>> > -#else
>> > -#define get_paca() local_paca
>> > #endif
>> >
>> > +static inline struct paca_struct *get_paca_no_preempt_check(void)
>> > +{
>> > + register struct paca_struct *paca asm("r13");
>> > +
>> > + return paca;
>> > +}
>>
>> Problem is it now changes the global register variable to a local
>> register variable. The compiler would presumably be within its rights
>> to "cache" that return value or use another register for it, which
>> is not really what we want.
>>
>>
>
> I've confirmed that at least with GCC 8.2.0, the generated assembler is
> similar, but yes, the compiler may be free to take a copy into another
> register (although that would be a terrible optimisation), and then
> operate on that value.
Yep.
>
> Subsequent uses would still have to call the function (ie. fetch the
> data from r13) regardless, so I believe this scenario is safe.
>
> Can you think of a scenario where this is a problem?
It wouldn't be the subsequent use but the one use. If you're preempted
then you can't be using a stale r13 value.
Quite possibly any bug like that would already be buggy now, the cases
where it matters tend to need asm to access it. And it's something we
need to really audit and have proper accessors and preempt warnings
that Ben always harped on about.
But to just work around this warning it seems pretty dangerous to
change this and hope.
Thanks,
Nick