Re: [PATCH] LoongArch: Mark __percpu functions as always inline

[Nathan Chancellor <nathan@kernel.org>]

On Thu, Nov 02, 2023 at 10:18:55AM +0800, Huacai Chen wrote:
> Hi, Nathan,
> 
> On Thu, Nov 2, 2023 at 3:55 AM Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > On Wed, Nov 01, 2023 at 12:43:29PM -0700, Nathan Chancellor wrote:
> > > A recent change to the optimization pipeline in LLVM reveals some
> > > fragility around the inlining of LoongArch's __percpu functions, which
> > > manifests as a BUILD_BUG() failure:
> > >
> > >   In file included from kernel/sched/build_policy.c:17:
> > >   In file included from include/linux/sched/cputime.h:5:
> > >   In file included from include/linux/sched/signal.h:5:
> > >   In file included from include/linux/rculist.h:11:
> > >   In file included from include/linux/rcupdate.h:26:
> > >   In file included from include/linux/irqflags.h:18:
> > >   arch/loongarch/include/asm/percpu.h:97:3: error: call to '__compiletime_assert_51' declared with 'error' attribute: BUILD_BUG failed
> > >      97 |                 BUILD_BUG();
> > >         |                 ^
> > >   include/linux/build_bug.h:59:21: note: expanded from macro 'BUILD_BUG'
> > >      59 | #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
> > >         |                     ^
> > >   include/linux/build_bug.h:39:37: note: expanded from macro 'BUILD_BUG_ON_MSG'
> > >      39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> > >         |                                     ^
> > >   include/linux/compiler_types.h:425:2: note: expanded from macro 'compiletime_assert'
> > >     425 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > >         |         ^
> > >   include/linux/compiler_types.h:413:2: note: expanded from macro '_compiletime_assert'
> > >     413 |         __compiletime_assert(condition, msg, prefix, suffix)
> > >         |         ^
> > >   include/linux/compiler_types.h:406:4: note: expanded from macro '__compiletime_assert'
> > >     406 |                         prefix ## suffix();                             \
> > >         |                         ^
> > >   <scratch space>:86:1: note: expanded from here
> > >      86 | __compiletime_assert_51
> > >         | ^
> > >   1 error generated.
> > >
> > > If these functions are not inlined, the BUILD_BUG() in the default case
> > > cannot be eliminated since the compiler cannot prove it is never used,
> > > resulting in a build failure due to the error attribute.
> > >
> > > Mark these functions as __always_inline so that the BUILD_BUG() only
> > > triggers when the default case genuinely cannot be eliminated due to an
> > > unexpected size.
> > >
> > > Cc:  <stable@vger.kernel.org>
> > > Closes: https://github.com/ClangBuiltLinux/linux/issues/1955
> > > Fixes: 46859ac8af52 ("LoongArch: Add multi-processor (SMP) support")
> > > Link: https://github.com/llvm/llvm-project/commit/1a2e77cf9e11dbf56b5720c607313a566eebb16e
> >
> > This should have also had:
> >
> > Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> >
> > I can pick it up if I have to send a v2.
> >
> > > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> > > ---
> > >  arch/loongarch/include/asm/percpu.h | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/loongarch/include/asm/percpu.h b/arch/loongarch/include/asm/percpu.h
> > > index b9f567e66016..8fb857ae962b 100644
> > > --- a/arch/loongarch/include/asm/percpu.h
> > > +++ b/arch/loongarch/include/asm/percpu.h
> > > @@ -32,7 +32,7 @@ static inline void set_my_cpu_offset(unsigned long off)
> > >  #define __my_cpu_offset __my_cpu_offset
> > >
> > >  #define PERCPU_OP(op, asm_op, c_op)                                  \
> > > -static inline unsigned long __percpu_##op(void *ptr,                 \
> > > +static __always_inline unsigned long __percpu_##op(void *ptr,                        \
> > >                       unsigned long val, int size)                    \
> > >  {                                                                    \
> > >       unsigned long ret;                                              \
> > > @@ -63,7 +63,7 @@ PERCPU_OP(and, and, &)
> > >  PERCPU_OP(or, or, |)
> > >  #undef PERCPU_OP
> > >
> > > -static inline unsigned long __percpu_read(void *ptr, int size)
> > > +static __always_inline unsigned long __percpu_read(void *ptr, int size)
> > >  {
> > >       unsigned long ret;
> > >
> Thank you for your patch, but I think __percpu_write() and
> __percpu_xchg() also need __always_inline because they also have
> BUILD_BUG(). Although we don't meet problems about them, they have
> potential problems in theory.

Yes, you are absolutely correct! I thought I did a survey for other
BUILD_BUG() locations but seems like I did not do a good job if I did :/
I will send a v2 tomorrow with those two functions changed as well.

Cheers,
Nathan

> > >
> > > ---
> > > base-commit: 278be83601dd1725d4732241f066d528e160a39d
> > > change-id: 20231101-loongarch-always-inline-percpu-ops-cf77c161871f
> > >
> > > Best regards,
> > > --
> > > Nathan Chancellor <nathan@kernel.org>
> > >
> >