Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1635843092-12907-1-git-send-email-yangtiezhu@loongson.cn>
In-Reply-To: <1635843092-12907-1-git-send-email-yangtiezhu@loongson.cn>
From:   =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn.topel@gmail.com>
Date:   Tue, 2 Nov 2021 18:06:52 +0100
Message-ID: <CAJ+HfNhtZFnxNjzDo8gSJdyDhEEicgUo5hDdCeBYJ=rutBZ93w@mail.gmail.com>
Subject: Re: [PATCH bpf-next v4] bpf: Change value of MAX_TAIL_CALL_CNT from
 32 to 33
To:     Tiezhu Yang <yangtiezhu@loongson.cn>
Cc:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Netdev <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Xuefeng Li <lixuefeng@loongson.cn>,
        Johan Almbladh <johan.almbladh@anyfinetworks.com>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>, illusionist.neo@gmail.com,
        zlim.lnx@gmail.com, naveen.n.rao@linux.ibm.com,
        Luke Nelson <luke.r.nels@gmail.com>,
        Xi Wang <xi.wang@gmail.com>, iii@linux.ibm.com,
        hca@linux.ibm.com, gor@linux.ibm.com, udknight@gmail.com,
        David Miller <davem@davemloft.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, 2 Nov 2021 at 09:51, Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>
> In the current code, the actual max tail call count is 33 which is greate=
r
> than MAX_TAIL_CALL_CNT (defined as 32), the actual limit is not consisten=
t
> with the meaning of MAX_TAIL_CALL_CNT, there is some confusion and need t=
o
> 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 ca=
n
> 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 !=3D 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 !=3D 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_C=
NT
> can reflect the actual max tail call count, the related tailcall testcase=
s
> 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_com=
p32.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_j=
it_context *ctx)
>         emit_bcc(BPF_JGE, lo(idx_reg), RV_REG_T1, off, ctx);
>
>         /*
> -        * temp_tcc =3D tcc - 1;
> -        * if (tcc < 0)
> +        * if (--tcc < 0)
>          *   goto out;
>          */
>         emit(rv_addi(RV_REG_T1, RV_REG_TCC, -1), ctx);
>         off =3D 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 =3D array->ptrs[index];
> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_com=
p64.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_j=
it_context *ctx)
>         off =3D 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 =3D 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 =3D 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_comp3=
2.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 =3D 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 =3D 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_comp6=
4.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 =3D 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 =3D 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 =3D 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=C3=B6rn T=C3=B6pel <bjorn@kernel.org>


Bj=C3=B6rn