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>,
linux-kernel@vger.kernel.org, Jann Horn <jannh@google.com>
Subject: Re: [PATCH bpf-next v3 09/14] bpf: Pull out a macro for interpreting atomic ALU operations
Date: Fri, 4 Dec 2020 09:29:13 +0000 [thread overview]
Message-ID: <X8oBaf4c+EAd8LQE@google.com> (raw)
In-Reply-To: <f1d5ec7e-6231-0876-f25d-9dd5da4112d0@fb.com>
On Thu, Dec 03, 2020 at 10:30:18PM -0800, Yonghong Song wrote:
>
>
> On 12/3/20 8:02 AM, Brendan Jackman wrote:
> > Since the atomic operations that are added in subsequent commits are
> > all isomorphic with BPF_ADD, pull out a macro to avoid the
> > interpreter becoming dominated by lines of atomic-related code.
> >
> > Note that this sacrificies interpreter performance (combining
> > STX_ATOMIC_W and STX_ATOMIC_DW into single switch case means that we
> > need an extra conditional branch to differentiate them) in favour of
> > compact and (relatively!) simple C code.
> >
> > Change-Id: I8cae5b66e75f34393de6063b91c05a8006fdd9e6
> > Signed-off-by: Brendan Jackman <jackmanb@google.com>
>
> Ack with a minor suggestion below.
>
> Acked-by: Yonghong Song <yhs@fb.com>
>
> > ---
> > kernel/bpf/core.c | 79 +++++++++++++++++++++++------------------------
> > 1 file changed, 38 insertions(+), 41 deletions(-)
> >
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index 28f960bc2e30..498d3f067be7 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -1618,55 +1618,52 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
> > LDX_PROBE(DW, 8)
> > #undef LDX_PROBE
> > - STX_ATOMIC_W:
> > - switch (IMM) {
> > - case BPF_ADD:
> > - /* lock xadd *(u32 *)(dst_reg + off16) += src_reg */
> > - atomic_add((u32) SRC, (atomic_t *)(unsigned long)
> > - (DST + insn->off));
> > - break;
> > - case BPF_ADD | BPF_FETCH:
> > - SRC = (u32) atomic_fetch_add(
> > - (u32) SRC,
> > - (atomic_t *)(unsigned long) (DST + insn->off));
> > - break;
> > - case BPF_XCHG:
> > - SRC = (u32) atomic_xchg(
> > - (atomic_t *)(unsigned long) (DST + insn->off),
> > - (u32) SRC);
> > - break;
> > - case BPF_CMPXCHG:
> > - BPF_R0 = (u32) atomic_cmpxchg(
> > - (atomic_t *)(unsigned long) (DST + insn->off),
> > - (u32) BPF_R0, (u32) SRC);
> > +#define ATOMIC(BOP, KOP) \
>
> ATOMIC a little bit generic. Maybe ATOMIC_FETCH_BOP?
Well it doesn't fetch in all cases and "BOP" is intended to
differentiate from KOP i.e. BOP = BPF operation KOP = Kernel operation.
Could go with ATOMIC_ALU_OP?
> > + case BOP: \
> > + if (BPF_SIZE(insn->code) == BPF_W) \
> > + atomic_##KOP((u32) SRC, (atomic_t *)(unsigned long) \
> > + (DST + insn->off)); \
> > + else \
> > + atomic64_##KOP((u64) SRC, (atomic64_t *)(unsigned long) \
> > + (DST + insn->off)); \
> > + break; \
> > + case BOP | BPF_FETCH: \
> > + if (BPF_SIZE(insn->code) == BPF_W) \
> > + SRC = (u32) atomic_fetch_##KOP( \
> > + (u32) SRC, \
> > + (atomic_t *)(unsigned long) (DST + insn->off)); \
> > + else \
> > + SRC = (u64) atomic64_fetch_##KOP( \
> > + (u64) SRC, \
> > + (atomic64_t *)(s64) (DST + insn->off)); \
> > break;
> > - default:
> > - goto default_label;
> > - }
> > - CONT;
> > STX_ATOMIC_DW:
> > + STX_ATOMIC_W:
> > switch (IMM) {
> > - case BPF_ADD:
> > - /* lock xadd *(u64 *)(dst_reg + off16) += src_reg */
> > - atomic64_add((u64) SRC, (atomic64_t *)(unsigned long)
> > - (DST + insn->off));
> > - break;
> > - case BPF_ADD | BPF_FETCH:
> > - SRC = (u64) atomic64_fetch_add(
> > - (u64) SRC,
> > - (atomic64_t *)(s64) (DST + insn->off));
> > - break;
> > + ATOMIC(BPF_ADD, add)
> > +
> > case BPF_XCHG:
> > - SRC = (u64) atomic64_xchg(
> > - (atomic64_t *)(u64) (DST + insn->off),
> > - (u64) SRC);
> > + if (BPF_SIZE(insn->code) == BPF_W)
> > + SRC = (u32) atomic_xchg(
> > + (atomic_t *)(unsigned long) (DST + insn->off),
> > + (u32) SRC);
> > + else
> > + SRC = (u64) atomic64_xchg(
> > + (atomic64_t *)(u64) (DST + insn->off),
> > + (u64) SRC);
> > break;
> > case BPF_CMPXCHG:
> > - BPF_R0 = (u64) atomic64_cmpxchg(
> > - (atomic64_t *)(u64) (DST + insn->off),
> > - (u64) BPF_R0, (u64) SRC);
> > + if (BPF_SIZE(insn->code) == BPF_W)
> > + BPF_R0 = (u32) atomic_cmpxchg(
> > + (atomic_t *)(unsigned long) (DST + insn->off),
> > + (u32) BPF_R0, (u32) SRC);
> > + else
> > + BPF_R0 = (u64) atomic64_cmpxchg(
> > + (atomic64_t *)(u64) (DST + insn->off),
> > + (u64) BPF_R0, (u64) SRC);
> > break;
> > +
> > default:
> > goto default_label;
> > }
> >
next prev parent reply other threads:[~2020-12-04 9:30 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-03 16:02 [PATCH bpf-next v3 00/14] Atomics for eBPF Brendan Jackman
2020-12-03 16:02 ` [PATCH bpf-next v3 01/14] bpf: x86: Factor out emission of ModR/M for *(reg + off) Brendan Jackman
2020-12-03 16:02 ` [PATCH bpf-next v3 02/14] bpf: x86: Factor out emission of REX byte Brendan Jackman
2020-12-03 16:02 ` [PATCH bpf-next v3 03/14] bpf: x86: Factor out function to emit NEG Brendan Jackman
2020-12-03 16:02 ` [PATCH bpf-next v3 04/14] bpf: x86: Factor out a lookup table for some ALU opcodes Brendan Jackman
2020-12-03 16:02 ` [PATCH bpf-next v3 05/14] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm Brendan Jackman
2020-12-04 4:49 ` Yonghong Song
2020-12-03 16:02 ` [PATCH bpf-next v3 06/14] bpf: Move BPF_STX reserved field check into BPF_STX verifier code Brendan Jackman
2020-12-04 4:51 ` Yonghong Song
2020-12-03 16:02 ` [PATCH bpf-next v3 07/14] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction Brendan Jackman
2020-12-04 5:02 ` Yonghong Song
2020-12-04 5:27 ` Yonghong Song
2020-12-04 9:12 ` Brendan Jackman
2020-12-03 16:02 ` [PATCH bpf-next v3 08/14] bpf: Add instructions for atomic_[cmp]xchg Brendan Jackman
2020-12-04 5:34 ` Yonghong Song
2020-12-04 9:26 ` Brendan Jackman
2020-12-03 16:02 ` [PATCH bpf-next v3 09/14] bpf: Pull out a macro for interpreting atomic ALU operations Brendan Jackman
2020-12-04 6:30 ` Yonghong Song
2020-12-04 9:29 ` Brendan Jackman [this message]
2020-12-04 15:20 ` Yonghong Song
2020-12-03 16:02 ` [PATCH bpf-next v3 10/14] bpf: Add bitwise atomic instructions Brendan Jackman
2020-12-04 6:42 ` Yonghong Song
2020-12-04 9:36 ` Brendan Jackman
2020-12-04 15:21 ` Yonghong Song
2020-12-07 11:28 ` Brendan Jackman
2020-12-07 15:58 ` Yonghong Song
2020-12-07 16:14 ` Brendan Jackman
2020-12-03 16:02 ` [PATCH bpf-next v3 11/14] tools build: Implement feature check for BPF atomics in Clang Brendan Jackman
2020-12-03 21:02 ` Andrii Nakryiko
2020-12-03 16:02 ` [PATCH bpf-next v3 12/14] bpf: Pull tools/build/feature biz into selftests Makefile Brendan Jackman
2020-12-03 21:01 ` Andrii Nakryiko
2020-12-04 9:41 ` Brendan Jackman
2020-12-04 19:00 ` Andrii Nakryiko
2020-12-07 11:00 ` Brendan Jackman
2020-12-08 2:19 ` Andrii Nakryiko
2020-12-08 17:04 ` Brendan Jackman
2020-12-08 18:31 ` Andrii Nakryiko
2020-12-03 16:02 ` [PATCH bpf-next v3 13/14] bpf: Add tests for new BPF atomic operations Brendan Jackman
2020-12-04 7:06 ` Yonghong Song
2020-12-04 9:45 ` Brendan Jackman
2020-12-04 15:28 ` Yonghong Song
2020-12-04 19:49 ` Andrii Nakryiko
2020-12-07 15:48 ` Brendan Jackman
2020-12-03 16:02 ` [PATCH bpf-next v3 14/14] bpf: Document new atomic instructions Brendan Jackman
2020-12-03 16:10 ` [PATCH bpf-next v3 00/14] Atomics for eBPF Brendan Jackman
2020-12-04 4:46 ` Yonghong Song
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=X8oBaf4c+EAd8LQE@google.com \
--to=jackmanb@google.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=jannh@google.com \
--cc=kpsingh@chromium.org \
--cc=linux-kernel@vger.kernel.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.