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

["Chen, Zide" <zide.chen@intel.com>]

On 10/23/2025 7:30 PM, Namhyung Kim wrote:
> Hello,
> 
> On Wed, Oct 22, 2025 at 03:08:02PM -0700, Zide Chen 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
> 
> I'm curious about this part.  Why the kernel set 'inherit = false'?  IOW
> how did the leader event (mem-loads-aux) succeed with inherit = true
> then?

Initially, the inherit = true for both the group leader
(cpu/mem-loads-aux/S) and the event in question (cpu/mem-loads,ldlat=3/PS).

When the second event fails with EINVAL, the current logic calls
evsel__detect_missing_features() first. Since this is a PERF_SAMPLE_READ
event, the inherit attribute falls back to false, according to the
fallback order implemented in evsel__detect_missing_features().

> 
>> 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.
> 
> I'm not sure.. some events may need a different (i.e. lower) precise
> level than the max.  I think checking the missing feature later will
> use the max precise level always.

Yes, but seems the basic idea of the event open fallback logic is to
check whether it's lucky enough to open the event by falling back one
single attribute, not multiple attributes.

evsel__precise_ip_fallback() can restore the precise_ip level after a
failed attempt, while evsel__detect_missing_features() cannot recover
the event attributes from its failed try.

Therefore, falling back precise_ip first maintains the intended
“one-by-one” fallback logic. If it’s placed later, it may combine two
fallbacks, which can cause failures like the example above.  Of course,
in theory, there might be cases where an event can be opened if both
precise_ip and another feature are relaxed together, but I haven’t
exhaustively checked whether such cases actually exist.

> Thanks,
> Namhyung
> 
>>
>> 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
>>