* [PATCH v2 1/2] perf stat: Move create_perf_stat_counter to builtin-stat @ 2025-10-02 22:07 Ian Rogers 2025-10-02 22:07 ` [PATCH v2 2/2] perf stat: Refactor retry/skip/fatal error handling Ian Rogers 2025-10-03 11:31 ` [PATCH v2 1/2] perf stat: Move create_perf_stat_counter to builtin-stat James Clark 0 siblings, 2 replies; 6+ messages in thread From: Ian Rogers @ 2025-10-02 22:07 UTC (permalink / raw) To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter, Kan Liang, James Clark, Howard Chu, Thomas Falcon, Chun-Tse Shao, Dapeng Mi, linux-perf-users, linux-kernel The function create_perf_stat_counter is only used in builtin-stat.c and contains logic about retrying events specific to builtin-stat.c. Move the code to builtin-stat to tidy this up. Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/builtin-stat.c | 60 +++++++++++++++++++++++++++++++++++++-- tools/perf/util/stat.c | 56 ------------------------------------ tools/perf/util/stat.h | 4 --- 3 files changed, 58 insertions(+), 62 deletions(-) diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c index ab567919b89a..75b9979c6c05 100644 --- a/tools/perf/builtin-stat.c +++ b/tools/perf/builtin-stat.c @@ -676,6 +676,62 @@ static enum counter_recovery stat_handle_error(struct evsel *counter, int err) return COUNTER_FATAL; } +static int create_perf_stat_counter(struct evsel *evsel, + struct perf_stat_config *config, + int cpu_map_idx) +{ + struct perf_event_attr *attr = &evsel->core.attr; + struct evsel *leader = evsel__leader(evsel); + + /* Reset supported flag as creating a stat counter is retried. */ + attr->read_format = PERF_FORMAT_TOTAL_TIME_ENABLED | + PERF_FORMAT_TOTAL_TIME_RUNNING; + + /* + * The event is part of non trivial group, let's enable + * the group read (for leader) and ID retrieval for all + * members. + */ + if (leader->core.nr_members > 1) + attr->read_format |= PERF_FORMAT_ID|PERF_FORMAT_GROUP; + + attr->inherit = !config->no_inherit && list_empty(&evsel->bpf_counter_list); + + /* + * Some events get initialized with sample_(period/type) set, + * like tracepoints. Clear it up for counting. + */ + attr->sample_period = 0; + + if (config->identifier) + attr->sample_type = PERF_SAMPLE_IDENTIFIER; + + if (config->all_user) { + attr->exclude_kernel = 1; + attr->exclude_user = 0; + } + + if (config->all_kernel) { + attr->exclude_kernel = 0; + attr->exclude_user = 1; + } + + /* + * Disabling all counters initially, they will be enabled + * either manually by us or by kernel via enable_on_exec + * set later. + */ + if (evsel__is_group_leader(evsel)) { + attr->disabled = 1; + + if (target__enable_on_exec(&target)) + attr->enable_on_exec = 1; + } + + return evsel__open_per_cpu_and_thread(evsel, evsel__cpus(evsel), cpu_map_idx, + evsel->core.threads); +} + static int __run_perf_stat(int argc, const char **argv, int run_idx) { int interval = stat_config.interval; @@ -736,7 +792,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) if (evsel__is_bperf(counter)) continue; try_again: - if (create_perf_stat_counter(counter, &stat_config, &target, + if (create_perf_stat_counter(counter, &stat_config, evlist_cpu_itr.cpu_map_idx) < 0) { /* @@ -794,7 +850,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) continue; try_again_reset: pr_debug2("reopening weak %s\n", evsel__name(counter)); - if (create_perf_stat_counter(counter, &stat_config, &target, + if (create_perf_stat_counter(counter, &stat_config, evlist_cpu_itr.cpu_map_idx) < 0) { switch (stat_handle_error(counter, errno)) { diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c index 50b1a92d16df..101ed6c497bc 100644 --- a/tools/perf/util/stat.c +++ b/tools/perf/util/stat.c @@ -716,59 +716,3 @@ size_t perf_event__fprintf_stat_config(union perf_event *event, FILE *fp) return ret; } - -int create_perf_stat_counter(struct evsel *evsel, - struct perf_stat_config *config, - struct target *target, - int cpu_map_idx) -{ - struct perf_event_attr *attr = &evsel->core.attr; - struct evsel *leader = evsel__leader(evsel); - - attr->read_format = PERF_FORMAT_TOTAL_TIME_ENABLED | - PERF_FORMAT_TOTAL_TIME_RUNNING; - - /* - * The event is part of non trivial group, let's enable - * the group read (for leader) and ID retrieval for all - * members. - */ - if (leader->core.nr_members > 1) - attr->read_format |= PERF_FORMAT_ID|PERF_FORMAT_GROUP; - - attr->inherit = !config->no_inherit && list_empty(&evsel->bpf_counter_list); - - /* - * Some events get initialized with sample_(period/type) set, - * like tracepoints. Clear it up for counting. - */ - attr->sample_period = 0; - - if (config->identifier) - attr->sample_type = PERF_SAMPLE_IDENTIFIER; - - if (config->all_user) { - attr->exclude_kernel = 1; - attr->exclude_user = 0; - } - - if (config->all_kernel) { - attr->exclude_kernel = 0; - attr->exclude_user = 1; - } - - /* - * Disabling all counters initially, they will be enabled - * either manually by us or by kernel via enable_on_exec - * set later. - */ - if (evsel__is_group_leader(evsel)) { - attr->disabled = 1; - - if (target__enable_on_exec(target)) - attr->enable_on_exec = 1; - } - - return evsel__open_per_cpu_and_thread(evsel, evsel__cpus(evsel), cpu_map_idx, - evsel->core.threads); -} diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h index 4b0f14ae4e5f..34f30a295f89 100644 --- a/tools/perf/util/stat.h +++ b/tools/perf/util/stat.h @@ -223,10 +223,6 @@ size_t perf_event__fprintf_stat(union perf_event *event, FILE *fp); size_t perf_event__fprintf_stat_round(union perf_event *event, FILE *fp); size_t perf_event__fprintf_stat_config(union perf_event *event, FILE *fp); -int create_perf_stat_counter(struct evsel *evsel, - struct perf_stat_config *config, - struct target *target, - int cpu_map_idx); void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *config, struct target *_target, struct timespec *ts, int argc, const char **argv); -- 2.51.0.618.g983fd99d29-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] perf stat: Refactor retry/skip/fatal error handling 2025-10-02 22:07 [PATCH v2 1/2] perf stat: Move create_perf_stat_counter to builtin-stat Ian Rogers @ 2025-10-02 22:07 ` Ian Rogers 2025-10-03 12:03 ` James Clark 2025-10-03 11:31 ` [PATCH v2 1/2] perf stat: Move create_perf_stat_counter to builtin-stat James Clark 1 sibling, 1 reply; 6+ messages in thread From: Ian Rogers @ 2025-10-02 22:07 UTC (permalink / raw) To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter, Kan Liang, James Clark, Howard Chu, Thomas Falcon, Chun-Tse Shao, Dapeng Mi, linux-perf-users, linux-kernel For the sake of Intel topdown events commit 9eac5612da1c ("perf stat: Don't skip failing group events") changed perf stat error handling making it so that more errors were fatal and didn't report "<not supported>" events. The change outside of topdown events was unintentional. The notion of "fatal" error handling was introduced in commit e0e6a6ca3ac2 ("perf stat: Factor out open error handling") and refined in commits like commit cb5ef60067c1 ("perf stat: Error out unsupported group leader immediately) to be an approach for avoiding later assertion failures in the code base. This change fixes those issues and removes the notion of a fatal error on an event. If all events fail to open then a fatal error occurs with the previous fatal error message. This seems to best match the notion of supported events and allowing some errors not to stop perf stat, while allowing the truly fatal no event case to terminate the tool early. The evsel->errored flag is only used in the stat code but always just meaning !evsel->supported although there is a comment about it being sticky. Force all evsels to be supported in evsel__init and then clear this when evsel__open fails. When an event is tried the supported is set to true again. This simplifies the notion of whether an evsel is broken. In the get_group_fd code, fail to get a group fd when the evsel isn't supported. If the leader isn't supported then it is also expected that there is no group_fd as the leader will have been skipped. Therefore change the BUG_ON test to be on supported rather than skippable. This corrects the assertion errors that were the reason for the previous fatal error handling. Fixes: 9eac5612da1c ("perf stat: Don't skip failing group events") Signed-off-by: Ian Rogers <irogers@google.com> --- v2: Missed setting supported for weak groups. When no supported events, report the error on the first event in the evlist. An earlier version of this fix exists in: https://lore.kernel.org/lkml/20250923223312.238185-2-irogers@google.com/ This version is more thorough and tries to make the overall code base more consistent. --- tools/perf/builtin-record.c | 2 - tools/perf/builtin-stat.c | 123 +++++++++++++++--------------------- tools/perf/util/evsel.c | 40 +++++++----- tools/perf/util/evsel.h | 1 - 4 files changed, 76 insertions(+), 90 deletions(-) diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c index 7ea3a11aca70..d76f01956e33 100644 --- a/tools/perf/builtin-record.c +++ b/tools/perf/builtin-record.c @@ -1408,8 +1408,6 @@ static int record__open(struct record *rec) ui__error("%s\n", msg); goto out; } - - pos->supported = true; } if (symbol_conf.kptr_restrict && !evlist__exclude_kernel(evlist)) { diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c index 75b9979c6c05..7006f848f87a 100644 --- a/tools/perf/builtin-stat.c +++ b/tools/perf/builtin-stat.c @@ -610,22 +610,13 @@ static int dispatch_events(bool forks, int timeout, int interval, int *times) enum counter_recovery { COUNTER_SKIP, COUNTER_RETRY, - COUNTER_FATAL, }; static enum counter_recovery stat_handle_error(struct evsel *counter, int err) { char msg[BUFSIZ]; - if (counter->skippable) { - if (verbose > 0) { - ui__warning("skipping event %s that kernel failed to open .\n", - evsel__name(counter)); - } - counter->supported = false; - counter->errored = true; - return COUNTER_SKIP; - } + assert(!counter->supported); /* * PPC returns ENXIO for HW counters until 2.6.37 @@ -636,19 +627,16 @@ static enum counter_recovery stat_handle_error(struct evsel *counter, int err) ui__warning("%s event is not supported by the kernel.\n", evsel__name(counter)); } - counter->supported = false; - /* - * errored is a sticky flag that means one of the counter's - * cpu event had a problem and needs to be reexamined. - */ - counter->errored = true; - } else if (evsel__fallback(counter, &target, err, msg, sizeof(msg))) { + return COUNTER_SKIP; + } + if (evsel__fallback(counter, &target, err, msg, sizeof(msg))) { if (verbose > 0) ui__warning("%s\n", msg); + counter->supported = true; return COUNTER_RETRY; - } else if (target__has_per_thread(&target) && err != EOPNOTSUPP && - evsel_list->core.threads && - evsel_list->core.threads->err_thread != -1) { + } + if (target__has_per_thread(&target) && err != EOPNOTSUPP && + evsel_list->core.threads && evsel_list->core.threads->err_thread != -1) { /* * For global --per-thread case, skip current * error thread. @@ -656,24 +644,17 @@ static enum counter_recovery stat_handle_error(struct evsel *counter, int err) if (!thread_map__remove(evsel_list->core.threads, evsel_list->core.threads->err_thread)) { evsel_list->core.threads->err_thread = -1; + counter->supported = true; return COUNTER_RETRY; } - } else if (err == EOPNOTSUPP) { - if (verbose > 0) { - ui__warning("%s event is not supported by the kernel.\n", - evsel__name(counter)); - } - counter->supported = false; - counter->errored = true; } - - evsel__open_strerror(counter, &target, err, msg, sizeof(msg)); - ui__error("%s\n", msg); - - if (child_pid != -1) - kill(child_pid, SIGTERM); - - return COUNTER_FATAL; + if (verbose > 0) { + ui__warning(err == EOPNOTSUPP + ? "%s event is not supported by the kernel.\n" + : "skipping event %s that kernel failed to open.\n", + evsel__name(counter)); + } + return COUNTER_SKIP; } static int create_perf_stat_counter(struct evsel *evsel, @@ -746,8 +727,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) bool is_pipe = STAT_RECORD ? perf_stat.data.is_pipe : false; struct evlist_cpu_iterator evlist_cpu_itr; struct affinity saved_affinity, *affinity = NULL; - int err; - bool second_pass = false; + int err, open_err = 0; + bool second_pass = false, has_supported_counters; if (forks) { if (evlist__prepare_workload(evsel_list, &target, argv, is_pipe, workload_exec_failed_signal) < 0) { @@ -787,14 +768,17 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) if (target.use_bpf) break; - if (counter->reset_group || counter->errored) + if (counter->reset_group || !counter->supported) continue; if (evsel__is_bperf(counter)) continue; -try_again: - if (create_perf_stat_counter(counter, &stat_config, - evlist_cpu_itr.cpu_map_idx) < 0) { + while (true) { + if (create_perf_stat_counter(counter, &stat_config, + evlist_cpu_itr.cpu_map_idx) == 0) + break; + + open_err = errno; /* * Weak group failed. We cannot just undo this here * because earlier CPUs might be in group mode, and the kernel @@ -802,29 +786,19 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) * it to later. * Don't close here because we're in the wrong affinity. */ - if ((errno == EINVAL || errno == EBADF) && + if ((open_err == EINVAL || open_err == EBADF) && evsel__leader(counter) != counter && counter->weak_group) { evlist__reset_weak_group(evsel_list, counter, false); assert(counter->reset_group); + counter->supported = true; second_pass = true; - continue; - } - - switch (stat_handle_error(counter, errno)) { - case COUNTER_FATAL: - err = -1; - goto err_out; - case COUNTER_RETRY: - goto try_again; - case COUNTER_SKIP: - continue; - default: break; } + if (stat_handle_error(counter, open_err) != COUNTER_RETRY) + break; } - counter->supported = true; } if (second_pass) { @@ -837,7 +811,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) evlist__for_each_cpu(evlist_cpu_itr, evsel_list, affinity) { counter = evlist_cpu_itr.evsel; - if (!counter->reset_group && !counter->errored) + if (!counter->reset_group && counter->supported) continue; perf_evsel__close_cpu(&counter->core, evlist_cpu_itr.cpu_map_idx); @@ -848,34 +822,29 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) if (!counter->reset_group) continue; -try_again_reset: - pr_debug2("reopening weak %s\n", evsel__name(counter)); - if (create_perf_stat_counter(counter, &stat_config, - evlist_cpu_itr.cpu_map_idx) < 0) { - - switch (stat_handle_error(counter, errno)) { - case COUNTER_FATAL: - err = -1; - goto err_out; - case COUNTER_RETRY: - goto try_again_reset; - case COUNTER_SKIP: - continue; - default: + + while (true) { + pr_debug2("reopening weak %s\n", evsel__name(counter)); + if (create_perf_stat_counter(counter, &stat_config, + evlist_cpu_itr.cpu_map_idx) == 0) + break; + + open_err = errno; + if (stat_handle_error(counter, open_err) != COUNTER_RETRY) break; - } } - counter->supported = true; } } affinity__cleanup(affinity); affinity = NULL; + has_supported_counters = false; evlist__for_each_entry(evsel_list, counter) { if (!counter->supported) { perf_evsel__free_fd(&counter->core); continue; } + has_supported_counters = true; l = strlen(counter->unit); if (l > stat_config.unit_width) @@ -887,6 +856,16 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) goto err_out; } } + if (!has_supported_counters) { + evsel__open_strerror(evlist__first(evsel_list), &target, open_err, + msg, sizeof(msg)); + ui__error("No supported events found.\n%s\n", msg); + + if (child_pid != -1) + kill(child_pid, SIGTERM); + err = -1; + goto err_out; + } if (evlist__apply_filters(evsel_list, &counter, &target)) { pr_err("failed to set filter \"%s\" on event %s with %d (%s)\n", diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c index 1a29d4f47bbf..fe93f11cf3d1 100644 --- a/tools/perf/util/evsel.c +++ b/tools/perf/util/evsel.c @@ -407,6 +407,7 @@ void evsel__init(struct evsel *evsel, evsel->collect_stat = false; evsel->group_pmu_name = NULL; evsel->skippable = false; + evsel->supported = true; evsel->alternate_hw_config = PERF_COUNT_HW_MAX; evsel->script_output_type = -1; // FIXME: OUTPUT_TYPE_UNSET, see builtin-script.c } @@ -1941,7 +1942,7 @@ static int get_group_fd(struct evsel *evsel, int cpu_map_idx, int thread) struct evsel *leader = evsel__leader(evsel); int fd; - if (evsel__is_group_leader(evsel)) + if (!evsel->supported || evsel__is_group_leader(evsel)) return -1; /* @@ -1955,7 +1956,7 @@ static int get_group_fd(struct evsel *evsel, int cpu_map_idx, int thread) return -1; fd = FD(leader, cpu_map_idx, thread); - BUG_ON(fd == -1 && !leader->skippable); + BUG_ON(fd == -1 && leader->supported); /* * When the leader has been skipped, return -2 to distinguish from no @@ -2573,12 +2574,14 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus, enum rlimit_action set_rlimit = NO_CHANGE; struct perf_cpu cpu; - if (evsel__is_retire_lat(evsel)) - return evsel__tpebs_open(evsel); + if (evsel__is_retire_lat(evsel)) { + err = evsel__tpebs_open(evsel); + goto out; + } err = __evsel__prepare_open(evsel, cpus, threads); if (err) - return err; + goto out; if (cpus == NULL) cpus = empty_cpu_map; @@ -2598,19 +2601,22 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus, display_attr(&evsel->core.attr); if (evsel__is_tool(evsel)) { - return evsel__tool_pmu_open(evsel, threads, - start_cpu_map_idx, - end_cpu_map_idx); + err = evsel__tool_pmu_open(evsel, threads, + start_cpu_map_idx, + end_cpu_map_idx); + goto out; } if (evsel__is_hwmon(evsel)) { - return evsel__hwmon_pmu_open(evsel, threads, - start_cpu_map_idx, - end_cpu_map_idx); + err = evsel__hwmon_pmu_open(evsel, threads, + start_cpu_map_idx, + end_cpu_map_idx); + goto out; } if (evsel__is_drm(evsel)) { - return evsel__drm_pmu_open(evsel, threads, - start_cpu_map_idx, - end_cpu_map_idx); + err = evsel__drm_pmu_open(evsel, threads, + start_cpu_map_idx, + end_cpu_map_idx); + goto out; } for (idx = start_cpu_map_idx; idx < end_cpu_map_idx; idx++) { @@ -2689,7 +2695,8 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus, } } - return 0; + err = 0; + goto out; try_fallback: if (evsel__ignore_missing_thread(evsel, perf_cpu_map__nr(cpus), @@ -2728,6 +2735,9 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus, thread = nthreads; } while (--idx >= 0); errno = old_errno; +out: + if (err) + evsel->supported = false; return err; } diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h index 03f9f22e3a0c..1ad0b3fcd559 100644 --- a/tools/perf/util/evsel.h +++ b/tools/perf/util/evsel.h @@ -121,7 +121,6 @@ struct evsel { bool forced_leader; bool cmdline_group_boundary; bool reset_group; - bool errored; bool needs_auxtrace_mmap; bool default_metricgroup; /* A member of the Default metricgroup */ bool needs_uniquify; -- 2.51.0.618.g983fd99d29-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] perf stat: Refactor retry/skip/fatal error handling 2025-10-02 22:07 ` [PATCH v2 2/2] perf stat: Refactor retry/skip/fatal error handling Ian Rogers @ 2025-10-03 12:03 ` James Clark 2025-10-03 19:08 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 6+ messages in thread From: James Clark @ 2025-10-03 12:03 UTC (permalink / raw) To: Ian Rogers Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Alexander Shishkin, Jiri Olsa, Adrian Hunter, Kan Liang, Howard Chu, Thomas Falcon, Chun-Tse Shao, Dapeng Mi, linux-perf-users, linux-kernel On 02/10/2025 11:07 pm, Ian Rogers wrote: > For the sake of Intel topdown events commit 9eac5612da1c ("perf stat: > Don't skip failing group events") changed perf stat error handling > making it so that more errors were fatal and didn't report "<not > supported>" events. The change outside of topdown events was > unintentional. > > The notion of "fatal" error handling was introduced in commit > e0e6a6ca3ac2 ("perf stat: Factor out open error handling") and refined > in commits like commit cb5ef60067c1 ("perf stat: Error out unsupported > group leader immediately) to be an approach for avoiding later > assertion failures in the code base. This change fixes those issues > and removes the notion of a fatal error on an event. If all events > fail to open then a fatal error occurs with the previous fatal error > message. This seems to best match the notion of supported events and > allowing some errors not to stop perf stat, while allowing the truly > fatal no event case to terminate the tool early. > > The evsel->errored flag is only used in the stat code but always just > meaning !evsel->supported although there is a comment about it being > sticky. Force all evsels to be supported in evsel__init and then clear > this when evsel__open fails. When an event is tried the supported is > set to true again. This simplifies the notion of whether an evsel is > broken. > > In the get_group_fd code, fail to get a group fd when the evsel isn't > supported. If the leader isn't supported then it is also expected that > there is no group_fd as the leader will have been skipped. Therefore > change the BUG_ON test to be on supported rather than skippable. This > corrects the assertion errors that were the reason for the previous > fatal error handling. > > Fixes: 9eac5612da1c ("perf stat: Don't skip failing group events") > Signed-off-by: Ian Rogers <irogers@google.com> > --- > v2: Missed setting supported for weak groups. When no supported > events, report the error on the first event in the evlist. > > An earlier version of this fix exists in: > https://lore.kernel.org/lkml/20250923223312.238185-2-irogers@google.com/ > This version is more thorough and tries to make the overall code base > more consistent. > --- Reviewed-by: James Clark <james.clark@linaro.org> > tools/perf/builtin-record.c | 2 - > tools/perf/builtin-stat.c | 123 +++++++++++++++--------------------- > tools/perf/util/evsel.c | 40 +++++++----- > tools/perf/util/evsel.h | 1 - > 4 files changed, 76 insertions(+), 90 deletions(-) > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > index 7ea3a11aca70..d76f01956e33 100644 > --- a/tools/perf/builtin-record.c > +++ b/tools/perf/builtin-record.c > @@ -1408,8 +1408,6 @@ static int record__open(struct record *rec) > ui__error("%s\n", msg); > goto out; > } > - > - pos->supported = true; > } > > if (symbol_conf.kptr_restrict && !evlist__exclude_kernel(evlist)) { > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c > index 75b9979c6c05..7006f848f87a 100644 > --- a/tools/perf/builtin-stat.c > +++ b/tools/perf/builtin-stat.c > @@ -610,22 +610,13 @@ static int dispatch_events(bool forks, int timeout, int interval, int *times) > enum counter_recovery { > COUNTER_SKIP, > COUNTER_RETRY, > - COUNTER_FATAL, > }; > > static enum counter_recovery stat_handle_error(struct evsel *counter, int err) > { > char msg[BUFSIZ]; > > - if (counter->skippable) { > - if (verbose > 0) { > - ui__warning("skipping event %s that kernel failed to open .\n", > - evsel__name(counter)); > - } > - counter->supported = false; > - counter->errored = true; > - return COUNTER_SKIP; > - } > + assert(!counter->supported); > > /* > * PPC returns ENXIO for HW counters until 2.6.37 > @@ -636,19 +627,16 @@ static enum counter_recovery stat_handle_error(struct evsel *counter, int err) > ui__warning("%s event is not supported by the kernel.\n", > evsel__name(counter)); > } > - counter->supported = false; > - /* > - * errored is a sticky flag that means one of the counter's > - * cpu event had a problem and needs to be reexamined. > - */ > - counter->errored = true; > - } else if (evsel__fallback(counter, &target, err, msg, sizeof(msg))) { > + return COUNTER_SKIP; > + } > + if (evsel__fallback(counter, &target, err, msg, sizeof(msg))) { > if (verbose > 0) > ui__warning("%s\n", msg); > + counter->supported = true; > return COUNTER_RETRY; > - } else if (target__has_per_thread(&target) && err != EOPNOTSUPP && > - evsel_list->core.threads && > - evsel_list->core.threads->err_thread != -1) { > + } > + if (target__has_per_thread(&target) && err != EOPNOTSUPP && > + evsel_list->core.threads && evsel_list->core.threads->err_thread != -1) { > /* > * For global --per-thread case, skip current > * error thread. > @@ -656,24 +644,17 @@ static enum counter_recovery stat_handle_error(struct evsel *counter, int err) > if (!thread_map__remove(evsel_list->core.threads, > evsel_list->core.threads->err_thread)) { > evsel_list->core.threads->err_thread = -1; > + counter->supported = true; > return COUNTER_RETRY; > } > - } else if (err == EOPNOTSUPP) { > - if (verbose > 0) { > - ui__warning("%s event is not supported by the kernel.\n", > - evsel__name(counter)); > - } > - counter->supported = false; > - counter->errored = true; > } > - > - evsel__open_strerror(counter, &target, err, msg, sizeof(msg)); > - ui__error("%s\n", msg); > - > - if (child_pid != -1) > - kill(child_pid, SIGTERM); > - > - return COUNTER_FATAL; > + if (verbose > 0) { > + ui__warning(err == EOPNOTSUPP > + ? "%s event is not supported by the kernel.\n" > + : "skipping event %s that kernel failed to open.\n", > + evsel__name(counter)); > + } > + return COUNTER_SKIP; > } > > static int create_perf_stat_counter(struct evsel *evsel, > @@ -746,8 +727,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) > bool is_pipe = STAT_RECORD ? perf_stat.data.is_pipe : false; > struct evlist_cpu_iterator evlist_cpu_itr; > struct affinity saved_affinity, *affinity = NULL; > - int err; > - bool second_pass = false; > + int err, open_err = 0; > + bool second_pass = false, has_supported_counters; > > if (forks) { > if (evlist__prepare_workload(evsel_list, &target, argv, is_pipe, workload_exec_failed_signal) < 0) { > @@ -787,14 +768,17 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) > if (target.use_bpf) > break; > > - if (counter->reset_group || counter->errored) > + if (counter->reset_group || !counter->supported) > continue; > if (evsel__is_bperf(counter)) > continue; > -try_again: > - if (create_perf_stat_counter(counter, &stat_config, > - evlist_cpu_itr.cpu_map_idx) < 0) { > > + while (true) { > + if (create_perf_stat_counter(counter, &stat_config, > + evlist_cpu_itr.cpu_map_idx) == 0) > + break; > + > + open_err = errno; > /* > * Weak group failed. We cannot just undo this here > * because earlier CPUs might be in group mode, and the kernel > @@ -802,29 +786,19 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) > * it to later. > * Don't close here because we're in the wrong affinity. > */ > - if ((errno == EINVAL || errno == EBADF) && > + if ((open_err == EINVAL || open_err == EBADF) && > evsel__leader(counter) != counter && > counter->weak_group) { > evlist__reset_weak_group(evsel_list, counter, false); > assert(counter->reset_group); > + counter->supported = true; > second_pass = true; > - continue; > - } > - > - switch (stat_handle_error(counter, errno)) { > - case COUNTER_FATAL: > - err = -1; > - goto err_out; > - case COUNTER_RETRY: > - goto try_again; > - case COUNTER_SKIP: > - continue; > - default: > break; > } > > + if (stat_handle_error(counter, open_err) != COUNTER_RETRY) > + break; > } > - counter->supported = true; > } > > if (second_pass) { > @@ -837,7 +811,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) > evlist__for_each_cpu(evlist_cpu_itr, evsel_list, affinity) { > counter = evlist_cpu_itr.evsel; > > - if (!counter->reset_group && !counter->errored) > + if (!counter->reset_group && counter->supported) > continue; > > perf_evsel__close_cpu(&counter->core, evlist_cpu_itr.cpu_map_idx); > @@ -848,34 +822,29 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) > > if (!counter->reset_group) > continue; > -try_again_reset: > - pr_debug2("reopening weak %s\n", evsel__name(counter)); > - if (create_perf_stat_counter(counter, &stat_config, > - evlist_cpu_itr.cpu_map_idx) < 0) { > - > - switch (stat_handle_error(counter, errno)) { > - case COUNTER_FATAL: > - err = -1; > - goto err_out; > - case COUNTER_RETRY: > - goto try_again_reset; > - case COUNTER_SKIP: > - continue; > - default: > + > + while (true) { > + pr_debug2("reopening weak %s\n", evsel__name(counter)); > + if (create_perf_stat_counter(counter, &stat_config, > + evlist_cpu_itr.cpu_map_idx) == 0) > + break; > + > + open_err = errno; > + if (stat_handle_error(counter, open_err) != COUNTER_RETRY) > break; > - } > } > - counter->supported = true; > } > } > affinity__cleanup(affinity); > affinity = NULL; > > + has_supported_counters = false; > evlist__for_each_entry(evsel_list, counter) { > if (!counter->supported) { > perf_evsel__free_fd(&counter->core); > continue; > } > + has_supported_counters = true; > > l = strlen(counter->unit); > if (l > stat_config.unit_width) > @@ -887,6 +856,16 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) > goto err_out; > } > } > + if (!has_supported_counters) { > + evsel__open_strerror(evlist__first(evsel_list), &target, open_err, > + msg, sizeof(msg)); > + ui__error("No supported events found.\n%s\n", msg); > + > + if (child_pid != -1) > + kill(child_pid, SIGTERM); > + err = -1; > + goto err_out; > + } > > if (evlist__apply_filters(evsel_list, &counter, &target)) { > pr_err("failed to set filter \"%s\" on event %s with %d (%s)\n", > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > index 1a29d4f47bbf..fe93f11cf3d1 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -407,6 +407,7 @@ void evsel__init(struct evsel *evsel, > evsel->collect_stat = false; > evsel->group_pmu_name = NULL; > evsel->skippable = false; > + evsel->supported = true; > evsel->alternate_hw_config = PERF_COUNT_HW_MAX; > evsel->script_output_type = -1; // FIXME: OUTPUT_TYPE_UNSET, see builtin-script.c > } > @@ -1941,7 +1942,7 @@ static int get_group_fd(struct evsel *evsel, int cpu_map_idx, int thread) > struct evsel *leader = evsel__leader(evsel); > int fd; > > - if (evsel__is_group_leader(evsel)) > + if (!evsel->supported || evsel__is_group_leader(evsel)) > return -1; > > /* > @@ -1955,7 +1956,7 @@ static int get_group_fd(struct evsel *evsel, int cpu_map_idx, int thread) > return -1; > > fd = FD(leader, cpu_map_idx, thread); > - BUG_ON(fd == -1 && !leader->skippable); > + BUG_ON(fd == -1 && leader->supported); > > /* > * When the leader has been skipped, return -2 to distinguish from no > @@ -2573,12 +2574,14 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus, > enum rlimit_action set_rlimit = NO_CHANGE; > struct perf_cpu cpu; > > - if (evsel__is_retire_lat(evsel)) > - return evsel__tpebs_open(evsel); > + if (evsel__is_retire_lat(evsel)) { > + err = evsel__tpebs_open(evsel); > + goto out; > + } > > err = __evsel__prepare_open(evsel, cpus, threads); > if (err) > - return err; > + goto out; > > if (cpus == NULL) > cpus = empty_cpu_map; > @@ -2598,19 +2601,22 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus, > display_attr(&evsel->core.attr); > > if (evsel__is_tool(evsel)) { > - return evsel__tool_pmu_open(evsel, threads, > - start_cpu_map_idx, > - end_cpu_map_idx); > + err = evsel__tool_pmu_open(evsel, threads, > + start_cpu_map_idx, > + end_cpu_map_idx); > + goto out; > } > if (evsel__is_hwmon(evsel)) { > - return evsel__hwmon_pmu_open(evsel, threads, > - start_cpu_map_idx, > - end_cpu_map_idx); > + err = evsel__hwmon_pmu_open(evsel, threads, > + start_cpu_map_idx, > + end_cpu_map_idx); > + goto out; > } > if (evsel__is_drm(evsel)) { > - return evsel__drm_pmu_open(evsel, threads, > - start_cpu_map_idx, > - end_cpu_map_idx); > + err = evsel__drm_pmu_open(evsel, threads, > + start_cpu_map_idx, > + end_cpu_map_idx); > + goto out; > } > > for (idx = start_cpu_map_idx; idx < end_cpu_map_idx; idx++) { > @@ -2689,7 +2695,8 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus, > } > } > > - return 0; > + err = 0; > + goto out; > > try_fallback: > if (evsel__ignore_missing_thread(evsel, perf_cpu_map__nr(cpus), > @@ -2728,6 +2735,9 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus, > thread = nthreads; > } while (--idx >= 0); > errno = old_errno; > +out: > + if (err) > + evsel->supported = false; > return err; > } > > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h > index 03f9f22e3a0c..1ad0b3fcd559 100644 > --- a/tools/perf/util/evsel.h > +++ b/tools/perf/util/evsel.h > @@ -121,7 +121,6 @@ struct evsel { > bool forced_leader; > bool cmdline_group_boundary; > bool reset_group; > - bool errored; > bool needs_auxtrace_mmap; > bool default_metricgroup; /* A member of the Default metricgroup */ > bool needs_uniquify; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] perf stat: Refactor retry/skip/fatal error handling 2025-10-03 12:03 ` James Clark @ 2025-10-03 19:08 ` Arnaldo Carvalho de Melo 2025-10-03 22:10 ` Ian Rogers 0 siblings, 1 reply; 6+ messages in thread From: Arnaldo Carvalho de Melo @ 2025-10-03 19:08 UTC (permalink / raw) To: James Clark Cc: Ian Rogers, Peter Zijlstra, Ingo Molnar, Namhyung Kim, Alexander Shishkin, Jiri Olsa, Adrian Hunter, Kan Liang, Howard Chu, Thomas Falcon, Chun-Tse Shao, Dapeng Mi, linux-perf-users, linux-kernel On Fri, Oct 03, 2025 at 01:03:25PM +0100, James Clark wrote: > Reviewed-by: James Clark <james.clark@linaro.org> Thanks, applied to perf-tools-next, - Arnaldo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] perf stat: Refactor retry/skip/fatal error handling 2025-10-03 19:08 ` Arnaldo Carvalho de Melo @ 2025-10-03 22:10 ` Ian Rogers 0 siblings, 0 replies; 6+ messages in thread From: Ian Rogers @ 2025-10-03 22:10 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: James Clark, Peter Zijlstra, Ingo Molnar, Namhyung Kim, Alexander Shishkin, Jiri Olsa, Adrian Hunter, Kan Liang, Howard Chu, Thomas Falcon, Chun-Tse Shao, Dapeng Mi, linux-perf-users, linux-kernel On Fri, Oct 3, 2025 at 12:08 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > On Fri, Oct 03, 2025 at 01:03:25PM +0100, James Clark wrote: > > Reviewed-by: James Clark <james.clark@linaro.org> > > Thanks, applied to perf-tools-next, Thanks James and Arnaldo! Ian ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] perf stat: Move create_perf_stat_counter to builtin-stat 2025-10-02 22:07 [PATCH v2 1/2] perf stat: Move create_perf_stat_counter to builtin-stat Ian Rogers 2025-10-02 22:07 ` [PATCH v2 2/2] perf stat: Refactor retry/skip/fatal error handling Ian Rogers @ 2025-10-03 11:31 ` James Clark 1 sibling, 0 replies; 6+ messages in thread From: James Clark @ 2025-10-03 11:31 UTC (permalink / raw) To: Ian Rogers Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Alexander Shishkin, Jiri Olsa, Adrian Hunter, Kan Liang, Howard Chu, Thomas Falcon, Chun-Tse Shao, Dapeng Mi, linux-perf-users, linux-kernel On 02/10/2025 11:07 pm, Ian Rogers wrote: > The function create_perf_stat_counter is only used in builtin-stat.c > and contains logic about retrying events specific to > builtin-stat.c. Move the code to builtin-stat to tidy this up. > > Signed-off-by: Ian Rogers <irogers@google.com> Reviewed-by: James Clark <james.clark@linaro.org> > --- > tools/perf/builtin-stat.c | 60 +++++++++++++++++++++++++++++++++++++-- > tools/perf/util/stat.c | 56 ------------------------------------ > tools/perf/util/stat.h | 4 --- > 3 files changed, 58 insertions(+), 62 deletions(-) > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c > index ab567919b89a..75b9979c6c05 100644 > --- a/tools/perf/builtin-stat.c > +++ b/tools/perf/builtin-stat.c > @@ -676,6 +676,62 @@ static enum counter_recovery stat_handle_error(struct evsel *counter, int err) > return COUNTER_FATAL; > } > > +static int create_perf_stat_counter(struct evsel *evsel, > + struct perf_stat_config *config, > + int cpu_map_idx) > +{ > + struct perf_event_attr *attr = &evsel->core.attr; > + struct evsel *leader = evsel__leader(evsel); > + > + /* Reset supported flag as creating a stat counter is retried. */ > + attr->read_format = PERF_FORMAT_TOTAL_TIME_ENABLED | > + PERF_FORMAT_TOTAL_TIME_RUNNING; > + > + /* > + * The event is part of non trivial group, let's enable > + * the group read (for leader) and ID retrieval for all > + * members. > + */ > + if (leader->core.nr_members > 1) > + attr->read_format |= PERF_FORMAT_ID|PERF_FORMAT_GROUP; > + > + attr->inherit = !config->no_inherit && list_empty(&evsel->bpf_counter_list); > + > + /* > + * Some events get initialized with sample_(period/type) set, > + * like tracepoints. Clear it up for counting. > + */ > + attr->sample_period = 0; > + > + if (config->identifier) > + attr->sample_type = PERF_SAMPLE_IDENTIFIER; > + > + if (config->all_user) { > + attr->exclude_kernel = 1; > + attr->exclude_user = 0; > + } > + > + if (config->all_kernel) { > + attr->exclude_kernel = 0; > + attr->exclude_user = 1; > + } > + > + /* > + * Disabling all counters initially, they will be enabled > + * either manually by us or by kernel via enable_on_exec > + * set later. > + */ > + if (evsel__is_group_leader(evsel)) { > + attr->disabled = 1; > + > + if (target__enable_on_exec(&target)) > + attr->enable_on_exec = 1; > + } > + > + return evsel__open_per_cpu_and_thread(evsel, evsel__cpus(evsel), cpu_map_idx, > + evsel->core.threads); > +} > + > static int __run_perf_stat(int argc, const char **argv, int run_idx) > { > int interval = stat_config.interval; > @@ -736,7 +792,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) > if (evsel__is_bperf(counter)) > continue; > try_again: > - if (create_perf_stat_counter(counter, &stat_config, &target, > + if (create_perf_stat_counter(counter, &stat_config, > evlist_cpu_itr.cpu_map_idx) < 0) { > > /* > @@ -794,7 +850,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) > continue; > try_again_reset: > pr_debug2("reopening weak %s\n", evsel__name(counter)); > - if (create_perf_stat_counter(counter, &stat_config, &target, > + if (create_perf_stat_counter(counter, &stat_config, > evlist_cpu_itr.cpu_map_idx) < 0) { > > switch (stat_handle_error(counter, errno)) { > diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c > index 50b1a92d16df..101ed6c497bc 100644 > --- a/tools/perf/util/stat.c > +++ b/tools/perf/util/stat.c > @@ -716,59 +716,3 @@ size_t perf_event__fprintf_stat_config(union perf_event *event, FILE *fp) > > return ret; > } > - > -int create_perf_stat_counter(struct evsel *evsel, > - struct perf_stat_config *config, > - struct target *target, > - int cpu_map_idx) > -{ > - struct perf_event_attr *attr = &evsel->core.attr; > - struct evsel *leader = evsel__leader(evsel); > - > - attr->read_format = PERF_FORMAT_TOTAL_TIME_ENABLED | > - PERF_FORMAT_TOTAL_TIME_RUNNING; > - > - /* > - * The event is part of non trivial group, let's enable > - * the group read (for leader) and ID retrieval for all > - * members. > - */ > - if (leader->core.nr_members > 1) > - attr->read_format |= PERF_FORMAT_ID|PERF_FORMAT_GROUP; > - > - attr->inherit = !config->no_inherit && list_empty(&evsel->bpf_counter_list); > - > - /* > - * Some events get initialized with sample_(period/type) set, > - * like tracepoints. Clear it up for counting. > - */ > - attr->sample_period = 0; > - > - if (config->identifier) > - attr->sample_type = PERF_SAMPLE_IDENTIFIER; > - > - if (config->all_user) { > - attr->exclude_kernel = 1; > - attr->exclude_user = 0; > - } > - > - if (config->all_kernel) { > - attr->exclude_kernel = 0; > - attr->exclude_user = 1; > - } > - > - /* > - * Disabling all counters initially, they will be enabled > - * either manually by us or by kernel via enable_on_exec > - * set later. > - */ > - if (evsel__is_group_leader(evsel)) { > - attr->disabled = 1; > - > - if (target__enable_on_exec(target)) > - attr->enable_on_exec = 1; > - } > - > - return evsel__open_per_cpu_and_thread(evsel, evsel__cpus(evsel), cpu_map_idx, > - evsel->core.threads); > -} > diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h > index 4b0f14ae4e5f..34f30a295f89 100644 > --- a/tools/perf/util/stat.h > +++ b/tools/perf/util/stat.h > @@ -223,10 +223,6 @@ size_t perf_event__fprintf_stat(union perf_event *event, FILE *fp); > size_t perf_event__fprintf_stat_round(union perf_event *event, FILE *fp); > size_t perf_event__fprintf_stat_config(union perf_event *event, FILE *fp); > > -int create_perf_stat_counter(struct evsel *evsel, > - struct perf_stat_config *config, > - struct target *target, > - int cpu_map_idx); > void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *config, > struct target *_target, struct timespec *ts, int argc, const char **argv); > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-10-03 22:10 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-02 22:07 [PATCH v2 1/2] perf stat: Move create_perf_stat_counter to builtin-stat Ian Rogers 2025-10-02 22:07 ` [PATCH v2 2/2] perf stat: Refactor retry/skip/fatal error handling Ian Rogers 2025-10-03 12:03 ` James Clark 2025-10-03 19:08 ` Arnaldo Carvalho de Melo 2025-10-03 22:10 ` Ian Rogers 2025-10-03 11:31 ` [PATCH v2 1/2] perf stat: Move create_perf_stat_counter to builtin-stat James Clark
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).