Re: [PATCH 1/1] arch/powerpc: Rework local_paca to avoid LTO warnings
On Thu, 2019-03-14 at 10:54 +1100, Daniel Axtens wrote: > "Alastair D'Silva" writes: > > > From: Alastair D'Silva > > > > 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
Re: [PATCH 1/1] arch/powerpc: Rework local_paca to avoid LTO warnings
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 > > > > 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 > > 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 > > #include > > > > -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)
Re: [PATCH 1/1] arch/powerpc: Rework local_paca to avoid LTO warnings
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 are **right** and (b) K are right. Besides, functions are special anyway (you can't nest them in C). -- Andrew Donnellan OzLabs, ADL Canberra andrew.donnel...@au1.ibm.com IBM Australia Limited
Re: [PATCH 1/1] arch/powerpc: Rework local_paca to avoid LTO warnings
"Alastair D'Silva" writes: > From: Alastair D'Silva > > 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
Re: [PATCH 1/1] arch/powerpc: Rework local_paca to avoid LTO warnings
Hello, Le 13/03/2019 à 04:42, Alastair D'Silva a écrit : From: Alastair D'Silva 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 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 #include -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