Re: [PATCH v4 2/2] perf symbol: Lazily compute idle and use the perf_env

[Ian Rogers]

On Sun, Apr 5, 2026 at 10:10 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> On Thu, Mar 26, 2026 at 09:50:25PM -0700, Ian Rogers wrote:
> > Move the idle boolean to a helper symbol__is_idle function. In the
> > function lazily compute whether a symbol is an idle function taking
> > into consideration the kernel version and architecture of the
> > machine. As symbols__insert no longer needs to know if a symbol is for
> > the kernel, remove the argument.
> >
> > This change is inspired by mailing list discussion, particularly from
> > Thomas Richter <tmricht@xxxxxxxxxxxxx> and Heiko Carstens
> > <hca@xxxxxxxxxxxxx>:
> > https://lore.kernel.org/lkml/20260219113850.354271-1-tmricht@xxxxxxxxxxxxx/
> >
> > The change switches x86 matches to use strstarts which means
> > intel_idle_irq is matched as part of strstarts(name, "intel_idle"), a
> > change suggested by Honglei Wang <jameshongleiwang@xxxxxxx> in:
> > https://lore.kernel.org/lkml/20260323085255.98173-1-jameshongleiwang@xxxxxxx/
> >
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> [SNIP]
> > +     if (e_machine == EM_S390 && strstarts(name, "psw_idle")) {
> > +             int major = 0, minor = 0;
> > +             const char *release = env && env->os_release
> > +                     ? env->os_release : perf_version_string;
>
> I think Sashiko's review is right.  You need to check the kernel version
> instead of perf.

Doing this can create more problems and complexity than it solves. If
we state that `os_release` can be NULL at this point, we recompute it
using `uname`:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/header.c?h=perf-tools-next#n378
then do we cache the value in env? What happens if a data/pipe file
that assigns to the env later? Ad-hoc users of env->os_release
recomputing it shouldn't happen; instead, in 'live' mode, we should
assign os_release using uname either when the perf_env is created or
lazily with a helper function. I dislike that with a helper we could
potentially have multiple notions of os_release.

I'll add a patch to refactor the use of os_release, but can we be
mindful that this is clear feature creep with little benefit? We will
still fall back on `perf_version_string` if uname fails and for all
practical purposes, `perf_version_string` will differ little from
uname in this case. I'm only going to add the patch because checking
other uses of os_release suggests the change is benign.

Thanks,
Ian

> Thanks,
> Namhyung
>
> > +
> > +             /* Before v6.10, s390 used psw_idle. */
> > +             if (sscanf(release, "%d.%d", &major, &minor) != 2 ||
> > +                 major < 6 || (major == 6 && minor < 10)) {
> > +                     sym->idle = SYMBOL_IDLE__IDLE;
> > +                     return true;
> > +             }
> > +     }
> > +
> > +     sym->idle = SYMBOL_IDLE__NOT_IDLE;
> > +     return false;
> >  }