Messages in this thread | | | Date | Mon, 4 Jul 2022 17:10:50 +0800 | Subject | Re: [PATCH] perf record: Support "--cputype" option for hybrid events | From | Xing Zhengjun <> |
| |
Hi Ian,
On 6/21/2022 1:13 PM, Xing Zhengjun wrote: > Hi Ian, > > On 6/16/2022 10:31 PM, Liang, Kan wrote: >> >> >> On 6/16/2022 4:31 AM, Xing Zhengjun wrote: >>> Hi Ian, >>> >>> On 6/15/2022 11:16 PM, Ian Rogers wrote: >>>> On Wed, Jun 15, 2022 at 8:07 AM <zhengjun.xing@linux.intel.com> wrote: >>>>> >>>>> From: Zhengjun Xing <zhengjun.xing@linux.intel.com> >>>>> >>>>> perf stat already has the "--cputype" option to enable events only >>>>> on the >>>>> specified PMU for the hybrid platform, this commit extends the >>>>> "--cputype" >>>>> support to perf record. >>>>> >>>>> Without "--cputype", it reports events for both cpu_core and cpu_atom. >>>>> >>>>> # ./perf record -e cycles -a sleep 1 | ./perf report >>>>> >>>>> # To display the perf.data header info, please use >>>>> --header/--header-only options. >>>>> # >>>>> [ perf record: Woken up 1 times to write data ] >>>>> [ perf record: Captured and wrote 0.000 MB (null) ] >>>>> # >>>>> # Total Lost Samples: 0 >>>>> # >>>>> # Samples: 335 of event 'cpu_core/cycles/' >>>>> # Event count (approx.): 35855267 >>>>> # >>>>> # Overhead Command Shared Object Symbol >>>>> # ........ ............... ................. >>>>> ......................................... >>>>> # >>>>> 10.31% swapper [kernel.kallsyms] [k] poll_idle >>>>> 9.42% swapper [kernel.kallsyms] [k] menu_select >>>>> ... ... ... ... ... >>>>> >>>>> # Samples: 61 of event 'cpu_atom/cycles/' >>>>> # Event count (approx.): 16453825 >>>>> # >>>>> # Overhead Command Shared Object Symbol >>>>> # ........ ............. ................. >>>>> ...................................... >>>>> # >>>>> 26.36% snapd [unknown] [.] 0x0000563cc6d03841 >>>>> 7.43% migration/13 [kernel.kallsyms] [k] >>>>> update_sd_lb_stats.constprop.0 >>>>> ... ... ... ... ... >>>>> >>>>> With "--cputype", it reports events only for the specified PMU. >>>>> >>>>> # ./perf record --cputype core -e cycles -a sleep 1 | ./perf report >>>>> >>>>> # To display the perf.data header info, please use >>>>> --header/--header-only options. >>>>> # >>>>> [ perf record: Woken up 1 times to write data ] >>>>> [ perf record: Captured and wrote 0.000 MB (null) ] >>>>> # >>>>> # Total Lost Samples: 0 >>>>> # >>>>> # Samples: 221 of event 'cpu_core/cycles/' >>>>> # Event count (approx.): 27121818 >>>>> # >>>>> # Overhead Command Shared Object Symbol >>>>> # ........ ............... ................. >>>>> ......................................... >>>>> # >>>>> 11.24% swapper [kernel.kallsyms] [k] e1000_irq_enable >>>>> 7.77% swapper [kernel.kallsyms] [k] >>>>> mwait_idle_with_hints.constprop.0 >>>>> ... ... ... ... ... >>>> >>>> This is already possible by having the PMU name as part of the event, >>>> cpu_atom/cycles/ or cpu_core/cycles/. I don't know why we're adding >>>> "hybrid" all over the code base, I wish it would stop. You are asking >>>> for an option here for an implied PMU for events that don't specify a >>>> PMU. The option should be called --pmutype and consider all PMUs. >> >> The --pmutype should be redundant because other PMUs have to specify >> the PMU name with the event. Only the CPU type of PMUs have events >> which doesn't have a PMU name prefix, e.g., cycles, instructions. >> So the "--cputype" was introduced for perf stat to avoid the annoying >> pmu prefix for the CPU type of PMU. >> >> This patch just extends the "--cputype" to perf record to address the >> request from the community. It reuses the existing function. >> > > Yes, "--cputype" is better. Ian, what do you think? Thanks.
Ian, Could you help to comment on it? Thanks.
> >>>> We >>>> should remove the "hybrid" PMU list and make all of the "hybrid" code >>>> generic. >> >> We already start to rework on the "hybrid" code. >> >> We recently rework the perf stat default >> https://lore.kernel.org/lkml/20220610025449.2089232-1-zhengjun.xing@linux.intel.com/ >> >> >> With the help of Ravi, we clean up the hybrid CPU in the perf header. >> https://lore.kernel.org/all/20220604044519.594-6-ravi.bangoria@amd.com/ >> >> I think Zhengjun will work on the perf record default cleanup shortly. >> >> Then I guess we can further clean up the "--cputype", e.g., removing >> hybrid_pmu_name from evlist... as next step. >> >> Thanks, >> Kan >>>> >>> >>> I can change the option name from "cputype" to "pmutype". We have >>> planned to clean up the hybrid code, and try to reduce the code with >>> "if perf_pmu__has_hybrid()", this will be done step by step, from >>> this patch, we have begun to do some clean-up, move the hybrid API >>> from the common file to the hybrid-specific files. For the clean-up, >>> Do you have any ideas? Thanks. >>> >>>> Thanks, >>>> Ian >>>> >>>>> Signed-off-by: Zhengjun Xing <zhengjun.xing@linux.intel.com> >>>>> Reviewed-by: Kan Liang <kan.liang@linux.intel.com> >>>>> --- >>>>> tools/perf/Documentation/perf-record.txt | 4 ++++ >>>>> tools/perf/builtin-record.c | 3 +++ >>>>> tools/perf/builtin-stat.c | 20 -------------------- >>>>> tools/perf/util/pmu-hybrid.c | 19 +++++++++++++++++++ >>>>> tools/perf/util/pmu-hybrid.h | 2 ++ >>>>> 5 files changed, 28 insertions(+), 20 deletions(-) >>>>> >>>>> diff --git a/tools/perf/Documentation/perf-record.txt >>>>> b/tools/perf/Documentation/perf-record.txt >>>>> index cf8ad50f3de1..ba8d680da1ac 100644 >>>>> --- a/tools/perf/Documentation/perf-record.txt >>>>> +++ b/tools/perf/Documentation/perf-record.txt >>>>> @@ -402,6 +402,10 @@ Enable weightened sampling. An additional >>>>> weight is recorded per sample and can >>>>> displayed with the weight and local_weight sort keys. This >>>>> currently works for TSX >>>>> abort events and some memory events in precise mode on modern >>>>> Intel CPUs. >>>>> >>>>> +--cputype:: >>>>> +Only enable events on applying cpu with this type for hybrid >>>>> platform(e.g. core or atom). >>>>> +For non-hybrid events, it should be no effect. >>>>> + >>>>> --namespaces:: >>>>> Record events of type PERF_RECORD_NAMESPACES. This enables >>>>> 'cgroup_id' sort key. >>>>> >>>>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c >>>>> index 9a71f0330137..e1edd4e98358 100644 >>>>> --- a/tools/perf/builtin-record.c >>>>> +++ b/tools/perf/builtin-record.c >>>>> @@ -3183,6 +3183,9 @@ static struct option __record_options[] = { >>>>> OPT_INCR('v', "verbose", &verbose, >>>>> "be more verbose (show counter open errors, >>>>> etc)"), >>>>> OPT_BOOLEAN('q', "quiet", &quiet, "don't print any message"), >>>>> + OPT_CALLBACK(0, "cputype", &record.evlist, "hybrid cpu type", >>>>> + "Only enable events on applying cpu with this >>>>> type for hybrid platform (e.g. core or atom)", >>>>> + parse_hybrid_type), >>>>> OPT_BOOLEAN('s', "stat", &record.opts.inherit_stat, >>>>> "per thread counts"), >>>>> OPT_BOOLEAN('d', "data", &record.opts.sample_address, >>>>> "Record the sample addresses"), >>>>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c >>>>> index 4ce87a8eb7d7..0d95b29273f4 100644 >>>>> --- a/tools/perf/builtin-stat.c >>>>> +++ b/tools/perf/builtin-stat.c >>>>> @@ -1184,26 +1184,6 @@ static int parse_stat_cgroups(const struct >>>>> option *opt, >>>>> return parse_cgroups(opt, str, unset); >>>>> } >>>>> >>>>> -static int parse_hybrid_type(const struct option *opt, >>>>> - const char *str, >>>>> - int unset __maybe_unused) >>>>> -{ >>>>> - struct evlist *evlist = *(struct evlist **)opt->value; >>>>> - >>>>> - if (!list_empty(&evlist->core.entries)) { >>>>> - fprintf(stderr, "Must define cputype before >>>>> events/metrics\n"); >>>>> - return -1; >>>>> - } >>>>> - >>>>> - evlist->hybrid_pmu_name = perf_pmu__hybrid_type_to_pmu(str); >>>>> - if (!evlist->hybrid_pmu_name) { >>>>> - fprintf(stderr, "--cputype %s is not supported!\n", >>>>> str); >>>>> - return -1; >>>>> - } >>>>> - >>>>> - return 0; >>>>> -} >>>>> - >>>>> static struct option stat_options[] = { >>>>> OPT_BOOLEAN('T', "transaction", &transaction_run, >>>>> "hardware transaction statistics"), >>>>> diff --git a/tools/perf/util/pmu-hybrid.c >>>>> b/tools/perf/util/pmu-hybrid.c >>>>> index f51ccaac60ee..5c490b5201b7 100644 >>>>> --- a/tools/perf/util/pmu-hybrid.c >>>>> +++ b/tools/perf/util/pmu-hybrid.c >>>>> @@ -13,6 +13,7 @@ >>>>> #include <stdarg.h> >>>>> #include <locale.h> >>>>> #include <api/fs/fs.h> >>>>> +#include "util/evlist.h" >>>>> #include "fncache.h" >>>>> #include "pmu-hybrid.h" >>>>> >>>>> @@ -87,3 +88,21 @@ char *perf_pmu__hybrid_type_to_pmu(const char >>>>> *type) >>>>> free(pmu_name); >>>>> return NULL; >>>>> } >>>>> + >>>>> +int parse_hybrid_type(const struct option *opt, const char *str, >>>>> int unset __maybe_unused) >>>>> +{ >>>>> + struct evlist *evlist = *(struct evlist **)opt->value; >>>>> + >>>>> + if (!list_empty(&evlist->core.entries)) { >>>>> + fprintf(stderr, "Must define cputype before >>>>> events/metrics\n"); >>>>> + return -1; >>>>> + } >>>>> + >>>>> + evlist->hybrid_pmu_name = perf_pmu__hybrid_type_to_pmu(str); >>>>> + if (!evlist->hybrid_pmu_name) { >>>>> + fprintf(stderr, "--cputype %s is not supported!\n", >>>>> str); >>>>> + return -1; >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> diff --git a/tools/perf/util/pmu-hybrid.h >>>>> b/tools/perf/util/pmu-hybrid.h >>>>> index 2b186c26a43e..26101f134a3a 100644 >>>>> --- a/tools/perf/util/pmu-hybrid.h >>>>> +++ b/tools/perf/util/pmu-hybrid.h >>>>> @@ -5,6 +5,7 @@ >>>>> #include <linux/perf_event.h> >>>>> #include <linux/compiler.h> >>>>> #include <linux/list.h> >>>>> +#include <subcmd/parse-options.h> >>>>> #include <stdbool.h> >>>>> #include "pmu.h" >>>>> >>>>> @@ -18,6 +19,7 @@ bool perf_pmu__hybrid_mounted(const char *name); >>>>> struct perf_pmu *perf_pmu__find_hybrid_pmu(const char *name); >>>>> bool perf_pmu__is_hybrid(const char *name); >>>>> char *perf_pmu__hybrid_type_to_pmu(const char *type); >>>>> +int parse_hybrid_type(const struct option *opt, const char *str, >>>>> int unset __maybe_unused); >>>>> >>>>> static inline int perf_pmu__hybrid_pmu_num(void) >>>>> { >>>>> -- >>>>> 2.25.1 >>>>> >>> >
-- Zhengjun Xing
| |