Re: [PATCH perf-tools-next v13 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: stale min_raw_indent fails to prune frames after migration
--

commit deda012d1c25c88f1250fe9b40670db3ff14f5e6
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 perf ftrace function graph tracer. It parses context switch
banners and verbose mode task columns to re-key, prune, and merge per-task
call stacks across CPU migrations.

> diff --git a/tools/perf/util/ftrace.c b/tools/perf/util/ftrace.c
> index 21d40c6eb288d..5c7d1c71c6350 100644
> --- a/tools/perf/util/ftrace.c
> +++ b/tools/perf/util/ftrace.c
[ ... ]
> @@ -161,6 +177,205 @@ static struct ftrace_task_state *get_task_state(pid_t pid)
>  	return ts;
>  }
>  
> +/**
> + * ftrace_task_state__record_indent - Record an indentation watermark for a task
> + * @ts: Task state to update
> + * @raw_indent: Raw indentation (leading spaces) of the observed trace record
> + *
> + * Record the shallowest indentation observed on @ts along with its event
> + * sequence number so that earlier frames that have already exited can be
> + * pruned when merging task states across CPUs.
> + */
> +static void ftrace_task_state__record_indent(struct ftrace_task_state *ts,
> +					     int raw_indent)
> +{
> +	if (ts->min_raw_indent < 0 || raw_indent <= ts->min_raw_indent) {
> +		ts->min_raw_indent = raw_indent;
> +		ts->min_indent_seq = ts->seq;
> +	}
> +}
> +
> +/**
> + * ftrace_task_state__prune_stack - Prune frames that exited on another task state
> + * @ts: Task state whose call stack should be pruned
> + * @other: Other task state representing the same task on another CPU
> + *
> + * Remove any frames on @ts->stack that are known to have exited based on
> + * subsequent trace events recorded on @other (i.e. @other->min_indent_seq >
> + * frame->seq and @other->min_raw_indent <= frame->indent). Synthesised
> + * inline frames are retained if and only if their enclosing real frame
> + * survives.
> + */
> +static void ftrace_task_state__prune_stack(struct ftrace_task_state *ts,
> +					   const struct ftrace_task_state *other)
> +{
> +	bool keep_inlines = false;
> +	bool seen_real = false;
> +	int i, dst = 0;
> +
> +	for (i = ts->depth - 1; i >= 0; i--) {
> +		struct ftrace_stack_frame *frame = &ts->stack[i];
> +		int min_indent = (other->min_indent_seq > frame->seq) ?
> +				 other->min_raw_indent : -1;
> +		bool prune;
> +
> +		if (frame->inlined) {
> +			if (seen_real)
> +				prune = !keep_inlines;
> +			else
> +				prune = (min_indent >= 0);
> +		} else {
> +			prune = (min_indent >= 0 && frame->indent >= min_indent);
> +			seen_real = true;
> +			keep_inlines = !prune;
> +		}
> +
> +		if (prune)
> +			zfree(&frame->name);
> +	}
[ ... ]
> +static struct ftrace_task_state *ftrace_task_state__rekey(struct ftrace_task_state *ts,
> +							  pid_t new_pid)
> +{
[ ... ]
> +	if (to_copy > 0) {
> +		int e_idx = 0, t_idx = 0, out_idx = 0;
> +
> +		if (orig_existing_depth > 0) {
> +			memmove(&existing->stack[to_copy],
> +				&existing->stack[0],
> +				orig_existing_depth * sizeof(existing->stack[0]));
> +		}
> +
> +		while (e_idx < orig_existing_depth && t_idx < to_copy) {
> +			if (existing->stack[to_copy + e_idx].seq <=
> +			    ts->stack[t_idx].seq) {
> +				existing->stack[out_idx++] =
> +					existing->stack[to_copy + e_idx++];
> +			} else {
> +				existing->stack[out_idx++] =
> +					ts->stack[t_idx++];
> +			}
> +		}

[Severity: Medium]
Can a shallow indentation recorded early in a task state's lifetime prevent
ftrace_task_state__prune_stack() from pruning deeper frames that exit after a
round-trip CPU migration?

In ftrace_task_state__record_indent(), ts->min_raw_indent and
ts->min_indent_seq are only updated when raw_indent <= ts->min_raw_indent:

	if (ts->min_raw_indent < 0 || raw_indent <= ts->min_raw_indent) {
		ts->min_raw_indent = raw_indent;
		ts->min_indent_seq = ts->seq;
	}

Suppose a task executes on CPU 0 at indent 2 (seq 1), pinning CPU 0's
min_raw_indent = 2 and min_indent_seq = 1. The task then migrates to CPU 1
before a context-switch banner is emitted on CPU 1, enters deeper functions
on CPU 1 at indent 4 (seq 2) and indent 6 (seq 3), and migrates back to
CPU 0 while the outer indent-2 frame is still active.

When those deeper frames subsequently exit or new frames are entered at
indent 4 or 6 on CPU 0 (seqs 4..6), raw_indent <= ts->min_raw_indent
(4 <= 2 or 6 <= 2) is false, so CPU 0's min_indent_seq remains stuck at 1.

When the delayed context-switch banner on CPU 1 is later parsed and
ftrace_task_state__rekey() calls ftrace_task_state__prune_stack(),
other->min_indent_seq > frame->seq evaluates to false (1 > 2 and 1 > 3),
causing min_indent to be -1. As a result, the exited deeper frames (along
with any attached inline frames) are not pruned, and the merge loop in
ftrace_task_state__rekey() splices them into existing->stack purely by
sequence number ahead of newer frames at shallower or equal indentations.

Should ftrace_task_state__rekey() also unwind earlier frames whose
indentation is greater than or equal to a later frame's indentation during
the merge, or track indentation watermarks relative to the migration point?

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