Re: [PATCH v2 01/10] riscv: Implement cmpxchg32/64() using Zacas

Alexandre Ghiti
2024-07-16T12:20:10Z

Hi Nathan,

On Fri, Jul 5, 2024 at 7:27 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Thu, Jul 04, 2024 at 11:38:46AM +0800, kernel test robot wrote:
> > Hi Alexandre,
> >
> > kernel test robot noticed the following build errors:
> >
> > [auto build test ERROR on soc/for-next]
> > [also build test ERROR on linus/master v6.10-rc6 next-20240703]
> > [cannot apply to arnd-asm-generic/master robh/for-next tip/locking/core]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >
> > url: https://github.com/intel-lab-lkp/linux/commits/Alexandre-Ghiti/riscv-Implement-cmpxchg32-64-using-Zacas/20240627-034946
> > base: https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
> > patch link: https://lore.kernel.org/r/20240626130347.520750-2-alexghiti%40rivosinc.com
> > patch subject: [PATCH v2 01/10] riscv: Implement cmpxchg32/64() using Zacas
> > config: riscv-randconfig-002-20240704 (https://download.01.org/0day-ci/archive/20240704/202407041157.odTZAYZ6-lkp@intel.com/config)
> > compiler: clang version 16.0.6 (https://github.com/llvm/llvm-project 7cbf1a2591520c2491aa35339f227775f4d3adf6)
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240704/202407041157.odTZAYZ6-lkp@intel.com/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202407041157.odTZAYZ6-lkp@intel.com/
> >
> > All errors (new ones prefixed by >>):
> >
> > >> kernel/sched/core.c:11873:7: error: cannot jump from this asm goto statement to one of its possible targets
> > if (try_cmpxchg(&pcpu_cid->cid, &lazy_cid, MM_CID_UNSET))
> > ^
> > include/linux/atomic/atomic-instrumented.h:4880:2: note: expanded from macro 'try_cmpxchg'
> > raw_try_cmpxchg(__ai_ptr, __ai_oldp, __VA_ARGS__); \
> > ^
> > include/linux/atomic/atomic-arch-fallback.h:192:9: note: expanded from macro 'raw_try_cmpxchg'
> > ___r = raw_cmpxchg((_ptr), ___o, (_new)); \
> > ^
> > include/linux/atomic/atomic-arch-fallback.h:55:21: note: expanded from macro 'raw_cmpxchg'
> > #define raw_cmpxchg arch_cmpxchg
> > ^
> > arch/riscv/include/asm/cmpxchg.h:212:2: note: expanded from macro 'arch_cmpxchg'
> > _arch_cmpxchg((ptr), (o), (n), ".rl", "", " fence rw, rw\n")
> > ^
> > arch/riscv/include/asm/cmpxchg.h:189:3: note: expanded from macro '_arch_cmpxchg'
> > __arch_cmpxchg(".w", ".w" sc_sfx, prepend, append, \
> > ^
> > arch/riscv/include/asm/cmpxchg.h:144:3: note: expanded from macro '__arch_cmpxchg'
> > asm goto(ALTERNATIVE("nop", "j %[zacas]", 0, \
> > ^
> > kernel/sched/core.c:11840:7: note: possible target of asm goto statement
> > if (!try_cmpxchg(&pcpu_cid->cid, &cid, lazy_cid))
> > ^
> > include/linux/atomic/atomic-instrumented.h:4880:2: note: expanded from macro 'try_cmpxchg'
> > raw_try_cmpxchg(__ai_ptr, __ai_oldp, __VA_ARGS__); \
> > ^
> > include/linux/atomic/atomic-arch-fallback.h:192:9: note: expanded from macro 'raw_try_cmpxchg'
> > ___r = raw_cmpxchg((_ptr), ___o, (_new)); \
> > ^
> > include/linux/atomic/atomic-arch-fallback.h:55:21: note: expanded from macro 'raw_cmpxchg'
> > #define raw_cmpxchg arch_cmpxchg
> > ^
> > arch/riscv/include/asm/cmpxchg.h:212:2: note: expanded from macro 'arch_cmpxchg'
> > _arch_cmpxchg((ptr), (o), (n), ".rl", "", " fence rw, rw\n")
> > ^
> > arch/riscv/include/asm/cmpxchg.h:189:3: note: expanded from macro '_arch_cmpxchg'
> > __arch_cmpxchg(".w", ".w" sc_sfx, prepend, append, \
> > ^
> > arch/riscv/include/asm/cmpxchg.h:161:10: note: expanded from macro '__arch_cmpxchg'
> > \
> > ^
> > kernel/sched/core.c:11872:2: note: jump exits scope of variable with __attribute__((cleanup))
> > scoped_guard (irqsave) {
> > ^
> > include/linux/cleanup.h:169:20: note: expanded from macro 'scoped_guard'
> > for (CLASS(_name, scope)(args), \
> > ^
> > kernel/sched/core.c:11840:7: error: cannot jump from this asm goto statement to one of its possible targets
> > if (!try_cmpxchg(&pcpu_cid->cid, &cid, lazy_cid))
> > ^
> > include/linux/atomic/atomic-instrumented.h:4880:2: note: expanded from macro 'try_cmpxchg'
> > raw_try_cmpxchg(__ai_ptr, __ai_oldp, __VA_ARGS__); \
> > ^
> > include/linux/atomic/atomic-arch-fallback.h:192:9: note: expanded from macro 'raw_try_cmpxchg'
> > ___r = raw_cmpxchg((_ptr), ___o, (_new)); \
> > ^
> > include/linux/atomic/atomic-arch-fallback.h:55:21: note: expanded from macro 'raw_cmpxchg'
> > #define raw_cmpxchg arch_cmpxchg
> > ^
> > arch/riscv/include/asm/cmpxchg.h:212:2: note: expanded from macro 'arch_cmpxchg'
> > _arch_cmpxchg((ptr), (o), (n), ".rl", "", " fence rw, rw\n")
> > ^
> > arch/riscv/include/asm/cmpxchg.h:189:3: note: expanded from macro '_arch_cmpxchg'
> > __arch_cmpxchg(".w", ".w" sc_sfx, prepend, append, \
> > ^
> > arch/riscv/include/asm/cmpxchg.h:144:3: note: expanded from macro '__arch_cmpxchg'
> > asm goto(ALTERNATIVE("nop", "j %[zacas]", 0, \
> > ^
> > kernel/sched/core.c:11873:7: note: possible target of asm goto statement
> > if (try_cmpxchg(&pcpu_cid->cid, &lazy_cid, MM_CID_UNSET))
> > ^
> > include/linux/atomic/atomic-instrumented.h:4880:2: note: expanded from macro 'try_cmpxchg'
> > raw_try_cmpxchg(__ai_ptr, __ai_oldp, __VA_ARGS__); \
> > ^
> > include/linux/atomic/atomic-arch-fallback.h:192:9: note: expanded from macro 'raw_try_cmpxchg'
> > ___r = raw_cmpxchg((_ptr), ___o, (_new)); \
> > ^
> > include/linux/atomic/atomic-arch-fallback.h:55:21: note: expanded from macro 'raw_cmpxchg'
> > #define raw_cmpxchg arch_cmpxchg
> > ^
> > arch/riscv/include/asm/cmpxchg.h:212:2: note: expanded from macro 'arch_cmpxchg'
> > _arch_cmpxchg((ptr), (o), (n), ".rl", "", " fence rw, rw\n")
> > ^
> > arch/riscv/include/asm/cmpxchg.h:189:3: note: expanded from macro '_arch_cmpxchg'
> > __arch_cmpxchg(".w", ".w" sc_sfx, prepend, append, \
> > ^
> > arch/riscv/include/asm/cmpxchg.h:161:10: note: expanded from macro '__arch_cmpxchg'
> > \
> > ^
> > kernel/sched/core.c:11872:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
> > scoped_guard (irqsave) {
> > ^
> > include/linux/cleanup.h:169:20: note: expanded from macro 'scoped_guard'
> > for (CLASS(_name, scope)(args), \
> > ^
> > 2 errors generated.
>
> Ugh, this is an unfortunate interaction with clang's jump scope analysis
> and asm goto in LLVM releases prior to 17 :/
>
> https://github.com/ClangBuiltLinux/linux/issues/1886#issuecomment-1645979992
>
> Unfortunately, 'if (0)' does not prevent this (the analysis runs early
> in the front end as far as I understand it), we would need to workaround
> this with full preprocessor guards...

Thanks for jumping in on this one, I was quite puzzled :)

>
> Another alternative would be to require LLVM 17+ for RISC-V, which may
> not be the worst alternative, since I think most people doing serious
> work with clang will probably be living close to tip of tree anyways
> because of all the extension work that goes on upstream.

Stupid question but why the fix in llvm 17 was not backported to
previous versions?

Anyway, I'd rather require llvm 17+ than add a bunch of preprocessor
guards in this file (IIUC what you said above) as it is complex
enough.

@Conor Dooley @Palmer Dabbelt WDYT? Is there any interest in
supporting llvm < 17? We may encounter this bug again in the future so
I'd be in favor of moving to llvm 17+.

Thanks again Nathan,

Alex

>
> I am open to other thoughts though.
>
> Cheers,
> Nathan