Re: [PATCH v6 2/4] perf sched: Handle missing trace samples in pipe mode

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Using `evsel->handler == NULL` to conditionally initialize handlers inside the sample processing hot path causes repeated O(N) evlist iterations for unhandled events and drops late-arriving tracepoints.

Pre-existing issues:
- [High] `symbol__init` accesses the `perf_env` before it is populated by pipe feature events, leading to incorrect host-fallback initialization for cross-platform traces.
- [High] `perf sched timehist` remains completely broken for pipe mode streams.
--

--- Patch [2]: [PATCH v6 2/4] perf sched: Handle missing trace samples in pipe mode ---
commit bdcbf92a5b81c957ee21cce440488c2da3569c14
Author: Aaron Tomlin <[email protected]>

perf sched: Handle missing trace samples in pipe mode

For pipe mode streams, event attributes are received dynamically during
event processing, meaning session->evlist is not populated prior to
perf_session__process_events(). To handle pipe input correctly:
  - Register the missing .attr, .tracing_data, .build_id, and .feature
    callbacks in cmd_sched()
  - Promote the handlers array to file-scope (latency_handlers[]) and
    invoke evlist__set_tracepoints_handlers() dynamically inside
    perf_sched__process_tracepoint_sample() when evsel->handler is NULL
  - Perform the trace check post-processing when handling pipe data

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 94a323da0799a..0abf9f1f3c2e2 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
[ ... ]
> @@ -1946,6 +1955,14 @@ static int perf_sched__process_tracepoint_sample(const struct perf_tool *tool __
>  	struct evsel *evsel = sample->evsel;
>  	int err = 0;
>  
> +	if (evsel->handler == NULL && sample->evsel->evlist) {
> +		/* prefer sched_waking if it is captured */
> +		if (evlist__find_tracepoint_by_name(sample->evsel->evlist, "sched:sched_waking"))
> +			latency_handlers[2].handler = process_sched_wakeup_ignore;
> +
> +		evlist__set_tracepoints_handlers(sample->evsel->evlist, latency_handlers);
> +	}
> +

[Severity: High]
Does this cause an O(N) traversal on every sample for unhandled events?

If a tracepoint is explicitly recorded but not in latency_handlers (such as
sched:sched_process_fork), evsel->handler will remain NULL. For every sample
of these unhandled events, this condition evaluates to true.

This repeatedly triggers evlist__find_tracepoint_by_name() and a call to
evlist__set_tracepoints_handlers(), which immediately aborts with -EEXIST.

Could this drop late-arriving tracepoints and cause a significant performance
regression on the event processing hot path?

>  	if (evsel->handler != NULL) {
>  		tracepoint_handler f = evsel->handler;
>  		err = f(tool, sample, machine);
[ ... ]
> @@ -2011,14 +2020,17 @@ static int perf_sched__read_events(struct perf_sched *sched)
>  
>  	symbol__init(perf_session__env(session));

[Severity: High]
This is a pre-existing issue, but does this early symbol__init() call break
cross-platform pipe mode?

In pipe mode, perf_env is populated during event processing. Calling
symbol__init() here accesses an uninitialized env->arch.

This causes perf_env__os_release() to fall back to the host's uname(),
permanently poisoning env->os_release and vmlinux_path__init() with host
paths.

> -	/* prefer sched_waking if it is captured */
> -	if (evlist__find_tracepoint_by_name(session->evlist, "sched:sched_waking"))
> -		handlers[2].handler = process_sched_wakeup_ignore;
[ ... ]

I also noticed a related pre-existing issue in perf_sched__timehist()
where timehist remains broken for pipe mode streams:

tools/perf/builtin-sched.c:perf_sched__timehist() {
[ ... ]
	/* setup per-evsel handlers */
	if (perf_session__set_tracepoints_handlers(session, handlers))
		goto out;

	/* sched_switch event at a minimum needs to exist */
	if (!evlist__find_tracepoint_by_name(session->evlist, "sched:sched_switch")) {
		pr_err("No sched_switch events found. Have you run 'perf sched record'?\n");
		goto out;
	}
[ ... ]
}

[Severity: High]
This is a pre-existing issue, but does this strict check break the
perf sched timehist subcommand for pipe streams?

Since the evlist is unpopulated before processing pipe events,
evlist__find_tracepoint_by_name() fails immediately, causing the timehist
command to abort.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2