Re: [PATCH v11 19/33] perf evlist: Try to avoid computing evsel from sample

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [Low] The fallback check `if (!evsel) evsel = evlist__id2evsel(..., sample.id);` introduced by this patch is mathematically unreachable dead code.
--

commit 70039c4c6052c77672e3d3e7b9f3b386bae9924f
Author: Ian Rogers <irogers@google.com>

perf evlist: Try to avoid computing evsel from sample

This commit modifies the event processing logic to use the evsel already
present in the sample, falling back to looking it up by id if it is not
present.

> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
[ ... ]
> @@ -1164,7 +1164,9 @@ static int deliver_event(struct ordered_events *qe,
>  		goto next_event;
>  	}
>  
> -	evsel = evlist__id2evsel(session->evlist, sample.id);
> +	evsel = sample.evsel;
> +	if (!evsel)
> +		evsel = evlist__id2evsel(session->evlist, sample.id);
>  	assert(evsel != NULL);

Is the fallback check for !evsel ever reachable in this path, or could this
be a dead code regression?

Looking at the code just prior to this in deliver_event(),
evlist__parse_sample() is executed and any parsing failure results in
jumping to next_event.

If evlist__parse_sample() succeeds, it appears to unconditionally
populate sample.evsel. Does evlist__parse_sample() have a success path
that leaves sample.evsel unpopulated?

> diff --git a/tools/perf/tests/mmap-basic.c b/tools/perf/tests/mmap-basic.c
> --- a/tools/perf/tests/mmap-basic.c
> +++ b/tools/perf/tests/mmap-basic.c
[ ... ]
> @@ -142,7 +142,9 @@ static int test__basic_mmap(struct test_suite *test __maybe_unused, int subtest
>  		}
>  
>  		err = -1;
> -		evsel = evlist__id2evsel(evlist, sample.id);
> +		evsel = sample.evsel;
> +		if (!evsel)
> +			evsel = evlist__id2evsel(evlist, sample.id);

A similar pattern occurs here. Since evlist__parse_sample() is called
just before this and errors branch to out_delete_evlist, would
sample.evsel always be populated by the time we reach this check?

> diff --git a/tools/perf/tests/switch-tracking.c b/tools/perf/tests/switch-tracking.c
> --- a/tools/perf/tests/switch-tracking.c
> +++ b/tools/perf/tests/switch-tracking.c
[ ... ]
> @@ -138,7 +138,10 @@ static int process_sample_event(struct evlist *evlist,
>  		goto out;
>  	}
>  
> -	evsel = evlist__id2evsel(evlist, sample.id);
> +	evsel = sample.evsel;
> +	if (!evsel)
> +		evsel = evlist__id2evsel(evlist, sample.id);
> +

This section also shares the same behavior following
evlist__parse_sample().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260413012227.1089445-1-irogers@google.com?part=19