Received-SPF: pass (google.com: domain of linux-kernel+bounces-165606-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240430234739.79185-1-puranjay@kernel.org> <20240430234739.79185-3-puranjay@kernel.org>
In-Reply-To: <20240430234739.79185-3-puranjay@kernel.org>
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date: Wed, 1 May 2024 09:50:16 -0700
Message-ID: <CAEf4BzbszfMGoOCx7hQPBfDLNLfBuAQ1PQDEq=ut=WiEubm_oA@mail.gmail.com>
Subject: Re: [PATCH bpf-next v5 2/2] bpf, arm64: inline bpf_get_smp_processor_id()
 helper
To: Puranjay Mohan <puranjay@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, 
	Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, 
	Andrii Nakryiko <andrii@kernel.org>, Martin KaFai Lau <martin.lau@linux.dev>, 
	Eduard Zingerman <eddyz87@gmail.com>, Song Liu <song@kernel.org>, 
	Yonghong Song <yonghong.song@linux.dev>, John Fastabend <john.fastabend@gmail.com>, 
	KP Singh <kpsingh@kernel.org>, Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, 
	Jiri Olsa <jolsa@kernel.org>, Zi Shen Lim <zlim.lnx@gmail.com>, Xu Kuohai <xukuohai@huawei.com>, 
	Florent Revest <revest@chromium.org>, linux-arm-kernel@lists.infradead.org, 
	linux-kernel@vger.kernel.org, bpf@vger.kernel.org, 
	Kumar Kartikeya Dwivedi <memxor@gmail.com>, puranjay12@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 30, 2024 at 4:48=E2=80=AFPM Puranjay Mohan <puranjay@kernel.org=
> wrote:
>
> Inline calls to bpf_get_smp_processor_id() helper in the JIT by emitting
> a read from struct thread_info. The SP_EL0 system register holds the
> pointer to the task_struct and thread_info is the first member of this
> struct. We can read the cpu number from the thread_info.
>
> Here is how the ARM64 JITed assembly changes after this commit:
>
>                                       ARM64 JIT
>                                      =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>
>               BEFORE                                    AFTER
>              --------                                  -------
>
> int cpu =3D bpf_get_smp_processor_id();        int cpu =3D bpf_get_smp_pr=
ocessor_id();
>
> mov     x10, #0xfffffffffffff4d0             mrs     x10, sp_el0
> movk    x10, #0x802b, lsl #16                ldr     w7, [x10, #24]
> movk    x10, #0x8000, lsl #32
> blr     x10
> add     x7, x0, #0x0
>
>                Performance improvement using benchmark[1]
>
> ./benchs/run_bench_trigger.sh glob-arr-inc arr-inc hash-inc
>
> +---------------+-------------------+-------------------+--------------+
> |      Name     |      Before       |        After      |   % change   |
> |---------------+-------------------+-------------------+--------------|
> | glob-arr-inc  | 23.380 =C2=B1 1.675M/s | 25.893 =C2=B1 0.026M/s |   + 1=
0.74%   |
> | arr-inc       | 23.928 =C2=B1 0.034M/s | 25.213 =C2=B1 0.063M/s |   + 5=
37%    |
> | hash-inc      | 12.352 =C2=B1 0.005M/s | 12.609 =C2=B1 0.013M/s |   + 2=
08%    |
> +---------------+-------------------+-------------------+--------------+
>
> [1] https://github.com/anakryiko/linux/commit/8dec900975ef
>
> Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
> ---
>  arch/arm64/include/asm/insn.h |  1 +
>  arch/arm64/net/bpf_jit.h      |  2 ++
>  arch/arm64/net/bpf_jit_comp.c | 23 +++++++++++++++++++++++
>  3 files changed, 26 insertions(+)
>

Nice improvements! I suggest combining arm64 and risc-v patches
together when resubmitting, so that we can land them in one go. This
one depends on RISC-V patches landing first to avoid a warning about
global function without a prototype, right?

Please add my ack as well

Acked-by: Andrii Nakryiko <andrii@kernel.org>


> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.=
h
> index 8de0e39b29f3..8c0a36f72d6f 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -138,6 +138,7 @@ enum aarch64_insn_special_register {
>  enum aarch64_insn_system_register {
>         AARCH64_INSN_SYSREG_TPIDR_EL1   =3D 0x4684,
>         AARCH64_INSN_SYSREG_TPIDR_EL2   =3D 0x6682,
> +       AARCH64_INSN_SYSREG_SP_EL0      =3D 0x4208,
>  };
>
>  enum aarch64_insn_variant {
> diff --git a/arch/arm64/net/bpf_jit.h b/arch/arm64/net/bpf_jit.h
> index b627ef7188c7..b22ab2f97a30 100644
> --- a/arch/arm64/net/bpf_jit.h
> +++ b/arch/arm64/net/bpf_jit.h
> @@ -302,5 +302,7 @@
>         aarch64_insn_gen_mrs(Rt, AARCH64_INSN_SYSREG_TPIDR_EL1)
>  #define A64_MRS_TPIDR_EL2(Rt) \
>         aarch64_insn_gen_mrs(Rt, AARCH64_INSN_SYSREG_TPIDR_EL2)
> +#define A64_MRS_SP_EL0(Rt) \
> +       aarch64_insn_gen_mrs(Rt, AARCH64_INSN_SYSREG_SP_EL0)
>
>  #endif /* _BPF_JIT_H */
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.=
c
> index ed8f9716d9d5..8084f3e61e0b 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -1215,6 +1215,19 @@ static int build_insn(const struct bpf_insn *insn,=
 struct jit_ctx *ctx,
>                 const u8 r0 =3D bpf2a64[BPF_REG_0];
>                 bool func_addr_fixed;
>                 u64 func_addr;
> +               u32 cpu_offset =3D offsetof(struct thread_info, cpu);
> +
> +               /* Implement helper call to bpf_get_smp_processor_id() in=
line */
> +               if (insn->src_reg =3D=3D 0 && insn->imm =3D=3D BPF_FUNC_g=
et_smp_processor_id) {
> +                       emit(A64_MRS_SP_EL0(tmp), ctx);
> +                       if (is_lsi_offset(cpu_offset, 2)) {
> +                               emit(A64_LDR32I(r0, tmp, cpu_offset), ctx=
);
> +                       } else {
> +                               emit_a64_mov_i(1, tmp2, cpu_offset, ctx);
> +                               emit(A64_LDR32(r0, tmp, tmp2), ctx);
> +                       }
> +                       break;
> +               }
>
>                 ret =3D bpf_jit_get_func_addr(ctx->prog, insn, extra_pass=
,
>                                             &func_addr, &func_addr_fixed)=
;
> @@ -2541,6 +2554,16 @@ bool bpf_jit_supports_percpu_insn(void)
>         return true;
>  }
>
> +bool bpf_jit_inlines_helper_call(s32 imm)
> +{
> +       switch (imm) {
> +       case BPF_FUNC_get_smp_processor_id:
> +               return true;
> +       }
> +
> +       return false;

same minor nit to use default: return false inside the switch itself


> +}
> +
>  void bpf_jit_free(struct bpf_prog *prog)
>  {
>         if (prog->jited) {
> --
> 2.40.1
>