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

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

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