Re: [PATCH bpf-next 08/13] tracing: parse the vmlinux BTF when BTF-typed output is enabled

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 6408 bytes --]

> tracing: parse the vmlinux BTF when BTF-typed output is enabled

TL;DR:
- irqsoff/wakeup tracers may print function lines without arguments, since
  they never call trace_btf_prepare() and the lazy BTF may be unparsed
- Author's Signed-off-by is a TODO placeholder

>     tracing: parse the vmlinux BTF when BTF-typed output is enabled
>
>     func-args of the function tracer, and funcgraph-args and
>     funcgraph-retval of the function_graph tracer, print arguments and
>     return values by their BTF types: print_function_args() and
>     btf_trim_retval() look the function up with btf_find_func_proto(),
>     which parses the vmlinux BTF if nobody has yet.  Since commit
>     42643ba35deb ("tracing: Avoid BTF lookup in atomic context") they do not
>     do that in atomic context, but they still can in the readers of the
>     trace: s_show() and tracing_read_pipe() print under trace_event_sem and
>     the trace access lock, while the tracer keeps recording, by default
>     into the buffer being read.
>
>     Today the BTF is always parsed at boot, so this cannot happen.  To keep
>     it that way once the vmlinux BTF can be parsed on first use
>     (CONFIG_DEBUG_INFO_BTF_LAZY, later in this series), parse it when such
>     output is enabled instead: trace_btf_prepare() is called from
>     select_trace_function() when func-args is set, from graph_trace_init()
>     when funcgraph-args or funcgraph-retval is set, and from
>     func_graph_set_flag() when either is turned on, all with
>     trace_types_lock held.  The two printers only peek at the BTF and fall
>     back to the output without it if it is not parsed, which then cannot
>     happen while the option is in effect.
>
>     "trace_options=" on the command line sets the options from
>     early_trace_init() in start_kernel(), with interrupts disabled and
>     before btf_alloc_id() can allocate; while the system is still booting
>     (system_state < SYSTEM_SCHEDULING) trace_btf_prepare() only records the
>     request, and a core_initcall parses the BTF.
>
>     Reading an event's btf_ids file looks the BTF up under event_mutex;
>     parse it before taking the mutex, as the CO-RE candidate lookup does
>     before cand_cache_mutex.
>
>     No functional change with today's eager parse, except that with such
>     an option on the command line the BTF is parsed at core_initcall time,
>     before the initcalls that would otherwise parse it.
>
>     TODO-NEEDS-IHOR: Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
>     [moved trace_func_args_prepare() to trace_btf.c as trace_btf_prepare();
>      extended it to funcgraph-retval and the btf_ids file; the printers check
>      the BTF with bpf_peek_btf_vmlinux() in front of btf_find_func_proto()]
>     Signed-off-by: Jay Wang <wanjay@amazon.com>

The patch is authored by Ihor Solodrai, but the only Signed-off-by in the
trailer block is Jay Wang's.  Is this line meant to be the author's
sign-off?

    TODO-NEEDS-IHOR: Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>

With the "TODO-NEEDS-IHOR:" prefix it is not a Signed-off-by tag, so the
patch carries no sign-off from its author, which
Documentation/process/submitting-patches.rst requires.  The placeholder
text would also end up in the changelog as is.

Should the prefix be dropped before this is applied?

> diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
> index a5ad76175d10..cdb926c9d4e2 100644
> --- a/kernel/trace/trace_output.c
> +++ b/kernel/trace/trace_output.c

[ ... ]

> @@ -739,6 +739,9 @@ void print_function_args(struct trace_seq *s, unsigned long *args,
>  	if (lookup_symbol_name(func, name))
>  		goto out;
>  
> +	/* see trace_btf_prepare() */
> +	if (IS_ERR_OR_NULL(bpf_peek_btf_vmlinux()))
> +		goto out;
>  	/* TODO: Pass module name here too */
>  	t = btf_find_func_proto(name, &btf);
>  	if (IS_ERR_OR_NULL(t))

Are func-args, funcgraph-args and funcgraph-retval the only producers of
entries that carry arguments?

Since commit 76fe0337c219 ("ftrace: Add arguments to function tracer"), the
latency tracers pass fregs straight to trace_function(), whatever the
func-args option says:

irqsoff_tracer_call() (kernel/trace/trace_irqsoff.c):
	trace_function(tr, ip, parent_ip, trace_ctx, fregs);

wakeup_tracer_call() (kernel/trace/trace_sched_wakeup.c):
	trace_function(tr, ip, parent_ip, trace_ctx, fregs);

trace_function() reserves room for FTRACE_REGS_MAX_ARGS arguments and
stores them whenever fregs is non-NULL.  On x86_64 and arm64, which are the
only architectures CONFIG_DEBUG_INFO_BTF_LAZY allows, ftrace_caller always
passes a non-NULL fregs.

When the irqsoff, preemptoff, preemptirqsoff, wakeup, wakeup_rt or
wakeup_dl tracer runs with function tracing on (the default, not in
display-graph mode), irqsoff_print_line()/wakeup_print_line() return
TRACE_TYPE_UNHANDLED.  The entry then goes through:

trace_fn_trace()
  print_fn_trace()
    print_function_args()

with args != NULL.

None of these tracers calls trace_btf_prepare(), and they have no func-args
option that would.  With CONFIG_DEBUG_INFO_BTF_LAZY (added later in this
series by 66c4a6a9293d, e46b5971bc91 and cae96a9f7395), if nothing else has
parsed the vmlinux BTF yet, the peek returns NULL and every function line
in these tracers prints as "func()" where it used to print
"func(a=..., b=0x...)".  Before this patch the reader would have parsed the
BTF on demand.

The same applies to a boot-mapped persistent instance that holds the
previous boot's func-args entries.  It is read-only and no tracer is
started on it, so nothing prepares the BTF.

The commit message says the fallback "then cannot happen while the option
is in effect", and the comment above trace_btf_prepare() lists only
func-args, funcgraph-args and funcgraph-retval.  Neither covers these
producers, and no later commit in 38e2751cdb40..4d32ca0c95ea touches
trace_irqsoff.c or trace_sched_wakeup.c or adds another
trace_btf_prepare() call.

Should the irqsoff/wakeup tracers call trace_btf_prepare() from their init
when function tracing records args, or should their trace_function() calls
pass NULL fregs unless args are wanted?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/41205212136