[PATCH bpf-next v2] selftests/bpf: Fix arena_atomics selftest failure due to llvm change

[PATCH bpf-next v2] selftests/bpf: Fix arena_atomics selftest failure due to llvm change

[Yonghong Song]

Peilen Ye reported an issue ([1]) where for __sync_fetch_and_add(...) without
return value like
  __sync_fetch_and_add(&foo, 1);
llvm BPF backend generates locked insn e.g.
  lock *(u32 *)(r1 + 0) += r2

If __sync_fetch_and_add(...) returns a value like
  res = __sync_fetch_and_add(&foo, 1);
llvm BPF backend generates like
  r2 = atomic_fetch_add((u32 *)(r1 + 0), r2)

But 'lock *(u32 *)(r1 + 0) += r2' caused a problem in jit
since proper barrier is not inserted based on __sync_fetch_and_add() semantics.

The above discrepancy is due to commit [2] where it tries to maintain backward
compatability since before commit [2], __sync_fetch_and_add(...) generates
lock insn in BPF backend.

Based on discussion in [1], now it is time to fix the above discrepancy so we can
have proper barrier support in jit. llvm patch [3] made sure that __sync_fetch_and_add(...)
always generates atomic_fetch_add(...) insns. Now 'lock *(u32 *)(r1 + 0) += r2' can only
be generated by inline asm. The same for __sync_fetch_and_and(), __sync_fetch_and_or()
and __sync_fetch_and_xor().

But the change in [3] caused arena_atomics selftest failure.

  test_arena_atomics:PASS:arena atomics skeleton open 0 nsec
  libbpf: prog 'and': BPF program load failed: Permission denied
  libbpf: prog 'and': -- BEGIN PROG LOAD LOG --
  arg#0 reference type('UNKNOWN ') size cannot be determined: -22
  0: R1=ctx() R10=fp0
  ; if (pid != (bpf_get_current_pid_tgid() >> 32)) @ arena_atomics.c:87
  0: (18) r1 = 0xffffc90000064000       ; R1_w=map_value(map=arena_at.bss,ks=4,vs=4)
  2: (61) r6 = *(u32 *)(r1 +0)          ; R1_w=map_value(map=arena_at.bss,ks=4,vs=4) R6_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff))
  3: (85) call bpf_get_current_pid_tgid#14      ; R0_w=scalar()
  4: (77) r0 >>= 32                     ; R0_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff))
  5: (5d) if r0 != r6 goto pc+11        ; R0_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff)) R6_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0x)
  ; __sync_fetch_and_and(&and64_value, 0x011ull << 32); @ arena_atomics.c:91
  6: (18) r1 = 0x100000000060           ; R1_w=scalar()
  8: (bf) r1 = addr_space_cast(r1, 0, 1)        ; R1_w=arena
  9: (18) r2 = 0x1100000000             ; R2_w=0x1100000000
  11: (db) r2 = atomic64_fetch_and((u64 *)(r1 +0), r2)
  BPF_ATOMIC stores into R1 arena is not allowed
  processed 9 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
  -- END PROG LOAD LOG --
  libbpf: prog 'and': failed to load: -13
  libbpf: failed to load object 'arena_atomics'
  libbpf: failed to load BPF skeleton 'arena_atomics': -13
  test_arena_atomics:FAIL:arena atomics skeleton load unexpected error: -13 (errno 13)
  #3       arena_atomics:FAIL

The reason of the failure is due to [4] where atomic{64,}_fetch_{and,or,xor}() are not
allowed by arena addresses. Without llvm patch [3], the compiler will generate 'lock ...'
insn and everything will work fine.

This patch fixed the problem by using inline asms. Instead of __sync_fetch_and_{and,or,xor}() functions,
the inline asm with 'lock' insn is used and it will work with or without [3].
Note that three bpf programs ('and', 'or' and 'xor') are guarded with __BPF_FEATURE_ADDR_SPACE_CAST
as well to ensure compilation failure for llvm <= 18 version. Note that for llvm <= 18 where
addr_space_cast is not supported, all arena_atomics subtests are skipped with below message:
  test_arena_atomics:SKIP:no ENABLE_ATOMICS_TESTS or no addr_space_cast support in clang
  #3 arena_atomics:SKIP

  [1] https://lore.kernel.org/bpf/ZqqiQQWRnz7H93Hc@google.com/T/#mb68d67bc8f39e35a0c3db52468b9de59b79f021f
  [2] https://github.com/llvm/llvm-project/commit/286daafd65129228e08a1d07aa4ca74488615744
  [3] https://github.com/llvm/llvm-project/pull/101428
  [4] d503a04f8bc0 ("bpf: Add support for certain atomics in bpf_arena to x86 JIT")

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 .../selftests/bpf/progs/arena_atomics.c       | 63 ++++++++++++++++---
 1 file changed, 54 insertions(+), 9 deletions(-)

Changelog:
  v1 -> v2:
    - Add __BPF_FEATURE_ADDR_SPACE_CAST to guard newly added asm codes for llvm >= 19

diff --git a/tools/testing/selftests/bpf/progs/arena_atomics.c b/tools/testing/selftests/bpf/progs/arena_atomics.c
index bb0acd79d28a..dea54557fc00 100644
--- a/tools/testing/selftests/bpf/progs/arena_atomics.c
+++ b/tools/testing/selftests/bpf/progs/arena_atomics.c
@@ -5,6 +5,7 @@
 #include <bpf/bpf_tracing.h>
 #include <stdbool.h>
 #include "bpf_arena_common.h"
