On 6/13/23 1:50 AM, [email protected] 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 <[email protected]>
> ---
> tools/testing/selftests/bpf/progs/tailcall_bpf2bpf6.c | 3 +--
> 1 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
> int subprog_tail(struct __sk_buff *skb)
> {
> /* Don't propagate the constant to the caller */
> - volatile int ret = 1;
>
> bpf_tail_call_static(skb, &jmp_table, 0);
> - return ret;
> + return 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
and 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.
> }
>
> SEC("tc")
On Tue, Jun 13, 2023 at 06:42 AM -07, Yonghong Song wrote:
> On 6/13/23 1:50 AM, [email protected] 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 <[email protected]>
>> ---
>> tools/testing/selftests/bpf/progs/tailcall_bpf2bpf6.c | 3 +--
>> 1 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
>> int subprog_tail(struct __sk_buff *skb)
>> {
>> /* Don't propagate the constant to the caller */
>> - volatile int ret = 1;
>> bpf_tail_call_static(skb, &jmp_table, 0);
>> - return ret;
>> + return 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 and 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 = r1
1: w1 = 1
2: *(u32 *)(r10 - 4) = r1
3: r7 = 0 ll
5: r1 = r6
6: r2 = r7
7: r3 = 0
8: call 12
9: r0 = *(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-arm/