Message-ID: <aQl3qfyTdAb68l1l@google.com>
Date: Mon, 3 Nov 2025 19:48:57 -0800
From: Namhyung Kim <namhyung@...nel.org>
To: "Chen, Zide" <zide.chen@...el.com>
Cc: linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.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>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Ian Rogers <irogers@...gle.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

Hello,

Sorry for the delay.

On Mon, Oct 27, 2025 at 11:56:52AM -0700, Chen, Zide wrote:
> 
> 
> On 10/25/2025 5:42 PM, Namhyung Kim wrote:
> > On Fri, Oct 24, 2025 at 11:03:17AM -0700, Chen, Zide wrote:
> >>
> >>
> >> 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().
> > 
> > Right, that means the kernel doesn't support PERF_SAMPLE_READ with
> > inherit = true.  How did the first event succeed to open then?
> 
> The perf tool sets PERF_SAMPLE_TID for Inherit + PERF_SAMPLE_READ
> events, as implemented in commit 90035d3cd876 ("tools/perf: Allow
> inherit + PERF_SAMPLE_READ when opening event").
> 
> Meanwhile, commit 7e8b255650fc ("perf: Support PERF_SAMPLE_READ with
> inherit") rejects a perf event if has_inherit_and_sample_read(attr) is
> true and PERF_SAMPLE_TID is not set in attr->sample_type.
> 
> Therefore, the first event succeeded, while the one opened in
> evsel__detect_missing_features() which doesn't have PERF_SAMPLE_TID failed.

Why does the first succeed and the second fail?  Don't they have the
same SAMPLE_READ and SAMPLE_TID + inherit flags?

> 
> 
> >>
> >>>
> >>>> 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.
> > 
> > I think precise_ip_fallback() is just a trial and error for each possible
> > value.  While detect_missing_features() checks what the current kernel
> > supports.  Trying different precise_ip values with unsupported attributes
> > doesn't make sense.
> 
> When it returns -EINVAL, the cause could be an unsupported precise_ip or
> something else. We could either end up with "trying different precise_ip
> values with unsupported attributes", or "trying attributes with
> unsupported precise_ip".
> 
> The perf tool’s fallback logic is a “best effort” mechanism to fix only
> one issue, not multiple ones. So, IMO, we should place
> evsel__detect_missing_features() as the last fallback attempt, since it
> does not restore the event attributes after a failed try.

The missing feature check is about the global kernel behavior so there's
no point to try if we know the kernel won't support those features.
While precise fallback is per-PMU (and per-event?) behavior so it'd be
natural to try that after removing must-fail attributes from the missing
feature tests.

Thanks,
Namhyung

> >>
> >> 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@...ux.intel.com>
> >>>> Signed-off-by: Zide Chen <zide.chen@...el.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
> >>>>
> >>
> 

Powered by blists - more mailing lists