+#include "bpf_misc.h"
 
 struct {
 	__uint(type, BPF_MAP_TYPE_ARENA);
@@ -85,10 +86,24 @@ int and(const void *ctx)
 {
 	if (pid != (bpf_get_current_pid_tgid() >> 32))
 		return 0;
-#ifdef ENABLE_ATOMICS_TESTS
+#if defined(ENABLE_ATOMICS_TESTS) && defined(__BPF_FEATURE_ADDR_SPACE_CAST)
 
-	__sync_fetch_and_and(&and64_value, 0x011ull << 32);
-	__sync_fetch_and_and(&and32_value, 0x011);
+	asm volatile(
+		"r1 = addr_space_cast(%[and64_value], 0, 1);"
+		"lock *(u64 *)(r1 + 0) &= %[val]"
+		:
+		: __imm_ptr(and64_value),
+		  [val]"r"(0x011ull << 32)
+		: "r1"
+	);
+	asm volatile(
+		"r1 = addr_space_cast(%[and32_value], 0, 1);"
+		"lock *(u32 *)(r1 + 0) &= %[val]"
+		:
+		: __imm_ptr(and32_value),
+		  [val]"w"(0x011)
+		: "r1"
+	);
 #endif
 
 	return 0;
@@ -102,9 +117,24 @@ int or(const void *ctx)
 {
 	if (pid != (bpf_get_current_pid_tgid() >> 32))
 		return 0;
-#ifdef ENABLE_ATOMICS_TESTS
-	__sync_fetch_and_or(&or64_value, 0x011ull << 32);
-	__sync_fetch_and_or(&or32_value, 0x011);
+#if defined(ENABLE_ATOMICS_TESTS) && defined(__BPF_FEATURE_ADDR_SPACE_CAST)
+
+	asm volatile(
+		"r1 = addr_space_cast(%[or64_value], 0, 1);"
+		"lock *(u64 *)(r1 + 0) |= %[val]"
+		:
+		: __imm_ptr(or64_value),
+		  [val]"r"(0x011ull << 32)
+		: "r1"
+	);
+	asm volatile(
+		"r1 = addr_space_cast(%[or32_value], 0, 1);"
+		"lock *(u32 *)(r1 + 0) |= %[val]"
+		:
+		: __imm_ptr(or32_value),
+		  [val]"w"(0x011)
+		: "r1"
+	);
 #endif
 
 	return 0;
@@ -118,9 +148,24 @@ int xor(const void *ctx)
 {
 	if (pid != (bpf_get_current_pid_tgid() >> 32))
 		return 0;
-#ifdef ENABLE_ATOMICS_TESTS
-	__sync_fetch_and_xor(&xor64_value, 0x011ull << 32);
-	__sync_fetch_and_xor(&xor32_value, 0x011);
+#if defined(ENABLE_ATOMICS_TESTS) && defined(__BPF_FEATURE_ADDR_SPACE_CAST)
+
+	asm volatile(
+		"r1 = addr_space_cast(%[xor64_value], 0, 1);"
+		"lock *(u64 *)(r1 + 0) ^= %[val]"
+		:
+		: __imm_ptr(xor64_value),
+		  [val]"r"(0x011ull << 32)
+		: "r1"
+	);
+	asm volatile(
+		"r1 = addr_space_cast(%[xor32_value], 0, 1);"
+		"lock *(u32 *)(r1 + 0) ^= %[val]"
+		:
+		: __imm_ptr(xor32_value),
+		  [val]"w"(0x011)
+		: "r1"
+	);
 #endif
 
 	return 0;
-- 
2.43.0

Re: [PATCH bpf-next v2] selftests/bpf: Fix arena_atomics selftest failure due to llvm change

[Alexei Starovoitov]

On Fri, Aug 2, 2024 at 7:59 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
> Peilen Ye reported an issue ([1]) where for __sync_fetch_and_add(...) without
> return value like
>   __sync_fetch_and_add(&foo, 1);
> llvm BPF backend generates locked insn e.g.
>   lock *(u32 *)(r1 + 0) += r2
>
> If __sync_fetch_and_add(...) returns a value like
>   res = __sync_fetch_and_add(&foo, 1);
> llvm BPF backend generates like
>   r2 = atomic_fetch_add((u32 *)(r1 + 0), r2)
>
> But 'lock *(u32 *)(r1 + 0) += r2' caused a problem in jit
> since proper barrier is not inserted based on __sync_fetch_and_add() semantics.
>
> The above discrepancy is due to commit [2] where it tries to maintain backward
> compatability since before commit [2], __sync_fetch_and_add(...) generates
> lock insn in BPF backend.
>
> Based on discussion in [1], now it is time to fix the above discrepancy so we can
> have proper barrier support in jit. llvm patch [3] made sure that __sync_fetch_and_add(...)
> always generates atomic_fetch_add(...) insns. Now 'lock *(u32 *)(r1 + 0) += r2' can only
> be generated by inline asm. The same for __sync_fetch_and_and(), __sync_fetch_and_or()
> and __sync_fetch_and_xor().
>
> But the change in [3] caused arena_atomics selftest failure.
>
>   test_arena_atomics:PASS:arena atomics skeleton open 0 nsec
>   libbpf: prog 'and': BPF program load failed: Permission denied
>   libbpf: prog 'and': -- BEGIN PROG LOAD LOG --
>   arg#0 reference type('UNKNOWN ') size cannot be determined: -22
>   0: R1=ctx() R10=fp0
>   ; if (pid != (bpf_get_current_pid_tgid() >> 32)) @ arena_atomics.c:87
>   0: (18) r1 = 0xffffc90000064000       ; R1_w=map_value(map=arena_at.bss,ks=4,vs=4)
>   2: (61) r6 = *(u32 *)(r1 +0)          ; R1_w=map_value(map=arena_at.bss,ks=4,vs=4) R6_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff))
>   3: (85) call bpf_get_current_pid_tgid#14      ; R0_w=scalar()
>   4: (77) r0 >>= 32                     ; R0_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff))
>   5: (5d) if r0 != r6 goto pc+11        ; R0_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff)) R6_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0x)
>   ; __sync_fetch_and_and(&and64_value, 0x011ull << 32); @ arena_atomics.c:91
>   6: (18) r1 = 0x100000000060           ; R1_w=scalar()
>   8: (bf) r1 = addr_space_cast(r1, 0, 1)        ; R1_w=arena
>   9: (18) r2 = 0x1100000000             ; R2_w=0x1100000000
>   11: (db) r2 = atomic64_fetch_and((u64 *)(r1 +0), r2)
>   BPF_ATOMIC stores into R1 arena is not allowed
>   processed 9 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>   -- END PROG LOAD LOG --
>   libbpf: prog 'and': failed to load: -13
>   libbpf: failed to load object 'arena_atomics'
>   libbpf: failed to load BPF skeleton 'arena_atomics': -13
>   test_arena_atomics:FAIL:arena atomics skeleton load unexpected error: -13 (errno 13)
>   #3       arena_atomics:FAIL
>
> The reason of the failure is due to [4] where atomic{64,}_fetch_{and,or,xor}() are not
> allowed by arena addresses. Without llvm patch [3], the compiler will generate 'lock ...'
> insn and everything will work fine.
>
> This patch fixed the problem by using inline asms. Instead of __sync_fetch_and_{and,or,xor}() functions,
> the inline asm with 'lock' insn is used and it will work with or without [3].
> Note that three bpf programs ('and', 'or' and 'xor') are guarded with __BPF_FEATURE_ADDR_SPACE_CAST
> as well to ensure compilation failure for llvm <= 18 version. Note that for llvm <= 18 where
> addr_space_cast is not supported, all arena_atomics subtests are skipped with below message:
>   test_arena_atomics:SKIP:no ENABLE_ATOMICS_TESTS or no addr_space_cast support in clang
>   #3 arena_atomics:SKIP
>
>   [1] https://lore.kernel.org/bpf/ZqqiQQWRnz7H93Hc@google.com/T/#mb68d67bc8f39e35a0c3db52468b9de59b79f021f
>   [2] https://github.com/llvm/llvm-project/commit/286daafd65129228e08a1d07aa4ca74488615744
>   [3] https://github.com/llvm/llvm-project/pull/101428
>   [4] d503a04f8bc0 ("bpf: Add support for certain atomics in bpf_arena to x86 JIT")
>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
>  .../selftests/bpf/progs/arena_atomics.c       | 63 ++++++++++++++++---
>  1 file changed, 54 insertions(+), 9 deletions(-)
>
> Changelog:
>   v1 -> v2:
>     - Add __BPF_FEATURE_ADDR_SPACE_CAST to guard newly added asm codes for llvm >= 19
>
> diff --git a/tools/testing/selftests/bpf/progs/arena_atomics.c b/tools/testing/selftests/bpf/progs/arena_atomics.c
> index bb0acd79d28a..dea54557fc00 100644
> --- a/tools/testing/selftests/bpf/progs/arena_atomics.c
> +++ b/tools/testing/selftests/bpf/progs/arena_atomics.c
> @@ -5,6 +5,7 @@
>  #include <bpf/bpf_tracing.h>
>  #include <stdbool.h>
>  #include "bpf_arena_common.h"
> +#include "bpf_misc.h"
>
>  struct {
>         __uint(type, BPF_MAP_TYPE_ARENA);
> @@ -85,10 +86,24 @@ int and(const void *ctx)
>  {
>         if (pid != (bpf_get_current_pid_tgid() >> 32))
>                 return 0;
> -#ifdef ENABLE_ATOMICS_TESTS
> +#if defined(ENABLE_ATOMICS_TESTS) && defined(__BPF_FEATURE_ADDR_SPACE_CAST)
>
> -       __sync_fetch_and_and(&and64_value, 0x011ull << 32);
> -       __sync_fetch_and_and(&and32_value, 0x011);
> +       asm volatile(
> +               "r1 = addr_space_cast(%[and64_value], 0, 1);"
> +               "lock *(u64 *)(r1 + 0) &= %[val]"
> +               :
> +               : __imm_ptr(and64_value),
> +                 [val]"r"(0x011ull << 32)
> +               : "r1"
> +       );
> +       asm volatile(
> +               "r1 = addr_space_cast(%[and32_value], 0, 1);"
> +               "lock *(u32 *)(r1 + 0) &= %[val]"
> +               :
> +               : __imm_ptr(and32_value),
> +                 [val]"w"(0x011)
> +               : "r1"
> +       );

Instead of inline asm there is a better way to do the same in C.
https://godbolt.org/z/71PYx1oqE

void foo(int a, _Atomic int *b)
{
 *b += a;
}

generates:
lock *(u32 *)(r2 + 0) += r1

but
*b &= a;

crashes llvm :( with

<source>:3:5: error: unsupported atomic operation, please use 64 bit version
    3 |  *b &= a;

but works with -mcpu=v3

So let's make this test mcpu=v3 only and use normal C ?

pw-bot: cr

Re: [PATCH bpf-next v2] selftests/bpf: Fix arena_atomics selftest failure due to llvm change

[Yonghong Song]

On 8/5/24 10:53 AM, Alexei Starovoitov wrote:
> On Fri, Aug 2, 2024 at 7:59 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>> Peilen Ye reported an issue ([1]) where for __sync_fetch_and_add(...) without
>> return value like
>>    __sync_fetch_and_add(&foo, 1);
>> llvm BPF backend generates locked insn e.g.
>>    lock *(u32 *)(r1 + 0) += r2
>>
>> If __sync_fetch_and_add(...) returns a value like
>>    res = __sync_fetch_and_add(&foo, 1);
>> llvm BPF backend generates like
>>    r2 = atomic_fetch_add((u32 *)(r1 + 0), r2)
>>
>> But 'lock *(u32 *)(r1 + 0) += r2' caused a problem in jit
>> since proper barrier is not inserted based on __sync_fetch_and_add() semantics.
>>
>> The above discrepancy is due to commit [2] where it tries to maintain backward
>> compatability since before commit [2], __sync_fetch_and_add(...) generates
>> lock insn in BPF backend.
>>
>> Based on discussion in [1], now it is time to fix the above discrepancy so we can
>> have proper barrier support in jit. llvm patch [3] made sure that __sync_fetch_and_add(...)
>> always generates atomic_fetch_add(...) insns. Now 'lock *(u32 *)(r1 + 0) += r2' can only
>> be generated by inline asm. The same for __sync_fetch_and_and(), __sync_fetch_and_or()
>> and __sync_fetch_and_xor().
>>
>> But the change in [3] caused arena_atomics selftest failure.
>>
>>    test_arena_atomics:PASS:arena atomics skeleton open 0 nsec
>>    libbpf: prog 'and': BPF program load failed: Permission denied
>>    libbpf: prog 'and': -- BEGIN PROG LOAD LOG --
>>    arg#0 reference type('UNKNOWN ') size cannot be determined: -22
>>    0: R1=ctx() R10=fp0
>>    ; if (pid != (bpf_get_current_pid_tgid() >> 32)) @ arena_atomics.c:87
>>    0: (18) r1 = 0xffffc90000064000       ; R1_w=map_value(map=arena_at.bss,ks=4,vs=4)
>>    2: (61) r6 = *(u32 *)(r1 +0)          ; R1_w=map_value(map=arena_at.bss,ks=4,vs=4) R6_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff))
>>    3: (85) call bpf_get_current_pid_tgid#14      ; R0_w=scalar()
>>    4: (77) r0 >>= 32                     ; R0_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff))
>>    5: (5d) if r0 != r6 goto pc+11        ; R0_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff)) R6_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0x)
>>    ; __sync_fetch_and_and(&and64_value, 0x011ull << 32); @ arena_atomics.c:91
>>    6: (18) r1 = 0x100000000060           ; R1_w=scalar()
>>    8: (bf) r1 = addr_space_cast(r1, 0, 1)        ; R1_w=arena
>>    9: (18) r2 = 0x1100000000             ; R2_w=0x1100000000
>>    11: (db) r2 = atomic64_fetch_and((u64 *)(r1 +0), r2)
>>    BPF_ATOMIC stores into R1 arena is not allowed
>>    processed 9 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>>    -- END PROG LOAD LOG --
>>    libbpf: prog 'and': failed to load: -13
>>    libbpf: failed to load object 'arena_atomics'
>>    libbpf: failed to load BPF skeleton 'arena_atomics': -13
>>    test_arena_atomics:FAIL:arena atomics skeleton load unexpected error: -13 (errno 13)
>>    #3       arena_atomics:FAIL
>>
>> The reason of the failure is due to [4] where atomic{64,}_fetch_{and,or,xor}() are not
>> allowed by arena addresses. Without llvm patch [3], the compiler will generate 'lock ...'
>> insn and everything will work fine.
>>
>> This patch fixed the problem by using inline asms. Instead of __sync_fetch_and_{and,or,xor}() functions,
>> the inline asm with 'lock' insn is used and it will work with or without [3].
>> Note that three bpf programs ('and', 'or' and 'xor') are guarded with __BPF_FEATURE_ADDR_SPACE_CAST
>> as well to ensure compilation failure for llvm <= 18 version. Note that for llvm <= 18 where
>> addr_space_cast is not supported, all arena_atomics subtests are skipped with below message:
>>    test_arena_atomics:SKIP:no ENABLE_ATOMICS_TESTS or no addr_space_cast support in clang
>>    #3 arena_atomics:SKIP
>>
>>    [1] https://lore.kernel.org/bpf/ZqqiQQWRnz7H93Hc@google.com/T/#mb68d67bc8f39e35a0c3db52468b9de59b79f021f
>>    [2] https://github.com/llvm/llvm-project/commit/286daafd65129228e08a1d07aa4ca74488615744
>>    [3] https://github.com/llvm/llvm-project/pull/101428
>>    [4] d503a04f8bc0 ("bpf: Add support for certain atomics in bpf_arena to x86 JIT")
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   .../selftests/bpf/progs/arena_atomics.c       | 63 ++++++++++++++++---
>>   1 file changed, 54 insertions(+), 9 deletions(-)
>>
>> Changelog:
>>    v1 -> v2:
>>      - Add __BPF_FEATURE_ADDR_SPACE_CAST to guard newly added asm codes for llvm >= 19
>>
>> diff --git a/tools/testing/selftests/bpf/progs/arena_atomics.c b/tools/testing/selftests/bpf/progs/arena_atomics.c
>> index bb0acd79d28a..dea54557fc00 100644
>> --- a/tools/testing/selftests/bpf/progs/arena_atomics.c
>> +++ b/tools/testing/selftests/bpf/progs/arena_atomics.c
>> @@ -5,6 +5,7 @@
>>   #include <bpf/bpf_tracing.h>
>>   #include <stdbool.h>
>>   #include "bpf_arena_common.h"
>> +#include "bpf_misc.h"
>>
>>   struct {
>>          __uint(type, BPF_MAP_TYPE_ARENA);
>> @@ -85,10 +86,24 @@ int and(const void *ctx)
>>   {
>>          if (pid != (bpf_get_current_pid_tgid() >> 32))
>>                  return 0;
>> -#ifdef ENABLE_ATOMICS_TESTS
>> +#if defined(ENABLE_ATOMICS_TESTS) && defined(__BPF_FEATURE_ADDR_SPACE_CAST)
>>
>> -       __sync_fetch_and_and(&and64_value, 0x011ull << 32);
>> -       __sync_fetch_and_and(&and32_value, 0x011);
>> +       asm volatile(
>> +               "r1 = addr_space_cast(%[and64_value], 0, 1);"
>> +               "lock *(u64 *)(r1 + 0) &= %[val]"
>> +               :
>> +               : __imm_ptr(and64_value),
>> +                 [val]"r"(0x011ull << 32)
>> +               : "r1"
>> +       );
>> +       asm volatile(
>> +               "r1 = addr_space_cast(%[and32_value], 0, 1);"
>> +               "lock *(u32 *)(r1 + 0) &= %[val]"
>> +               :
>> +               : __imm_ptr(and32_value),
>> +                 [val]"w"(0x011)
>> +               : "r1"
>> +       );
> Instead of inline asm there is a better way to do the same in C.
> https://godbolt.org/z/71PYx1oqE
>
> void foo(int a, _Atomic int *b)
> {
>   *b += a;
> }
>
> generates:
> lock *(u32 *)(r2 + 0) += r1

If you use latest llvm-project with
https://github.com/llvm/llvm-project/pull/101428
included, the above code will generate like

$ clang --target=bpf -O2 -c t.c && llvm-objdump -d t.o
t.o:    file format elf64-bpf
Disassembly of section .text:
0000000000000000 <foo>:
        0:       c3 12 00 00 01 00 00 00 r1 = atomic_fetch_add((u32 *)(r2 + 0x0), r1)
        1:       95 00 00 00 00 00 00 00 exit


With -mcpu=v3 the same code can be generated.

>
> but
> *b &= a;
>
> crashes llvm :( with
>
> <source>:3:5: error: unsupported atomic operation, please use 64 bit version
>      3 |  *b &= a;

It failed with the following llvm error message:
t.c:1:6: error: unsupported atomic operation, please use 64 bit version
     1 | void foo(int a, _Atomic int *b)
       |      ^
fatal error: error in backend: Cannot select: t8: i64,ch = AtomicLoadAnd<(load store seq_cst (s32) on %ir.b)> t0, t4, t2
   t4: i64,ch = CopyFromReg t0, Register:i64 %1
     t3: i64 = Register %1
   t2: i64,ch = CopyFromReg t0, Register:i64 %0
     t1: i64 = Register %0
In function: foo

>
> but works with -mcpu=v3

Yes. it does work with -mcpu=v3:

$ clang --target=bpf -O2 -mcpu=v3 -c t.c && llvm-objdump -d --mcpu=v3 t.o

t.o:    file format elf64-bpf
Disassembly of section .text:
0000000000000000 <foo>:
        0:       c3 12 00 00 51 00 00 00 w1 = atomic_fetch_and((u32 *)(r2 + 0x0), w1)
        1:       95 00 00 00 00 00 00 00 exit

NOTE: I need -mcpu=v3 for llvm-objdump to print asm code 'atomic_fetch_and' properly.
Will double check this.

For code:
void foo(int a, _Atomic int *b)
{
  *b &= a;
}

The initial IR generated by clang frontend is:

define dso_local void @foo(i32 noundef %a, ptr noundef %b) #0 {
entry:
   %a.addr = alloca i32, align 4
   %b.addr = alloca ptr, align 8
   store i32 %a, ptr %a.addr, align 4, !tbaa !3
   store ptr %b, ptr %b.addr, align 8, !tbaa !7
   %0 = load i32, ptr %a.addr, align 4, !tbaa !3
   %1 = load ptr, ptr %b.addr, align 8, !tbaa !7
   %2 = atomicrmw and ptr %1, i32 %0 seq_cst, align 4
   %3 = and i32 %2, %0
   ret void
}

Note that atomicrmw in the above. Eventually it optimized to

define dso_local void @foo(i32 noundef %a, ptr noundef %b) #0 {
entry:
   %0 = atomicrmw and ptr %b, i32 %a seq_cst, align 4
   ret void
}

The 'atomicrmw' is the same IR as generated by
__sync_fetch_and_*() and eventually will generate atomic_fetch_*() bpf 
insn.
Discussed with Andrii, and
another option is to specify relaxed consistency, so llvm
internal could translate it into locked insn. For example,

$ cat t1.c
#include <stdatomic.h>

void f(_Atomic int *i) {
   __c11_atomic_fetch_and(i, 1, memory_order_relaxed);
}

# to have gnu/stubs-32.h in the current directory to make it compile
[yhs@devvm1513.prn0 ~/tmp6]$ ls gnu
stubs-32.h
[yhs@devvm1513.prn0 ~/tmp6]$ clang --target=bpf -O2 -I. -c -mcpu=v3 t1.c

The initial IR:
define dso_local void @f(ptr noundef %i) #0 {
entry:
   %i.addr = alloca ptr, align 8
   %.atomictmp = alloca i32, align 4
   %atomic-temp = alloca i32, align 4
   store ptr %i, ptr %i.addr, align 8, !tbaa !3
   %0 = load ptr, ptr %i.addr, align 8, !tbaa !3
   store i32 1, ptr %.atomictmp, align 4, !tbaa !7
   %1 = load i32, ptr %.atomictmp, align 4
   %2 = atomicrmw and ptr %0, i32 %1 monotonic, align 4
   store i32 %2, ptr %atomic-temp, align 4
   %3 = load i32, ptr %atomic-temp, align 4, !tbaa !7
   ret void
}

