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