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