The IR right before machine code generation:

define dso_local void @f(ptr nocapture noundef %i) local_unnamed_addr #0 {
entry:
   %0 = atomicrmw and ptr %i, i32 1 monotonic, align 4
   ret void
}

Maybe we could special process the above to generate
a locked insn if
   - atomicrmw operator
   - monotonic (related) consistency
   - return value is not used

So this will not violate original program semantics.
Does this sound a reasonable apporach?

>
> So let's make this test mcpu=v3 only and use normal C ?
>
> pw-bot: cr

Re: [PATCH bpf-next v2] selftests/bpf: Fix arena_atomics selftest failure due to llvm change

[Jose E. Marchesi]

> On 8/5/24 10:53 AM, Alexei Starovoitov wrote:
>> On Fri, Aug 2, 2024 at 7:59 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>> Peilen Ye reported an issue ([1]) where for __sync_fetch_and_add(...) without
>>> return value like
>>>    __sync_fetch_and_add(&foo, 1);
>>> llvm BPF backend generates locked insn e.g.
>>>    lock *(u32 *)(r1 + 0) += r2
>>>
>>> If __sync_fetch_and_add(...) returns a value like
>>>    res = __sync_fetch_and_add(&foo, 1);
>>> llvm BPF backend generates like
>>>    r2 = atomic_fetch_add((u32 *)(r1 + 0), r2)
>>>
>>> But 'lock *(u32 *)(r1 + 0) += r2' caused a problem in jit
>>> since proper barrier is not inserted based on __sync_fetch_and_add() semantics.
>>>
>>> The above discrepancy is due to commit [2] where it tries to maintain backward
>>> compatability since before commit [2], __sync_fetch_and_add(...) generates
>>> lock insn in BPF backend.
>>>
>>> Based on discussion in [1], now it is time to fix the above discrepancy so we can
>>> have proper barrier support in jit. llvm patch [3] made sure that __sync_fetch_and_add(...)
>>> always generates atomic_fetch_add(...) insns. Now 'lock *(u32 *)(r1 + 0) += r2' can only
>>> be generated by inline asm. The same for __sync_fetch_and_and(), __sync_fetch_and_or()
>>> and __sync_fetch_and_xor().
>>>
>>> But the change in [3] caused arena_atomics selftest failure.
>>>
>>>    test_arena_atomics:PASS:arena atomics skeleton open 0 nsec
>>>    libbpf: prog 'and': BPF program load failed: Permission denied
>>>    libbpf: prog 'and': -- BEGIN PROG LOAD LOG --
>>>    arg#0 reference type('UNKNOWN ') size cannot be determined: -22
>>>    0: R1=ctx() R10=fp0
>>>    ; if (pid != (bpf_get_current_pid_tgid() >> 32)) @ arena_atomics.c:87
>>>    0: (18) r1 = 0xffffc90000064000       ; R1_w=map_value(map=arena_at.bss,ks=4,vs=4)
>>>    2: (61) r6 = *(u32 *)(r1 +0)          ; R1_w=map_value(map=arena_at.bss,ks=4,vs=4) R6_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff))
>>>    3: (85) call bpf_get_current_pid_tgid#14      ; R0_w=scalar()
>>>    4: (77) r0 >>= 32                     ; R0_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff))
>>>    5: (5d) if r0 != r6 goto pc+11        ; R0_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff)) R6_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0x)
>>>    ; __sync_fetch_and_and(&and64_value, 0x011ull << 32); @ arena_atomics.c:91
>>>    6: (18) r1 = 0x100000000060           ; R1_w=scalar()
>>>    8: (bf) r1 = addr_space_cast(r1, 0, 1)        ; R1_w=arena
>>>    9: (18) r2 = 0x1100000000             ; R2_w=0x1100000000
>>>    11: (db) r2 = atomic64_fetch_and((u64 *)(r1 +0), r2)
>>>    BPF_ATOMIC stores into R1 arena is not allowed
>>>    processed 9 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>>>    -- END PROG LOAD LOG --
>>>    libbpf: prog 'and': failed to load: -13
>>>    libbpf: failed to load object 'arena_atomics'
>>>    libbpf: failed to load BPF skeleton 'arena_atomics': -13
>>>    test_arena_atomics:FAIL:arena atomics skeleton load unexpected error: -13 (errno 13)
>>>    #3       arena_atomics:FAIL
>>>
>>> The reason of the failure is due to [4] where atomic{64,}_fetch_{and,or,xor}() are not
>>> allowed by arena addresses. Without llvm patch [3], the compiler will generate 'lock ...'
>>> insn and everything will work fine.
>>>
>>> This patch fixed the problem by using inline asms. Instead of __sync_fetch_and_{and,or,xor}() functions,
>>> the inline asm with 'lock' insn is used and it will work with or without [3].
>>> Note that three bpf programs ('and', 'or' and 'xor') are guarded with __BPF_FEATURE_ADDR_SPACE_CAST
>>> as well to ensure compilation failure for llvm <= 18 version. Note that for llvm <= 18 where
>>> addr_space_cast is not supported, all arena_atomics subtests are skipped with below message:
>>>    test_arena_atomics:SKIP:no ENABLE_ATOMICS_TESTS or no addr_space_cast support in clang
>>>    #3 arena_atomics:SKIP
>>>
>>>    [1] https://lore.kernel.org/bpf/ZqqiQQWRnz7H93Hc@google.com/T/#mb68d67bc8f39e35a0c3db52468b9de59b79f021f
>>>    [2] https://github.com/llvm/llvm-project/commit/286daafd65129228e08a1d07aa4ca74488615744
>>>    [3] https://github.com/llvm/llvm-project/pull/101428
>>>    [4] d503a04f8bc0 ("bpf: Add support for certain atomics in bpf_arena to x86 JIT")
>>>
>>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>>> ---
>>>   .../selftests/bpf/progs/arena_atomics.c       | 63 ++++++++++++++++---
>>>   1 file changed, 54 insertions(+), 9 deletions(-)
>>>
>>> Changelog:
>>>    v1 -> v2:
>>>      - Add __BPF_FEATURE_ADDR_SPACE_CAST to guard newly added asm codes for llvm >= 19
>>>
>>> diff --git a/tools/testing/selftests/bpf/progs/arena_atomics.c b/tools/testing/selftests/bpf/progs/arena_atomics.c
>>> index bb0acd79d28a..dea54557fc00 100644
>>> --- a/tools/testing/selftests/bpf/progs/arena_atomics.c
>>> +++ b/tools/testing/selftests/bpf/progs/arena_atomics.c
>>> @@ -5,6 +5,7 @@
>>>   #include <bpf/bpf_tracing.h>
>>>   #include <stdbool.h>
>>>   #include "bpf_arena_common.h"
>>> +#include "bpf_misc.h"
>>>
>>>   struct {
>>>          __uint(type, BPF_MAP_TYPE_ARENA);
>>> @@ -85,10 +86,24 @@ int and(const void *ctx)
>>>   {
>>>          if (pid != (bpf_get_current_pid_tgid() >> 32))
>>>                  return 0;
>>> -#ifdef ENABLE_ATOMICS_TESTS
>>> +#if defined(ENABLE_ATOMICS_TESTS) && defined(__BPF_FEATURE_ADDR_SPACE_CAST)
>>>
>>> -       __sync_fetch_and_and(&and64_value, 0x011ull << 32);
>>> -       __sync_fetch_and_and(&and32_value, 0x011);
>>> +       asm volatile(
>>> +               "r1 = addr_space_cast(%[and64_value], 0, 1);"
>>> +               "lock *(u64 *)(r1 + 0) &= %[val]"
>>> +               :
>>> +               : __imm_ptr(and64_value),
>>> +                 [val]"r"(0x011ull << 32)
>>> +               : "r1"
>>> +       );
>>> +       asm volatile(
>>> +               "r1 = addr_space_cast(%[and32_value], 0, 1);"
>>> +               "lock *(u32 *)(r1 + 0) &= %[val]"
>>> +               :
>>> +               : __imm_ptr(and32_value),
>>> +                 [val]"w"(0x011)
>>> +               : "r1"
>>> +       );
>> Instead of inline asm there is a better way to do the same in C.
>> https://godbolt.org/z/71PYx1oqE
>>
>> void foo(int a, _Atomic int *b)
>> {
>>   *b += a;
>> }
>>
>> generates:
>> lock *(u32 *)(r2 + 0) += r1
>
> If you use latest llvm-project with
> https://github.com/llvm/llvm-project/pull/101428
> included, the above code will generate like
>
> $ clang --target=bpf -O2 -c t.c && llvm-objdump -d t.o
> t.o:    file format elf64-bpf
> Disassembly of section .text:
> 0000000000000000 <foo>:
>        0:       c3 12 00 00 01 00 00 00 r1 = atomic_fetch_add((u32 *)(r2 + 0x0), r1)
>        1:       95 00 00 00 00 00 00 00 exit
>
>
> With -mcpu=v3 the same code can be generated.

Same for current GCC.

>>
>> but
>> *b &= a;
>>
>> crashes llvm :( with
>>
>> <source>:3:5: error: unsupported atomic operation, please use 64 bit version
>>      3 |  *b &= a;
>
> It failed with the following llvm error message:
> t.c:1:6: error: unsupported atomic operation, please use 64 bit version
>     1 | void foo(int a, _Atomic int *b)
>       |      ^
> fatal error: error in backend: Cannot select: t8: i64,ch = AtomicLoadAnd<(load store seq_cst (s32) on %ir.b)> t0, t4, t2
>   t4: i64,ch = CopyFromReg t0, Register:i64 %1
>     t3: i64 = Register %1
>   t2: i64,ch = CopyFromReg t0, Register:i64 %0
>     t1: i64 = Register %0
> In function: foo
>
>>
>> but works with -mcpu=v3
>
> Yes. it does work with -mcpu=v3:

In GCC if you specify -mcpu=v2 and use atomic built-ins you get
workaround code in the form of libcalls (like calls to
__atomic_fetch_and_4) which are of course of no use in BPF atm.

> $ clang --target=bpf -O2 -mcpu=v3 -c t.c && llvm-objdump -d --mcpu=v3 t.o
>
> t.o:    file format elf64-bpf
> Disassembly of section .text:
> 0000000000000000 <foo>:
>        0:       c3 12 00 00 51 00 00 00 w1 = atomic_fetch_and((u32 *)(r2 + 0x0), w1)
>        1:       95 00 00 00 00 00 00 00 exit
>
> NOTE: I need -mcpu=v3 for llvm-objdump to print asm code 'atomic_fetch_and' properly.
> Will double check this.

The GNU binutils objdump will try to recognize instructions in the
latest supported cpu version, unless an explicit option is passed to
specify a particular ISA version:

$ bpf-unknown-none-objdump -M pseudoc -d foo.o

foo.o:     file format elf64-bpfle


Disassembly of section .text:

0000000000000000 <foo>:
   0:	bf 11 20 00 00 00 00 00 	r1 = (s32) r1
   8:	c3 12 00 00 51 00 00 00 	w1=atomic_fetch_and((u32*)(r2+0),w1)
  10:	95 00 00 00 00 00 00 00 	exit

$ bpf-unknown-none-objdump -M v2,pseudoc -d foo.o

foo.o:     file format elf64-bpfle


Disassembly of section .text:

0000000000000000 <foo>:
   0:	bf 11 20 00 00 00 00 00 	r1=r1
   8:	c3 12 00 00 51 00 00 00 	<unknown>
  10:	95 00 00 00 00 00 00 00 	exit

> For code:
> void foo(int a, _Atomic int *b)
> {
>  *b &= a;
> }
>
> The initial IR generated by clang frontend is:
>
> define dso_local void @foo(i32 noundef %a, ptr noundef %b) #0 {
> entry:
>   %a.addr = alloca i32, align 4
>   %b.addr = alloca ptr, align 8
>   store i32 %a, ptr %a.addr, align 4, !tbaa !3
>   store ptr %b, ptr %b.addr, align 8, !tbaa !7
>   %0 = load i32, ptr %a.addr, align 4, !tbaa !3
>   %1 = load ptr, ptr %b.addr, align 8, !tbaa !7
>   %2 = atomicrmw and ptr %1, i32 %0 seq_cst, align 4
>   %3 = and i32 %2, %0
>   ret void
> }
>
> Note that atomicrmw in the above. Eventually it optimized to
>
> define dso_local void @foo(i32 noundef %a, ptr noundef %b) #0 {
> entry:
>   %0 = atomicrmw and ptr %b, i32 %a seq_cst, align 4
>   ret void
> }
>
> The 'atomicrmw' is the same IR as generated by
> __sync_fetch_and_*() and eventually will generate atomic_fetch_*() bpf
> insn.
> Discussed with Andrii, and
> another option is to specify relaxed consistency, so llvm
> internal could translate it into locked insn. For example,
>
> $ cat t1.c
> #include <stdatomic.h>
>
> void f(_Atomic int *i) {
>   __c11_atomic_fetch_and(i, 1, memory_order_relaxed);
> }

I think this makes sense.  Currently we removed the GCC insns
atomic_{add,or,xor,and} all together so the compiler is forced to
implement them in terms of atomic_fetch_and_{add,or,xor,and} regardless
of the memory ordering policy specified to the __sync_fetch_and_OP.  So
even if you specify relaxed memory ordered, the resulting ordering is
whatever implied by the fetching operation.

> # to have gnu/stubs-32.h in the current directory to make it compile
> [yhs@devvm1513.prn0 ~/tmp6]$ ls gnu
> stubs-32.h
> [yhs@devvm1513.prn0 ~/tmp6]$ clang --target=bpf -O2 -I. -c -mcpu=v3 t1.c
>
> The initial IR:
> define dso_local void @f(ptr noundef %i) #0 {
> entry:
>   %i.addr = alloca ptr, align 8
>   %.atomictmp = alloca i32, align 4
>   %atomic-temp = alloca i32, align 4
>   store ptr %i, ptr %i.addr, align 8, !tbaa !3
>   %0 = load ptr, ptr %i.addr, align 8, !tbaa !3
>   store i32 1, ptr %.atomictmp, align 4, !tbaa !7
>   %1 = load i32, ptr %.atomictmp, align 4
>   %2 = atomicrmw and ptr %0, i32 %1 monotonic, align 4
>   store i32 %2, ptr %atomic-temp, align 4
>   %3 = load i32, ptr %atomic-temp, align 4, !tbaa !7
>   ret void
> }
>
> The IR right before machine code generation:
>
> define dso_local void @f(ptr nocapture noundef %i) local_unnamed_addr #0 {
> entry:
>   %0 = atomicrmw and ptr %i, i32 1 monotonic, align 4
>   ret void
> }
>
> Maybe we could special process the above to generate
> a locked insn if
>   - atomicrmw operator
>   - monotonic (related) consistency
>   - return value is not used
>
> So this will not violate original program semantics.
> Does this sound a reasonable apporach?

