Messages in this thread | | | Date | Mon, 14 Nov 2022 09:53:43 -0500 | Subject | Re: [PATCH v1 4/9] perf list: Generalize limiting to a PMU name | From | "Liang, Kan" <> |
| |
On 2022-11-14 9:02 a.m., Ian Rogers wrote: > On Mon, Nov 14, 2022 at 5:58 AM Liang, Kan <kan.liang@linux.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@google.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 :
| |