Re: [PATCH] perf tools: Refactor precise_ip fallback logic

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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@intel.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@linux.intel.com>
> > Signed-off-by: Zide Chen <zide.chen@intel.com>
> 
> Acked-by: Ian Rogers <irogers@google.com>

Fell thru the cracks... Still applies cleanly.

Thanks, applied to perf-tools-next,

- 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
> >