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

[Leon Hwang <leon.hwang@linux.dev>]

On 25/10/24 06:09, Alexei Starovoitov wrote:
> On Mon, Oct 21, 2024 at 6:39 AM Leon Hwang <leon.hwang@linux.dev> 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) {
>>                                 LOAD_TAIL_CALL_CNT_PTR(bpf_prog->aux->stack_depth);
>>                                 ip += 7;
>>                         }
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index f514247ba8ba8..6e7e42c7bc7b1 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -19990,6 +19990,12 @@ static int jit_subprogs(struct bpf_verifier_env *env)
>>                         insn[0].imm = (u32)addr;
>>                         insn[1].imm = addr >> 32;
>>                 }
>> +
>> +               if (bpf_pseudo_call(insn))
>> +                       /* In the x86_64 JIT, tailcall information can only be
>> +                        * propagated if the subprog is tail_call_reachable.
>> +                        */
>> +                       insn->dst_reg = env->subprog_info[subprog].tail_call_reachable;
> 
> I really don't like hacking flags into dst_reg.
> We already abuse insn->off which is ugly too,
> but at least we clean insns later after JIT.
> 
> I'd rather live with this tail call inefficiency than abuse insns
> fields further.
> 

OK, let us use 'pseudo_call && tail_call_reachable' in x86 JIT to avoid
touching 'insn->dst_reg'.

Thanks,
Leon

> pw-bot: cr