Whether monotonic consistency is desired (ordered writes) can be
probably deduced from the memory_order_* flag of the built-ins, but I
don't know what atomiccrmw is...  what is it in non-llvm terms?

>>
>> So let's make this test mcpu=v3 only and use normal C ?
>>
>> pw-bot: cr

Re: [PATCH bpf-next v2] selftests/bpf: Fix arena_atomics selftest failure due to llvm change

[Yonghong Song]

On 8/5/24 9:48 PM, Jose E. Marchesi wrote:
>> On 8/5/24 10:53 AM, Alexei Starovoitov wrote:
>>> On Fri, Aug 2, 2024 at 7:59 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>>> Peilen Ye reported an issue ([1]) where for __sync_fetch_and_add(...) without
>>>> return value like
>>>>     __sync_fetch_and_add(&foo, 1);
>>>> llvm BPF backend generates locked insn e.g.
>>>>     lock *(u32 *)(r1 + 0) += r2
>>>>
>>>> If __sync_fetch_and_add(...) returns a value like
>>>>     res = __sync_fetch_and_add(&foo, 1);
>>>> llvm BPF backend generates like
>>>>     r2 = atomic_fetch_add((u32 *)(r1 + 0), r2)
>>>>
>>>> But 'lock *(u32 *)(r1 + 0) += r2' caused a problem in jit
>>>> since proper barrier is not inserted based on __sync_fetch_and_add() semantics.
>>>>
>>>> The above discrepancy is due to commit [2] where it tries to maintain backward
>>>> compatability since before commit [2], __sync_fetch_and_add(...) generates
>>>> lock insn in BPF backend.
>>>>
>>>> Based on discussion in [1], now it is time to fix the above discrepancy so we can
>>>> have proper barrier support in jit. llvm patch [3] made sure that __sync_fetch_and_add(...)
>>>> always generates atomic_fetch_add(...) insns. Now 'lock *(u32 *)(r1 + 0) += r2' can only
>>>> be generated by inline asm. The same for __sync_fetch_and_and(), __sync_fetch_and_or()
>>>> and __sync_fetch_and_xor().
>>>>
>>>> But the change in [3] caused arena_atomics selftest failure.
>>>>
>>>>     test_arena_atomics:PASS:arena atomics skeleton open 0 nsec
>>>>     libbpf: prog 'and': BPF program load failed: Permission denied
>>>>     libbpf: prog 'and': -- BEGIN PROG LOAD LOG --
>>>>     arg#0 reference type('UNKNOWN ') size cannot be determined: -22
>>>>     0: R1=ctx() R10=fp0
>>>>     ; if (pid != (bpf_get_current_pid_tgid() >> 32)) @ arena_atomics.c:87
>>>>     0: (18) r1 = 0xffffc90000064000       ; R1_w=map_value(map=arena_at.bss,ks=4,vs=4)
>>>>     2: (61) r6 = *(u32 *)(r1 +0)          ; R1_w=map_value(map=arena_at.bss,ks=4,vs=4) R6_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff))
>>>>     3: (85) call bpf_get_current_pid_tgid#14      ; R0_w=scalar()
>>>>     4: (77) r0 >>= 32                     ; R0_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff))
>>>>     5: (5d) if r0 != r6 goto pc+11        ; R0_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff)) R6_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0x)
>>>>     ; __sync_fetch_and_and(&and64_value, 0x011ull << 32); @ arena_atomics.c:91
>>>>     6: (18) r1 = 0x100000000060           ; R1_w=scalar()
>>>>     8: (bf) r1 = addr_space_cast(r1, 0, 1)        ; R1_w=arena
>>>>     9: (18) r2 = 0x1100000000             ; R2_w=0x1100000000
>>>>     11: (db) r2 = atomic64_fetch_and((u64 *)(r1 +0), r2)
>>>>     BPF_ATOMIC stores into R1 arena is not allowed
>>>>     processed 9 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>>>>     -- END PROG LOAD LOG --
>>>>     libbpf: prog 'and': failed to load: -13
>>>>     libbpf: failed to load object 'arena_atomics'
>>>>     libbpf: failed to load BPF skeleton 'arena_atomics': -13
>>>>     test_arena_atomics:FAIL:arena atomics skeleton load unexpected error: -13 (errno 13)
>>>>     #3       arena_atomics:FAIL
>>>>
>>>> The reason of the failure is due to [4] where atomic{64,}_fetch_{and,or,xor}() are not
>>>> allowed by arena addresses. Without llvm patch [3], the compiler will generate 'lock ...'
>>>> insn and everything will work fine.
>>>>
>>>> This patch fixed the problem by using inline asms. Instead of __sync_fetch_and_{and,or,xor}() functions,
>>>> the inline asm with 'lock' insn is used and it will work with or without [3].
>>>> Note that three bpf programs ('and', 'or' and 'xor') are guarded with __BPF_FEATURE_ADDR_SPACE_CAST
>>>> as well to ensure compilation failure for llvm <= 18 version. Note that for llvm <= 18 where
>>>> addr_space_cast is not supported, all arena_atomics subtests are skipped with below message:
>>>>     test_arena_atomics:SKIP:no ENABLE_ATOMICS_TESTS or no addr_space_cast support in clang
>>>>     #3 arena_atomics:SKIP
>>>>
>>>>     [1] https://lore.kernel.org/bpf/ZqqiQQWRnz7H93Hc@google.com/T/#mb68d67bc8f39e35a0c3db52468b9de59b79f021f
>>>>     [2] https://github.com/llvm/llvm-project/commit/286daafd65129228e08a1d07aa4ca74488615744
>>>>     [3] https://github.com/llvm/llvm-project/pull/101428
>>>>     [4] d503a04f8bc0 ("bpf: Add support for certain atomics in bpf_arena to x86 JIT")
>>>>
>>>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>>>> ---
>>>>    .../selftests/bpf/progs/arena_atomics.c       | 63 ++++++++++++++++---
>>>>    1 file changed, 54 insertions(+), 9 deletions(-)
>>>>
>>>> Changelog:
>>>>     v1 -> v2:
>>>>       - Add __BPF_FEATURE_ADDR_SPACE_CAST to guard newly added asm codes for llvm >= 19
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/progs/arena_atomics.c b/tools/testing/selftests/bpf/progs/arena_atomics.c
>>>> index bb0acd79d28a..dea54557fc00 100644
>>>> --- a/tools/testing/selftests/bpf/progs/arena_atomics.c
>>>> +++ b/tools/testing/selftests/bpf/progs/arena_atomics.c
>>>> @@ -5,6 +5,7 @@
>>>>    #include <bpf/bpf_tracing.h>
>>>>    #include <stdbool.h>
>>>>    #include "bpf_arena_common.h"
>>>> +#include "bpf_misc.h"
>>>>
>>>>    struct {
>>>>           __uint(type, BPF_MAP_TYPE_ARENA);
>>>> @@ -85,10 +86,24 @@ int and(const void *ctx)
>>>>    {
>>>>           if (pid != (bpf_get_current_pid_tgid() >> 32))
>>>>                   return 0;
>>>> -#ifdef ENABLE_ATOMICS_TESTS
>>>> +#if defined(ENABLE_ATOMICS_TESTS) && defined(__BPF_FEATURE_ADDR_SPACE_CAST)
>>>>
>>>> -       __sync_fetch_and_and(&and64_value, 0x011ull << 32);
>>>> -       __sync_fetch_and_and(&and32_value, 0x011);
>>>> +       asm volatile(
>>>> +               "r1 = addr_space_cast(%[and64_value], 0, 1);"
>>>> +               "lock *(u64 *)(r1 + 0) &= %[val]"
>>>> +               :
>>>> +               : __imm_ptr(and64_value),
>>>> +                 [val]"r"(0x011ull << 32)
>>>> +               : "r1"
>>>> +       );
>>>> +       asm volatile(
>>>> +               "r1 = addr_space_cast(%[and32_value], 0, 1);"
>>>> +               "lock *(u32 *)(r1 + 0) &= %[val]"
>>>> +               :
>>>> +               : __imm_ptr(and32_value),
>>>> +                 [val]"w"(0x011)
>>>> +               : "r1"
>>>> +       );
>>> Instead of inline asm there is a better way to do the same in C.
>>> https://godbolt.org/z/71PYx1oqE
>>>
>>> void foo(int a, _Atomic int *b)
>>> {
>>>    *b += a;
>>> }
>>>
>>> generates:
>>> lock *(u32 *)(r2 + 0) += r1
>> If you use latest llvm-project with
>> https://github.com/llvm/llvm-project/pull/101428
>> included, the above code will generate like
>>
>> $ clang --target=bpf -O2 -c t.c && llvm-objdump -d t.o
>> t.o:    file format elf64-bpf
>> Disassembly of section .text:
>> 0000000000000000 <foo>:
>>         0:       c3 12 00 00 01 00 00 00 r1 = atomic_fetch_add((u32 *)(r2 + 0x0), r1)
>>         1:       95 00 00 00 00 00 00 00 exit
>>
>>
>> With -mcpu=v3 the same code can be generated.
> Same for current GCC.
>
>>> but
>>> *b &= a;
>>>
>>> crashes llvm :( with
>>>
>>> <source>:3:5: error: unsupported atomic operation, please use 64 bit version
>>>       3 |  *b &= a;
>> It failed with the following llvm error message:
>> t.c:1:6: error: unsupported atomic operation, please use 64 bit version
>>      1 | void foo(int a, _Atomic int *b)
>>        |      ^
>> fatal error: error in backend: Cannot select: t8: i64,ch = AtomicLoadAnd<(load store seq_cst (s32) on %ir.b)> t0, t4, t2
>>    t4: i64,ch = CopyFromReg t0, Register:i64 %1
>>      t3: i64 = Register %1
>>    t2: i64,ch = CopyFromReg t0, Register:i64 %0
>>      t1: i64 = Register %0
>> In function: foo
>>
>>> but works with -mcpu=v3
>> Yes. it does work with -mcpu=v3:
> In GCC if you specify -mcpu=v2 and use atomic built-ins you get
> workaround code in the form of libcalls (like calls to
> __atomic_fetch_and_4) which are of course of no use in BPF atm.
>> $ clang --target=bpf -O2 -mcpu=v3 -c t.c && llvm-objdump -d --mcpu=v3 t.o
>>
>> t.o:    file format elf64-bpf
>> Disassembly of section .text:
>> 0000000000000000 <foo>:
>>         0:       c3 12 00 00 51 00 00 00 w1 = atomic_fetch_and((u32 *)(r2 + 0x0), w1)
>>         1:       95 00 00 00 00 00 00 00 exit
>>
>> NOTE: I need -mcpu=v3 for llvm-objdump to print asm code 'atomic_fetch_and' properly.
>> Will double check this.
> The GNU binutils objdump will try to recognize instructions in the
> latest supported cpu version, unless an explicit option is passed to
> specify a particular ISA version:

Agree. As I mentioned in the above, I will take a look at this soon.

>
> $ bpf-unknown-none-objdump -M pseudoc -d foo.o
>
> foo.o:     file format elf64-bpfle
>
>
> Disassembly of section .text:
>
> 0000000000000000 <foo>:
>     0:	bf 11 20 00 00 00 00 00 	r1 = (s32) r1
>     8:	c3 12 00 00 51 00 00 00 	w1=atomic_fetch_and((u32*)(r2+0),w1)
>    10:	95 00 00 00 00 00 00 00 	exit
>
> $ bpf-unknown-none-objdump -M v2,pseudoc -d foo.o
>
> foo.o:     file format elf64-bpfle
>
>
> Disassembly of section .text:
>
> 0000000000000000 <foo>:
>     0:	bf 11 20 00 00 00 00 00 	r1=r1
>     8:	c3 12 00 00 51 00 00 00 	<unknown>
>    10:	95 00 00 00 00 00 00 00 	exit
>
>> For code:
>> void foo(int a, _Atomic int *b)
>> {
>>   *b &= a;
>> }
>>
>> The initial IR generated by clang frontend is:
>>
>> define dso_local void @foo(i32 noundef %a, ptr noundef %b) #0 {
>> entry:
>>    %a.addr = alloca i32, align 4
>>    %b.addr = alloca ptr, align 8
>>    store i32 %a, ptr %a.addr, align 4, !tbaa !3
>>    store ptr %b, ptr %b.addr, align 8, !tbaa !7
>>    %0 = load i32, ptr %a.addr, align 4, !tbaa !3
>>    %1 = load ptr, ptr %b.addr, align 8, !tbaa !7
>>    %2 = atomicrmw and ptr %1, i32 %0 seq_cst, align 4
>>    %3 = and i32 %2, %0
>>    ret void
>> }
>>
>> Note that atomicrmw in the above. Eventually it optimized to
>>
>> define dso_local void @foo(i32 noundef %a, ptr noundef %b) #0 {
>> entry:
>>    %0 = atomicrmw and ptr %b, i32 %a seq_cst, align 4
>>    ret void
>> }
>>
>> The 'atomicrmw' is the same IR as generated by
>> __sync_fetch_and_*() and eventually will generate atomic_fetch_*() bpf
>> insn.
>> Discussed with Andrii, and
>> another option is to specify relaxed consistency, so llvm
>> internal could translate it into locked insn. For example,
>>
>> $ cat t1.c
>> #include <stdatomic.h>
>>
>> void f(_Atomic int *i) {
>>    __c11_atomic_fetch_and(i, 1, memory_order_relaxed);
>> }
> I think this makes sense.  Currently we removed the GCC insns
> atomic_{add,or,xor,and} all together so the compiler is forced to
> implement them in terms of atomic_fetch_and_{add,or,xor,and} regardless
> of the memory ordering policy specified to the __sync_fetch_and_OP.  So
> even if you specify relaxed memory ordered, the resulting ordering is
> whatever implied by the fetching operation.

Encoding memory order constraints in BPF insn might be too complicated.
I am not sure. But at least we could map different memory constraints
into different insns so jit can add proper barrier for those insns.

>
>> # to have gnu/stubs-32.h in the current directory to make it compile
>> [yhs@devvm1513.prn0 ~/tmp6]$ ls gnu
>> stubs-32.h
>> [yhs@devvm1513.prn0 ~/tmp6]$ clang --target=bpf -O2 -I. -c -mcpu=v3 t1.c
>>
>> The initial IR:
>> define dso_local void @f(ptr noundef %i) #0 {
>> entry:
>>    %i.addr = alloca ptr, align 8
>>    %.atomictmp = alloca i32, align 4
>>    %atomic-temp = alloca i32, align 4
>>    store ptr %i, ptr %i.addr, align 8, !tbaa !3
>>    %0 = load ptr, ptr %i.addr, align 8, !tbaa !3
>>    store i32 1, ptr %.atomictmp, align 4, !tbaa !7
>>    %1 = load i32, ptr %.atomictmp, align 4
>>    %2 = atomicrmw and ptr %0, i32 %1 monotonic, align 4
>>    store i32 %2, ptr %atomic-temp, align 4
>>    %3 = load i32, ptr %atomic-temp, align 4, !tbaa !7
>>    ret void
>> }
>>
>> The IR right before machine code generation:
>>
>> define dso_local void @f(ptr nocapture noundef %i) local_unnamed_addr #0 {
>> entry:
>>    %0 = atomicrmw and ptr %i, i32 1 monotonic, align 4
>>    ret void
>> }
>>
>> Maybe we could special process the above to generate
>> a locked insn if
>>    - atomicrmw operator
>>    - monotonic (related) consistency
>>    - return value is not used
>>
>> So this will not violate original program semantics.
>> Does this sound a reasonable apporach?
> Whether monotonic consistency is desired (ordered writes) can be
> probably deduced from the memory_order_* flag of the built-ins, but I
> don't know what atomiccrmw is...  what is it in non-llvm terms?

