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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi, Nathan,

On Thu, Nov 2, 2023 at 3:55 AM Nathan Chancellor <nathan@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > 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@xxxxxxxxxx>
>
> I can pick it up if I have to send a v2.
>
> > Signed-off-by: Nathan Chancellor <nathan@xxxxxxxxxx>
> > ---
> >  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.

Huacai

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




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux