On Mon, Nov 23, 2020 at 05:31:58PM +0000, Brendan Jackman wrote:
> A subsequent patch will add additional atomic operations. These new
> operations will use the same opcode field as the existing XADD, with
> the immediate discriminating different operations.
>
> In preparation, rename the instruction mode BPF_ATOMIC and start
> calling the zero immediate BPF_ADD.
>
> This is possible (doesn't break existing valid BPF progs) because the
> immediate field is currently reserved MBZ and BPF_ADD is zero.
>
> All uses are removed from the tree but the BPF_XADD definition is
> kept around to avoid breaking builds for people including kernel
> headers.
>
> Signed-off-by: Brendan Jackman <jackmanb@xxxxxxxxxx>
> ---
> Documentation/networking/filter.rst | 27 +++++++++-------
> arch/arm/net/bpf_jit_32.c | 7 ++---
> arch/arm64/net/bpf_jit_comp.c | 16 +++++++---
> arch/mips/net/ebpf_jit.c | 11 +++++--
> arch/powerpc/net/bpf_jit_comp64.c | 25 ++++++++++++---
> arch/riscv/net/bpf_jit_comp32.c | 20 +++++++++---
> arch/riscv/net/bpf_jit_comp64.c | 16 +++++++---
> arch/s390/net/bpf_jit_comp.c | 26 +++++++++-------
> arch/sparc/net/bpf_jit_comp_64.c | 14 +++++++--
> arch/x86/net/bpf_jit_comp.c | 30 +++++++++++-------
> arch/x86/net/bpf_jit_comp32.c | 6 ++--
I think this massive rename is not needed.
BPF_XADD is uapi and won't be removed.
Updating documentation, samples and tests is probably enough.
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 1b62397bd124..ce19988fb312 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -261,13 +261,15 @@ static inline bool insn_is_zext(const struct bpf_insn *insn)
>
> /* Atomic memory add, *(uint *)(dst_reg + off16) += src_reg */
>
> -#define BPF_STX_XADD(SIZE, DST, SRC, OFF) \
> +#define BPF_ATOMIC_ADD(SIZE, DST, SRC, OFF) \
> ((struct bpf_insn) { \
> - .code = BPF_STX | BPF_SIZE(SIZE) | BPF_XADD, \
> + .code = BPF_STX | BPF_SIZE(SIZE) | BPF_ATOMIC, \
> .dst_reg = DST, \
> .src_reg = SRC, \
> .off = OFF, \
> - .imm = 0 })
> + .imm = BPF_ADD })
> +#define BPF_STX_XADD BPF_ATOMIC_ADD /* alias */
this is fine.
> +
>
> /* Memory store, *(uint *) (dst_reg + off16) = imm32 */
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 3ca6146f001a..dcd08783647d 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -19,7 +19,8 @@
>
> /* ld/ldx fields */
> #define BPF_DW 0x18 /* double word (64-bit) */
> -#define BPF_XADD 0xc0 /* exclusive add */
> +#define BPF_ATOMIC 0xc0 /* atomic memory ops - op type in immediate */
> +#define BPF_XADD 0xc0 /* legacy name, don't add new uses */
I think the comment sounds too strong.
New uses of BPF_XADD are discouraged, but they're still correct.
>
> /* alu/jmp fields */
> #define BPF_MOV 0xb0 /* mov reg to reg */
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index ff55cbcfbab4..48b192a8edce 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -1317,8 +1317,8 @@ EXPORT_SYMBOL_GPL(__bpf_call_base);
> INSN_3(STX, MEM, H), \
> INSN_3(STX, MEM, W), \
> INSN_3(STX, MEM, DW), \
> - INSN_3(STX, XADD, W), \
> - INSN_3(STX, XADD, DW), \
> + INSN_3(STX, ATOMIC, W), \
> + INSN_3(STX, ATOMIC, DW), \
> /* Immediate based. */ \
> INSN_3(ST, MEM, B), \
> INSN_3(ST, MEM, H), \
> @@ -1626,13 +1626,25 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
> LDX_PROBE(DW, 8)
> #undef LDX_PROBE
>
> - STX_XADD_W: /* lock xadd *(u32 *)(dst_reg + off16) += src_reg */
> - atomic_add((u32) SRC, (atomic_t *)(unsigned long)
> - (DST + insn->off));
> + STX_ATOMIC_W:
> + switch (insn->imm) {
> + case BPF_ADD:
> + /* lock xadd *(u32 *)(dst_reg + off16) += src_reg */
> + atomic_add((u32) SRC, (atomic_t *)(unsigned long)
> + (DST + insn->off));
> + default:
> + goto default_label;
> + }
> CONT;
> - STX_XADD_DW: /* lock xadd *(u64 *)(dst_reg + off16) += src_reg */
> - atomic64_add((u64) SRC, (atomic64_t *)(unsigned long)
> - (DST + insn->off));
> + STX_ATOMIC_DW:
> + switch (insn->imm) {
> + case BPF_ADD:
> + /* lock xadd *(u64 *)(dst_reg + off16) += src_reg */
> + atomic64_add((u64) SRC, (atomic64_t *)(unsigned long)
> + (DST + insn->off));
> + default:
> + goto default_label;
> + }
+1
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index fb2943ea715d..06885e2501f8 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3598,13 +3598,17 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
> return err;
> }
>
> -static int check_xadd(struct bpf_verifier_env *env, int insn_idx, struct bpf_insn *insn)
> +static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_insn *insn)
+1
> diff --git a/lib/test_bpf.c b/lib/test_bpf.c
> index ca7d635bccd9..fbb13ef9207c 100644
> --- a/lib/test_bpf.c
> +++ b/lib/test_bpf.c
> @@ -4295,7 +4295,7 @@ static struct bpf_test tests[] = {
> { { 0, 0xffffffff } },
> .stack_depth = 40,
> },
> - /* BPF_STX | BPF_XADD | BPF_W/DW */
> + /* BPF_STX | BPF_ATOMIC | BPF_W/DW */
I would leave this comment as-is.
There are several uses of BPF_STX_XADD in that file.
So the comment is fine.
> {
> "STX_XADD_W: Test: 0x12 + 0x10 = 0x22",
> .u.insns_int = {
> diff --git a/samples/bpf/bpf_insn.h b/samples/bpf/bpf_insn.h
> index 544237980582..db67a2847395 100644
> --- a/samples/bpf/bpf_insn.h
> +++ b/samples/bpf/bpf_insn.h
> @@ -138,11 +138,11 @@ struct bpf_insn;
>
> #define BPF_STX_XADD(SIZE, DST, SRC, OFF) \
> ((struct bpf_insn) { \
> - .code = BPF_STX | BPF_SIZE(SIZE) | BPF_XADD, \
> + .code = BPF_STX | BPF_SIZE(SIZE) | BPF_ATOMIC, \
> .dst_reg = DST, \
> .src_reg = SRC, \
> .off = OFF, \
> - .imm = 0 })
> + .imm = BPF_ADD })
>
> /* Memory store, *(uint *) (dst_reg + off16) = imm32 */
>
> diff --git a/samples/bpf/sock_example.c b/samples/bpf/sock_example.c
> index 00aae1d33fca..41ec3ca3bc40 100644
> --- a/samples/bpf/sock_example.c
> +++ b/samples/bpf/sock_example.c
> @@ -54,7 +54,8 @@ static int test_sock(void)
> BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
> BPF_MOV64_IMM(BPF_REG_1, 1), /* r1 = 1 */
> - BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_DW, BPF_REG_0, BPF_REG_1, 0, 0), /* xadd r0 += r1 */
> + BPF_RAW_INSN(BPF_STX | BPF_ATOMIC | BPF_DW,
> + BPF_REG_0, BPF_REG_1, 0, BPF_ADD), /* xadd r0 += r1 */
+1
> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c
> index b549fcfacc0b..79401a59a988 100644
> --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c
> @@ -45,13 +45,15 @@ static int prog_load_cnt(int verdict, int val)
> BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
> BPF_MOV64_IMM(BPF_REG_1, val), /* r1 = 1 */
> - BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_DW, BPF_REG_0, BPF_REG_1, 0, 0), /* xadd r0 += r1 */
> + BPF_RAW_INSN(BPF_STX | BPF_ATOMIC | BPF_DW,
> + BPF_REG_0, BPF_REG_1, 0, BPF_ADD), /* xadd r0 += r1 */
+1
