linux-kernel - Re: [PATCH] perf tools: Simplify is_event

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Thu, 11 Apr 2024 12:01:48 +0300
From: Adrian Hunter <adrian.hunter@...el.com>
To: Namhyung Kim <namhyung@...nel.org>, Ian Rogers <irogers@...gle.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Jiri Olsa <jolsa@...nel.org>,
 linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH] perf tools: Simplify is_event_supported()

On 10/04/24 20:45, Namhyung Kim wrote:
> On Wed, Apr 10, 2024 at 9:08 AM Ian Rogers <irogers@...gle.com> wrote:
>>
>> On Wed, Apr 10, 2024 at 3:45 AM Adrian Hunter <adrian.hunter@...el.com> wrote:
>>>
>>> Simplify is_event_supported by using sys_perf_event_open() directly like
>>> other perf API probe functions and move it into perf_api_probe.c where
>>> other perf API probe functions reside.
>>>
>>> A side effect is that the probed events do not appear when debug prints
>>> are enabled, which is beneficial because otherwise they can be confused
>>> with selected events.
>>>
>>> This also affects "Test per-thread recording" in
>>> "Miscellaneous Intel PT testing" which expects the debug prints of
>>> only selected events to appear between the debug prints:
>>> "perf record opening and mmapping events" and
>>> "perf record done opening and mmapping events"
>>>
>>> Signed-off-by: Adrian Hunter <adrian.hunter@...el.com>
>>
>> nit:
>> Closes: https://lore.kernel.org/lkml/ZhVfc5jYLarnGzKa@x1/
>>
>>> ---
>>>  tools/perf/util/perf_api_probe.c | 40 +++++++++++++++++++++++++
>>>  tools/perf/util/perf_api_probe.h |  2 ++
>>>  tools/perf/util/pmus.c           |  1 +
>>>  tools/perf/util/print-events.c   | 50 +-------------------------------
>>>  tools/perf/util/print-events.h   |  1 -
>>>  5 files changed, 44 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/tools/perf/util/perf_api_probe.c b/tools/perf/util/perf_api_probe.c
>>> index 1de3b69cdf4a..13acb34a4e1c 100644
>>> --- a/tools/perf/util/perf_api_probe.c
>>> +++ b/tools/perf/util/perf_api_probe.c
>>> @@ -195,3 +195,43 @@ bool perf_can_record_cgroup(void)
>>>  {
>>>         return perf_probe_api(perf_probe_cgroup);
>>>  }
>>> +
>>> +bool is_event_supported(u8 type, u64 config)
>>> +{
>>> +       struct perf_event_attr attr = {
>>> +               .type = type,
>>> +               .config = config,
>>> +               .disabled = 1,
>>> +       };
>>> +       int fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
>>
>> It looks like this is a change to the actual perf_event_open
>> arguments, I don't think it is an issue but wanted to flag it.
>>
>>> +
>>> +       if (fd < 0) {
>>> +               /*
>>> +                * The event may fail to open if the paranoid value
>>> +                * /proc/sys/kernel/perf_event_paranoid is set to 2
>>> +                * Re-run with exclude_kernel set; we don't do that by
>>> +                * default as some ARM machines do not support it.
>>> +                */
>>> +               attr.exclude_kernel = 1;
>>
>> I worry about the duplicated fallback logic getting out of sync,
>> perhaps we could have a quiet option for evsel__open option, or better
>> delineate the particular log entries.

That seemed like it would be messy, but upon closer inspection
was straight forward.  Patch here:

	https://lore.kernel.org/linux-perf-users/20240411075447.17306-1-adrian.hunter@intel.com/T/#u	

>>                                       I don't really have a good
>> alternative idea and kind of like that detecting an event is available
>> loses the evsel baggage. I would kind of like event parsing just to
>> give 1 or more perf_event_attr for similar reasons.
> 
> We have the missing feature check in the evsel open code,
> and I think we should check the exclude-bits first than others.
> Currently struct pmu has missing_features.exclude_guest only
> and it can have exclude_kernel or others too.
> 
> Anyway, I'm ok with this change.
> 
> Acked-by: Namhyung Kim <namhyung@...nel.org>
> 
> Thanks,
> Namhyung