Re: Help with atomic fallback

[Uros Bizjak]

On Tue, Dec 3, 2024 at 1:39 AM Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
>
> Hi Mark/Uros,
>
> 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);
>
> 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);
>      ^^^^^^^^^^^^^^^^^^^^^
>
> 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)
> {

Please note that arch_atomic64_cmpxchg() and arch_cmpxchg64() are two
different things.

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

No, this is *atomic64* version. Arc has to define arch_cmpxchg64 in
its cmpxchg.h. An example can be seen in e.g.
arch/m68k/include/asm/cmpxchg.h [1], where we have:

#include <asm-generic/cmpxchg-local.h>

#define arch_cmpxchg64_local(ptr, o, n)
__generic_cmpxchg64_local((ptr), (o), (n))

#define arch_cmpxchg64(ptr, o, n) arch_cmpxchg64_local((ptr), (o), (n))

[1] https://elixir.bootlin.com/linux/v6.12.1/source/arch/m68k/include/asm/cmpxchg.h

> 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 :\
>
> Any advice?

Please use system_has_cmpxchg64 define, as is the case in
source/mm/slab.h. Arc does not define arch_cmpxchg64() and should be
excluded from compilation.

Uros.