[PATCH bpf-next v3 0/2] bpf, x64: Introduce two tailcall enhancements

[PATCH bpf-next v3 0/2] bpf, x64: Introduce two tailcall enhancements

[Leon Hwang]

This patch set introduces two enhancements aimed at improving tailcall
handling in the x64 JIT:

1. Tailcall info is propagated only for subprogs.
2. Tailcall info is propagated through the trampoline only when the target
   is a subprog and it is tail_call_reachable.

Changes:
v2 -> v3:
  * Add Yonghong's ACK.

v1 -> v2:
  * Address comment from Alexei:
    * Rather live with tail call inefficiency than abuse insns fields
      further.

Leon Hwang (2):
  bpf, x64: Propagate tailcall info only for subprogs
  bpf, verifier: Check trampoline target is tail_call_reachable subprog

 arch/x86/net/bpf_jit_comp.c | 3 ++-
 include/linux/bpf.h         | 1 +
 kernel/bpf/verifier.c       | 4 +++-
 3 files changed, 6 insertions(+), 2 deletions(-)

-- 
2.44.0

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

[Leon Hwang]

In x64 JIT, propagate tailcall info only for subprogs, not for helpers
or kfuncs.

Acked-by: Yonghong Song <yonghong.song@linux.dev>
Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
---
 arch/x86/net/bpf_jit_comp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 06b080b61aa57..eb08cc6d66401 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -2124,10 +2124,11 @@ st:			if (is_imm8(insn->off))
 
 			/* call */
 		case BPF_JMP | BPF_CALL: {
+			bool pseudo_call = src_reg == BPF_PSEUDO_CALL;
 			u8 *ip = image + addrs[i - 1];
 
 			func = (u8 *) __bpf_call_base + imm32;
-			if (tail_call_reachable) {
+			if (pseudo_call && tail_call_reachable) {
 				LOAD_TAIL_CALL_CNT_PTR(bpf_prog->aux->stack_depth);
 				ip += 7;
 			}
-- 
2.44.0

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

[Alexei Starovoitov]

On Thu, Nov 7, 2024 at 5:46 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>
> In x64 JIT, propagate tailcall info only for subprogs, not for helpers
> or kfuncs.
>
> Acked-by: Yonghong Song <yonghong.song@linux.dev>
> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
> ---
>  arch/x86/net/bpf_jit_comp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 06b080b61aa57..eb08cc6d66401 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -2124,10 +2124,11 @@ st:                     if (is_imm8(insn->off))
>
>                         /* call */
>                 case BPF_JMP | BPF_CALL: {
> +                       bool pseudo_call = src_reg == BPF_PSEUDO_CALL;
>                         u8 *ip = image + addrs[i - 1];
>
>                         func = (u8 *) __bpf_call_base + imm32;
> -                       if (tail_call_reachable) {
> +                       if (pseudo_call && tail_call_reachable) {
>                                 LOAD_TAIL_CALL_CNT_PTR(bpf_prog->aux->stack_depth);
>                                 ip += 7;
>                         }

I've applied this patch with this tweak:
if (src_reg == BPF_PSEUDO_CALL && tail_call_reachable)

I don't see much value in patch 2.
The tail_call feature is an old approach. It is now causing
maintenance issues with other features.
I'd rather not touch anything tail call related.
So I dropped patch 2.

I'd like to see proper indirect goto and indirect call
support being developed further.
Anton started working on it, but dropped the ball.
We need to commandeer the patches.

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

[Leon Hwang]

On 13/11/24 09:31, Alexei Starovoitov wrote:
> On Thu, Nov 7, 2024 at 5:46 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>>
>> In x64 JIT, propagate tailcall info only for subprogs, not for helpers
>> or kfuncs.
>>
>> Acked-by: Yonghong Song <yonghong.song@linux.dev>
>> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
>> ---
>>  arch/x86/net/bpf_jit_comp.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>> index 06b080b61aa57..eb08cc6d66401 100644
>> --- a/arch/x86/net/bpf_jit_comp.c
>> +++ b/arch/x86/net/bpf_jit_comp.c
>> @@ -2124,10 +2124,11 @@ st:                     if (is_imm8(insn->off))
>>
>>                         /* call */
>>                 case BPF_JMP | BPF_CALL: {
>> +                       bool pseudo_call = src_reg == BPF_PSEUDO_CALL;
>>                         u8 *ip = image + addrs[i - 1];
>>
>>                         func = (u8 *) __bpf_call_base + imm32;
>> -                       if (tail_call_reachable) {
>> +                       if (pseudo_call && tail_call_reachable) {
>>                                 LOAD_TAIL_CALL_CNT_PTR(bpf_prog->aux->stack_depth);
>>                                 ip += 7;
>>                         }
> 
> I've applied this patch with this tweak:
> if (src_reg == BPF_PSEUDO_CALL && tail_call_reachable)
> 
> I don't see much value in patch 2.
> The tail_call feature is an old approach. It is now causing
> maintenance issues with other features.
> I'd rather not touch anything tail call related.
> So I dropped patch 2.
> 
> I'd like to see proper indirect goto and indirect call
> support being developed further.
> Anton started working on it, but dropped the ball.
> We need to commandeer the patches.

Great to see jmp table supporting tail call.

Thanks,
Leon

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

[Anton Protopopov]

On 24/11/12 05:31PM, Alexei Starovoitov wrote:
> On Thu, Nov 7, 2024 at 5:46 AM Leon Hwang <leon.hwang@linux.dev> wrote:
> >
> > In x64 JIT, propagate tailcall info only for subprogs, not for helpers
> > or kfuncs.
> >
> > Acked-by: Yonghong Song <yonghong.song@linux.dev>
> > Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
> > ---
> >  arch/x86/net/bpf_jit_comp.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > index 06b080b61aa57..eb08cc6d66401 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -2124,10 +2124,11 @@ st:                     if (is_imm8(insn->off))
> >
> >                         /* call */
> >                 case BPF_JMP | BPF_CALL: {
> > +                       bool pseudo_call = src_reg == BPF_PSEUDO_CALL;
> >                         u8 *ip = image + addrs[i - 1];
> >
> >                         func = (u8 *) __bpf_call_base + imm32;
> > -                       if (tail_call_reachable) {
> > +                       if (pseudo_call && tail_call_reachable) {
> >                                 LOAD_TAIL_CALL_CNT_PTR(bpf_prog->aux->stack_depth);
> >                                 ip += 7;
> >                         }
> 
> I've applied this patch with this tweak:
> if (src_reg == BPF_PSEUDO_CALL && tail_call_reachable)
> 
> I don't see much value in patch 2.
> The tail_call feature is an old approach. It is now causing
> maintenance issues with other features.
> I'd rather not touch anything tail call related.
> So I dropped patch 2.
> 
> I'd like to see proper indirect goto and indirect call
> support being developed further.
> Anton started working on it, but dropped the ball.
> We need to commandeer the patches.

Actually, I am still on it. I will start sending patches this week.

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

[Alexei Starovoitov]

On Tue, Nov 12, 2024 at 6:11 PM Anton Protopopov <aspsk@isovalent.com> wrote:
>
> Actually, I am still on it. I will start sending patches this week.

Ohh. Great. Cannot wait.

[PATCH bpf-next v3 2/2] bpf, verifier: Check trampoline target is tail_call_reachable subprog

[Leon Hwang]

In the x86_64 JIT, tailcall info is propagated through the trampoline when
the target program is tail_call_reachable. However, this propagation is
unnecessary if the target is a main prog, or a subprog that is not
tail_call_reachable.

Since the verifier can determine if a subprog is tail_call_reachable, it
should only propagate tailcall info when the target is subprog and the
subprog is actually tail_call_reachable.

Acked-by: Yonghong Song <yonghong.song@linux.dev>
Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
---
 include/linux/bpf.h   | 1 +
 kernel/bpf/verifier.c | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 1b84613b10ac7..1e5b439ab4c7d 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1253,6 +1253,7 @@ struct bpf_attach_target_info {
 	struct module *tgt_mod;
 	const char *tgt_name;
 	const struct btf_type *tgt_type;
+	bool tgt_tail_call_reachable;
 };
 
 #define BPF_DISPATCHER_MAX 48 /* Fits in 2048B */
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 7958d6ff6b739..e5b9754a2f0be 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -22004,6 +22004,8 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 			bpf_log(log, "Subprog %s doesn't exist\n", tname);
 			return -EINVAL;
 		}
+		tgt_info->tgt_tail_call_reachable = subprog &&
+						    aux->func[subprog]->aux->tail_call_reachable;
 		if (aux->func && aux->func[subprog]->aux->exception_cb) {
 			bpf_log(log,
 				"%s programs cannot attach to exception callback\n",
@@ -22373,7 +22375,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 	if (!tr)
 		return -ENOMEM;
 
-	if (tgt_prog && tgt_prog->aux->tail_call_reachable)
+	if (tgt_prog && tgt_info.tgt_tail_call_reachable)
 		tr->flags = BPF_TRAMP_F_TAIL_CALL_CTX;
 
 	prog->aux->dst_trampoline = tr;
-- 
2.44.0

Re: [PATCH bpf-next v3 0/2] bpf, x64: Introduce two tailcall enhancements

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Thu,  7 Nov 2024 21:45:27 +0800 you wrote:
> This patch set introduces two enhancements aimed at improving tailcall
> handling in the x64 JIT:
> 
> 1. Tailcall info is propagated only for subprogs.
> 2. Tailcall info is propagated through the trampoline only when the target
>    is a subprog and it is tail_call_reachable.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v3,1/2] bpf, x64: Propagate tailcall info only for subprogs
    https://git.kernel.org/bpf/bpf-next/c/a1087da9d11e
  - [bpf-next,v3,2/2] bpf, verifier: Check trampoline target is tail_call_reachable subprog
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html