| 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
| ||
|
Date: Mon, 14 Nov 2022 09:53:43 -0500
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Ian Rogers <irogers@...gle.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 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.
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