Re: [PATCH perf-tools-next v12 04/10] perf ftrace: Add basic task call stack management

[Namhyung Kim]

Hello,

On Sat, Oct 31, 2026 at 06:27:42PM -0400, Aaron Tomlin wrote:
> On Sat, Oct 31, 2026 at 12:05:52AM -0700, Namhyung Kim wrote:
> > On Sun, Oct 25, 2026 at 10:57:29PM -0400, Aaron Tomlin wrote:
> > > The function_graph tracer records kernel function entries and exits with
> > > nested indentation. To perform call graph analysis and subsequent inlined
> > > function reconstruction, userspace must track the active call stack on a
> > > per-task and per-CPU basis.
> > > 
> > > Introduce basic call stack management for the function graph tracer:
> > > 
> > >     1.  Data structures
> > > 
> > >         Define struct ftrace_stack_frame to track function names,
> > >         indentation, and inlined status.
> > > 
> > >         Define struct ftrace_task_state with a dynamically resisable call
> > >         stack array, tracked in a hash table keyed by PID alongside per-CPU
> > >         active task bindings (ftrace_cpu_tasks).
> > > 
> > >     2.  Trace stream processing
> > > 
> > >         Implement find_func_column() to isolate context columns (CPU,
> > >         timestamps, duration) from function graph trace lines across
> > >         standard and verbose layouts.
> > > 
> > >         Implement ftrace_process_fgraph_line() to parse function entries
> > >         ("{") and exits ("}"), pushing and popping frames on the tracked
> > >         stack and matching bare closing braces against preceding frames.
> > > 
> > >     3.  Integration in __cmd_ftrace()
> > > 
> > >         Stream complete trace lines through ftrace_process_fgraph_line()
> > >         in __cmd_ftrace() and clean up allocated CPU states on exit via
> > >         ftrace_cpu_states__clear().
> > > 
> > > Tested-by: Ian Rogers <irogers@xxxxxxxxxx>
> > > Reviewed-by: Ian Rogers <irogers@xxxxxxxxxx>
> > > Signed-off-by: Aaron Tomlin <atomlin@xxxxxxxxxxx>
> > > ---
> > [SNIP]
> > > +/**
> > > + * ftrace_process_fgraph_line - Process and output a function graph trace line
> > > + * @ftrace: Perf ftrace context
> > > + * @line: The trace line to process
> > > + * @out: File stream to output to
> > > + *
> > > + * Parse function entries and exits, maintain the task's call stack, and
> > > + * output the trace line.
> > > + *
> > > + * Return: 0 on success, or a negative error code on failure.
> > > + */
> > > +int ftrace_process_fgraph_line(struct perf_ftrace *ftrace, const char *line, FILE *out)
> > > +{
> > > +	const char *pipe_char;
> > > +	const char *after_pipe;
> > > +	const char *func_start;
> > > +	struct ftrace_task_state *cs;
> > > +	int cpu = -1;
> > > +	int leading_spaces;
> > > +	int top_raw_indent;
> > > +	int match_idx;
> > > +	int i;
> > > +	bool is_exit;
> > > +	bool has_open_brace;
> > > +	bool has_semicolon;
> > > +
> > > +	pipe_char = find_func_column(ftrace, line, &cpu);
> > > +	if (!pipe_char)
> > > +		return print_line(ftrace, line, out);
> > > +
> > > +	cs = get_cpu_state(ftrace, cpu);
> > > +	if (!cs)
> > > +		return print_line(ftrace, line, out);
> > > +
> > > +	after_pipe = pipe_char + 1;
> > > +	func_start = skip_spaces(after_pipe);
> > > +	leading_spaces = func_start - after_pipe;
> > > +
> > > +	if (*func_start == '\0')
> > > +		return print_line(ftrace, line, out);
> > > +
> > > +	is_exit = (*func_start == '}');
> > > +
> > > +	if (is_exit) {
> > > +		char exit_name[128];
> > > +
> > > +		extract_exit_func_name(func_start, exit_name, sizeof(exit_name));
> > > +
> > > +		top_raw_indent = -1;
> > > +		for (i = cs->depth - 1; i >= 0; i--) {
> > > +			top_raw_indent = cs->stack[i].raw_indent;
> > > +			break;
> > > +		}
> > 
> > This loop doesn't look natural.  Why not just use an if statement?
> 
> Hi Namhyung,
> 
> Thank you. I completely agree.
> 
> A loop that unconditionally breaks on the first iteration is
> unnecessary here. I will replace it with a simple conditional check in
> the next revision:
> 
>     top_raw_indent = -1;
>     if (cs->depth > 0)
>         top_raw_indent = cs->stack[cs->depth - 1].raw_indent;
> 
> All subsequent similar instances will be addressed accordingly as well.
> 
> > > +struct ftrace_stack_frame {
> > > +	char *name;
> > > +	bool inlined;
> > > +	int indent;
> > > +	int raw_indent;
> > 
> > Can you please add comments that explain why it has two indents?
> > Also inlined field may be added later.
> 
> Certainly. The two indent fields serve distinct purposes once inlined
> frames are injected into the output.
> 
>     - raw_indent: Records the original leading whitespace count after "|"
>       as emitted by the kernel's function_graph tracer. Because the kernel
>       only traces real stack frames, raw_indent is used to match entry and
>       exit frames against the raw trace stream.
> 
>     - indent: Records the adjusted visual indentation (in spaces) used when
>       rendering the frame to output, accounting for any extra indentation
>       added by preceding synthetic inlined frames so that the closing "}"
>       aligns with its opening "{".
> 
> Agreed, these fields are not used yet. I will move inlined and
> inlined_depth into the subsequent patch that introduces inlined function
> display, and add explanatory comments to both structures.
> 
> > > +};
> > > +
> > > +struct ftrace_task_state {
> > > +	struct hlist_node	node;
> > > +	pid_t			pid;
> > > +	struct ftrace_stack_frame *stack;
> > > +	int			max_stack;
> > > +	int			depth;
> > > +	int			inlined_depth;
> > > +	int			overflow_depth;
> > 
> > Ditto.  What are these depths?
> 
>     - max_stack: Current allocated capacity (in frames) of the dynamically
>       resized 'stack' array.
> 
>     - depth: Current number of active frames stored on stack.
> 
>     - inlined_depth: Number of active synthetic inlined frames currently on
>       stack, used to compute the extra visual indentation offset for
>       child frames. This will also be moved to the later patch.
> 
>     - overflow_depth: Number of nested call frames entered while stack is
>       saturated (at the maximum depth cap or on allocation failure), used
>       so their matching exit braces can be consumed without prematurely
>       popping valid parent frames.
> 
> I will document all of these fields with comments in the next revision.
> 
> Please see an example. When synthetic inlined frames are injected, the
> visual display column (indent) shifts right relative to the kernel stream
> (raw_indent):
> 
>     1.  What kernel function_graph emits:
> 
>            |  foo() {                 <-- raw_indent = 2
>            |    bar() {               <-- raw_indent = 4
>            |    } /* bar */           <-- raw_indent = 4
>            |  } /* foo */             <-- raw_indent = 2
> 
>     2.  Rendered output and stack state:
> 
>         When perf injects inlined function inline_helper() onto cs->stack,
>         inlined_depth becomes 1. Every subsequent child frame shifts right
>         by (inlined_depth * 2) spaces so bar() nests visually inside
>         inline_helper(), while still retaining raw_indent = 4 to match the
>         kernel's closing "}" event:
> 
>            Rendered Output            cs->stack[idx] and ftrace_task_state
>            ---------------            ---------------------------
>            |  foo() {                 [0] foo:
>            :                              raw_indent=2, indent=2, inlined=false
>            :                              (depth=1, inlined_depth=0)
>            :
>            |    inline_helper() {     [1] inline_helper:
>            :                              raw_indent=4, indent=4, inlined=true
>            :                              (depth=2, inlined_depth=1)
>            :
>            |      bar() {             [2] bar:
>            :                              raw_indent=4, indent=6, inlined=false
>            :                              (depth=3, inlined_depth=1)
>            :                              ^
>            :                              '-- indent = raw_indent (4) +
>            :                                           inlined_depth * 2 (2) = 6
>            :
>            |      } /* bar */         <-- kernel function_graph emits "}"
>            :                              at raw_indent=4. Matched against [2]
>            :                              via raw_indent=4, but printed at
>            :                              [2].indent (6 spaces)
>            :
>            |    } /* inline_helper */ <-- Popped automatically; printed at
>            :                              [1].indent (4 spaces)
>            :
>            |  } /* foo */             <-- kernel function_graph emits "}"
>                                           at raw_indent=2. Matched against [0]
>                                           via raw_indent=2, printed at
>                                           [0].indent (2 spaces)
> 

Thanks for the example,  the comment also look good.

Namhyung