linux-kernel - Re: [PATCH v1 05/10] perf evsel: Limit in group test to CPUs

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]

Message-ID: <fe88eb57-41bb-ab17-daa1-83a8cd3985b8@linux.intel.com>
Date:   Thu, 2 Mar 2023 15:28:48 -0500
From:   "Liang, Kan" <kan.liang@...ux.intel.com>
To:     Ian Rogers <irogers@...gle.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Zhengjun Xing <zhengjun.xing@...ux.intel.com>,
        Ravi Bangoria <ravi.bangoria@....com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        "Steinar H. Gunderson" <sesse@...gle.com>,
        Qi Liu <liuqi115@...wei.com>,
        Kim Phillips <kim.phillips@....com>,
        Florian Fischer <florian.fischer@...q.space>,
        James Clark <james.clark@....com>,
        Suzuki Poulouse <suzuki.poulose@....com>,
        Sean Christopherson <seanjc@...gle.com>,
        Leo Yan <leo.yan@...aro.org>,
        John Garry <john.g.garry@...cle.com>,
        Kajol Jain <kjain@...ux.ibm.com>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH v1 05/10] perf evsel: Limit in group test to CPUs



On 2023-03-02 2:38 p.m., Ian Rogers wrote:
> On Thu, Mar 2, 2023 at 7:24 AM Liang, Kan <kan.liang@...ux.intel.com> wrote:
>>
>>
>>
>> On 2023-03-01 11:12 p.m., Ian Rogers wrote:
>>> Don't just match on the event name, restict based on the PMU too.
>>>
>>> Signed-off-by: Ian Rogers <irogers@...gle.com>
>>> ---
>>>  tools/perf/arch/x86/util/evsel.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
>>> index ea3972d785d1..580b0a172136 100644
>>> --- a/tools/perf/arch/x86/util/evsel.c
>>> +++ b/tools/perf/arch/x86/util/evsel.c
>>> @@ -61,6 +61,9 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>>>       if (!evsel__sys_has_perf_metrics(evsel))
>>>               return false;
>>>
>>> +     if (evsel->pmu_name && strncmp(evsel->pmu_name, "cpu", 3))
>>> +             return false;
>>
>> I'm not sure why we want to check the pmu name. It seems better to move
>> it into evsel__sys_has_perf_metrics(), since perf_metrics is a core PMU
>> only feature.
>>
>> I think the strncmp(evsel->pmu_name, "cpu", 3) is to check whether it is
>> a core PMU. It is also used in other places. I think it's better to
>> factor out it, e.g., arch_evsel__is_core_pmu(). It will deliver a clear
>> message of what we are doing here.
>>
>> Thanks,
>> Kan
> 
> I missed the behavior of evsel__sys_has_perf_metrics and think we can
> just drop this change.

Yes, dropping the change is also OK for me.

> We should probably rename
> evsel__sys_has_perf_metrics perhaps something like
> arch_evsel__pmu_has_topdown_events.

The topdown is a tricky feature. For the big core, to support the
topdown events, we have a dedicated perf metrics register, which has to
be grouped with the fixed counter 3. That brings all of these troubles.
Sorry for that.
For the atom, the topdown events are still supported, but with GP
counters. There is no perf metrics register at all.

The evsel__sys_has_perf_metrics() is used to check whether the perf
metrics register is supported. If so, we have to specially handle it.
It's not to check whether the topdown events are supported. So I think
it's better to keep the perf_metrics name, rather than topdown_events.

Thanks,
Kan
> 
> Thanks,
> Ian
> 
>>> +
>>>       return evsel->name &&
>>>               (strcasestr(evsel->name, "slots") ||
>>>                strcasestr(evsel->name, "topdown"));