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] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAP-5=fVS79+oKx8P6ghhrviO_zb2Go=2bp2LV02zP32VtiiH0A@mail.gmail.com>
Date:   Mon, 14 Nov 2022 09:10:01 -0800
From:   Ian Rogers <irogers@...gle.com>
To:     "Liang, Kan" <kan.liang@...ux.intel.com>
Cc:     Weilin Wang <weilin.wang@...el.com>,
        Perry Taylor <perry.taylor@...el.com>,
        Caleb Biggers <caleb.biggers@...el.com>,
        Leo Yan <leo.yan@...aro.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        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>,
        Sandipan Das <sandipan.das@....com>,
        Kajol Jain <kjain@...ux.ibm.com>,
        Zhengjun Xing <zhengjun.xing@...ux.intel.com>,
        Ravi Bangoria <ravi.bangoria@....com>,
        Xin Gao <gaoxin@...rlc.com>, Rob Herring <robh@...nel.org>,
        linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH v1 4/9] perf list: Generalize limiting to a PMU name

On Mon, Nov 14, 2022 at 6:55 AM Liang, Kan <kan.liang@...ux.intel.com> wrote:
>
>
>
> On 2022-11-14 9:02 a.m., Ian Rogers wrote:
> > On Mon, Nov 14, 2022 at 5:58 AM Liang, Kan <kan.liang@...ux.intel.com> wrote:
> >>
> >>
> >>
> >> On 2022-11-14 2:51 a.m., Ian Rogers wrote:
> >>> Deprecate the --cputype option and add a --unit option where '--unit
> >>> cpu_atom' behaves like '--cputype atom'. The --unit option can be used
> >>> with arbitrary PMUs, for example:
> >>>
> >>> ```
> >>> $ perf list --unit msr pmu
> >>>
> >>> List of pre-defined events (to be used in -e or -M):
> >>>
> >>>   msr/aperf/                                         [Kernel PMU event]
> >>>   msr/cpu_thermal_margin/                            [Kernel PMU event]
> >>>   msr/mperf/                                         [Kernel PMU event]
> >>>   msr/pperf/                                         [Kernel PMU event]
> >>>   msr/smi/                                           [Kernel PMU event]
> >>>   msr/tsc/                                           [Kernel PMU event]
> >>> ```
> >>>
> >>> Signed-off-by: Ian Rogers <irogers@...gle.com>
> >>> ---
> >>>  tools/perf/Documentation/perf-list.txt |  6 +++---
> >>>  tools/perf/builtin-list.c              | 18 ++++++++++++------
> >>>  tools/perf/util/metricgroup.c          |  3 ++-
> >>>  tools/perf/util/pmu.c                  |  4 +---
> >>>  4 files changed, 18 insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/tools/perf/Documentation/perf-list.txt b/tools/perf/Documentation/perf-list.txt
> >>> index 57384a97c04f..44a819af573d 100644
> >>> --- a/tools/perf/Documentation/perf-list.txt
> >>> +++ b/tools/perf/Documentation/perf-list.txt
> >>> @@ -39,9 +39,9 @@ any extra expressions computed by perf stat.
> >>>  --deprecated::
> >>>  Print deprecated events. By default the deprecated events are hidden.
> >>>
> >>> ---cputype::
> >>> -Print events applying cpu with this type for hybrid platform
> >>> -(e.g. --cputype core or --cputype atom)
> >>
> >> The "--cputype" is removed from the documentation, but the code is still
> >> available. It sounds weird.
> >>
> >> Can we still keep the "--cputype" in the documentation? Just say that
> >> it's a deprecated option, please use the --unit cpu_atom instead. I
> >> think even better if we can throw a warning and point to --unit when the
> >> "--cputype" is used.
> >
> > I think we want to remove --cputype widely in the code and replace
> > what it does with specifying a PMU name. Advertising a flag but then
> > warning seems strange and is a behavioral change from what is
> > currently done. For raw-dump we don't document it in the man page and
> > hide the flag, this is the pattern being followed here.
>
> I see. So the --cputype is still supported, but only be hidden in the
> default. Sure, we can follow the pattern.
>
> >
> > Thanks,
> > Ian
> >
> >> Thanks,
> >> Kan
> >>> +--unit::
> >>> +Print PMU events and metrics limited to the specific PMU name.
> >>> +(e.g. --unit cpu, --unit msr, --unit cpu_core, --unit cpu_atom)
> >>>
> >>>  [[EVENT_MODIFIERS]]
> >>>  EVENT MODIFIERS
> >>> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> >>> index 58e1ec1654ef..cc84ced6da26 100644
> >>> --- a/tools/perf/builtin-list.c
> >>> +++ b/tools/perf/builtin-list.c
> >>> @@ -21,7 +21,6 @@
> >>>
> >>>  static bool desc_flag = true;
> >>>  static bool details_flag;
> >>> -static const char *hybrid_type;
> >>>
> >>>  int cmd_list(int argc, const char **argv)
> >>>  {
> >>> @@ -30,6 +29,8 @@ int cmd_list(int argc, const char **argv)
> >>>       bool long_desc_flag = false;
> >>>       bool deprecated = false;
> >>>       char *pmu_name = NULL;
> >>> +     const char *hybrid_name = NULL;
> >>> +     const char *unit_name = NULL;
> >>>       struct option list_options[] = {
> >>>               OPT_BOOLEAN(0, "raw-dump", &raw_dump, "Dump raw events"),
> >>>               OPT_BOOLEAN('d', "desc", &desc_flag,
> >>> @@ -40,9 +41,10 @@ int cmd_list(int argc, const char **argv)
> >>>                           "Print information on the perf event names and expressions used internally by events."),
> >>>               OPT_BOOLEAN(0, "deprecated", &deprecated,
> >>>                           "Print deprecated events."),
> >>> -             OPT_STRING(0, "cputype", &hybrid_type, "hybrid cpu type",
> >>> -                        "Print events applying cpu with this type for hybrid platform "
> >>> -                        "(e.g. core or atom)"),
> >>> +             OPT_STRING(0, "cputype", &hybrid_name, "hybrid cpu type",
> >>> +                        "Limit PMU or metric printing to the given hybrid PMU (e.g. core or atom)."),
> >>> +             OPT_STRING(0, "unit", &unit_name, "PMU name",
> >>> +                        "Limit PMU or metric printing to the specified PMU."),
> >>>               OPT_INCR(0, "debug", &verbose,
> >>>                            "Enable debugging output"),
> >>>               OPT_END()
> >>> @@ -53,6 +55,8 @@ int cmd_list(int argc, const char **argv)
> >>>       };
> >>>
> >>>       set_option_flag(list_options, 0, "raw-dump", PARSE_OPT_HIDDEN);
> >>> +     /* Hide hybrid flag for the more generic 'unit' flag. */
> >>> +     set_option_flag(list_options, 0, "cputype", PARSE_OPT_HIDDEN);
> >>>
> >>>       argc = parse_options(argc, argv, list_options, list_usage,
> >>>                            PARSE_OPT_STOP_AT_NON_OPTION);
> >>> @@ -62,8 +66,10 @@ int cmd_list(int argc, const char **argv)
> >>>       if (!raw_dump && pager_in_use())
> >>>               printf("\nList of pre-defined events (to be used in -e or -M):\n\n");
> >>>
> >>> -     if (hybrid_type) {
> >>> -             pmu_name = perf_pmu__hybrid_type_to_pmu(hybrid_type);
> >>> +     if (unit_name)
> >>> +             pmu_name = strdup(unit_name);
> >>> +     else if (hybrid_name) {
> >>> +             pmu_name = perf_pmu__hybrid_type_to_pmu(hybrid_name);
> >>>               if (!pmu_name)
> >>>                       pr_warning("WARNING: hybrid cputype is not supported!\n");
> >>>       }
>
> Can the tool implicitly convert the --cputype to --unit at the very
> beginning? (Just need to append a prefix "cpu_".). Here we only need to
> handle the unit_name.
> The same logic may be applied for other tools if someone implements the
> --unit for the stat or record later.
>
> BTW: we may want to check the existence of a PMU here, just like what we
> did for the hybrid. If a user perf list a nonexistence PMU, we can warn
> here.

That'd be a fine follow up. I think it would be problematic as it
would warn because of a lack of permissions.

Thanks,
Ian

> Thanks,
> Kan
>
> >>> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> >>> index 4c98ac29ee13..1943fed9b6d9 100644
> >>> --- a/tools/perf/util/metricgroup.c
> >>> +++ b/tools/perf/util/metricgroup.c
> >>> @@ -556,11 +556,12 @@ static int metricgroup__print_callback(const struct pmu_event *pe,
> >>>                                      void *vdata)
> >>>  {
> >>>       struct metricgroup_print_data *data = vdata;
> >>> +     const char *pmu = pe->pmu ?: "cpu";
> >>>
> >>>       if (!pe->metric_expr)
> >>>               return 0;
> >>>
> >>> -     if (data->pmu_name && perf_pmu__is_hybrid(pe->pmu) && strcmp(data->pmu_name, pe->pmu))
> >>> +     if (data->pmu_name && strcmp(data->pmu_name, pmu))
> >>>               return 0;
> >>>
> >>>       return metricgroup__print_pmu_event(pe, data->metricgroups, data->filter,
> >>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> >>> index a8f9f47c6ed9..9c771f136b81 100644
> >>> --- a/tools/perf/util/pmu.c
> >>> +++ b/tools/perf/util/pmu.c
> >>> @@ -1694,10 +1694,8 @@ void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
> >>>       pmu = NULL;
> >>>       j = 0;
> >>>       while ((pmu = perf_pmu__scan(pmu)) != NULL) {
> >>> -             if (pmu_name && perf_pmu__is_hybrid(pmu->name) &&
> >>> -                 strcmp(pmu_name, pmu->name)) {
> >>> +             if (pmu_name && pmu->name && strcmp(pmu_name, pmu->name))
> >>>                       continue;
> >>> -             }
> >>>
> >>>               list_for_each_entry(alias, &pmu->aliases, list) {
> >>>                       char *name = alias->desc ? alias->name :

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