Re: [PATCH v4 1/3] perf sched: Suppress latency table output when trace samples are missing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Tracepoint handlers are not set for events read from a pipe, causing all trace samples to be silently dropped.
- [Medium] The patch adds dynamic callbacks for pipe mode but omits the `.feature` callback, leaving `perf_env` unpopulated.
--

--- Patch [1]: [PATCH v4 1/3] perf sched: Suppress latency table output when trace samples are missing ---
commit e9be5c25be0368a795779cd7fdb10479f3b3c099
Author: Aaron Tomlin <atomlin@atomlin.com>

perf sched: Suppress latency table output when trace samples are missing

This commit updates perf_sched__read_events() to abort early and return an
error when trace samples are missing. It also adds callbacks in cmd_sched()
to support pipe mode streams and adds a null check in map_switch_event().

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 7fd63a9db4574..fabd99a18aad4 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c

[ ... ]

> @@ -2001,7 +2001,7 @@ static int perf_sched__read_events(struct perf_sched *sched)
>  		.mode  = PERF_DATA_MODE_READ,
>  		.force = sched->force,
>  	};
> -	int rc = -1;
> +	int rc = -1, err;
>  
>  	session = perf_session__new(&data, &sched->tool);
>  	if (IS_ERR(session)) {
> @@ -2018,18 +2018,25 @@ static int perf_sched__read_events(struct perf_sched *sched)
>  	if (perf_session__set_tracepoints_handlers(session, handlers))
>  		goto out_delete;

[Severity: Medium]
Does setting the tracepoint handlers before processing events work for
pipe mode?

Since the evlist is empty in pipe mode until events are processed, it appears
perf_session__set_tracepoints_handlers() won't set up the handlers.

When samples later arrive in perf_sched__process_tracepoint_sample():

    if (evsel->handler != NULL) {
        tracepoint_handler f = evsel->handler;
        err = f(tool, sample, machine);
    }

Will they be silently dropped because evsel->handler is NULL?

> -	if (perf_session__has_traces(session, "record -R")) {
> -		int err = perf_session__process_events(session);
> -		if (err) {
> -			pr_err("Failed to process events, error %d", err);
> -			goto out_delete;
> -		}
> +	if (!perf_data__is_pipe(session->data) &&
> +	    !perf_session__has_traces(session, "record -R"))
> +		goto out_delete;
>  
> -		sched->nr_events      = session->evlist->stats.nr_events[0];
> -		sched->nr_lost_events = session->evlist->stats.total_lost;
> -		sched->nr_lost_chunks = session->evlist->stats.nr_events[PERF_RECORD_LOST];
> +	err = perf_session__process_events(session);

[ ... ]

> @@ -5167,6 +5174,9 @@ int cmd_sched(int argc, const char **argv)
>  	sched.tool.namespaces	 = perf_event__process_namespaces;
>  	sched.tool.lost		 = perf_event__process_lost;
>  	sched.tool.fork		 = perf_sched__process_fork_event;
> +	sched.tool.attr		 = perf_event__process_attr;
> +	sched.tool.tracing_data	 = perf_event__process_tracing_data;
> +	sched.tool.build_id	 = perf_event__process_build_id;

[Severity: Medium]
Does this code omit the feature callback required for pipe mode?

Without registering perf_event__process_feature to tool.feature, it seems
feature events from the pipe stream will be silently dropped, leaving
struct perf_env unpopulated.

>  
>  	argc = parse_options_subcommand(argc, argv, sched_options, sched_subcommands,
>  					sched_usage, PARSE_OPT_STOP_AT_NON_OPTION);

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