On 14/02/2023 22:12, Jiri Olsa wrote: > On Tue, Feb 14, 2023 at 01:17:56PM -0300, Arnaldo Carvalho de Melo wrote: >> Em Tue, Feb 14, 2023 at 01:27:43PM +0100, Jiri Olsa escreveu: >>> On Mon, Feb 13, 2023 at 07:12:33PM -0800, Alexei Starovoitov wrote: >>>> On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote: >>>>> >>>>> v1.25 of pahole supports filtering out functions with multiple >>>>> inconsistent function prototypes or optimized-out parameters >>>>> from the BTF representation. These present problems because >>>>> there is no additional info in BTF saying which inconsistent >>>>> prototype matches which function instance to help guide >>>>> attachment, and functions with optimized-out parameters can >>>>> lead to incorrect assumptions about register contents. >>>>> >>>>> So for now, filter out such functions while adding BTF >>>>> representations for functions that have "."-suffixes >>>>> (foo.isra.0) but not optimized-out parameters. >>>>> >>>>> This patch assumes changes in [1] land and pahole is bumped >>>>> to v1.25. >>>>> >>>>> [1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@xxxxxxxxxx/ >>>>> >>>>> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx> >>>>> Tested-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> >>>>> >>>>> --- >>>>> scripts/pahole-flags.sh | 3 +++ >>>>> 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh >>>>> index 1f1f1d3..728d551 100755 >>>>> --- a/scripts/pahole-flags.sh >>>>> +++ b/scripts/pahole-flags.sh >>>>> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then >>>>> # see PAHOLE_HAS_LANG_EXCLUDE >>>>> extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" >>>>> fi >>>>> +if [ "${pahole_ver}" -ge "125" ]; then >>>>> + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" >>>>> +fi >>>> >>>> We landed this too soon. >>>> #229 tracing_struct:FAIL >>>> is failing now. >>>> since bpf_testmod.ko is missing a bunch of functions though they're global. >>>> >>> >>> hum, didn't see this one failing.. I'll try that again >> >> /me too, redoing tests her, with gcc and clang, running selftests on a >> system booted with a kernel built with pahole 1.25, etc. > > ok, can't see that with gcc, but reproduced with clang 16 > > resolve_btfids complains because those functions are not in btf > > BTFIDS vmlinux > WARN: resolve_btfids: unresolved symbol tcp_reno_cong_avoid > WARN: resolve_btfids: unresolved symbol should_failslab > WARN: resolve_btfids: unresolved symbol should_fail_alloc_page > WARN: resolve_btfids: unresolved symbol cubictcp_cong_avoid > WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_timestamp > WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash > WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get > WARN: resolve_btfids: unresolved symbol bpf_task_acquire_not_zero > WARN: resolve_btfids: unresolved symbol bpf_rdonly_cast > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_static_unused_arg > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_ref > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass_ctx > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass2 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass1 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_pass1 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_fail2 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_fail1 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_kptr_get > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail3 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail2 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail1 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_acquire > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test2 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test1 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_memb_release > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_memb1_release > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_int_mem_release > NM System.map > Jiri kindly provided a clang-generated vmlinux, and I also managed to reproduce issues by building the kernel with clang 17. The first question is if we're detecting optimizations correctly. From an initial look, I _think_ we are, in some cases at least. For tcp_reno_cong_avoid(), the function signature is __bpf_kfunc void tcp_reno_cong_avoid(struct sock *sk, u32 ack, u32 acked) ...but our handling of the DWARF generated spots that the "ack" parameter has no location info; and looking at the source it is never used. Here is the DWARF - note no location info for the second parameter ("ack"): 0x0891dab0: DW_TAG_subprogram DW_AT_low_pc (0xffffffff82031180) DW_AT_high_pc (0xffffffff820311d8) DW_AT_frame_base (DW_OP_reg7 RSP) DW_AT_call_all_calls (true) DW_AT_name ("tcp_reno_cong_avoid") DW_AT_decl_file ("/home/jolsa/kernel/linux-qemu/net/ipv4/tcp_cong.c") DW_AT_decl_line (446) DW_AT_prototyped (true) DW_AT_external (true) 0x0891dabe: DW_TAG_formal_parameter DW_AT_location (indexed (0x7a) loclist = 0x00f50eb1: [0xffffffff82031185, 0xffffffff8203119e): DW_OP_reg5 RDI [0xffffffff8203119e, 0xffffffff820311cc): DW_OP_reg3 RBX [0xffffffff820311cc, 0xffffffff820311d1): DW_OP_reg5 RDI [0xffffffff820311d1, 0xffffffff820311d2): DW_OP_reg3 RBX [0xffffffff820311d2, 0xffffffff820311d8): DW_OP_entry_value(DW_OP_reg5 RDI), DW_OP_stack_value) DW_AT_name ("sk") DW_AT_decl_file ("/home/jolsa/kernel/linux-qemu/net/ipv4/tcp_cong.c") DW_AT_decl_line (446) DW_AT_type (0x08902c4d "sock *") 0x0891dac9: DW_TAG_formal_parameter DW_AT_name ("ack") DW_AT_decl_file ("/home/jolsa/kernel/linux-qemu/net/ipv4/tcp_cong.c") DW_AT_decl_line (446) DW_AT_type (0x08902c3d "u32") 0x0891dad4: DW_TAG_formal_parameter DW_AT_location (indexed (0x7b) loclist = 0x00f50eda: [0xffffffff82031185, 0xffffffff820311bc): DW_OP_reg1 RDX [0xffffffff820311bc, 0xffffffff820311c8): DW_OP_reg0 RAX [0xffffffff820311c8, 0xffffffff820311d1): DW_OP_reg1 RDX) DW_AT_name ("acked") DW_AT_decl_file ("/home/jolsa/kernel/linux-qemu/net/ipv4/tcp_cong.c") DW_AT_decl_line (446) DW_AT_type (0x08902c3d "u32") Disassembling we see the following: (gdb) disassemble/s tcp_reno_cong_avoid Dump of assembler code for function tcp_reno_cong_avoid: net/ipv4/tcp_cong.c: 447 { 0xffffffff82031180 <+0>: nopl 0x0(%rax,%rax,1) 0xffffffff82031185 <+5>: push %rbx 0xffffffff82031186 <+6>: mov %rdi,%rbx ./include/net/tcp.h: 1305 if (tp->is_cwnd_limited) 0xffffffff82031189 <+9>: cmpb $0x0,0x95f(%rdi) 0xffffffff82031190 <+16>: mov 0x9e4(%rdi),%eax 0xffffffff82031196 <+22>: mov 0x9e8(%rdi),%esi 0xffffffff8203119c <+28>: js 0xffffffff820311ae <tcp_reno_cong_avoid+46> 1238 return tcp_snd_cwnd(tp) < tp->snd_ssthresh; 0xffffffff8203119e <+30>: cmp %eax,%esi 1306 return true; 1307 1308 /* If in slow start, ensure cwnd grows to twice what was ACKed. */ 1309 if (tcp_in_slow_start(tp)) 0xffffffff820311a0 <+32>: jae 0xffffffff820311d1 <tcp_reno_cong_avoid+81> 1310 return tcp_snd_cwnd(tp) < 2 * tp->max_packets_out; 0xffffffff820311a2 <+34>: mov 0x9b4(%rbx),%ecx 0xffffffff820311a8 <+40>: add %ecx,%ecx 0xffffffff820311aa <+42>: cmp %ecx,%esi net/ipv4/tcp_cong.c: 450 if (!tcp_is_cwnd_limited(sk)) 0xffffffff820311ac <+44>: jae 0xffffffff820311d1 <tcp_reno_cong_avoid+81> ./include/net/tcp.h: 1238 return tcp_snd_cwnd(tp) < tp->snd_ssthresh; 0xffffffff820311ae <+46>: cmp %eax,%esi net/ipv4/tcp_cong.c: 454 if (tcp_in_slow_start(tp)) { 0xffffffff820311b0 <+48>: jae 0xffffffff820311c8 <tcp_reno_cong_avoid+72> 455 acked = tcp_slow_start(tp, acked); 0xffffffff820311b2 <+50>: mov %rbx,%rdi 0xffffffff820311b5 <+53>: mov %edx,%esi 0xffffffff820311b7 <+55>: call 0xffffffff82031080 <tcp_slow_start> --Type <RET> for more, q to quit, c to continue without paging-- 456 if (!acked) 0xffffffff820311bc <+60>: test %eax,%eax 0xffffffff820311be <+62>: je 0xffffffff820311d1 <tcp_reno_cong_avoid+81> 0xffffffff820311c0 <+64>: mov %eax,%edx ./include/net/tcp.h: 1227 return tp->snd_cwnd; 0xffffffff820311c2 <+66>: mov 0x9e8(%rbx),%esi net/ipv4/tcp_cong.c: 460 tcp_cong_avoid_ai(tp, tcp_snd_cwnd(tp), acked); 0xffffffff820311c8 <+72>: mov %rbx,%rdi 0xffffffff820311cb <+75>: pop %rbx 0xffffffff820311cc <+76>: jmp 0xffffffff820310d0 <tcp_cong_avoid_ai> 461 } 0xffffffff820311d1 <+81>: pop %rbx 0xffffffff820311d2 <+82>: cs jmp 0xffffffff8223c240 <__x86_return_thunk> End of assembler dump. >From what I can see above - unless I'm misreading it - we see something interesting here. Note that in preparing the call to tcp_cong_avoid_ai(), we get the tcp_snd_cwnd() value into %esi, but nothing needs to be done with %rdx because it's already got the "acked" value in it. Now this is good news, because if we're calling this kfunc - that only uses the first and third parameters - preparing all three will not lead to any nasty surprises (we just wasted a bit of time preparing the second unused parameter). If this held true for all kfuncs it would mean that skipping representing them in BTF due to optimized-out parameters would be the wrong answer. The key thing to figure out is this - if we optimize out a parameter, will the subsequent parameters that are not optimized out still use the registers that they would be expected to if no optimization had happened? So if I optimize out the first parameter say, will the second parameter use the "right" register (%rsi on x86_64)? If that were guaranteed, we could relax the cases where we skip BTF generation to the following: 1. cases where multiple inconsistent function prototypes for the same name exist. 2. cases where a function has multiple instances with different optimization states (optimized out parameter in one CU but not another). This is another instance of 1 really, and shouldn't be an issue for kfuncs. 3. cases where an optimized-out parameter has knock-on effects for the registers used to handle other unoptimized parameters such that assumptions we would make from the function signature are violated for non-optimized out parameters. So the parameter would be flagged as using an unexpected register, and that unexpected case would instead lead to skipping BTF encoding. I can have a go at implementing the above in pahole and seeing how it effects our list of functions. Note though that what's good for kfuncs isn't necessarily good for tracing accuracy; if assumptions I make about parameter presence are violated, I will see strange trace results based upon reading the code, whereas if I am preparing kfunc parameters, a bit of extra work is done preparing an unused parameter, but no harm is done. For the tracing case, function annotations flagging optimized-out parameters via BTF tags could help. However, none of the above will help problematic cases like bpf_xdp_metadata_rx_timestamp() or bpf_xdp_metadata_rx_hash(); again we see missing location info in their DWARF representations 0x07449011: DW_TAG_subprogram DW_AT_low_pc (0xffffffff81ec8c80) DW_AT_high_pc (0xffffffff81ec8c90) DW_AT_frame_base (DW_OP_reg7 RSP) DW_AT_call_all_calls (true) DW_AT_name ("bpf_xdp_metadata_rx_timestamp") DW_AT_decl_file ("/home/jolsa/kernel/linux-qemu/net/core/xdp.c") DW_AT_decl_line (725) DW_AT_prototyped (true) DW_AT_type (0x0742f1ae "int") DW_AT_external (true) 0x07449023: DW_TAG_formal_parameter DW_AT_name ("ctx") DW_AT_decl_file ("/home/jolsa/kernel/linux-qemu/net/core/xdp.c") DW_AT_decl_line (725) DW_AT_type (0x0743dff0 "const xdp_md *") 0x0744902d: DW_TAG_formal_parameter DW_AT_name ("timestamp") DW_AT_decl_file ("/home/jolsa/kernel/linux-qemu/net/core/xdp.c") DW_AT_decl_line (725) DW_AT_type (0x0743217a "u64 *") ...due to the function just being a "return -EOPNOTSUPP;": Dump of assembler code for function bpf_xdp_metadata_rx_timestamp: 0xffffffff81ec8c80 <+0>: nopl 0x0(%rax,%rax,1) 0xffffffff81ec8c85 <+5>: mov $0xffffffa1,%eax 0xffffffff81ec8c8a <+10>: cs jmp 0xffffffff8223c240 <__x86_return_thunk> End of assembler dump. __bpf_kfunc int bpf_xdp_metadata_rx_timestamp(const struct xdp_md *ctx, u64 *timestamp) { return -EOPNOTSUPP; } ...and playing around with various attributes here does not seem to help. should_failslab() has a similar story, I suspect because __should_failslab() got optimized out due to CONFIG_FAILSLAB=n and we get (gdb) disassemble/s should_failslab Dump of assembler code for function should_failslab: mm/slab_common.c: 1462 { 0xffffffff81422490 <+0>: nopl 0x0(%rax,%rax,1) 1463 if (__should_failslab(s, gfpflags)) 1464 return -ENOMEM; 1465 return 0; 1466 } 0xffffffff81422495 <+5>: xor %eax,%eax 0xffffffff81422497 <+7>: cs jmp 0xffffffff8223c240 <__x86_return_thunk> End of assembler dump. However, the caller of this function still prepares parameters as if they were going to be used: (gdb) disassemble slab_pre_alloc_hook Dump of assembler code for function slab_pre_alloc_hook: 0xffffffff813bc5b0 <+0>: push %rbp 0xffffffff813bc5b1 <+1>: mov %rsp,%rbp 0xffffffff813bc5b4 <+4>: push %r15 0xffffffff813bc5b6 <+6>: push %r14 0xffffffff813bc5b8 <+8>: push %r13 0xffffffff813bc5ba <+10>: push %r12 0xffffffff813bc5bc <+12>: push %rbx 0xffffffff813bc5bd <+13>: sub $0x20,%rsp 0xffffffff813bc5c1 <+17>: mov %r8d,%r15d 0xffffffff813bc5c4 <+20>: mov %rcx,%r12 0xffffffff813bc5c7 <+23>: mov %rdx,-0x48(%rbp) 0xffffffff813bc5cb <+27>: mov %rsi,%rbx 0xffffffff813bc5ce <+30>: mov %rdi,%r13 0xffffffff813bc5d1 <+33>: and 0x219ff00(%rip),%r15d # 0xffffffff8355c4d8 <gfp_allowed_mask> 0xffffffff813bc5d8 <+40>: test $0x400,%r15d 0xffffffff813bc5df <+47>: je 0xffffffff813bc5e6 <slab_pre_alloc_hook+54> 0xffffffff813bc5e1 <+49>: callq 0xffffffff81d27b68 <__SCT__might_resched> 0xffffffff813bc5e6 <+54>: mov %r13,%rdi 0xffffffff813bc5e9 <+57>: mov %r15d,%esi 0xffffffff813bc5ec <+60>: callq 0xffffffff81340a70 <should_failslab> ...so the function is still being called with the right register values. This points to a conceptual flaw in the approach I was taking - we cannot equate register _state_ on entry to a function with register _use_ by that function. So from a DWARF perspective, the fact that there is no location information does not necessarily tell us the function is not _called_ with that parameter; rather it tells us it is not used within the body of the function. This all combines to suggest that the only time we should be definitive in rejecting a function for BTF encoding due to optimizations is when they interfere with the expectations we have about _used_ parameter->register mappings. So concretely, where the second parameter uses a register other than the one the calling conventions dictate should be used by the second parameter say, or indeed does not use a register at all. It is possible that we could be more definitive by examining DWARF call site info, but there is none for some of the above functions like should_failslab(), so that will not help here as far as I can see. Does this seem reasonable, or am I missing something here? I'm worried we're going to end up playing whack-a-mole with increasingly clever compiler optimizations, so my instinct is that we need to be conservative in choosing when to skip BTF encoding, only doing so when we are certain it will mislead badly. I _think_ there is some justification to that based on the above. What do you think? Alan
