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

[Aaron Tomlin]

On Tue, Nov 03, 2026 at 11:35:37PM -0800, Namhyung Kim wrote:
> 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.

Hi Namhyung,

Thank you and you are very welcome.

Kind regards,
-- 
Aaron Tomlin