Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <20230613084315.62021-1-luojianhong@cdjrlc.com>
 <6228af14241b831be4bae6ebcd63799e@208suo.com>
 <53510828-ee5b-1d91-0f85-b79da4422741@meta.com>
User-agent: mu4e 1.6.10; emacs 28.2
From:   Jakub Sitnicki <jakub@cloudflare.com>
To:     Yonghong Song <yhs@meta.com>
Cc:     baomingtong001@208suo.com, ast@kernel.org, daniel@iogearbox.net,
        andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
        yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org,
        sdf@google.com, haoluo@google.com, jolsa@kernel.org,
        mykolal@fb.com, shuah@kernel.org, Jakub Kicinski <kuba@kernel.org>,
        bpf@vger.kernel.org, linux-kselftest@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] selftests/bpf: Remove unneeded variable "ret"
Date:   Tue, 13 Jun 2023 20:13:00 +0200
In-reply-to: <53510828-ee5b-1d91-0f85-b79da4422741@meta.com>
Message-ID: <87pm5z9q36.fsf@cloudflare.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Jun 13, 2023 at 06:42 AM -07, Yonghong Song wrote:
> On 6/13/23 1:50 AM, baomingtong001@208suo.com wrote:
>> Fix the following coccicheck warning:
>> tools/testing/selftests/bpf/progs/tailcall_bpf2bpf6.c:28:14-17: Unneeded
>> variable: "ret".
>> Return "1".
>> Signed-off-by: Mingtong Bao <baomingtong001@208suo.com>
>> ---
>>  =C2=A0tools/testing/selftests/bpf/progs/tailcall_bpf2bpf6.c | 3 +--
>>  =C2=A01 file changed, 1 insertion(+), 2 deletions(-)
>> diff --git a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf6.c
>> b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf6.c
>> index 4a9f63bea66c..7f0146682577 100644
>> --- a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf6.c
>> +++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf6.c
>> @@ -25,10 +25,9 @@ static __noinline
>>  =C2=A0int subprog_tail(struct __sk_buff *skb)
>>  =C2=A0{
>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* Don't propagate the constant to the ca=
ller */
>> - =C2=A0=C2=A0=C2=A0volatile int ret =3D 1;
>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0bpf_tail_call_static(skb, &jmp_table, 0);
>> - =C2=A0=C2=A0=C2=A0return ret;
>> + =C2=A0=C2=A0=C2=A0return 1;
>
> Please pay attention to the comment:
>    /* Don't propagate the constant to the caller */
> which clearly says 'constant' is not preferred.
>
> The patch introduced this change is:
>     5e0b0a4c52d30   selftests/bpf: Test tail call counting with bpf2bpf a=
nd data
>    on stack
>
> The test intentionally want to:
>   'Specifically when the size      of data allocated on BPF stack is not a
>  multiple on 8.'
>
> Note that with volatile and without volatile, the generated
> code will be different and it will result in different
> verification path.
>
> cc Jakub for further clarification.

Yonghong is right. We can't replace it like that.

Compiler is smart and pull up the constant into subprog_tail() caller.

And it doesn't have the slightest idea that bpf_tail_call_static() is
actually tail call (destroy frame + jump) and control doesn't return to
subprog_tail().

(You can read more about BPF tail calls in [1] and [2] if they are not
familiar.)

IOW, we need an r0 store to happen after a call to BPF tail call helper
(call 12) to remain in subprog_tail body for the regression test to
work:

$ llvm-objdump -d --no-show-raw-insn tailcall_bpf2bpf6.bpf.o

tailcall_bpf2bpf6.bpf.o:        file format elf64-bpf

Disassembly of section .text:

0000000000000000 <subprog_tail>:
       0:       r6 =3D r1
       1:       w1 =3D 1
       2:       *(u32 *)(r10 - 4) =3D r1
       3:       r7 =3D 0 ll
       5:       r1 =3D r6
       6:       r2 =3D r7
       7:       r3 =3D 0
       8:       call 12
       9:       r0 =3D *(u32 *)(r10 - 4) <-- this must stay
      10:       exit

You could take a shot at replacing it with inline asm, if you want.

[1] https://docs.cilium.io/en/stable/bpf/architecture/#bpf-to-bpf-calls
[2] https://blog.cloudflare.com/assembly-within-bpf-tail-calls-on-x86-and-a=
rm/