by Namhyung Kim

[permalink] [raw]

Subject: Re: [PATCH v2 01/23] perf tools: Warn if no user requested CPUs match PMU's CPUs

Hi Ian,

On Sun, May 21, 2023 at 11:43 PM Ian Rogers <[email protected]> wrote:
>
> In commit 1d3351e631fc ("perf tools: Enable on a list of CPUs for hybrid")
> perf on hybrid will warn if a user requested CPU doesn't match the PMU
> of the given event but only for hybrid PMUs. Make the logic generic
> for all PMUs and remove the hybrid logic.
>
> Warn if a CPU is requested that is offline for uncore events. Warn if
> a CPU is requested for a core PMU, but the CPU isn't within the cpu
> map of that PMU.
>
> For example on a 16 (0-15) CPU system:
> ```
> $ perf stat -e imc_free_running/data_read/,cycles -C 16 true
> WARNING: Requested CPU(s) '16' not supported by PMU 'uncore_imc_free_running_1' for event 'imc_free_running/data_read/'
> WARNING: Requested CPU(s) '16' not supported by PMU 'uncore_imc_free_running_0' for event 'imc_free_running/data_read/'
> WARNING: Requested CPU(s) '16' not supported by PMU 'cpu' for event 'cycles'
>
> Performance counter stats for 'CPU(s) 16':
>
> <not supported> MiB imc_free_running/data_read/
> <not supported> cycles
>
> 0.000570094 seconds time elapsed
> ```

I'm ok with the warning changes, but it also removed the fixup logic
for the hybrid PMUs to change the cpu map in the events. I'm not sure
if it's your intention. If so, I think you'd better splitting it into
a separate
commit with some explanation.

Thanks,
Namhyung

