* [PATCH bpf-next 1/2] bpf, arm64: Simplify if logic in emit_lse_atomic()
@ 2024-12-27 23:35 Peilin Ye
2024-12-27 23:36 ` [PATCH bpf-next 2/2] bpf, arm64: Emit A64_{ADD,SUB}_I when possible in emit_{lse,ll_sc}_atomic() Peilin Ye
0 siblings, 1 reply; 4+ messages in thread
From: Peilin Ye @ 2024-12-27 23:35 UTC (permalink / raw)
To: bpf
Cc: Peilin Ye, Xu Kuohai, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Puranjay Mohan, Catalin Marinas, Will Deacon,
Josh Don, Barret Rhoden, linux-arm-kernel, linux-kernel
Delete that unnecessary outer if clause. No functional changes.
Signed-off-by: Peilin Ye <yepeilin@google.com>
---
arch/arm64/net/bpf_jit_comp.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 66708b95493a..9040033eb1ea 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -648,16 +648,14 @@ static int emit_lse_atomic(const struct bpf_insn *insn, struct jit_ctx *ctx)
const s16 off = insn->off;
u8 reg = dst;
- if (off || arena) {
- if (off) {
- emit_a64_mov_i(1, tmp, off, ctx);
- emit(A64_ADD(1, tmp, tmp, dst), ctx);
- reg = tmp;
- }
- if (arena) {
- emit(A64_ADD(1, tmp, reg, arena_vm_base), ctx);
- reg = tmp;
- }
+ if (off) {
+ emit_a64_mov_i(1, tmp, off, ctx);
+ emit(A64_ADD(1, tmp, tmp, dst), ctx);
+ reg = tmp;
+ }
+ if (arena) {
+ emit(A64_ADD(1, tmp, reg, arena_vm_base), ctx);
+ reg = tmp;
}
switch (insn->imm) {
--
2.47.1.613.gc27f4b7a9f-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH bpf-next 2/2] bpf, arm64: Emit A64_{ADD,SUB}_I when possible in emit_{lse,ll_sc}_atomic()
2024-12-27 23:35 [PATCH bpf-next 1/2] bpf, arm64: Simplify if logic in emit_lse_atomic() Peilin Ye
@ 2024-12-27 23:36 ` Peilin Ye
2024-12-30 8:44 ` Xu Kuohai
0 siblings, 1 reply; 4+ messages in thread
From: Peilin Ye @ 2024-12-27 23:36 UTC (permalink / raw)
To: bpf
Cc: Peilin Ye, Xu Kuohai, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Puranjay Mohan, Catalin Marinas, Will Deacon,
Josh Don, Barret Rhoden, linux-arm-kernel, linux-kernel
Currently in emit_{lse,ll_sc}_atomic(), if there is an offset, we add it
to the base address by emitting two instructions, for example:
if (off) {
emit_a64_mov_i(1, tmp, off, ctx);
emit(A64_ADD(1, tmp, tmp, dst), ctx);
...
As pointed out by Xu, we can combine the above into a single A64_ADD_I
instruction if 'is_addsub_imm(off)' is true, or an A64_SUB_I, if
'is_addsub_imm(-off)' is true.
Suggested-by: Xu Kuohai <xukuohai@huaweicloud.com>
Signed-off-by: Peilin Ye <yepeilin@google.com>
---
Hi all,
This was pointed out by Xu in [1] . Tested on x86-64, using
PLATFORM=aarch64 CROSS_COMPILE=aarch64-linux-gnu- vmtest.sh:
LSE:
* ./test_progs-cpuv4 -a atomics,arena_atomics
2/15 PASSED, 0 SKIPPED, 0 FAILED
* ./test_verifier
790 PASSED, 0 SKIPPED, 0 FAILED
LL/SC:
(In vmtest.sh, changed '-cpu' QEMU option from 'cortex-a76' to
'cortex-a57', to make LSE atomics unavailable.)
* ./test_progs-cpuv4 -a atomics
1/7 PASSED, 0 SKIPPED, 0 FAILED
* ./test_verifier
790 PASSED, 0 SKIPPED, 0 FAILED
Thanks,
Peilin Ye
[1] https://lore.kernel.org/bpf/f704019d-a8fa-4cf5-a606-9d8328360a3e@huaweicloud.com/
arch/arm64/net/bpf_jit_comp.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 9040033eb1ea..f15bbe92fed9 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -649,8 +649,14 @@ static int emit_lse_atomic(const struct bpf_insn *insn, struct jit_ctx *ctx)
u8 reg = dst;
if (off) {
- emit_a64_mov_i(1, tmp, off, ctx);
- emit(A64_ADD(1, tmp, tmp, dst), ctx);
+ if (is_addsub_imm(off)) {
+ emit(A64_ADD_I(1, tmp, reg, off), ctx);
+ } else if (is_addsub_imm(-off)) {
+ emit(A64_SUB_I(1, tmp, reg, -off), ctx);
+ } else {
+ emit_a64_mov_i(1, tmp, off, ctx);
+ emit(A64_ADD(1, tmp, tmp, reg), ctx);
+ }
reg = tmp;
}
if (arena) {
@@ -721,7 +727,7 @@ static int emit_ll_sc_atomic(const struct bpf_insn *insn, struct jit_ctx *ctx)
const s32 imm = insn->imm;
const s16 off = insn->off;
const bool isdw = BPF_SIZE(code) == BPF_DW;
- u8 reg;
+ u8 reg = dst;
s32 jmp_offset;
if (BPF_MODE(code) == BPF_PROBE_ATOMIC) {
@@ -730,11 +736,15 @@ static int emit_ll_sc_atomic(const struct bpf_insn *insn, struct jit_ctx *ctx)
return -EINVAL;
}
- if (!off) {
- reg = dst;
- } else {
- emit_a64_mov_i(1, tmp, off, ctx);
- emit(A64_ADD(1, tmp, tmp, dst), ctx);
+ if (off) {
+ if (is_addsub_imm(off)) {
+ emit(A64_ADD_I(1, tmp, reg, off), ctx);
+ } else if (is_addsub_imm(-off)) {
+ emit(A64_SUB_I(1, tmp, reg, -off), ctx);
+ } else {
+ emit_a64_mov_i(1, tmp, off, ctx);
+ emit(A64_ADD(1, tmp, tmp, reg), ctx);
+ }
reg = tmp;
}
--
2.47.1.613.gc27f4b7a9f-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next 2/2] bpf, arm64: Emit A64_{ADD,SUB}_I when possible in emit_{lse,ll_sc}_atomic()
2024-12-27 23:36 ` [PATCH bpf-next 2/2] bpf, arm64: Emit A64_{ADD,SUB}_I when possible in emit_{lse,ll_sc}_atomic() Peilin Ye
@ 2024-12-30 8:44 ` Xu Kuohai
2024-12-30 23:25 ` Peilin Ye
0 siblings, 1 reply; 4+ messages in thread
From: Xu Kuohai @ 2024-12-30 8:44 UTC (permalink / raw)
To: Peilin Ye, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Puranjay Mohan, Catalin Marinas, Will Deacon, Josh Don,
Barret Rhoden, linux-arm-kernel, linux-kernel
On 12/28/2024 7:36 AM, Peilin Ye wrote:
> Currently in emit_{lse,ll_sc}_atomic(), if there is an offset, we add it
> to the base address by emitting two instructions, for example:
>
> if (off) {
> emit_a64_mov_i(1, tmp, off, ctx);
> emit(A64_ADD(1, tmp, tmp, dst), ctx);
> ...
>
> As pointed out by Xu, we can combine the above into a single A64_ADD_I
> instruction if 'is_addsub_imm(off)' is true, or an A64_SUB_I, if
> 'is_addsub_imm(-off)' is true.
>
> Suggested-by: Xu Kuohai <xukuohai@huaweicloud.com>
> Signed-off-by: Peilin Ye <yepeilin@google.com>
> ---
> Hi all,
>
> This was pointed out by Xu in [1] . Tested on x86-64, using
> PLATFORM=aarch64 CROSS_COMPILE=aarch64-linux-gnu- vmtest.sh:
>
> LSE:
> * ./test_progs-cpuv4 -a atomics,arena_atomics
> 2/15 PASSED, 0 SKIPPED, 0 FAILED
> * ./test_verifier
> 790 PASSED, 0 SKIPPED, 0 FAILED
>
> LL/SC:
> (In vmtest.sh, changed '-cpu' QEMU option from 'cortex-a76' to
> 'cortex-a57', to make LSE atomics unavailable.)
> * ./test_progs-cpuv4 -a atomics
> 1/7 PASSED, 0 SKIPPED, 0 FAILED
> * ./test_verifier
> 790 PASSED, 0 SKIPPED, 0 FAILED
>
> Thanks,
> Peilin Ye
>
> [1] https://lore.kernel.org/bpf/f704019d-a8fa-4cf5-a606-9d8328360a3e@huaweicloud.com/
>
> arch/arm64/net/bpf_jit_comp.c | 26 ++++++++++++++++++--------
> 1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index 9040033eb1ea..f15bbe92fed9 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -649,8 +649,14 @@ static int emit_lse_atomic(const struct bpf_insn *insn, struct jit_ctx *ctx)
> u8 reg = dst;
>
> if (off) {
> - emit_a64_mov_i(1, tmp, off, ctx);
> - emit(A64_ADD(1, tmp, tmp, dst), ctx);
> + if (is_addsub_imm(off)) {
> + emit(A64_ADD_I(1, tmp, reg, off), ctx);
> + } else if (is_addsub_imm(-off)) {
> + emit(A64_SUB_I(1, tmp, reg, -off), ctx);
> + } else {
> + emit_a64_mov_i(1, tmp, off, ctx);
> + emit(A64_ADD(1, tmp, tmp, reg), ctx);
> + }
> reg = tmp;
> }
> if (arena) {
> @@ -721,7 +727,7 @@ static int emit_ll_sc_atomic(const struct bpf_insn *insn, struct jit_ctx *ctx)
> const s32 imm = insn->imm;
> const s16 off = insn->off;
> const bool isdw = BPF_SIZE(code) == BPF_DW;
> - u8 reg;
> + u8 reg = dst;
> s32 jmp_offset;
>
> if (BPF_MODE(code) == BPF_PROBE_ATOMIC) {
> @@ -730,11 +736,15 @@ static int emit_ll_sc_atomic(const struct bpf_insn *insn, struct jit_ctx *ctx)
> return -EINVAL;
> }
>
> - if (!off) {
> - reg = dst;
> - } else {
> - emit_a64_mov_i(1, tmp, off, ctx);
> - emit(A64_ADD(1, tmp, tmp, dst), ctx);
> + if (off) {
> + if (is_addsub_imm(off)) {
> + emit(A64_ADD_I(1, tmp, reg, off), ctx);
> + } else if (is_addsub_imm(-off)) {
> + emit(A64_SUB_I(1, tmp, reg, -off), ctx);
> + } else {
> + emit_a64_mov_i(1, tmp, off, ctx);
> + emit(A64_ADD(1, tmp, tmp, reg), ctx);
> + }
> reg = tmp;
> }
>
Thanks, this looks good to me, but we now have serveral repetitive code
snippets like this. It would be better to refactor them into a common
function.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next 2/2] bpf, arm64: Emit A64_{ADD,SUB}_I when possible in emit_{lse,ll_sc}_atomic()
2024-12-30 8:44 ` Xu Kuohai
@ 2024-12-30 23:25 ` Peilin Ye
0 siblings, 0 replies; 4+ messages in thread
From: Peilin Ye @ 2024-12-30 23:25 UTC (permalink / raw)
To: Xu Kuohai
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Puranjay Mohan, Catalin Marinas, Will Deacon, Josh Don,
Barret Rhoden, linux-arm-kernel, linux-kernel
On Mon, Dec 30, 2024 at 04:44:26PM +0800, Xu Kuohai wrote:
> > @@ -721,7 +727,7 @@ static int emit_ll_sc_atomic(const struct bpf_insn *insn, struct jit_ctx *ctx)
> > const s32 imm = insn->imm;
> > const s16 off = insn->off;
> > const bool isdw = BPF_SIZE(code) == BPF_DW;
> > - u8 reg;
> > + u8 reg = dst;
> > s32 jmp_offset;
> > if (BPF_MODE(code) == BPF_PROBE_ATOMIC) {
> > @@ -730,11 +736,15 @@ static int emit_ll_sc_atomic(const struct bpf_insn *insn, struct jit_ctx *ctx)
> > return -EINVAL;
> > }
> > - if (!off) {
> > - reg = dst;
> > - } else {
> > - emit_a64_mov_i(1, tmp, off, ctx);
> > - emit(A64_ADD(1, tmp, tmp, dst), ctx);
> > + if (off) {
> > + if (is_addsub_imm(off)) {
> > + emit(A64_ADD_I(1, tmp, reg, off), ctx);
> > + } else if (is_addsub_imm(-off)) {
> > + emit(A64_SUB_I(1, tmp, reg, -off), ctx);
> > + } else {
> > + emit_a64_mov_i(1, tmp, off, ctx);
> > + emit(A64_ADD(1, tmp, tmp, reg), ctx);
> > + }
> > reg = tmp;
> > }
>
> Thanks, this looks good to me, but we now have serveral repetitive code
> snippets like this. It would be better to refactor them into a common
> function.
Sure! I will do that in v2.
Thanks,
Peilin Ye
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-12-30 23:27 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-27 23:35 [PATCH bpf-next 1/2] bpf, arm64: Simplify if logic in emit_lse_atomic() Peilin Ye
2024-12-27 23:36 ` [PATCH bpf-next 2/2] bpf, arm64: Emit A64_{ADD,SUB}_I when possible in emit_{lse,ll_sc}_atomic() Peilin Ye
2024-12-30 8:44 ` Xu Kuohai
2024-12-30 23:25 ` Peilin Ye
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).