Re: Help with atomic fallback

[Jason Gunthorpe <jgg@nvidia.com>]

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.

> 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

> However, there are other cases where cmpxchg64 doesn't exist or cannot
> be used, and the existing (x86-specific) system_has_cmpxchg64() isn't
> ideal. I suspect we need both a Kconfig symbol and a runtime check to
> handle this properly.

> I think if we fix up arc along the lines of the above (with xchg too,
> and handled in the cmpxchg header), then we can rely on the Kconfig
> check that the existing io-pgtable code has:
> 
>   depends on !GENERIC_ATOMIC64    # for cmpxchg64()

Yes, I have been relying on this as it seems the closest thing we have
today.

> ... and we'll (separately) need to figure out what to do for the runtime
> system_has_cmpxchg64() check.

It is gross, but at least today we can do as slab does and #ifdef
system_has_cmpxchg64

Thanks,
Jason