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.
