Message-ID: <Z0747n5bSep4_1VX@J2N7QTR9R3>
Date: Tue, 3 Dec 2024 12:26:22 +0000
From: Mark Rutland <mark.rutland@....com>
To: Jason Gunthorpe <jgg@...dia.com>
Cc: Uros Bizjak <ubizjak@...il.com>,
	Alejandro Jimenez <alejandro.j.jimenez@...cle.com>,
	linux-kernel@...r.kernel.org
Subject: Re: Help with atomic fallback

On Mon, Dec 02, 2024 at 08:38:56PM -0400, Jason Gunthorpe wrote:
> Hi Mark/Uros,

Hi Jason,

> I hope one of you can help me unravel this, I'm trying to use
> try_cmpxchg64_release() from driver code and 0-day is saying arc
> compiles explode:
> 
>    In file included from include/linux/atomic.h:80,
>                     from drivers/iommu/generic_pt/fmt/../pt_defs.h:17,
>                     from drivers/iommu/generic_pt/fmt/iommu_template.h:35,
>                     from drivers/iommu/generic_pt/fmt/iommu_armv8_4k.c:13:
>    drivers/iommu/generic_pt/fmt/../pt_defs.h: In function 'pt_table_install64':
> >> include/linux/atomic/atomic-arch-fallback.h:295:14: error: void value not ignored as it ought to be
>      295 |         ___r = raw_cmpxchg64_release((_ptr), ___o, (_new)); \
>          |              ^
>    include/linux/atomic/atomic-instrumented.h:4937:9: note: in expansion of macro 'raw_try_cmpxchg64_release'
>     4937 |         raw_try_cmpxchg64_release(__ai_ptr, __ai_oldp, __VA_ARGS__); \
>          |         ^~~~~~~~~~~~~~~~~~~~~~~~~
>    drivers/iommu/generic_pt/fmt/../pt_defs.h:144:16: note: in expansion of macro 'try_cmpxchg64_release'
>      144 |         return try_cmpxchg64_release(entryp, &old_entry, table_entry);

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

> 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.

> That should return int to make the compiler happy, but then it will
> fail to link (I think, my cross compiler ICEs before it gets there)
> 
> However, arc defines:
> 
> static inline s64
> arch_atomic64_cmpxchg(atomic64_t *ptr, s64 expected, s64 new)
> {
>
> And I see a:
> 
> static __always_inline s64
> raw_atomic64_cmpxchg_release(atomic64_t *v, s64 old, s64 new)
> {
> #if defined(arch_atomic64_cmpxchg_release)
> 	return arch_atomic64_cmpxchg_release(v, old, new);
> #elif defined(arch_atomic64_cmpxchg_relaxed)
> 	__atomic_release_fence();
> 	return arch_atomic64_cmpxchg_relaxed(v, old, new);
> #elif defined(arch_atomic64_cmpxchg)
> 	return arch_atomic64_cmpxchg(v, old, new);
> 
> Which seems to strongly imply that arc can do the cmpxchg64_release
> primitive.
> 
> But I haven't been able to figure out what is expected here for
> arch_atomic64 vs try_cmpxchg64 to guess what is missing part here :\

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)
|  {

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()

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

Mark.

Powered by blists - more mailing lists