>
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/builtin-record.c | 6 +--
> tools/perf/builtin-stat.c | 5 +--
> tools/perf/util/cpumap.h | 2 +-
> tools/perf/util/evlist-hybrid.c | 74 ---------------------------------
> tools/perf/util/evlist-hybrid.h | 1 -
> tools/perf/util/evlist.c | 44 ++++++++++++++++++++
> tools/perf/util/evlist.h | 2 +
> tools/perf/util/pmu.c | 33 ---------------
> tools/perf/util/pmu.h | 4 --
> 9 files changed, 49 insertions(+), 122 deletions(-)
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index ec0f2d5f189f..9d212236c75a 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -4198,11 +4198,7 @@ int cmd_record(int argc, const char **argv)
> /* Enable ignoring missing threads when -u/-p option is defined. */
> rec->opts.ignore_missing_thread = rec->opts.target.uid != UINT_MAX || rec->opts.target.pid;
>
> - if (evlist__fix_hybrid_cpus(rec->evlist, rec->opts.target.cpu_list)) {
> - pr_err("failed to use cpu list %s\n",
> - rec->opts.target.cpu_list);
> - goto out;
> - }
> + evlist__warn_user_requested_cpus(rec->evlist, rec->opts.target.cpu_list);
>
> rec->opts.target.hybrid = perf_pmu__has_hybrid();
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index bc45cee3f77c..612467216306 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -2462,10 +2462,7 @@ int cmd_stat(int argc, const char **argv)
> }
> }
>
> - if (evlist__fix_hybrid_cpus(evsel_list, target.cpu_list)) {
> - pr_err("failed to use cpu list %s\n", target.cpu_list);
> - goto out;
> - }
> + evlist__warn_user_requested_cpus(evsel_list, target.cpu_list);
>
> target.hybrid = perf_pmu__has_hybrid();
> if (evlist__create_maps(evsel_list, &target) < 0) {
> diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
> index e3426541e0aa..c1de993c083f 100644
> --- a/tools/perf/util/cpumap.h
> +++ b/tools/perf/util/cpumap.h
> @@ -59,7 +59,7 @@ struct perf_cpu cpu__max_present_cpu(void);
> /**
> * cpu_map__is_dummy - Events associated with a pid, rather than a CPU, use a single dummy map with an entry of -1.
> */
> -static inline bool cpu_map__is_dummy(struct perf_cpu_map *cpus)
> +static inline bool cpu_map__is_dummy(const struct perf_cpu_map *cpus)
> {
> return perf_cpu_map__nr(cpus) == 1 && perf_cpu_map__cpu(cpus, 0).cpu == -1;
> }
> diff --git a/tools/perf/util/evlist-hybrid.c b/tools/perf/util/evlist-hybrid.c
> index 57f02beef023..db3f5fbdebe1 100644
> --- a/tools/perf/util/evlist-hybrid.c
> +++ b/tools/perf/util/evlist-hybrid.c
> @@ -86,77 +86,3 @@ bool evlist__has_hybrid(struct evlist *evlist)
>
> return false;
> }
> -
> -int evlist__fix_hybrid_cpus(struct evlist *evlist, const char *cpu_list)
> -{
> - struct perf_cpu_map *cpus;
> - struct evsel *evsel, *tmp;
> - struct perf_pmu *pmu;
> - int ret, unmatched_count = 0, events_nr = 0;
> -
> - if (!perf_pmu__has_hybrid() || !cpu_list)
> - return 0;
> -
> - cpus = perf_cpu_map__new(cpu_list);
> - if (!cpus)
> - return -1;
> -
> - /*
> - * The evsels are created with hybrid pmu's cpus. But now we
> - * need to check and adjust the cpus of evsel by cpu_list because
> - * cpu_list may cause conflicts with cpus of evsel. For example,
> - * cpus of evsel is cpu0-7, but the cpu_list is cpu6-8, we need
> - * to adjust the cpus of evsel to cpu6-7. And then propatate maps
> - * in evlist__create_maps().
> - */
> - evlist__for_each_entry_safe(evlist, tmp, evsel) {
> - struct perf_cpu_map *matched_cpus, *unmatched_cpus;
> - char buf1[128], buf2[128];
> -
> - pmu = perf_pmu__find_hybrid_pmu(evsel->pmu_name);
> - if (!pmu)
> - continue;
> -
> - ret = perf_pmu__cpus_match(pmu, cpus, &matched_cpus,
> - &unmatched_cpus);
> - if (ret)
> - goto out;
> -
> - events_nr++;
> -
> - if (perf_cpu_map__nr(matched_cpus) > 0 &&
> - (perf_cpu_map__nr(unmatched_cpus) > 0 ||
> - perf_cpu_map__nr(matched_cpus) < perf_cpu_map__nr(cpus) ||
> - perf_cpu_map__nr(matched_cpus) < perf_cpu_map__nr(pmu->cpus))) {
> - perf_cpu_map__put(evsel->core.cpus);
> - perf_cpu_map__put(evsel->core.own_cpus);
> - evsel->core.cpus = perf_cpu_map__get(matched_cpus);
> - evsel->core.own_cpus = perf_cpu_map__get(matched_cpus);
> -
> - if (perf_cpu_map__nr(unmatched_cpus) > 0) {
> - cpu_map__snprint(matched_cpus, buf1, sizeof(buf1));
> - pr_warning("WARNING: use %s in '%s' for '%s', skip other cpus in list.\n",
> - buf1, pmu->name, evsel->name);
> - }
> - }
> -
> - if (perf_cpu_map__nr(matched_cpus) == 0) {
> - evlist__remove(evlist, evsel);
> - evsel__delete(evsel);
> -
> - cpu_map__snprint(cpus, buf1, sizeof(buf1));
> - cpu_map__snprint(pmu->cpus, buf2, sizeof(buf2));
> - pr_warning("WARNING: %s isn't a '%s', please use a CPU list in the '%s' range (%s)\n",
> - buf1, pmu->name, pmu->name, buf2);
> - unmatched_count++;
> - }
> -
> - perf_cpu_map__put(matched_cpus);
> - perf_cpu_map__put(unmatched_cpus);
> - }
> - if (events_nr)
> - ret = (unmatched_count == events_nr) ? -1 : 0;
> -out:
> - perf_cpu_map__put(cpus);
> - return ret;
> -}
> diff --git a/tools/perf/util/evlist-hybrid.h b/tools/perf/util/evlist-hybrid.h
> index aacdb1b0f948..19f74b4c340a 100644
> --- a/tools/perf/util/evlist-hybrid.h
> +++ b/tools/perf/util/evlist-hybrid.h
> @@ -10,6 +10,5 @@
> int evlist__add_default_hybrid(struct evlist *evlist, bool precise);
> void evlist__warn_hybrid_group(struct evlist *evlist);
> bool evlist__has_hybrid(struct evlist *evlist);
> -int evlist__fix_hybrid_cpus(struct evlist *evlist, const char *cpu_list);
>
> #endif /* __PERF_EVLIST_HYBRID_H */
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index a0504316b06f..5d0d99127a90 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -2465,3 +2465,47 @@ void evlist__check_mem_load_aux(struct evlist *evlist)
> }
> }
> }
> +
> +/**
> + * evlist__warn_user_requested_cpus() - Check each evsel against requested CPUs
> + * and warn if the user CPU list is inapplicable for the event's PMUs
> + * CPUs. Uncore PMUs list a CPU in sysfs, but this may be overwritten by a
> + * user requested CPU and so any online CPU is applicable. Core PMUs handle
> + * events on the CPUs in their list and otherwise the event isn't supported.
> + * @evlist: The list of events being checked.
> + * @cpu_list: The user provided list of CPUs.
> + */
> +void evlist__warn_user_requested_cpus(struct evlist *evlist, const char *cpu_list)
> +{
> + struct perf_cpu_map *user_requested_cpus;
> + struct evsel *pos;
> +
> + if (!cpu_list)
> + return;
> +
> + user_requested_cpus = perf_cpu_map__new(cpu_list);
> + if (!user_requested_cpus)
> + return;
> +
> + evlist__for_each_entry(evlist, pos) {
> + const struct perf_cpu_map *to_test;
> + struct perf_cpu cpu;
> + int idx;
> + bool warn = true;
> + const struct perf_pmu *pmu = evsel__find_pmu(pos);
> +
> + to_test = pmu && pmu->is_uncore ? cpu_map__online() : evsel__cpus(pos);
> +
> + perf_cpu_map__for_each_cpu(cpu, idx, to_test) {
> + if (perf_cpu_map__has(user_requested_cpus, cpu)) {
> + warn = false;
> + break;
> + }
> + }
> + if (warn) {
> + pr_warning("WARNING: Requested CPU(s) '%s' not supported by PMU '%s' for event '%s'\n",
> + cpu_list, pmu ? pmu->name : "cpu", evsel__name(pos));
> + }
> + }
> + perf_cpu_map__put(user_requested_cpus);
> +}
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index e7e5540cc970..5e7ff44f3043 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -447,4 +447,6 @@ struct evsel *evlist__find_evsel(struct evlist *evlist, int idx);
>
> int evlist__scnprintf_evsels(struct evlist *evlist, size_t size, char *bf);
> void evlist__check_mem_load_aux(struct evlist *evlist);
> +void evlist__warn_user_requested_cpus(struct evlist *evlist, const char *cpu_list);
> +
> #endif /* __PERF_EVLIST_H */
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index f4f0afbc391c..1e0be23d4dd7 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -2038,39 +2038,6 @@ int perf_pmu__match(char *pattern, char *name, char *tok)
> return 0;
> }
>
> -int perf_pmu__cpus_match(struct perf_pmu *pmu, struct perf_cpu_map *cpus,
> - struct perf_cpu_map **mcpus_ptr,
> - struct perf_cpu_map **ucpus_ptr)
> -{
> - struct perf_cpu_map *pmu_cpus = pmu->cpus;
> - struct perf_cpu_map *matched_cpus, *unmatched_cpus;
> - struct perf_cpu cpu;
> - int i, matched_nr = 0, unmatched_nr = 0;
> -
> - matched_cpus = perf_cpu_map__default_new();
> - if (!matched_cpus)
> - return -1;
> -
> - unmatched_cpus = perf_cpu_map__default_new();
> - if (!unmatched_cpus) {
> - perf_cpu_map__put(matched_cpus);
> - return -1;
> - }
> -
> - perf_cpu_map__for_each_cpu(cpu, i, cpus) {
> - if (!perf_cpu_map__has(pmu_cpus, cpu))
> - RC_CHK_ACCESS(unmatched_cpus)->map[unmatched_nr++] = cpu;
> - else
> - RC_CHK_ACCESS(matched_cpus)->map[matched_nr++] = cpu;
> - }
> -
> - perf_cpu_map__set_nr(unmatched_cpus, unmatched_nr);
> - perf_cpu_map__set_nr(matched_cpus, matched_nr);
> - *mcpus_ptr = matched_cpus;
> - *ucpus_ptr = unmatched_cpus;
> - return 0;
> -}
> -
> double __weak perf_pmu__cpu_slots_per_cycle(void)
> {
> return NAN;
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index 0e0cb6283594..49033bb134f3 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -257,10 +257,6 @@ void perf_pmu__warn_invalid_formats(struct perf_pmu *pmu);
> bool perf_pmu__has_hybrid(void);
> int perf_pmu__match(char *pattern, char *name, char *tok);
>
> -int perf_pmu__cpus_match(struct perf_pmu *pmu, struct perf_cpu_map *cpus,
> - struct perf_cpu_map **mcpus_ptr,
> - struct perf_cpu_map **ucpus_ptr);
> -
> char *pmu_find_real_name(const char *name);
> char *pmu_find_alias_name(const char *name);
> double perf_pmu__cpu_slots_per_cycle(void);
> --
> 2.40.1.698.g37aff9b760-goog
>

2023-05-24 15:05:54

by Liang, Kan

[permalink] [raw]

Subject: Re: [PATCH v2 00/23] PMU refactoring and improvements

On 2023-05-22 2:43 a.m., Ian Rogers wrote:
> Separate the code in pmu.[ch] into the set/list of PMUs and the code
> for a particular PMU. Move the set/list of PMUs code into
> pmus.[ch]. Clean up hybrid code and remove hybrid PMU list, it is
> sufficient to scan PMUs looking for core ones. Add core PMU list and
> perf_pmus__scan_core that just reads core PMUs. Switch code that skips
> non-core PMUs during a perf_pmus__scan, to use the
> perf_pmus__scan_core variant. Don't scan sysfs for PMUs if all such
> PMUs have been previously scanned/loaded. Scanning just core PMUs, for
> the cases it is applicable, can improve the sysfs reading time by more
> than 4 fold on my laptop, as servers generally have many more uncore
> PMUs the improvement there should be larger:
>
> ```
> $ perf bench internals pmu-scan -i 1000
> Computing performance of sysfs PMU event scan for 1000 times
> Average core PMU scanning took: 989.231 usec (+- 1.535 usec)
> Average PMU scanning took: 4309.425 usec (+- 74.322 usec)
> ```
>
> The patch "perf pmu: Separate pmu and pmus" moves and renames a lot of
> functions, and is consequently large. The changes are trivial, but
> kept together to keep the overall number of patches more reasonable.
>
> v2. Address Kan's review comments wrt "cycles" -> "cycles:P" and
> "uncore_pmus" -> "other_pmus".
>
> Ian Rogers (23):
> perf tools: Warn if no user requested CPUs match PMU's CPUs
> perf evlist: Remove evlist__warn_hybrid_group
> perf evlist: Remove __evlist__add_default
> perf evlist: Reduce scope of evlist__has_hybrid
> perf pmu: Remove perf_pmu__hybrid_mounted
> perf pmu: Detect ARM and hybrid PMUs with sysfs
> perf pmu: Add is_core to pmu
> perf pmu: Rewrite perf_pmu__has_hybrid to avoid list
> perf x86: Iterate hybrid PMUs as core PMUs
> perf topology: Avoid hybrid list for hybrid topology
> perf evsel: Compute is_hybrid from PMU being core
> perf header: Avoid hybrid PMU list in write_pmu_caps
> perf metrics: Remove perf_pmu__is_hybrid use
> perf stat: Avoid hybrid PMU list
> perf mem: Avoid hybrid PMU list
> perf pmu: Remove perf_pmu__hybrid_pmus list
> perf pmus: Prefer perf_pmu__scan over perf_pmus__for_each_pmu
> perf x86 mem: minor refactor to is_mem_loads_aux_event
> perf pmu: Separate pmu and pmus
> perf pmus: Split pmus list into core and other
> perf pmus: Allow just core PMU scanning
> perf pmus: Avoid repeated sysfs scanning
> perf pmus: Ensure all PMUs are read for find_by_type

The patch set also triggers Segmentation fault with default mode on my
hybrid machine.

# ./perf stat sleep 1

Performance counter stats for 'sleep 1':

0.53 msec task-clock # 0.001 CPUs
utilized
1 context-switches # 1.875 K/sec
0 cpu-migrations # 0.000 /sec
68 page-faults # 127.476 K/sec
Segmentation fault (core dumped)

Program received signal SIGSEGV, Segmentation fault.
evsel__is_hybrid (evsel=0x55555609a1a0) at util/evsel.c:3143
3143 return pmu->is_core;
(gdb) backtrace
#0 evsel__is_hybrid (evsel=0x55555609a1a0) at util/evsel.c:3143
#1 evsel__is_hybrid (evsel=evsel@entry=0x55555609a1a0) at util/evsel.c:3135
#2 0x0000555555759468 in hybrid_uniquify (config=0x555555f931e0
<stat_config>, evsel=0x55555609a1a0)
at util/stat-display.c:813
#3 uniquify_counter (counter=0x55555609a1a0, config=0x555555f931e0
<stat_config>) at util/stat-display.c:818
#4 print_counter_aggrdata (config=config@entry=0x555555f931e0
<stat_config>,
counter=counter@entry=0x55555609a1a0, aggr_idx=aggr_idx@entry=0,
os=os@entry=0x7fffffff8fe0)
at util/stat-display.c:888
#5 0x000055555575b119 in print_counter (os=<optimized out>,
counter=<optimized out>, config=<optimized out>)
at util/stat-display.c:1019
#6 print_counter (os=0x7fffffff8fe0, counter=0x55555609a1a0,
config=0x555555f931e0 <stat_config>)
at util/stat-display.c:1009
#7 evlist__print_counters (evlist=0x555556029da0,
config=config@entry=0x555555f931e0 <stat_config>,
_target=_target@entry=0x555555f42de0 <target>, ts=ts@entry=0x0,
argc=argc@entry=2,
argv=argv@entry=0x7fffffffe1d0) at util/stat-display.c:1480
#8 0x000055555562009c in print_counters (argv=0x7fffffffe1d0, argc=2,
ts=0x0) at builtin-stat.c:979
#9 print_counters (argv=0x7fffffffe1d0, argc=2, ts=0x0) at
builtin-stat.c:971
#10 cmd_stat (argc=2, argv=0x7fffffffe1d0) at builtin-stat.c:2832
#11 0x00005555556b6670 in run_builtin (p=p@entry=0x555555f9c590
<commands+336>, argc=argc@entry=3,
argv=argv@entry=0x7fffffffe1d0) at perf.c:323
#12 0x00005555555ff2d9 in handle_internal_command (argv=0x7fffffffe1d0,
argc=3) at perf.c:377
#13 run_argv (argv=<synthetic pointer>, argcp=<synthetic pointer>) at
perf.c:421
#14 main (argc=3, argv=0x7fffffffe1d0) at perf.c:537

