Re: Supporting New Memory Barrier Types in BPF

[Yonghong Song <yonghong.song@linux.dev>]

On 7/30/24 6:19 PM, Alexei Starovoitov wrote:
> On Mon, Jul 29, 2024 at 10:14 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>> This sounds like a compiler bug.
>>>
>>> Yonghong, Jose,
>>> do you know what compilers do for other backends?
>>> Is it allowed to convert sycn_fetch_add into sync_add when fetch part is unused?
>> This behavior is introduced by the following llvm commit:
>> https://github.com/llvm/llvm-project/commit/286daafd65129228e08a1d07aa4ca74488615744
>>
>> Specifically the following commit message:
>>
>> =======
>> Similar to xadd, atomic xadd, xor and xxor (atomic_<op>)
>> instructions are added for atomic operations which do not
>> have return values. LLVM will check the return value for
>> __sync_fetch_and_{add,and,or,xor}.
>> If the return value is used, instructions atomic_fetch_<op>
>> will be used. Otherwise, atomic_<op> instructions will be used.
> So it's a bpf backend bug. Great. That's fixable.
> Would have been much harder if this transformation was performed
> by the middle end.
>
>> ======
>>
>> Basically, if no return value, __sync_fetch_and_add() will use
>> xadd insn. The decision is made at that time to maintain backward compatibility.
>> For one example, in bcc
>>     https://github.com/iovisor/bcc/blob/master/src/cc/export/helpers.h#L1444
>> we have
>>     #define lock_xadd(ptr, val) ((void)__sync_fetch_and_add(ptr, val))
>>
>> Should we use atomic_fetch_*() always regardless of whether the return
>> val is used or not? Probably, it should still work. Not sure what gcc
>> does for this case.
> Right. We did it for backward compat. Older llvm was
> completely wrong to generate xadd for __sync_fetch_and_add.
> That was my hack from 10 years ago when xadd was all we had.
> So we fixed that old llvm bug, but introduced another with all
> good intentions.
> Since proper atomic insns were introduced 3 years ago we should
> remove this backward compat feature/bug from llvm.
> The only breakage is for kernels older than 5.12.
> I think that's an acceptable risk.

Sounds good, I will remove the backward compat part in llvm20.