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/23/24 8:33 PM, Leon Hwang wrote:
>
> On 24/10/24 10:29, Yonghong Song wrote:
>> 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?
>>   
> Sure, here's an example:
>
> struct {
> 	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
> 	__uint(key_size, sizeof(u32));
> 	__uint(value_size, sizeof(u32));
> 	__uint(max_entries, 1);
> } jmp_table SEC(".maps");
>
> static __noinline int
> subprog_tc1(struct __sk_buff *skb)
> {
> 	volatile int retval = TC_ACT_OK;
>
> 	bpf_tail_call_static(skb, jmp_table, 0);
> 	return retval;
> }
>
> static __noinline int
> subprog_tc2(struct __sk_buff *skb)
> {
> 	volatile int retval = TC_ACT_OK;
>
> 	return retval;
> }
>
> SEC("tc")
> int entry_tc(struct __sk_buff *skb)
> {
> 	u32 pid = bpf_get_smp_processor_id();
> 	// do something with pid
> 	subprog_tc2(skb);
> 	return subprog_tc1(skb);
> }
>
>  From the verifier's perspective, both entry_tc and subprog_tc1 are
> tail_call_reachable.
>
> When handling 'BPF_JMP | BPF_CALL' in the x86 do_jit() for entry_tc,
> three cases arise:
>
> 1. bpf_get_smp_processor_id()
> 2. subprog_tc1()
> 3. subprog_tc2()
>
> At this point in x86 do_jit() for entry_tc, entry_tc is considered
> tail_call_reachable. The check 'bool pseudo_call = src_reg ==
> BPF_PSEUDO_CALL' is used to determine whether to call a subprogram.
>
> The question is: when should tailcall info be propagated? Should it be
> when entry_tc is tail_call_reachable, even if subprog_tc2 is called, or
> when subprog_tc1 is specifically tail_call_reachable?
>
> I believe it is better to propagate the tailcall info when subprog_tc1
> is tail_call_reachable.

Okay, I see. Thanks for explanation.

You use the insn->dst_reg to record whether callee is tail call
reachable or not. I think you can reuse insn->off which currently
represents subprog number but it is not used for jit. We can
use that to indicate callee is tail call reachable or not.

Something like below:

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 06b080b61aa5..b3c76bf59e65 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -2127,7 +2127,8 @@ st:                       if (is_imm8(insn->off))
                         u8 *ip = image + addrs[i - 1];
  
                         func = (u8 *) __bpf_call_base + imm32;
-                       if (tail_call_reachable) {
+                       /* insn->off == 1 means the callee is tail call reachable */
+                       if (src_reg == BPF_PSEUDO_CALL && insn->off == 1) {
                                 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 f514247ba8ba..2ccadc1ac22e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20096,6 +20096,8 @@ static int jit_subprogs(struct bpf_verifier_env *env)
                                 continue;
                         subprog = insn->off;
                         insn->imm = BPF_CALL_IMM(func[subprog]->bpf_func);
+                       /* Indicate whether callee is tail call reachable or not */
+                       insn->off = func[subprog]->aux->tail_call_reachable;
                 }

WDYT?

>
> Thanks,
> Leon
>