The llvm language reference for atomicrmw:

   https://llvm.org/docs/LangRef.html#atomicrmw-instruction

>
>>> So let's make this test mcpu=v3 only and use normal C ?
>>>
>>> pw-bot: cr

Re: [PATCH bpf-next v2] selftests/bpf: Fix arena_atomics selftest failure due to llvm change

[Alexei Starovoitov]

On Mon, Aug 5, 2024 at 10:26 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> >>
> >> Maybe we could special process the above to generate
> >> a locked insn if
> >>    - atomicrmw operator
> >>    - monotonic (related) consistency
> >>    - return value is not used

This sounds like a good idea, but...

> >>
> >> So this will not violate original program semantics.
> >> Does this sound a reasonable apporach?
> > Whether monotonic consistency is desired (ordered writes) can be
> > probably deduced from the memory_order_* flag of the built-ins, but I
> > don't know what atomiccrmw is...  what is it in non-llvm terms?
>
> The llvm language reference for atomicrmw:
>
>    https://llvm.org/docs/LangRef.html#atomicrmw-instruction

I read it back and forth, but couldn't find whether it's ok
for the backend to use stronger ordering insn when weaker ordering
is specified in atomicrmw.
It's probably ok.
Otherwise atomicrmw with monotnic (memory_order_relaxed) and
return value is used cannot be mapped to any bpf insn.
x86 doesn't have monotonic either, but I suspect the backend
still generates the code without any warnings.
Would be good to clarify before we proceed with the above plan.