Thanks,
Kan
>
> tools/perf/arch/arm/util/auxtrace.c | 7 +-
> tools/perf/arch/arm/util/cs-etm.c | 4 +-
> tools/perf/arch/arm64/util/pmu.c | 6 +-
> tools/perf/arch/x86/tests/hybrid.c | 7 +-
> tools/perf/arch/x86/util/auxtrace.c | 5 +-
> tools/perf/arch/x86/util/evlist.c | 25 +-
> tools/perf/arch/x86/util/evsel.c | 27 +-
> tools/perf/arch/x86/util/intel-bts.c | 4 +-
> tools/perf/arch/x86/util/intel-pt.c | 4 +-
> tools/perf/arch/x86/util/mem-events.c | 17 +-
> tools/perf/arch/x86/util/perf_regs.c | 15 +-
> tools/perf/arch/x86/util/topdown.c | 5 +-
> tools/perf/bench/pmu-scan.c | 60 ++--
> tools/perf/builtin-c2c.c | 9 +-
> tools/perf/builtin-list.c | 4 +-
> tools/perf/builtin-mem.c | 9 +-
> tools/perf/builtin-record.c | 29 +-
> tools/perf/builtin-stat.c | 15 +-
> tools/perf/builtin-top.c | 10 +-
> tools/perf/tests/attr.c | 4 +-
> tools/perf/tests/event_groups.c | 7 +-
> tools/perf/tests/parse-events.c | 15 +-
> tools/perf/tests/parse-metric.c | 4 +-
> tools/perf/tests/pmu-events.c | 6 +-
> tools/perf/tests/switch-tracking.c | 4 +-
> tools/perf/tests/topology.c | 4 +-
> tools/perf/util/Build | 2 -
> tools/perf/util/cpumap.h | 2 +-
> tools/perf/util/cputopo.c | 16 +-
> tools/perf/util/env.c | 5 +-
> tools/perf/util/evlist-hybrid.c | 162 ---------
> tools/perf/util/evlist-hybrid.h | 15 -
> tools/perf/util/evlist.c | 67 +++-
> tools/perf/util/evlist.h | 9 +-
> tools/perf/util/evsel.c | 57 +--
> tools/perf/util/evsel.h | 3 -
> tools/perf/util/header.c | 27 +-
> tools/perf/util/mem-events.c | 17 +-
> tools/perf/util/metricgroup.c | 9 +-
> tools/perf/util/parse-events.c | 24 +-
> tools/perf/util/parse-events.y | 3 +-
> tools/perf/util/pfm.c | 6 +-
> tools/perf/util/pmu-hybrid.c | 52 ---
> tools/perf/util/pmu-hybrid.h | 32 --
> tools/perf/util/pmu.c | 482 ++------------------------
> tools/perf/util/pmu.h | 26 +-
> tools/perf/util/pmus.c | 477 ++++++++++++++++++++++++-
> tools/perf/util/pmus.h | 15 +-
> tools/perf/util/print-events.c | 15 +-
> tools/perf/util/python-ext-sources | 1 -
> tools/perf/util/stat-display.c | 21 +-
> 51 files changed, 819 insertions(+), 1032 deletions(-)
> delete mode 100644 tools/perf/util/evlist-hybrid.c
> delete mode 100644 tools/perf/util/evlist-hybrid.h
> delete mode 100644 tools/perf/util/pmu-hybrid.c
> delete mode 100644 tools/perf/util/pmu-hybrid.h
>