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

Commit Message

Leon Hwang Oct. 21, 2024, 1:39 p.m. UTC

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 <[e-mail]>
---
 arch/x86/net/bpf_jit_comp.c | 4 +++-
 kernel/bpf/verifier.c       | 6 ++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

Comments

Yonghong Song Oct. 21, 2024, 5:49 p.m. UTC | #1

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 <[e-mail]>
> ---
>   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?

>   				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;
>   	}
>   
>   	err = bpf_prog_alloc_jited_linfo(prog);

Leon Hwang Oct. 22, 2024, 1:46 a.m. UTC | #2

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 <[e-mail]>
>> ---
>>   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.

Thanks,
Leon

Patch

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;
 	}
 
 	err = bpf_prog_alloc_jited_linfo(prog);