Re: [PATCH perf-tools-next v8 4/6] perf ftrace: Support display of inlined functions in function graph tracer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] perf ftrace: find_func_column() misses latency-format column in verbose mode
- [Medium] perf ftrace: select_tracer() overrides function_graph tracer with --inline
- [Medium] perf ftrace: idle tasks across CPUs share a single ftrace_task_state
- [Medium] perf ftrace: perf_ftrace__setup_inlines() ignores vmlinux load failures
- [Medium] perf test ftrace: test_inline_processing() hardcodes build-specific offset
--

commit a1b712dfdc11c697976e8ccf628bd22443d5d421
Author: Aaron Tomlin <atomlin@atomlin.com>

perf ftrace: Support display of inlined functions in function graph tracer

This commit introduces the --inline option to perf ftrace to resolve caller
return addresses via DWARF debug information in vmlinux and synthesize
inlined function frames into the function_graph call stream. It also adds per-
task call stack tracking, the -k/--vmlinux option, and unit tests in
tools/perf/tests/ftrace.c.

> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> index c715cbebaabe9..cdacf5af14a65 100644
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c

[ ... ]

> @@ -1973,6 +1991,20 @@ int cmd_ftrace(int argc, const char **argv)
>  
>  	switch (subcmd) {
>  	case PERF_FTRACE_TRACE:
> +		if (ftrace.use_inline) {
> +			if (ftrace.tracer && strcmp(ftrace.tracer, "function_graph") != 0) {
> +				pr_err("Error: --inline option is only supported with function_graph tracer\n");
> +				ret = -EINVAL;
> +				goto out_delete_filters;
> +			}
> +			ftrace.tracer = "function_graph";
> +		}

[Severity: Medium]
Can select_tracer() override ftrace.tracer back to "function" after this
check runs?

In cmd_ftrace(), ftrace.tracer defaults to DEFAULT_TRACER ("function_graph"),
so running perf ftrace -T <func> --inline (or -N <func> --inline) passes this
validation. However, when __cmd_ftrace() starts, it calls select_tracer():

tools/perf/builtin-ftrace.c:select_tracer() {
	bool graph = !list_empty(&ftrace->graph_funcs) ||
		     !list_empty(&ftrace->nograph_funcs);
	bool func = !list_empty(&ftrace->filters) ||
		    !list_empty(&ftrace->notrace);

	/* The function_graph has priority over function tracer. */
	if (graph)
		ftrace->tracer = "function_graph";
	else if (func)
		ftrace->tracer = "function";
    ...
}

Because filters or notrace is non-empty and graph_funcs/nograph_funcs are
empty, select_tracer() overwrites ftrace->tracer to "function", causing
__cmd_ftrace() to configure the function tracer while feeding its output into
ftrace_process_fgraph_line(). Conversely, if ftrace.tracer is set to
"function" in perfconfig and the user runs perf ftrace -G <func> --inline,
cmd_ftrace() rejects it before select_tracer() can select "function_graph".

Should the --inline tracer check or enforcement happen in integration with
select_tracer()?

> diff --git a/tools/perf/tests/ftrace.c b/tools/perf/tests/ftrace.c
> index 596cecee844e3..6e20a61860a9e 100644
> --- a/tools/perf/tests/ftrace.c
> +++ b/tools/perf/tests/ftrace.c
> @@ -62,6 +66,465 @@ static int test_filter_retaddr(void)
>  	return TEST_OK;
>  }
>  
> +#define TEST_CHECK(text, cond)					\
> +do {								\
> +	if (!(cond)) {						\
> +		pr_debug("FAILED %s:%d %s\n", __FILE__, __LINE__, text); \
> +		ret = TEST_FAIL;				\
> +		goto out_cleanup;				\
> +	}							\
> +} while (0)
> +
> +static int test_inline_processing(void)
> +{
> +	static const char line_entry[] = " 0)               |  wake_up_new_task() {";
> +	static const char line_enqueue[] =
> +		" 0)               |    enqueue_task() { /* <-wake_up_new_task+0x1d1/0x3e0 */";

[ ... ]

> +	vmlinux = vmlinux_path__find();
> +
> +	/* If vmlinux is present, test resolving symbols */
> +	if (vmlinux) {
> +		symbol_conf.vmlinux_name = vmlinux;
> +		symbol_conf.ignore_vmlinux_buildid = true;
> +		ret = perf_ftrace__setup_inlines(&ftrace);
> +		if (ret < 0 || !ftrace.machine) {
> +			ret = TEST_SKIP;
> +			goto out_cleanup;
> +		}
> +
> +		out = open_memstream(&out_buf, &out_len);
> +		if (!out) {
> +			ret = TEST_FAIL;
> +			goto out_cleanup;
> +		}
> +
> +		/* Feed simulated function graph trace lines */
> +		ftrace_process_fgraph_line(&ftrace, line_entry, out);
> +		ftrace_process_fgraph_line(&ftrace, line_enqueue, out);
> +		ftrace_process_fgraph_line(&ftrace, line_exit_enqueue, out);
> +		ftrace_process_fgraph_line(&ftrace, line_exit_task, out);
> +
> +		fclose(out);
> +		ftrace_cpu_states__clear();
> +
> +		pr_debug("Processed output:\n%s\n", out_buf);
> +
> +		/* Verify activate_task entry with inline hint */
> +		TEST_CHECK("contains activate_task entry with inline hint",
> +			   strstr(out_buf, "activate_task() { /* (inline) */") != NULL);

[Severity: Medium]
Will test_inline_processing() fail on systems where the host vmlinux does not
have an inlined activate_task() call specifically at wake_up_new_task+0x1d1?

When vmlinux_path__find() finds a host vmlinux file, the test feeds
line_enqueue with the hardcoded offset wake_up_new_task+0x1d1/0x3e0 and
requires DWARF resolution to produce "activate_task() { /* (inline) */". Because
the instruction offset +0x1d1 (and whether activate_task is inlined) depends
on the architecture, compiler, optimization flags, and kernel configuration,
as well as DWARF support in both vmlinux and the perf build, this check will
return TEST_FAIL on other kernel builds.

Also, symbol_conf.vmlinux_name and symbol_conf.ignore_vmlinux_buildid are
modified here without being restored in out_cleanup.

> diff --git a/tools/perf/util/ftrace.c b/tools/perf/util/ftrace.c
> index aaf108369c365..020551a4d7e00 100644
> --- a/tools/perf/util/ftrace.c
> +++ b/tools/perf/util/ftrace.c

[ ... ]

> +static bool parse_context_switch(const char *line)
> +{
> +	const char *arrow = strstr(line, " => ");
> +	const char *p;
> +	const char *next;
> +	const char *hyphen;
> +	char *endptr;
> +	long cpu_val;
> +	long pid_val;
> +
> +	if (!arrow)
> +		return false;
> +
> +	/* Find CPU number before ')' */
> +	p = skip_spaces(line);
> +	cpu_val = strtol(p, &endptr, 10);
> +	if (endptr == p || *endptr != ')')
> +		return false;
> +
> +	if (cpu_val < 0 || cpu_val >= FTRACE_MAX_CPUS)
> +		return false;
> +
> +	/* Parse the target task's PID after the arrow (" => comm-pid") */
> +	next = skip_spaces(arrow + 4);
> +	hyphen = strrchr(next, '-');
> +	if (!hyphen)
> +		return false;
> +
> +	pid_val = strtol(hyphen + 1, &endptr, 10);
> +	if (endptr == hyphen + 1)
> +		return false;
> +
> +	ftrace_cpu_tasks[cpu_val] = get_task_state((pid_t)pid_val);
> +	return true;
> +}

[Severity: Medium]
What happens in system-wide tracing when multiple CPUs switch to the idle
task (<idle>-0)?

Each CPU has its own swapper thread, all with PID 0, which ftrace formats as
"<idle>-0" in context-switch banners and in the verbose comm-pid column. When
parse_context_switch() or find_func_column() sees "<idle>-0", pid_val is 0 and
get_task_state(0) returns the same struct ftrace_task_state instance for all
idle CPUs.

If multiple CPUs are executing in idle simultaneously, won't their interleaved
trace lines share and corrupt the same stack, depth, and inlined_depth? (Note
that get_cpu_state() uses -1 - cpu before a banner is seen; should pid_val == 0
similarly map to a per-CPU state such as -1 - cpu_val?)

[ ... ]

> +static void make_blank_prefix(const struct perf_ftrace *ftrace, const char *line,
> +			      const char *pipe_char, char *buf, size_t size)
> +{
> +	size_t len = pipe_char - line + 1;
> +	const char *start_delim = NULL;
> +	char *c;
> +
> +	if (!size)
> +		return;
> +
> +	if (len >= size)
> +		len = size - 1;
> +
> +	memcpy(buf, line, len);
> +	buf[len] = '\0';
> +
> +	if (*pipe_char == '|') {
> +		if (ftrace->graph_verbose) {
> +			/* In verbose mode, duration starts after the comm-pid '|' */
> +			for (const char *p = pipe_char - 1; p >= line; p--) {
> +				if (*p == '|') {
> +					start_delim = p;
> +					break;
> +				}
> +			}
> +		} else {
> +			/* In default mode, duration starts after CPU ')' */
> +			start_delim = strchr(line, ')');
> +		}
> +
> +		if (start_delim && start_delim < pipe_char && start_delim < line + len) {
> +			char *end = buf + (pipe_char - line);
> +
> +			if (end > buf + len)
> +				end = buf + len;
> +			for (c = buf + (start_delim - line) + 1; c < end; c++)
> +				*c = ' ';
> +		}
> +	}
> +}
> +
> +/**
> + * find_func_column - Locate the function call column in a trace record
> + * @ftrace: Perf ftrace context
> + * @line: The trace record to parse
> + * @cpu: Pointer to store the CPU number
> + *
> + * Locate the context columns that precede the function call column of a
> + * function_graph trace record. Depending on the funcgraph-* options set
> + * up by perf, a record looks like:
> + *
> + *   [ abstime |]  CPU)  [ comm-pid |]  [ duration |]  func
> + *
> + * The abstime and comm-pid columns are only present with --graph-opts
> + * verbose.
> + *
> + * Return: A pointer to the last character of the context columns (the final
> + * '|' delimiter). Returns NULL if @line is not a trace record.
> + */
> +static const char *find_func_column(const struct perf_ftrace *ftrace,
> +				    const char *line, int *cpu)
> +{
> +	const char *p = line;
> +	char *endptr;
> +	long val;
> +
> +	if (ftrace->graph_verbose) {
> +		p = strchr(p, '|');
> +		if (!p)
> +			return NULL;
> +		p++;
> +	}
> +
> +	p = skip_spaces(p);
> +	val = strtol(p, &endptr, 10);
> +	if (endptr == p || *endptr != ')')
> +		return NULL;
> +	p = endptr;
> +
> +	if (ftrace->graph_verbose) {
> +		const char *proc_start = p;
> +
> +		p = strchr(p, '|');
> +		if (!p)
> +			return NULL;
> +
> +		if (val >= 0 && val < FTRACE_MAX_CPUS) {
> +			const char *h = p - 1;
> +
> +			while (h >= proc_start && *h != '-')
> +				h--;
> +			if (h >= proc_start && *h == '-') {
> +				long pid_val = strtol(h + 1, NULL, 10);
> +				ftrace_cpu_tasks[(int)val] = get_task_state((pid_t)pid_val);
> +			}
> +		}
> +	}
> +
> +	p = strchr(p + 1, '|');
> +	if (!p)
> +		return NULL;
> +
> +	*cpu = (int)val;
> +	return p;
> +}

[Severity: Medium]
Does find_func_column() account for the latency-format column when
ftrace->graph_verbose is enabled?

In tools/perf/builtin-ftrace.c, set_tracing_funcgraph_verbose() enables three
tracing options:

static int set_tracing_funcgraph_verbose(struct perf_ftrace *ftrace)
{
	if (!ftrace->graph_verbose)
		return 0;

	if (write_tracing_option_file("funcgraph-proc", "1") < 0)
		return -1;

	if (write_tracing_option_file("funcgraph-abstime", "1") < 0)
		return -1;

	if (write_tracing_option_file("latency-format", "1") < 0)
		return -1;

	return 0;
}

When latency-format is enabled alongside funcgraph-abstime and funcgraph-proc,
the kernel's function_graph tracer emits four '|' delimiters before the
function column:

  <abstime> | <cpu>) <comm>-<pid> | <lat-fmt> | <duration> | <func>

However, find_func_column() only advances past three '|' characters in
verbose mode, returning a pointer to the '|' at the end of the latency-format
column instead of the '|' at the end of the duration column.

Consequently, in ftrace_process_fgraph_line():
1. On entry lines with a blank duration column, func_start points to the
   duration column's trailing "|  func() {", so extract_entry_func_name()
   extracts "|" as the function name.
2. On exit lines with a duration (e.g. "  1.234 us    |  }"), func_start
   points to the duration value rather than '}', so is_exit is false and
   function exits are never detected.
3. make_blank_prefix() blanks out the latency-format column and truncates the
   prefix before the duration column.

[ ... ]

> +int perf_ftrace__setup_inlines(struct perf_ftrace *ftrace)
> +{
> +	struct map *kmap;
> +
> +	if (!ftrace->use_inline)
> +		return 0;
> +
> +	symbol_conf.try_vmlinux_path = (symbol_conf.vmlinux_name == NULL);
> +	symbol_conf.inline_name = true;
> +	if (symbol_conf.vmlinux_name)
> +		symbol_conf.ignore_vmlinux_buildid = true;
> +
> +	if (symbol__init(NULL) < 0) {
> +		pr_warning("Failed to initialize symbols for inlined functions\n");
> +		return -1;
> +	}
> +
> +	ftrace->machine = machine__new_host(NULL);
> +	if (!ftrace->machine) {
> +		pr_warning("Failed to create host machine for symbols\n");
> +		return -1;
> +	}
> +
> +	if (symbol_conf.vmlinux_name) {
> +		kmap = machine__kernel_map(ftrace->machine);
> +		if (kmap)
> +			dso__load_vmlinux(map__dso(kmap), kmap, symbol_conf.vmlinux_name, false);
> +	} else {
> +		machine__load_vmlinux_path(ftrace->machine);
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Should perf_ftrace__setup_inlines() check the return values of
dso__load_vmlinux() and machine__load_vmlinux_path()?

Both functions return <= 0 when loading vmlinux fails (for example, if the
user passes a non-existent or invalid path via -k/--vmlinux, or if no vmlinux
with symbols is found on the system). Because the return values are ignored
and perf_ftrace__setup_inlines() unconditionally returns 0,perf ftrace
--inline proceeds without a loaded vmlinux DSO (and test_inline_processing()
does not skip when vmlinux cannot be loaded).

If loading fails, should perf_ftrace__setup_inlines() clean up ftrace->machine
and return an error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018023753.584873-1-atomlin@atomlin.com?part=4