Re: [PATCH bpf-next v4] bpf: Change value of MAX_TAIL_CALL_CNT from 32 to 33

[Björn Töpel]

On Tue, 2 Nov 2021 at 09:51, Tiezhu Yang <yangtiezhu@xxxxxxxxxxx> wrote:
>
> In the current code, the actual max tail call count is 33 which is greater
> than MAX_TAIL_CALL_CNT (defined as 32), the actual limit is not consistent
> with the meaning of MAX_TAIL_CALL_CNT, there is some confusion and need to
> spend some time to think about the reason at the first glance.
>
> We can see the historical evolution from commit 04fd61ab36ec ("bpf: allow
> bpf programs to tail-call other bpf programs") and commit f9dabe016b63
> ("bpf: Undo off-by-one in interpreter tail call count limit").
>
> In order to avoid changing existing behavior, the actual limit is 33 now,
> this is reasonable.
>
> After commit 874be05f525e ("bpf, tests: Add tail call test suite"), we can
> see there exists failed testcase.
>
> On all archs when CONFIG_BPF_JIT_ALWAYS_ON is not set:
>  # echo 0 > /proc/sys/net/core/bpf_jit_enable
>  # modprobe test_bpf
>  # dmesg | grep -w FAIL
>  Tail call error path, max count reached jited:0 ret 34 != 33 FAIL
>
> On some archs:
>  # echo 1 > /proc/sys/net/core/bpf_jit_enable
>  # modprobe test_bpf
>  # dmesg | grep -w FAIL
>  Tail call error path, max count reached jited:1 ret 34 != 33 FAIL
>
> Although the above failed testcase has been fixed in commit 18935a72eb25
> ("bpf/tests: Fix error in tail call limit tests"), it is still necessary
> to change the value of MAX_TAIL_CALL_CNT from 32 to 33 to make the code
> more readable, then do some small changes of the related code.
>
> With this patch, it does not change the current limit 33, MAX_TAIL_CALL_CNT
> can reflect the actual max tail call count, the related tailcall testcases
> in test_bpf and selftests can work well for the interpreter and the JIT.
>

[...]

> diff --git a/arch/riscv/net/bpf_jit_comp32.c b/arch/riscv/net/bpf_jit_comp32.c
> index e649742..ead9733 100644
> --- a/arch/riscv/net/bpf_jit_comp32.c
> +++ b/arch/riscv/net/bpf_jit_comp32.c
> @@ -799,13 +799,12 @@ static int emit_bpf_tail_call(int insn, struct rv_jit_context *ctx)
>         emit_bcc(BPF_JGE, lo(idx_reg), RV_REG_T1, off, ctx);
>
>         /*
> -        * temp_tcc = tcc - 1;
> -        * if (tcc < 0)
> +        * if (--tcc < 0)
>          *   goto out;
>          */
>         emit(rv_addi(RV_REG_T1, RV_REG_TCC, -1), ctx);
>         off = ninsns_rvoff(tc_ninsn - (ctx->ninsns - start_insn));
> -       emit_bcc(BPF_JSLT, RV_REG_TCC, RV_REG_ZERO, off, ctx);
> +       emit_bcc(BPF_JSLT, RV_REG_T1, RV_REG_ZERO, off, ctx);
>
>         /*
>          * prog = array->ptrs[index];
> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
> index 2ca345c..9822f58 100644
> --- a/arch/riscv/net/bpf_jit_comp64.c
> +++ b/arch/riscv/net/bpf_jit_comp64.c
> @@ -327,12 +327,12 @@ static int emit_bpf_tail_call(int insn, struct rv_jit_context *ctx)
>         off = ninsns_rvoff(tc_ninsn - (ctx->ninsns - start_insn));
>         emit_branch(BPF_JGE, RV_REG_A2, RV_REG_T1, off, ctx);
>
> -       /* if (TCC-- < 0)
> +       /* if (--tcc < 0)
>          *     goto out;
>          */
>         emit_addi(RV_REG_T1, tcc, -1, ctx);
>         off = ninsns_rvoff(tc_ninsn - (ctx->ninsns - start_insn));
> -       emit_branch(BPF_JSLT, tcc, RV_REG_ZERO, off, ctx);
> +       emit_branch(BPF_JSLT, RV_REG_T1, RV_REG_ZERO, off, ctx);
>
>         /* prog = array->ptrs[index];
>          * if (!prog)

The RISC-V code can be simplified, to save one move:

diff --git a/arch/riscv/net/bpf_jit_comp32.c b/arch/riscv/net/bpf_jit_comp32.c
index e6497424cbf6..529a83b85c1c 100644
--- a/arch/riscv/net/bpf_jit_comp32.c
+++ b/arch/riscv/net/bpf_jit_comp32.c
@@ -799,11 +799,10 @@ static int emit_bpf_tail_call(int insn, struct
rv_jit_context *ctx)
        emit_bcc(BPF_JGE, lo(idx_reg), RV_REG_T1, off, ctx);

        /*
-        * temp_tcc = tcc - 1;
-        * if (tcc < 0)
+        * if (--tcc < 0)
         *   goto out;
         */
-       emit(rv_addi(RV_REG_T1, RV_REG_TCC, -1), ctx);
+       emit(rv_addi(RV_REG_TCC, RV_REG_TCC, -1), ctx);
        off = ninsns_rvoff(tc_ninsn - (ctx->ninsns - start_insn));
        emit_bcc(BPF_JSLT, RV_REG_TCC, RV_REG_ZERO, off, ctx);

@@ -829,7 +828,6 @@ static int emit_bpf_tail_call(int insn, struct
rv_jit_context *ctx)
        if (is_12b_check(off, insn))
                return -1;
        emit(rv_lw(RV_REG_T0, off, RV_REG_T0), ctx);
-       emit(rv_addi(RV_REG_TCC, RV_REG_T1, 0), ctx);
        /* Epilogue jumps to *(t0 + 4). */
        __build_epilogue(true, ctx);
        return 0;
diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
index 2ca345c7b0bf..f4466b7997b5 100644
--- a/arch/riscv/net/bpf_jit_comp64.c
+++ b/arch/riscv/net/bpf_jit_comp64.c
@@ -327,12 +327,12 @@ static int emit_bpf_tail_call(int insn, struct
rv_jit_context *ctx)
        off = ninsns_rvoff(tc_ninsn - (ctx->ninsns - start_insn));
        emit_branch(BPF_JGE, RV_REG_A2, RV_REG_T1, off, ctx);

-       /* if (TCC-- < 0)
+       /* if (--TCC < 0)
         *     goto out;
         */
-       emit_addi(RV_REG_T1, tcc, -1, ctx);
+       emit_addi(RV_REG_TCC, tcc, -1, ctx);
        off = ninsns_rvoff(tc_ninsn - (ctx->ninsns - start_insn));
-       emit_branch(BPF_JSLT, tcc, RV_REG_ZERO, off, ctx);
+       emit_branch(BPF_JSLT, RV_REG_TCC, RV_REG_ZERO, off, ctx);

        /* prog = array->ptrs[index];
         * if (!prog)
@@ -352,7 +352,6 @@ static int emit_bpf_tail_call(int insn, struct
rv_jit_context *ctx)
        if (is_12b_check(off, insn))
                return -1;
        emit_ld(RV_REG_T3, off, RV_REG_T2, ctx);
-       emit_mv(RV_REG_TCC, RV_REG_T1, ctx);
        __build_epilogue(true, ctx);
        return 0;
 }

With that change applied, for RISC-V:

Acked-by: Björn Töpel <bjorn@xxxxxxxxxx>


Björn