[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aYZmyNz1tJ9u-y5s@x1>
Date: Fri, 6 Feb 2026 19:10:16 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Zide Chen <zide.chen@...el.com>, linux-kernel@...r.kernel.org,
linux-perf-users@...r.kernel.org,
Namhyung Kim <namhyung@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Adrian Hunter <adrian.hunter@...el.com>,
Ingo Molnar <mingo@...hat.com>, Jiri Olsa <jolsa@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
thomas.falcon@...el.com, dapeng1.mi@...ux.intel.com,
xudong.hao@...el.com
Subject: Re: [PATCH] perf tools: Refactor precise_ip fallback logic
On Fri, Feb 06, 2026 at 07:06:21PM -0300, Arnaldo Carvalho de Melo wrote:
> On Thu, Oct 23, 2025 at 09:14:17AM -0700, Ian Rogers wrote:
> > On Wed, Oct 22, 2025 at 3:14 PM Zide Chen <zide.chen@...el.com> wrote:
> > >
> > > Commit c33aea446bf555ab ("perf tools: Fix precise_ip fallback logic")
> > > unconditionally called the precise_ip fallback and moved it after the
> > > missing-feature checks so that it could handle EINVAL as well.
> > >
> > > However, this introduced an issue: after disabling missing features,
> > > the event could fail to open, which makes the subsequent precise_ip
> > > fallback useless since it will always fail.
> > >
> > > For example, run the following command on Intel SPR:
> > >
> > > $ perf record -e '{cpu/mem-loads-aux/S,cpu/mem-loads,ldlat=3/PS}' -- ls
> > >
> > > Opening the event "cpu/mem-loads,ldlat=3/PS" returns EINVAL when
> > > precise_ip == 3. It then sets attr.inherit = false, which triggers a
> > > kernel check failure since it doesn't match the group leader's inherit
> > > attribute. As a result, it continues to fail even after precise_ip is
> > > reduced.
> > >
> > > By moving the precise_ip fallback earlier, this issue is resolved, as
> > > well as the kernel test robot report mentioned in commit
> > > c33aea446bf555ab.
> > >
> > > No negative side effects are expected, because the precise_ip level is
> > > restored by evsel__precise_ip_fallback() if the fallback does not help.
> > >
> > > This also aligns with commit 2b70702917337a8d ("perf tools: Remove
> > > evsel__handle_error_quirks()").
> > >
> > > Fixes: af954f76eea56453 ("perf tools: Check fallback error and order")
> > > Fixes: c33aea446bf555ab ("perf tools: Fix precise_ip fallback logic")
> > > Reviewed-by: Dapeng Mi <dapeng1.mi@...ux.intel.com>
> > > Signed-off-by: Zide Chen <zide.chen@...el.com>
> >
> > Acked-by: Ian Rogers <irogers@...gle.com>
>
> Fell thru the cracks... Still applies cleanly.
>
> Thanks, applied to perf-tools-next,
So I saw the rest of the thread, removing it...
- Arnaldo
> - Arnaldo
>
> > Any chance you could help with a test case that covers this? The
> > fallback logic is spread out and easy to introduce subtle bugs into.
> > Just having a test case that does ` perf record -e
> > '{cpu/mem-loads-aux/S,cpu/mem-loads,ldlat=3/PS}' -- ls` and checks the
> > output for EINVAL when the events are present would be useful, as then
> > we can make sure this doesn't regress on SPR and later. Something with
> > more generic events would of course be better :-)
> >
> > Thanks,
> > Ian
> >
> > > ---
> > > tools/perf/util/evsel.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > > index ca74514c8707..6ce32533a213 100644
> > > --- a/tools/perf/util/evsel.c
> > > +++ b/tools/perf/util/evsel.c
> > > @@ -2714,12 +2714,12 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
> > > if (err == -EMFILE && rlimit__increase_nofile(&set_rlimit))
> > > goto retry_open;
> > >
> > > + if (evsel__precise_ip_fallback(evsel))
> > > + goto retry_open;
> > > +
> > > if (err == -EINVAL && evsel__detect_missing_features(evsel, cpu))
> > > goto fallback_missing_features;
> > >
> > > - if (evsel__precise_ip_fallback(evsel))
> > > - goto retry_open;
> > > -
> > > out_close:
> > > if (err)
> > > threads->err_thread = thread;
> > > --
> > > 2.51.0
> > >
Powered by blists - more mailing lists