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

[Leon Hwang <leon.hwang@xxxxxxxxx>]

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@xxxxxxxxx>
>>>> ---
>>>>    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);
}