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

[Aaron Tomlin]

On Sat, Oct 31, 2026 at 03:01:51PM -0700, Namhyung Kim wrote:
> On Sun, Oct 25, 2026 at 10:57:30PM -0400, Aaron Tomlin wrote:
> > When tracing multiple tasks across multiple CPUs, kernel context switches
> > cause different threads of execution to interleave on the same CPU. Without
> > context switch awareness, interleaved function calls corrupt the tracked
> > call stack.
> > 
> > Add context switch detection and per-task state tracking:
> > 
> >     1.  Context switch banner parsing
> > 
> >         Implement parse_context_switch() to parse context switch banners
> >         (e.g. " 0) <idle>-0 => migrati-19") in the trace stream, extracting
> >         the CPU number and both outgoing and incoming task PIDs.
> > 
> >     2.  Task state rekeying and migration
> > 
> >         Implement ftrace_task_state__rekey() to reassign initial unknown
> >         task states to real PIDs upon discovering outgoing task identity.
> 
> Yeah, it's unfortunate that the default ftrace output doesn't show PIDs. 

Hi Namhyung,

Thank you for your feedback.

Yes, it is indeed.

> > diff --git a/tools/perf/util/ftrace.c b/tools/perf/util/ftrace.c
> > index 95db57e70d11..fc385d680911 100644
> > --- a/tools/perf/util/ftrace.c
> > +++ b/tools/perf/util/ftrace.c
> > @@ -13,11 +13,14 @@
> >  #include <errno.h>
> >  #include <sys/types.h>
> >  #include "debug.h"
> > +#include "evlist.h"
> >  #include "util/ftrace.h"
> >  #include "util/string2.h"
> > +#include <perf/threadmap.h>
> >  
> >  static struct hlist_head ftrace_task_hash[FTRACE_PID_HASH_SIZE];
> >  static struct ftrace_task_state *ftrace_cpu_tasks[FTRACE_MAX_CPUS];
> > +static u64 ftrace_event_seq;
> >  
> >  /**
> >   * ftrace_cpu_states__clear - Free allocated CPU and task stack states
> > @@ -36,12 +39,14 @@ void ftrace_cpu_states__clear(void)
> >  			for (j = 0; j < ts->depth; j++)
> >  				free(ts->stack[j].name);
> >  			free(ts->stack);
> > +			free(ts->watermarks);
> >  			hlist_del_init(&ts->node);
> >  			free(ts);
> >  		}
> >  	}
> >  
> >  	memset(ftrace_cpu_tasks, 0, sizeof(ftrace_cpu_tasks));
> > +	ftrace_event_seq = 0;
> >  }
> >  
> >  /**
> > @@ -98,6 +103,119 @@ 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
> > + * @cs: Task state to update
> 
> Why is it called 'cs' instead of 'ts'?  Same goes to the later functions
> too.

Agreed. That is a relic from when the struct was named ftrace_cpu_state.
I will rename "cs" to "ts" consistently across all functions in the series.

> > +		if (prune) {
> > +			free(frame->name);
> > +			frame->name = NULL;
> 
> You can use zfree(&frame->name);

Acknowledged. I will switch to zfree(&frame->name).

> > +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) {
> 
> I think it's better to handle the else case first as it's much shorter.
> You can save an indentation level for the true case then.

Agreed.

> 
> > +		int orig_existing_depth;
> > +		int total_needed;
> > +
> > +		for (c = 0; c < FTRACE_MAX_CPUS; c++) {
> > +			if (ftrace_cpu_tasks[c] == ts)
> > +				ftrace_cpu_tasks[c] = existing;
> > +		}
> 
> I don't this is necessary.  IIUC the rekey function is supposed to be
> called for a specific CPU and caller will update the CPU state.

Fair. Since ts is keyed to a single CPU's synthetic unknown PID
(FTRACE_UNKNOWN_PID(cpu)) and parse_context_switch() immediately updates
ftrace_cpu_tasks[cpu_val], no other CPU slot can reference ts. I will
remove this loop.

> > +
> > +		/*
> > +		 * Prune frames on existing and ts that exited during subsequent
> > +		 * execution recorded on the other state.
> > +		 */
> > +		ftrace_task_state__prune_stack(existing, ts);
> > +		ftrace_task_state__prune_stack(ts, existing);
> 
> I'm not sure about this.  In this context, it knows the 'ts' is uptodate
> and the 'existing' contains the old state.  Does it need to prune the ts
> using the existing?
> 
> IIUC this code assumes either ts or existing may have uptodate state
> like in general cases but it would make the code unnecessarily longer.

The reason this handled both directions is due to how trace_pipe merges
per-CPU buffers when a task starts on CPU 0 (unidentified, tracked in ts
under FTRACE_UNKNOWN_PID(0)) and migrates to CPU 1:

    1.  Task X starts on CPU 0 before any context switch banner is seen on
        CPU 0, so its initial frames are recorded in ts.

    2.  Task X migrates to CPU 1. If CPU 1's incoming banner ("1) idle =>
        X") and subsequent function exits are read from trace_pipe before
        CPU 0's outgoing banner ("0) X => idle") is flushed, existing is
        created on CPU 1 first and actually holds the newer state, while
        ts holds the older pre-migration frames from CPU 0.

    3.  Conversely, if Task X was already known on CPU 1 (existing) and
        migrated to an unbannered CPU 0 (ts), then ts holds the newer state
        and existing holds the older state.

That said, I agree that maintaining a dynamic per-task watermarks[] array
for arbitrary multi-hop migrations before the first banner is superfluous.
Replacing the dynamic watermarks array with a single scalar minimum-indent
watermark on struct ftrace_task_state makes this much simpler and removes
ftrace_task_state__record_indent(), ftrace_task_state__min_indent_after(),
and the watermark merge loop in ftrace_task_state__rekey().

Thank you!

Kind regards,
-- 
Aaron Tomlin