Re: Supporting New Memory Barrier Types in BPF

[Peilin Ye <yepeilin@google.com>]

Hi Alexei,

Thanks for all the suggestions!  Some questions:

On Mon, Jul 29, 2024 at 06:28:16PM -0700, Alexei Starovoitov wrote:
> On Mon, Jul 29, 2024 at 11:33 AM Peilin Ye <yepeilin@google.com> wrote:
> > We need more.  During offline discussion with Paul, we agreed we can start
> > from:
> >
> >   * load-acquire: __atomic_load_n(... memorder=__ATOMIC_ACQUIRE);
> >   * store-release: __atomic_store_n(... memorder=__ATOMIC_RELEASE);
> 
> we would need inline asm equivalent too. Similar to kernel
> smp_load_acquire() macro.

I see, so something like:

    asm volatile("%0 = load_acquire((u64 *)(%1 + 0x0))" :
                 "=r"(ret) : "r"(&foo) : "memory");

and e.g. this in disassembly:

    r0 = load_acquire((u64 *)(r1 + 0x0))

I agree that we'd better not put the entire e.g.
"r0 = __atomic_load_n((u64 *)(r1 + 0x0), __ATOMIC_ACQUIRE)" into
disassembly.

> > Theoretically, the BPF JIT compiler could also reorder instructions just like
> > Clang or GCC, though it might not currently do so.  If we ever developed a more
> > optimizing BPF JIT compiler, it would also be nice to have an optimization
> > barrier for it.  However, Alexei Starovoitov has expressed that defining a BPF
> > instruction with 'asm volatile ("" ::: "memory");' semantics might be tricky.
> 
> It can be a standalone insn that is a compiler barrier only but that feels like
> a waste of an instruction. So depending how we end up encoding various
> real barriers
> there may be a bit to spend in such a barrier insn that is only a
> compiler barrier.
> In this case optimizing JIT barrier.

[...]

> > Roughly, the scope of this work includes:
> >
> >   * decide how to extend the BPF ISA (add new instructions and/or extend
> >     current ones)
> 
> ldx/stx insns support MEM and MEMSX modifiers.
> Adding MEM_ACQ_REL feels like a natural fit. Better name?

Do we allow aliases?  E.g., can we have "MEMACQ" for LDX and "MEMREL"
for STX, but let them share the same numeric value?

Speaking of numeric value, out of curiosity:

    IMM    0
    ABS    1
    IND    2
    MEM    3
    MEMSX  4
    ATOMIC 6

Was there a reason that we skipped 5?  Is 5 reserved?

> For barriers we would need a new insn. Not sure which class would fit the best.
> Maybe BPF_LD ?
> 
> Another alternative for barriers is to use nocsr kfuncs.
> Then we have the freedom to make mistakes and fix them later.
> One kfunc per barrier would do.
> JITs would inline them into appropriate insns.
> In bpf progs they will be used just like in the kernel code smp_mb(),
> smp_rmb(), etc.
> 
> I don't think compilers have to emit barriers from C code, so my
> preference is kfuncs atm.

Ah, I see; we recently supported [1] nocsr BPF helper functions.  The
cover letter says:

  """
  This patch-set seeks to allow using no_caller_saved_registers
  gcc/clang attribute with some BPF helper functions (and kfuncs in the
  future).
  """

It seems that nocsr BPF kfuncs are not supported yet.  Do we have a
schedule for it?

Thanks,
Peilin Ye

[1] [PATCH bpf-next v4 00/10] no_caller_saved_registers attribute for helper calls
    https://lore.kernel.org/bpf/20240722233844.1406874-1-eddyz87@gmail.com/