On 10/24/24 9:38 AM, Yonghong Song wrote:
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?
Sorry, the above seems not working since verifier do jit twice for the same prog
and two jit'ed results need to be the same. The above change could make jit result
different between two passes.
Thanks,
Leon
