Re: Help with atomic fallback

[Mark Rutland <mark.rutland@arm.com>]

On Tue, Dec 03, 2024 at 09:08:48AM -0400, Jason Gunthorpe wrote:
> On Tue, Dec 03, 2024 at 12:26:22PM +0000, Mark Rutland wrote:
> 
> > I'm assuming that's the report at:
> > 
> >   https://lore.kernel.org/oe-kbuild-all/202411301219.jHkzXdJD-lkp@intel.com/
> > 
> > ... for which the config is:
> > 
> >   https://download.01.org/0day-ci/archive/20241130/202411301219.jHkzXdJD-lkp@intel.com/config
> 
> Yeah, that is representative
> 
> > > Which is immediately because of a typo in atomic-arch-fallback.h code gen:
> > > 
> > > #if defined(arch_cmpxchg64_release)
> > > #define raw_cmpxchg64_release arch_cmpxchg64_release
> > > #elif defined(arch_cmpxchg64_relaxed)
> > > #define raw_cmpxchg64_release(...) \
> > > 	__atomic_op_release(arch_cmpxchg64, __VA_ARGS__)
> > > #elif defined(arch_cmpxchg64)
> > > #define raw_cmpxchg64_release arch_cmpxchg64
> > > #else
> > > extern void raw_cmpxchg64_release_not_implemented(void);
> > >      ^^^^^^^^^^^^^^^^^^^^^
> > 
> > This means that arc isn't providing a suitable defintion to build
> > raw_cmpxchg64_release() from, or for some reason the header includes up
> > to this point haven't included the relevant definition.
> > 
> > From the ifdeffery, there's no definition of:
> > 
> >   arch_cmpxchg64_release
> >   arch_cmpxchg64_relaxed
> >   arch_cmpxchg64
> > 
> > ... and hence no way to build raw_cmpxchg64_release().
> > 
> > The intent here is to have a build failure at point of use, since some
> > architectures do not or cannot provide these, but we should clean this
> > up to be clearer. The mismatch is intentional and this isn't a typo, but
> > I agree it's not great.
> 
> It is not consistent..
> 
> For instance on ARC io-pgtable-arm.c compiles OK it calls:
> 
> 	old = cmpxchg64_relaxed(ptep, curr, new);
> 
> Which expands to:
> 
>  old = ({ typeof(ptep) __ai_ptr = (ptep); instrument_atomic_read_write(__ai_ptr, sizeof(*__ai_ptr)); raw_cmpxchg64_relaxed_not_implemented(); });
> 
> And no compiler error. Presumably it doesn't link, but my compiler
> ICE's before it gets that far.

I don't think that "my compiler ICE's" implies "compiles OK".

When building io-pgtable-arm.c for ARC (with your tree!) I see the same error:

| [mark@lakrids:~/src/linux]% git describe HEAD
| v6.13-rc1-19-gf81fe181dcfff
| [mark@lakrids:~/src/linux]% usekorg 13.2.0 make ARCH=arc CROSS_COMPILE=arc-linux- drivers/iommu/io-pgtable-arm.o 
|   CC      kernel/bounds.s
|   CC      arch/arc/kernel/asm-offsets.s
|   CALL    scripts/checksyscalls.sh
|   CC      drivers/iommu/io-pgtable-arm.o
| drivers/iommu/io-pgtable-arm.c: In function 'arm_lpae_install_table':
| drivers/iommu/io-pgtable-arm.c:387:13: error: void value not ignored as it ought to be
|   387 |         old = cmpxchg64_relaxed(ptep, curr, new);
|       |             ^
| make[4]: *** [scripts/Makefile.build:194: drivers/iommu/io-pgtable-arm.o] Error 1
| make[3]: *** [scripts/Makefile.build:440: drivers/iommu] Error 2
| make[2]: *** [scripts/Makefile.build:440: drivers] Error 2
| make[1]: *** [/home/mark/src/linux/Makefile:1989: .] Error 2
| make: *** [Makefile:251: __sub-make] Error 2

... so I suspect whatever is causing your toolchain to ICE is masking
that, and (AFAICT) the error is consistent.

Note that per drivers/iommu/{Kconfig,Makefile}, if you build with
CONFIG_GENERIC_ATOMIC64=y, then CONFIG_IOMMU_IO_PGTABLE_LPAE=n and so
io-pgtable-arm.o won't actually be built.

FWIW, I'm using the cross toolchains from:

  https://mirrors.edge.kernel.org/pub/tools/crosstool/

> > In this case I think this is an oversight in the arc code, and arc *can*
> > provide a definition of arch_cmpxchg64(), as per the hack below (which
> > implicilty provides arch_atomic64_cmpxchg*()):
> > 
> > | diff --git a/arch/arc/include/asm/atomic64-arcv2.h b/arch/arc/include/asm/atomic64-arcv2.h
> > | index 9b5791b854713..ce3fdcb48b0f9 100644
> > | --- a/arch/arc/include/asm/atomic64-arcv2.h
> > | +++ b/arch/arc/include/asm/atomic64-arcv2.h
> > | @@ -137,12 +137,10 @@ ATOMIC64_OPS(xor, xor, xor)
> > |  #undef ATOMIC64_OP_RETURN
> > |  #undef ATOMIC64_OP
> > |  
> > | -static inline s64
> > | -arch_atomic64_cmpxchg(atomic64_t *ptr, s64 expected, s64 new)
> > | +static inline u64
> > | +__arch_cmpxchg64_relaxed(volatile void *ptr, u64 old, u64 new)
> > |  {
> > | -       s64 prev;
> > | -
> > | -       smp_mb();
> > | +       u64 prev;
> > |  
> > |         __asm__ __volatile__(
> > |         "1:     llockd  %0, [%1]        \n"
> > | @@ -152,14 +150,12 @@ arch_atomic64_cmpxchg(atomic64_t *ptr, s64 expected, s64 new)
> > |         "       bnz     1b              \n"
> > |         "2:                             \n"
> > |         : "=&r"(prev)
> > | -       : "r"(ptr), "ir"(expected), "r"(new)
> > | -       : "cc");        /* memory clobber comes from smp_mb() */
> > | -
> > | -       smp_mb();
> > | +       : "r"(ptr), "ir"(old), "r"(new)
> > | +       : "memory", "cc");
> > |  
> > |         return prev;
> > |  }
> > | -#define arch_atomic64_cmpxchg arch_atomic64_cmpxchg
> > | +#define arch_cmpxchg64_relaxed __arch_cmpxchg64_relaxed
> > |  
> > |  static inline s64 arch_atomic64_xchg(atomic64_t *ptr, s64 new)
> > |  {
> 
> Okay, that is what I was expecting to find, so I can ping the arc
> folks on this direction and maybe get this resolved.. I'll send the
> above to them as a patch to start a discussion

Cool; please Cc me for that.

Mark.