Re: [PATCH bpf-next 1/2] bpf, x64: Propagate tailcall info only for tail_call_reachable subprogs

[Yonghong Song <yonghong.song@linux.dev>]

On 10/21/24 6:46 PM, Leon Hwang wrote:
>
> On 22/10/24 01:49, Yonghong Song wrote:
>> On 10/21/24 6:39 AM, Leon Hwang wrote:
>>> In the x86_64 JIT, when calling a function, tailcall info is
>>> propagated if
>>> the program is tail_call_reachable, regardless of whether the function
>>> is a
>>> subprog, helper, or kfunc. However, this propagation is unnecessary for
>>> not-tail_call_reachable subprogs, helpers, or kfuncs.
>>>
>>> The verifier can determine if a subprog is tail_call_reachable.
>>> Therefore,
>>> it can be optimized to only propagate tailcall info when the callee is
>>> subprog and the subprog is actually tail_call_reachable.
>>>
>>> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
>>> ---
>>>    arch/x86/net/bpf_jit_comp.c | 4 +++-
>>>    kernel/bpf/verifier.c       | 6 ++++++
>>>    2 files changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>>> index 06b080b61aa57..6ad6886ecfc88 100644
>>> --- a/arch/x86/net/bpf_jit_comp.c
>>> +++ b/arch/x86/net/bpf_jit_comp.c
>>> @@ -2124,10 +2124,12 @@ st:            if (is_imm8(insn->off))
>>>                  /* call */
>>>            case BPF_JMP | BPF_CALL: {
>>> +            bool pseudo_call = src_reg == BPF_PSEUDO_CALL;
>>> +            bool subprog_tail_call_reachable = dst_reg;
>>>                u8 *ip = image + addrs[i - 1];
>>>                  func = (u8 *) __bpf_call_base + imm32;
>>> -            if (tail_call_reachable) {
>>> +            if (pseudo_call && subprog_tail_call_reachable) {
>> Why we need subprog_tail_call_reachable? Does
>>      tail_call_reachable && psueudo_call
>> work the same way?
>>
> 'tail_call_reachable && pseudo_call' works too. However, it will
> propagate tailcall info to subprog even if the subprog is not
> tail_call_reachable.
>
> subprog_tail_call_reachable indicates the subprog requires tailcall info
> from its caller.
> So, 'pseudo_call && subprog_tail_call_reachable' is better.

In verifier.c, we have
   func[i]->aux->tail_call_reachable = env->subprog_info[i].tail_call_reachable;
that is subprog_info tail_call_reachable has been transferred to func[i] tail_call_reachable.

In x86 do_jit() func, we have
   bool tail_call_reachable = bpf_prog->aux->tail_call_reachable

So looks like we do not need verifier.c change here.
Did I miss anything? Could you give a concrete example to show
subprog_tail_call_reachable approach is better than tail_call_reachable?
   

>
> Thanks,
> Leon
>