bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Brendan Jackman <jackmanb@google.com>
To: Yonghong Song <yhs@fb.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	KP Singh <kpsingh@chromium.org>,
	Florent Revest <revest@chromium.org>
Subject: Re: [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm
Date: Tue, 24 Nov 2020 11:02:09 +0000	[thread overview]
Message-ID: <20201124110209.GC1883487@google.com> (raw)
In-Reply-To: <e7d336ab-524f-9d60-e9ec-8c8426cae0d7@fb.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.

  reply	other threads:[~2020-11-24 11:02 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-23 17:31 [PATCH 0/7] Atomics for eBPF Brendan Jackman
2020-11-23 17:31 ` [PATCH 1/7] bpf: Factor out emission of ModR/M for *(reg + off) Brendan Jackman
2020-11-23 17:31 ` [PATCH 2/7] bpf: x86: Factor out emission of REX byte Brendan Jackman
2020-11-23 17:31 ` [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm Brendan Jackman
2020-11-23 23:54   ` Yonghong Song
2020-11-24 11:02     ` Brendan Jackman [this message]
2020-11-24 16:04       ` Yonghong Song
2020-11-24  3:28   ` kernel test robot
2020-11-24  6:50   ` Alexei Starovoitov
2020-11-24 11:21     ` Brendan Jackman
2020-11-24 22:43       ` Alexei Starovoitov
2020-11-23 17:31 ` [PATCH 4/7] bpf: Move BPF_STX reserved field check into BPF_STX verifier code Brendan Jackman
2020-11-23 17:32 ` [PATCH 5/7] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction Brendan Jackman
2020-11-23 21:12   ` kernel test robot
2020-11-23 21:51   ` kernel test robot
2020-11-24  6:52   ` Alexei Starovoitov
2020-11-24 10:48     ` Brendan Jackman
2020-11-23 17:32 ` [PATCH 6/7] bpf: Add instructions for atomic_cmpxchg and friends Brendan Jackman
2020-11-23 19:29   ` Brendan Jackman
2020-11-24  6:40   ` Alexei Starovoitov
2020-11-24 10:55     ` Brendan Jackman
2020-11-24 22:51       ` Alexei Starovoitov
2020-11-23 17:32 ` [PATCH 7/7] bpf: Add tests for new BPF atomic operations Brendan Jackman
2020-11-24  0:26   ` Yonghong Song
2020-11-24 13:10     ` Brendan Jackman
2020-11-23 17:36 ` [PATCH 0/7] Atomics for eBPF Brendan Jackman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201124110209.GC1883487@google.com \
    --to=jackmanb@google.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=kpsingh@chromium.org \
    --cc=revest@chromium.org \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).