Re: Supporting New Memory Barrier Types in BPF

[Peilin Ye <yepeilin@google.com>]

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!

Thanks,
Peilin Ye