From: "Liang, Kan" <kan.liang@linux.intel.com>
To: Ian Rogers <irogers@google.com>
Cc: Weilin Wang <weilin.wang@intel.com>,
Perry Taylor <perry.taylor@intel.com>,
Caleb Biggers <caleb.biggers@intel.com>,
Leo Yan <leo.yan@linaro.org>,
Adrian Hunter <adrian.hunter@intel.com>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>,
Sandipan Das <sandipan.das@amd.com>,
Kajol Jain <kjain@linux.ibm.com>,
Zhengjun Xing <zhengjun.xing@linux.intel.com>,
Ravi Bangoria <ravi.bangoria@amd.com>,
Xin Gao <gaoxin@cdjrlc.com>, Rob Herring <robh@kernel.org>,
linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
Stephane Eranian <eranian@google.com>
Subject: Re: [PATCH v1 4/9] perf list: Generalize limiting to a PMU name
Date: Mon, 14 Nov 2022 14:00:39 -0500 [thread overview]
Message-ID: <83571dec-50b9-d8fd-71b8-0f870156cbd9@linux.intel.com> (raw)
In-Reply-To: <CAP-5=fVS79+oKx8P6ghhrviO_zb2Go=2bp2LV02zP32VtiiH0A@mail.gmail.com>
On 2022-11-14 12:10 p.m., Ian Rogers wrote:
> On Mon, Nov 14, 2022 at 6:55 AM Liang, Kan <kan.liang@linux.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@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.
>
> That'd be a fine follow up. I think it would be problematic as it
> would warn because of a lack of permissions.
>
Just check whether the PMU folder exists in the sysfs. I don't think it
has a permission issue.
Thanks,
Kan
> 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 :
next prev parent reply other threads:[~2022-11-14 19:00 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-14 7:51 [PATCH v1 0/9] Restructure perf list and add json output Ian Rogers
2022-11-14 7:51 ` [PATCH v1 1/9] perf pmu: Add documentation Ian Rogers
2022-11-14 8:55 ` Adrian Hunter
2022-11-14 14:10 ` Ian Rogers
2022-11-14 13:40 ` Liang, Kan
2022-11-14 14:09 ` Ian Rogers
2022-11-14 15:26 ` Liang, Kan
2022-11-14 17:04 ` Ian Rogers
2022-11-14 18:49 ` Liang, Kan
2022-11-14 7:51 ` [PATCH v1 2/9] tools lib api fs tracing_path: Add scandir alphasort Ian Rogers
2022-11-14 7:51 ` [PATCH v1 3/9] perf tracepoint: Sort events in iterator Ian Rogers
2022-11-14 7:51 ` [PATCH v1 4/9] perf list: Generalize limiting to a PMU name Ian Rogers
2022-11-14 8:51 ` Xing Zhengjun
2022-11-14 13:58 ` Ian Rogers
2022-11-14 13:57 ` Liang, Kan
2022-11-14 14:02 ` Ian Rogers
2022-11-14 14:53 ` Liang, Kan
2022-11-14 17:10 ` Ian Rogers
2022-11-14 19:00 ` Liang, Kan [this message]
2022-11-14 7:51 ` [PATCH v1 5/9] perf list: Simplify cache event printing Ian Rogers
2022-11-14 7:51 ` [PATCH v1 6/9] perf list: Simplify symbol " Ian Rogers
2022-11-14 7:51 ` [PATCH v1 7/9] perf pmu: Restructure print_pmu_events Ian Rogers
2022-11-14 7:51 ` [PATCH v1 8/9] perf list: Reorganize to use callbacks Ian Rogers
2022-11-14 7:51 ` [PATCH v1 9/9] perf list: Add json output option Ian Rogers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=83571dec-50b9-d8fd-71b8-0f870156cbd9@linux.intel.com \
--to=kan.liang@linux.intel.com \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=caleb.biggers@intel.com \
--cc=eranian@google.com \
--cc=gaoxin@cdjrlc.com \
--cc=irogers@google.com \
--cc=jolsa@kernel.org \
--cc=kjain@linux.ibm.com \
--cc=leo.yan@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=perry.taylor@intel.com \
--cc=peterz@infradead.org \
--cc=ravi.bangoria@amd.com \
--cc=robh@kernel.org \
--cc=sandipan.das@amd.com \
--cc=weilin.wang@intel.com \
--cc=zhengjun.xing@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).