On Thu, Sep 14, 2023 at 7:36 AM Leon Hwang <hffilwlqm@xxxxxxxxx> wrote:
>
> It's unnecessary to check 'imm32' in both 'if' and 'else'.
>
> It's better to check it first.
>
> Meanwhile, refactor the code for 'offs' calculation.
>
> v1 -> v2:
> * Add '#define RESTORE_TAIL_CALL_CNT_INSN_SIZE 7'.
>
> Signed-off-by: Leon Hwang <hffilwlqm@xxxxxxxxx>
> ---
> arch/x86/net/bpf_jit_comp.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 2846c21d75bfa..fe0393c7e7b68 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1025,6 +1025,7 @@ static void emit_shiftx(u8 **pprog, u32 dst_reg, u8 src_reg, bool is64, u8 op)
> /* mov rax, qword ptr [rbp - rounded_stack_depth - 8] */
> #define RESTORE_TAIL_CALL_CNT(stack) \
> EMIT3_off32(0x48, 0x8B, 0x85, -round_up(stack, 8) - 8)
> +#define RESTORE_TAIL_CALL_CNT_INSN_SIZE 7
>
> static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image,
> int oldproglen, struct jit_context *ctx, bool jmp_padding)
> @@ -1629,17 +1630,16 @@ st: if (is_imm8(insn->off))
> case BPF_JMP | BPF_CALL: {
> int offs;
>
<snip>
> + if (tail_call_reachable)
> RESTORE_TAIL_CALL_CNT(bpf_prog->aux->stack_depth);
<snip>
> + offs = (tail_call_reachable ?
> + RESTORE_TAIL_CALL_CNT_INSN_SIZE : 0);
I'm not sure which is preferred style for the kernel, but this seems like it
could be replaced with
int offs = 0;
...
if (tail_call_reachable) {
RESTORE_TAIL_CALL_CNT(bpf_prog->aux->stack_depth);
offs = RESTORE_TAIL_CALL_CNT_INSN_SIZE;
}
which is easier for my brain to follow because it reduces the branches (in C,
I assume the compiler is smart enough to optimize). It does create an
unconditional write (of 0) that could otherwise be avoided (unless the compiler
is also smart enough to optimize that).
Also not sure if the performance difference matters here.
> + offs += x86_call_depth_emit_accounting(&prog, func);
> if (emit_call(&prog, func, image + addrs[i - 1] + offs))
> return -EINVAL;
> break;
>
> base-commit: cbb1dbcd99b0ae74c45c4c83c6d213c12c31785c
> --
> 2.41.0
>
>
