Message ID | 20190313034208.13134-1-alastair@au1.ibm.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [1/1] arch/powerpc: Rework local_paca to avoid LTO warnings | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (9580b71b5a7863c24a9bd18bcd2ad759b86b1eff) |
snowpatch_ozlabs/build-ppc64le | warning | build succeeded but added 3 new sparse warning(s) |
snowpatch_ozlabs/build-ppc64be | warning | build succeeded but added 3 new sparse warning(s) |
snowpatch_ozlabs/build-ppc64e | warning | build succeeded but added 2 new sparse warning(s) |
snowpatch_ozlabs/build-pmac32 | success | build succeeded & removed 0 sparse warning(s) |
snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 1 checks, 64 lines checked |
snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (9580b71b5a7863c24a9bd18bcd2ad759b86b1eff) |
snowpatch_ozlabs/build-ppc64le | warning | Build succeeded but added 3 new sparse warnings |
snowpatch_ozlabs/build-ppc64be | warning | Build succeeded but added 3 new sparse warnings |
snowpatch_ozlabs/build-ppc64e | warning | Build succeeded but added 2 new sparse warnings |
snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 1 checks, 64 lines checked |
snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (9580b71b5a7863c24a9bd18bcd2ad759b86b1eff) |
snowpatch_ozlabs/build-ppc64le | fail | build failed! |
snowpatch_ozlabs/build-ppc64be | fail | build failed! |
snowpatch_ozlabs/build-ppc64e | pending | build failed! |
snowpatch_ozlabs/build-pmac32 | fail | build failed! |
snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 1 checks, 64 lines checked |
snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (9580b71b5a7863c24a9bd18bcd2ad759b86b1eff) |
snowpatch_ozlabs/build-ppc64le | warning | Build succeeded but added 3 new sparse warnings |
snowpatch_ozlabs/build-ppc64be | warning | Build succeeded but added 3 new sparse warnings |
snowpatch_ozlabs/build-ppc64e | warning | Build succeeded but added 2 new sparse warnings |
snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 1 checks, 64 lines checked |
Hello, Le 13/03/2019 à 04:42, Alastair D'Silva a écrit : > From: Alastair D'Silva <alastair@d-silva.org> > > 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 <alastair@d-silva.org> 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" <alastair@au1.ibm.com> writes: > From: Alastair D'Silva <alastair@d-silva.org> > > 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).
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 <alastair@d-silva.org> > > > > 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 <alastair@d-silva.org> > > 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 >
On Thu, 2019-03-14 at 10:54 +1100, Daniel Axtens wrote: > "Alastair D'Silva" <alastair@au1.ibm.com> writes: > > > From: Alastair D'Silva <alastair@d-silva.org> > > > > 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.
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 */