Re: [PATCH bpf-next v2] selftests/bpf: Fix arena_atomics selftest failure due to llvm change

[Yonghong Song]

On 8/8/24 9:26 AM, Alexei Starovoitov wrote:
> On Mon, Aug 5, 2024 at 10:26 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>>>> Maybe we could special process the above to generate
>>>> a locked insn if
>>>>     - atomicrmw operator
>>>>     - monotonic (related) consistency
>>>>     - return value is not used
> This sounds like a good idea, but...
>
>>>> So this will not violate original program semantics.
>>>> Does this sound a reasonable apporach?
>>> Whether monotonic consistency is desired (ordered writes) can be
>>> probably deduced from the memory_order_* flag of the built-ins, but I
>>> don't know what atomiccrmw is...  what is it in non-llvm terms?
>> The llvm language reference for atomicrmw:
>>
>>     https://llvm.org/docs/LangRef.html#atomicrmw-instruction
> I read it back and forth, but couldn't find whether it's ok
> for the backend to use stronger ordering insn when weaker ordering
> is specified in atomicrmw.
> It's probably ok.
> Otherwise atomicrmw with monotnic (memory_order_relaxed) and
> return value is used cannot be mapped to any bpf insn.
> x86 doesn't have monotonic either, but I suspect the backend
> still generates the code without any warnings.

