Re: Supporting New Memory Barrier Types in BPF

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

On 7/31/24 1:44 PM, Peilin Ye wrote:
> Hi Alexei, Yonghong,
>
> On Tue, Jul 30, 2024 at 08:51:15PM -0700, Yonghong Song 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.
> Thanks for confirming!  Would you mind if I fix it myself?  It may
> affect some of the BPF code that we will be running on ARM, so we would
> like to get it fixed sooner.  Also, I would love to gain some
> experience in LLVM development!

Peilin, when I saw your email, I have almost done with the change.
The below is the llvm patch:
   https://github.com/llvm/llvm-project/pull/101428

Please help take a look. You are certainly welcome to do llvm
related work. Just respond earlier to mention you intend to do
a particular llvm patch and we are happy for you to contribute
and will help when you have any questions.

>
> Thanks,
> Peilin Ye
>