Received-SPF: pass (google.com: domain of linux-kernel+bounces-140128-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <7ef5951b-fab2-4364-99cd-3ad8fef2dd3f@intel.com>
Date: Thu, 11 Apr 2024 12:01:48 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] perf tools: Simplify is_event_supported()
To: Namhyung Kim <namhyung@kernel.org>, Ian Rogers <irogers@google.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>, Jiri Olsa <jolsa@kernel.org>,
 linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org
References: <20240410104450.15602-1-adrian.hunter@intel.com>
 <CAP-5=fXw+HEnyiry=6LWhpPcexbNu=CzknNfcjr=MHa78ujkpw@mail.gmail.com>
 <CAM9d7cg54QYtyOO0986ffBFU2yV49RV7OgTmX5drRAqV84aKSQ@mail.gmail.com>
Content-Language: en-US
From: Adrian Hunter <adrian.hunter@intel.com>
Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki,
 Business Identity Code: 0357606 - 4, Domiciled in Helsinki
In-Reply-To: <CAM9d7cg54QYtyOO0986ffBFU2yV49RV7OgTmX5drRAqV84aKSQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 10/04/24 20:45, Namhyung Kim wrote:
> On Wed, Apr 10, 2024 at 9:08 AM Ian Rogers <irogers@google.com> wrote:
>>
>> On Wed, Apr 10, 2024 at 3:45 AM Adrian Hunter <adrian.hunter@intel.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@intel.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@kernel.org>
> 
> Thanks,
> Namhyung