Re: [PATCH perf-tools-next v10 05/10] perf ftrace: Support context switch tracking in function graph tracer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf ftrace: misordered stack merge in ftrace_task_state__rekey()
--

commit 280dc48b3ef7d657647928030bf50a4696853eb9
Author: Aaron Tomlin <atomlin@atomlin.com>

perf ftrace: Support context switch tracking in function graph tracer

This commit adds context switch detection and per-task call stack state
tracking to the userspace perf ftrace function graph processor. It parses
context switch banners, rekeys initial unknown per-CPU states upon
discovering task PIDs, and binds task states in verbose and single-PID
modes.

> diff --git a/tools/perf/util/ftrace.c b/tools/perf/util/ftrace.c
> index 95db57e70d11b..fea61a9e17b16 100644
> --- a/tools/perf/util/ftrace.c
> +++ b/tools/perf/util/ftrace.c
[ ... ]
> @@ -124,6 +129,157 @@ static int ftrace_task_state__grow_stack(struct ftrace_task_state *cs)
>  	return 0;
>  }
>  
> +/**
> + * ftrace_task_state__rekey - Reassign a task state to a new PID
> + * @ts: The task state to re-key
> + * @new_pid: The new process ID to associate with the state
> + *
> + * Rehash @ts under @new_pid. If a task state for @new_pid already exists,
> + * merge or replace it and update active CPU references accordingly.
> + *
> + * Return: A pointer to the active task state associated with @new_pid.
> + */
> +static struct ftrace_task_state *ftrace_task_state__rekey(struct ftrace_task_state *ts,
> +							  pid_t new_pid)
> +{
> +	struct ftrace_task_state *existing;
> +	u32 key;
> +	int j, c;
> +	int to_copy;
> +
> +	if (ts->pid == new_pid)
> +		return ts;
> +
> +	hlist_del_init(&ts->node);
> +
> +	existing = find_task_state(new_pid);
> +	if (existing) {
> +		struct ftrace_task_state *older, *newer;
> +		int base_raw_indent;
> +
> +		for (c = 0; c < FTRACE_MAX_CPUS; c++) {
> +			if (ftrace_cpu_tasks[c] == ts)
> +				ftrace_cpu_tasks[c] = existing;
> +		}
> +
> +		if (ts->seq < existing->seq) {
> +			older = ts;
> +			newer = existing;
> +		} else {
> +			older = existing;
> +			newer = ts;
> +		}
> +
> +		base_raw_indent = -1;
> +		if (newer->depth > 0)
> +			base_raw_indent = newer->stack[0].raw_indent;
> +		else if (newer->min_raw_indent >= 0)
> +			base_raw_indent = newer->min_raw_indent;
> +
> +		/*
> +		 * Prune frames on the older stack that have already exited
> +		 * (i.e. whose raw_indent >= base_raw_indent, along with any
> +		 * synthesized inlined frames).
> +		 */
> +		if (base_raw_indent >= 0) {
> +			while (older->depth > 0) {
> +				struct ftrace_stack_frame *top =
> +					&older->stack[older->depth - 1];
> +
> +				if (top->inlined) {
> +					free(top->name);
> +					if (older->inlined_depth > 0)
> +						older->inlined_depth--;
> +					older->depth--;
> +				} else if (top->raw_indent >= base_raw_indent) {
> +					free(top->name);
> +					older->depth--;
> +				} else {
> +					break;
> +				}
> +			}
> +		}
> +
> +		to_copy = ts->depth;
> +		if (to_copy > 0) {
> +			while (existing->depth + to_copy > existing->max_stack) {
> +				if (ftrace_task_state__grow_stack(existing) < 0)
> +					break;
> +			}
> +			if (existing->depth + to_copy > existing->max_stack)
> +				to_copy = existing->max_stack - existing->depth;
> +
> +			if (older == ts) {
> +				memmove(&existing->stack[to_copy], &existing->stack[0],
> +					existing->depth * sizeof(existing->stack[0]));
> +				memcpy(&existing->stack[0], &ts->stack[0],
> +				       to_copy * sizeof(existing->stack[0]));
> +			} else {
> +				memcpy(&existing->stack[existing->depth], &ts->stack[0],
> +				       to_copy * sizeof(existing->stack[0]));
> +			}

[Severity: Medium]
Can ftrace_task_state__rekey() misorder the stack merge and prune active
frames when a task migrates to a CPU without an initial context-switch
banner and then migrates back before that CPU emits a banner?

In the kernel function_graph tracer (verif_pid()), each CPU initializes
last_pid = -1 and omits the context-switch banner for the first traced task
on that CPU until a different traced task executes on it.

Suppose a task has a known state (existing) on CPU 0 with stack frames
[foo(raw_indent=2), bar(raw_indent=4)] and seq = 2, and then migrates to
CPU 1 where no banner has been emitted yet, so events on CPU 1 are recorded
under ts = FTRACE_UNKNOWN_PID(1).

On CPU 1, the task exits bar(4) (setting ts->min_raw_indent = 4) and enters
qux(4) (giving ts->stack = [qux(4)] and ts->seq = 4), then migrates back to
CPU 0 before any other task is traced on CPU 1.

When the task then enters a child function baz(6) at leading_spaces = 6 on
CPU 0 in ftrace_process_fgraph_line(), baz(6) is pushed onto existing->stack
and existing->seq advances to 5 while foo(2) and the already-exited bar(4)
remain at the bottom of existing->stack. Even with the stale-frame unwinding
added in the follow-up commits ("perf ftrace: Support display of inlined
functions in function graph tracer" and "perf ftrace: Filter extraneous
functions in function graph tracer"), qux(4) resides on ts->stack rather
than existing->stack (so caller_idx is -1), meaning ftrace_process_fgraph_line()
only unwinds frames on existing->stack with raw_indent >= 6 and leaves
bar(4) in place.

When another task later runs on CPU 1 and emits the delayed context-switch
banner "1) P => other", parse_context_switch() calls
ftrace_task_state__rekey(ts, P):

		if (ts->seq < existing->seq) {
			older = ts;
			newer = existing;
		} else {
			older = existing;
			newer = ts;
		}

		base_raw_indent = -1;
		if (newer->depth > 0)
			base_raw_indent = newer->stack[0].raw_indent;

Because ts->seq (4) < existing->seq (5), ftrace_task_state__rekey()
classifies ts as older and the entire existing state as newer, setting
base_raw_indent from existing->stack[0].raw_indent (foo at raw_indent = 2,
which actually predates ts).

Wouldn't the pruning loop on older (ts) then see that qux(4) has
raw_indent (4) >= base_raw_indent (2) and free the active qux(4) frame (along
with any synthesized inlined frames on ts->stack), while leaving the exited
bar(4) frame on existing->stack?

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