perf tools: Refactor precise_ip fallback logic

[PATCH] perf tools: Refactor precise_ip fallback logic

Posted by Zide Chen 3 months, 2 weeks ago

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

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

Posted by Ian Rogers 3 months, 2 weeks ago

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>

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
>

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

Posted by Arnaldo Carvalho de Melo 22 hours ago

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

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

Posted by Arnaldo Carvalho de Melo 22 hours ago

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

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

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

Posted by Chen, Zide 3 months, 2 weeks ago


On 10/23/2025 9:14 AM, 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>
> 
> 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 :-)

OK. Maybe a new test "PMU event open fallback tests".