| 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
| ||
|
Message-ID: <2560dc81-7ff9-f907-9041-b309a8ef5509@linux.intel.com>
Date: Thu, 16 Jun 2022 10:31:43 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Xing Zhengjun <zhengjun.xing@...ux.intel.com>,
Ian Rogers <irogers@...gle.com>
Cc: acme@...nel.org, peterz@...radead.org, mingo@...hat.com,
alexander.shishkin@...el.com, jolsa@...nel.org,
namhyung@...nel.org, linux-kernel@...r.kernel.org,
linux-perf-users@...r.kernel.org, ak@...ux.intel.com
Subject: Re: [PATCH] perf record: Support "--cputype" option for hybrid events
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@...ux.intel.com> wrote:
>>>
>>> From: Zhengjun Xing <zhengjun.xing@...ux.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.
>> 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@...ux.intel.com>
>>> Reviewed-by: Kan Liang <kan.liang@...ux.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
>>>
>
Powered by blists - more mailing lists