linux-kernel - Re: [PATCH v2 2/3] perf pmus: Add scan that ignores duplicates, use for perf list

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: <0e528710-c89b-1561-774b-fabc283e0e72@oracle.com>
Date:   Tue, 15 Aug 2023 09:56:28 +0100
From:   John Garry <john.g.garry@...cle.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>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Ravi Bangoria <ravi.bangoria@....com>,
        Kajol Jain <kjain@...ux.ibm.com>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] perf pmus: Add scan that ignores duplicates, use
 for perf list

On 14/08/2023 17:09, Ian Rogers wrote:
> On Mon, Aug 14, 2023 at 8:57 AM Ian Rogers<irogers@...gle.com>  wrote:
>> On Fri, Aug 11, 2023 at 8:51 AM John Garry<john.g.garry@...cle.com>  wrote:
>>> On 10/08/2023 22:49, Ian Rogers wrote:
>>>> When there are multiple PMUs that differ only by suffix, by default
>>>> just list the first one and skip all others. As the PMUs are sorted,
>>>> the scan routine checks that the PMU names match and the numbers are
>>>> consecutive. If "-v" is passed to "perf list" then list all PMUs.
>>> I really think that this should be merged with the next change. I don't
>>> like the intermediate step of by default only printing the first PMU.
>> Ack. I'll leave it as 3 patches and then leave it to Arnaldo squash as
>> quite often he wants more patches.

Why are more patches desirable? I do like the approach of taking a bite 
at a time, but we should also maintain ability to easily bisect and keep 
logical steps as one.

>>
>>>> Signed-off-by: Ian Rogers<irogers@...gle.com>
>>>> ---
>>>>    tools/perf/builtin-list.c      |  8 +++++
>>>>    tools/perf/util/pmus.c         | 54 ++++++++++++++++++++++++++++++++--
>>>>    tools/perf/util/print-events.h |  1 +
>>>>    3 files changed, 61 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
>>>> index 7fec2cca759f..8fe4ddf02c14 100644
>>>> --- a/tools/perf/builtin-list.c
>>>> +++ b/tools/perf/builtin-list.c
>>>> @@ -423,6 +423,13 @@ static void json_print_metric(void *ps __maybe_unused, const char *group,
>>>>        strbuf_release(&buf);
>>>>    }
>>>>
>>>> +static bool default_skip_duplicate_pmus(void *ps)
>>>> +{
>>>> +     struct print_state *print_state = ps;
>>>> +
>>>> +     return !print_state->long_desc;
>>>> +}
>>>> +
>>>>    int cmd_list(int argc, const char **argv)
>>>>    {
>>>>        int i, ret = 0;
>>>> @@ -434,6 +441,7 @@ int cmd_list(int argc, const char **argv)
>>>>                .print_end = default_print_end,
>>>>                .print_event = default_print_event,
>>>>                .print_metric = default_print_metric,
>>>> +             .skip_duplicate_pmus = default_skip_duplicate_pmus,
>>>>        };
>>>>        const char *cputype = NULL;
>>>>        const char *unit_name = NULL;
>>>> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
>>>> index 3581710667b0..5073843aca19 100644
>>>> --- a/tools/perf/util/pmus.c
>>>> +++ b/tools/perf/util/pmus.c
>>>> @@ -275,6 +275,50 @@ struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu)
>>>>        return NULL;
>>>>    }
>>>>
>>>> +static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
>>>> +{
>>>> +     bool use_core_pmus = !pmu || pmu->is_core;
>>>> +     int last_pmu_name_len = 0;
>>>> +     unsigned long last_pmu_num = 0;
>>>> +     const char *last_pmu_name = (pmu && pmu->name) ? pmu->name : "";
>>>> +
>>>> +     if (!pmu) {
>>>> +             pmu_read_sysfs(/*core_only=*/false);
>>>> +             pmu = list_prepare_entry(pmu, &core_pmus, list);
>>>> +     } else
>>>> +             last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &last_pmu_num);
>>>> +
>>>> +     if (use_core_pmus) {
>>>> +             list_for_each_entry_continue(pmu, &core_pmus, list) {
>>>> +                     unsigned long pmu_num = 0;
>>>> +                     int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
>>>> +
>>>> +                     if (last_pmu_name_len == pmu_name_len &&
>>>> +                         (last_pmu_num + 1 == pmu_num) &&
>>>> +                         !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
>>>> +                             last_pmu_num++;
>>>> +                             continue;
>>>> +                     }
>>>> +                     return pmu;
>>>> +             }
>>>> +             pmu = NULL;
>>> you assign pmu NULL
>>>
>>>> +             pmu = list_prepare_entry(pmu, &other_pmus, list);
>>> and then re-assign it. If list_prepare_entry() needs first arg = NULL,
>>> then can just use NULL explicitly?
>> Done.
> So because of the macro magic in list_prepare_entry you can't
> explicitly pass NULL here as doing so yields:
> tools/include/linux/kernel.h:36:33: error: request for member ‘list’
> in something not a structure or union
>    36 |         const typeof(((type *)0)->member) * __mptr = (ptr);     \
>        |                                 ^~

ok, fine, so maybe add a comment as the code looks odd..

Thanks,
John