Re: [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm

[Brendan Jackman <jackmanb@google.com>]

On Mon, Nov 23, 2020 at 03:54:38PM -0800, Yonghong Song wrote:
> 
> 
> On 11/23/20 9:31 AM, Brendan Jackman wrote:
...
> > diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
> > index 0207b6ea6e8a..897634d0a67c 100644
> > --- a/arch/arm/net/bpf_jit_32.c
> > +++ b/arch/arm/net/bpf_jit_32.c
> > @@ -1620,10 +1620,9 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx)
> >   		}
> >   		emit_str_r(dst_lo, tmp2, off, ctx, BPF_SIZE(code));
> >   		break;
> > -	/* STX XADD: lock *(u32 *)(dst + off) += src */
> > -	case BPF_STX | BPF_XADD | BPF_W:
> > -	/* STX XADD: lock *(u64 *)(dst + off) += src */
> > -	case BPF_STX | BPF_XADD | BPF_DW:
> > +	/* Atomic ops */
> > +	case BPF_STX | BPF_ATOMIC | BPF_W:
> > +	case BPF_STX | BPF_ATOMIC | BPF_DW:
> >   		goto notyet;
> >   	/* STX: *(size *)(dst + off) = src */
> >   	case BPF_STX | BPF_MEM | BPF_W:
> > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> > index ef9f1d5e989d..f7b194878a99 100644
> > --- a/arch/arm64/net/bpf_jit_comp.c
> > +++ b/arch/arm64/net/bpf_jit_comp.c
> > @@ -875,10 +875,18 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
> >   		}
> >   		break;
> > -	/* STX XADD: lock *(u32 *)(dst + off) += src */
> > -	case BPF_STX | BPF_XADD | BPF_W:
> > -	/* STX XADD: lock *(u64 *)(dst + off) += src */
> > -	case BPF_STX | BPF_XADD | BPF_DW:
> > +	case BPF_STX | BPF_ATOMIC | BPF_W:
> > +	case BPF_STX | BPF_ATOMIC | BPF_DW:
> > +		if (insn->imm != BPF_ADD) {
> 
> Currently BPF_ADD (although it is 0) is encoded at bit 4-7 of imm.
> Do you think we should encode it in 0-3 to make such a comparision
> and subsequent insn->imm = BPF_ADD making more sense?

Sorry not quite sure what you mean by this... I think encoding in 4-7 is
nice because it lets us use BPF_OP. In this patchset wherever we have
(insn->imm == BPF_ADD) meaning "this is a traditional XADD without
fetch" and (BPF_OP(insn->imm) == BPF_ADD) meaning "this is an atomic
add, either with or without a fetch". 

Does that answer the question...?

> > diff --git a/drivers/net/ethernet/netronome/nfp/bpf/jit.c b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
> > index 0a721f6e8676..0767d7b579e9 100644
> > --- a/drivers/net/ethernet/netronome/nfp/bpf/jit.c
> > +++ b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
> > @@ -3109,13 +3109,19 @@ mem_xadd(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta, bool is64)
> >   	return 0;
> >   }
> > -static int mem_xadd4(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
> > +static int mem_atm4(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
> >   {
> > +	if (meta->insn.off != BPF_ADD)
> > +		return -EOPNOTSUPP;
> 
> meta->insn.imm?
> 
> > +
> >   	return mem_xadd(nfp_prog, meta, false);
> >   }
> > -static int mem_xadd8(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
> > +static int mem_atm(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
> >   {
> > +	if (meta->insn.off != BPF_ADD)
> 
> meta->insn.imm?

Yikes, thanks for spotting these! Apparently I wasn't even compiling
this code.