I did a little bit experiment for x86,

$ cat t.c
int foo;
void bar1(void) {
         __sync_fetch_and_add(&foo, 10);
}
int bar2(void) {
         return __sync_fetch_and_add(&foo, 10);
}
$ clang -O2 -I. -c t.c && llvm-objdump -d t.o

t.o:    file format elf64-x86-64

Disassembly of section .text:

0000000000000000 <bar1>:
        0: f0                            lock
        1: 83 05 00 00 00 00 0a          addl    $0xa, (%rip)            # 0x8 <bar1+0x8>
        8: c3                            retq
        9: 0f 1f 80 00 00 00 00          nopl    (%rax)
         
0000000000000010 <bar2>:
       10: b8 0a 00 00 00                movl    $0xa, %eax
       15: f0                            lock
       16: 0f c1 05 00 00 00 00          xaddl   %eax, (%rip)            # 0x1d <bar2+0xd>
       1d: c3                            retq

So without return value, __sync_fetch_and_add will generated locked add insn.
With return value, 'lock xaddl' is used which should be similar to bpf atomic_fetch_add.

Another example,

$ cat t1.c
#include <stdatomic.h>

void f1(_Atomic int *i) {
   __c11_atomic_fetch_and(i, 10, memory_order_relaxed);
}

void f2(_Atomic int *i) {
   __c11_atomic_fetch_and(i, 10, memory_order_seq_cst);
}

void f3(_Atomic int *i) {
   __c11_atomic_fetch_and(i, 10, memory_order_release);
}

_Atomic int f4(_Atomic int *i) {
   return __c11_atomic_fetch_and(i, 10, memory_order_release);
}
$ clang -O2 -I. -c t1.c && llvm-objdump -d t1.o

t1.o:   file format elf64-x86-64

Disassembly of section .text:

0000000000000000 <f1>:
        0: f0                            lock
        1: 83 27 0a                      andl    $0xa, (%rdi)
        4: c3                            retq
        5: 66 66 2e 0f 1f 84 00 00 00 00 00      nopw    %cs:(%rax,%rax)

0000000000000010 <f2>:
       10: f0                            lock
       11: 83 27 0a                      andl    $0xa, (%rdi)
       14: c3                            retq
       15: 66 66 2e 0f 1f 84 00 00 00 00 00      nopw    %cs:(%rax,%rax)

0000000000000020 <f3>:
       20: f0                            lock
       21: 83 27 0a                      andl    $0xa, (%rdi)
       24: c3                            retq
       25: 66 66 2e 0f 1f 84 00 00 00 00 00      nopw    %cs:(%rax,%rax)

0000000000000030 <f4>:
       30: 8b 07                         movl    (%rdi), %eax
       32: 66 66 66 66 66 2e 0f 1f 84 00 00 00 00 00     nopw    %cs:(%rax,%rax)
       40: 89 c1                         movl    %eax, %ecx
       42: 83 e1 0a                      andl    $0xa, %ecx
       45: f0                            lock
       46: 0f b1 0f                      cmpxchgl        %ecx, (%rdi)
       49: 75 f5                         jne     0x40 <f4+0x10>
       4b: c3                            retq
$

In all cases without return value, for x86, 'lock andl' insn is generated
regardless of the memory order constraint. This is similar to
what we had before.

Although previous 'lock' encoding matches to x86 nicely.
But for ARM64 and for 'lock ...' bpf insn,
the generated insn won't have a barrier (acquire or release or both)
semantics. So I guess that ARM64 wants to preserve the current hehavior:
   - for 'lock ...' insn, no barrier,
   - for 'atomic_fetch_add ...' insn, proper barrier.

For x86, for both 'lock ...' and 'atomic_fetch_add ...' will have proper barrier.

To keep 'lock ...' no-barrier required, user needs to specify
memory_order_relaxed constraint. This will permit bpf backend
to generate 'lock ...' insn.

Looks like x86 does check memory_order_relaxed to do some
special processing:

X86CompressEVEX.cpp:  if (!TableChecked.load(std::memory_order_relaxed)) {
X86CompressEVEX.cpp:    TableChecked.store(true, std::memory_order_relaxed);
X86FloatingPoint.cpp:    if (!TABLE##Checked.load(std::memory_order_relaxed)) {                     \
X86FloatingPoint.cpp:      TABLE##Checked.store(true, std::memory_order_relaxed);                   \
X86InstrFMA3Info.cpp:  if (!TableChecked.load(std::memory_order_relaxed)) {
X86InstrFMA3Info.cpp:    TableChecked.store(true, std::memory_order_relaxed);
X86InstrFoldTables.cpp:  if (!FoldTablesChecked.load(std::memory_order_relaxed)) {
X86InstrFoldTables.cpp:    FoldTablesChecked.store(true, std::memory_order_relaxed);

So I guess that bpf can do similar thing to check memory_order_relaxed in IR
and may generate different (more efficient insns) as needed.

> Would be good to clarify before we proceed with the above plan.