* [RFC PATCH v2] perf evsel: Fallback to task-clock when not system wide @ 2023-11-21 0:04 Ian Rogers 2023-12-04 16:02 ` Ian Rogers 2023-12-05 11:21 ` Athira Rajeev 0 siblings, 2 replies; 6+ messages in thread From: Ian Rogers @ 2023-11-21 0:04 UTC (permalink / raw) To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter, Kan Liang, Yang Jihong, Leo Yan, Sandipan Das, Ravi Bangoria, linux-perf-users, linux-kernel, Ajay Kaher, Alexey Makhalov When the cycles event isn't available evsel will fallback to the cpu-clock software event. task-clock is similar to cpu-clock but only runs when the process is running. Falling back to cpu-clock when not system wide leads to confusion, by falling back to task-clock it is hoped the confusion is less. Pass the target to determine if task-clock is more appropriate. Update a nearby comment and debug string for the change. --- v2. Use target__has_cpu as suggested by Namhyung. https://lpc.events/event/17/contributions/1556/ Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/builtin-record.c | 2 +- tools/perf/builtin-stat.c | 2 +- tools/perf/builtin-top.c | 2 +- tools/perf/util/evsel.c | 18 ++++++++++-------- tools/perf/util/evsel.h | 3 ++- 5 files changed, 15 insertions(+), 12 deletions(-) diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c index 8ec818568662..d8bb59511fdd 100644 --- a/tools/perf/builtin-record.c +++ b/tools/perf/builtin-record.c @@ -1350,7 +1350,7 @@ static int record__open(struct record *rec) evlist__for_each_entry(evlist, pos) { try_again: if (evsel__open(pos, pos->core.cpus, pos->core.threads) < 0) { - if (evsel__fallback(pos, errno, msg, sizeof(msg))) { + if (evsel__fallback(pos, &opts->target, errno, msg, sizeof(msg))) { if (verbose > 0) ui__warning("%s\n", msg); goto try_again; diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c index a3af805a1d57..d8e5d6f7a87a 100644 --- a/tools/perf/builtin-stat.c +++ b/tools/perf/builtin-stat.c @@ -653,7 +653,7 @@ static enum counter_recovery stat_handle_error(struct evsel *counter) if ((evsel__leader(counter) != counter) || !(counter->core.leader->nr_members > 1)) return COUNTER_SKIP; - } else if (evsel__fallback(counter, errno, msg, sizeof(msg))) { + } else if (evsel__fallback(counter, &target, errno, msg, sizeof(msg))) { if (verbose > 0) ui__warning("%s\n", msg); return COUNTER_RETRY; diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c index ea8c7eca5eee..1e42bd1c7d5a 100644 --- a/tools/perf/builtin-top.c +++ b/tools/perf/builtin-top.c @@ -1044,7 +1044,7 @@ static int perf_top__start_counters(struct perf_top *top) perf_top_overwrite_fallback(top, counter)) goto try_again; - if (evsel__fallback(counter, errno, msg, sizeof(msg))) { + if (evsel__fallback(counter, &opts->target, errno, msg, sizeof(msg))) { if (verbose > 0) ui__warning("%s\n", msg); goto try_again; diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c index a5da74e3a517..532f34d9fcb5 100644 --- a/tools/perf/util/evsel.c +++ b/tools/perf/util/evsel.c @@ -2853,7 +2853,8 @@ u64 evsel__intval_common(struct evsel *evsel, struct perf_sample *sample, const #endif -bool evsel__fallback(struct evsel *evsel, int err, char *msg, size_t msgsize) +bool evsel__fallback(struct evsel *evsel, struct target *target, int err, + char *msg, size_t msgsize) { int paranoid; @@ -2861,18 +2862,19 @@ bool evsel__fallback(struct evsel *evsel, int err, char *msg, size_t msgsize) evsel->core.attr.type == PERF_TYPE_HARDWARE && evsel->core.attr.config == PERF_COUNT_HW_CPU_CYCLES) { /* - * If it's cycles then fall back to hrtimer based - * cpu-clock-tick sw counter, which is always available even if - * no PMU support. + * If it's cycles then fall back to hrtimer based cpu-clock sw + * counter, which is always available even if no PMU support. * * PPC returns ENXIO until 2.6.37 (behavior changed with commit * b0a873e). */ - scnprintf(msg, msgsize, "%s", -"The cycles event is not supported, trying to fall back to cpu-clock-ticks"); - evsel->core.attr.type = PERF_TYPE_SOFTWARE; - evsel->core.attr.config = PERF_COUNT_SW_CPU_CLOCK; + evsel->core.attr.config = target__has_cpu(target) + ? PERF_COUNT_SW_CPU_CLOCK + : PERF_COUNT_SW_TASK_CLOCK; + scnprintf(msg, msgsize, + "The cycles event is not supported, trying to fall back to %s", + target__has_cpu(target) ? "cpu-clock" : "task-clock"); zfree(&evsel->name); return true; diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h index f19ac9f027ef..efbb6e848287 100644 --- a/tools/perf/util/evsel.h +++ b/tools/perf/util/evsel.h @@ -460,7 +460,8 @@ static inline bool evsel__is_clock(const struct evsel *evsel) evsel__match(evsel, SOFTWARE, SW_TASK_CLOCK); } -bool evsel__fallback(struct evsel *evsel, int err, char *msg, size_t msgsize); +bool evsel__fallback(struct evsel *evsel, struct target *target, int err, + char *msg, size_t msgsize); int evsel__open_strerror(struct evsel *evsel, struct target *target, int err, char *msg, size_t size); -- 2.43.0.rc1.413.gea7ed67945-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v2] perf evsel: Fallback to task-clock when not system wide 2023-11-21 0:04 [RFC PATCH v2] perf evsel: Fallback to task-clock when not system wide Ian Rogers @ 2023-12-04 16:02 ` Ian Rogers 2023-12-04 22:55 ` Namhyung Kim 2023-12-05 11:21 ` Athira Rajeev 1 sibling, 1 reply; 6+ messages in thread From: Ian Rogers @ 2023-12-04 16:02 UTC (permalink / raw) To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter, Kan Liang, Yang Jihong, Leo Yan, Sandipan Das, Ravi Bangoria, linux-perf-users, linux-kernel, Ajay Kaher, Alexey Makhalov On Mon, Nov 20, 2023 at 4:04 PM Ian Rogers <irogers@google.com> wrote: > > When the cycles event isn't available evsel will fallback to the > cpu-clock software event. task-clock is similar to cpu-clock but only > runs when the process is running. Falling back to cpu-clock when not > system wide leads to confusion, by falling back to task-clock it is > hoped the confusion is less. > > Pass the target to determine if task-clock is more appropriate. Update > a nearby comment and debug string for the change. > > --- > v2. Use target__has_cpu as suggested by Namhyung. > https://lpc.events/event/17/contributions/1556/ > > Signed-off-by: Ian Rogers <irogers@google.com> Ping. Thanks, Ian > --- > tools/perf/builtin-record.c | 2 +- > tools/perf/builtin-stat.c | 2 +- > tools/perf/builtin-top.c | 2 +- > tools/perf/util/evsel.c | 18 ++++++++++-------- > tools/perf/util/evsel.h | 3 ++- > 5 files changed, 15 insertions(+), 12 deletions(-) > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > index 8ec818568662..d8bb59511fdd 100644 > --- a/tools/perf/builtin-record.c > +++ b/tools/perf/builtin-record.c > @@ -1350,7 +1350,7 @@ static int record__open(struct record *rec) > evlist__for_each_entry(evlist, pos) { > try_again: > if (evsel__open(pos, pos->core.cpus, pos->core.threads) < 0) { > - if (evsel__fallback(pos, errno, msg, sizeof(msg))) { > + if (evsel__fallback(pos, &opts->target, errno, msg, sizeof(msg))) { > if (verbose > 0) > ui__warning("%s\n", msg); > goto try_again; > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c > index a3af805a1d57..d8e5d6f7a87a 100644 > --- a/tools/perf/builtin-stat.c > +++ b/tools/perf/builtin-stat.c > @@ -653,7 +653,7 @@ static enum counter_recovery stat_handle_error(struct evsel *counter) > if ((evsel__leader(counter) != counter) || > !(counter->core.leader->nr_members > 1)) > return COUNTER_SKIP; > - } else if (evsel__fallback(counter, errno, msg, sizeof(msg))) { > + } else if (evsel__fallback(counter, &target, errno, msg, sizeof(msg))) { > if (verbose > 0) > ui__warning("%s\n", msg); > return COUNTER_RETRY; > diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c > index ea8c7eca5eee..1e42bd1c7d5a 100644 > --- a/tools/perf/builtin-top.c > +++ b/tools/perf/builtin-top.c > @@ -1044,7 +1044,7 @@ static int perf_top__start_counters(struct perf_top *top) > perf_top_overwrite_fallback(top, counter)) > goto try_again; > > - if (evsel__fallback(counter, errno, msg, sizeof(msg))) { > + if (evsel__fallback(counter, &opts->target, errno, msg, sizeof(msg))) { > if (verbose > 0) > ui__warning("%s\n", msg); > goto try_again; > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > index a5da74e3a517..532f34d9fcb5 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -2853,7 +2853,8 @@ u64 evsel__intval_common(struct evsel *evsel, struct perf_sample *sample, const > > #endif > > -bool evsel__fallback(struct evsel *evsel, int err, char *msg, size_t msgsize) > +bool evsel__fallback(struct evsel *evsel, struct target *target, int err, > + char *msg, size_t msgsize) > { > int paranoid; > > @@ -2861,18 +2862,19 @@ bool evsel__fallback(struct evsel *evsel, int err, char *msg, size_t msgsize) > evsel->core.attr.type == PERF_TYPE_HARDWARE && > evsel->core.attr.config == PERF_COUNT_HW_CPU_CYCLES) { > /* > - * If it's cycles then fall back to hrtimer based > - * cpu-clock-tick sw counter, which is always available even if > - * no PMU support. > + * If it's cycles then fall back to hrtimer based cpu-clock sw > + * counter, which is always available even if no PMU support. > * > * PPC returns ENXIO until 2.6.37 (behavior changed with commit > * b0a873e). > */ > - scnprintf(msg, msgsize, "%s", > -"The cycles event is not supported, trying to fall back to cpu-clock-ticks"); > - > evsel->core.attr.type = PERF_TYPE_SOFTWARE; > - evsel->core.attr.config = PERF_COUNT_SW_CPU_CLOCK; > + evsel->core.attr.config = target__has_cpu(target) > + ? PERF_COUNT_SW_CPU_CLOCK > + : PERF_COUNT_SW_TASK_CLOCK; > + scnprintf(msg, msgsize, > + "The cycles event is not supported, trying to fall back to %s", > + target__has_cpu(target) ? "cpu-clock" : "task-clock"); > > zfree(&evsel->name); > return true; > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h > index f19ac9f027ef..efbb6e848287 100644 > --- a/tools/perf/util/evsel.h > +++ b/tools/perf/util/evsel.h > @@ -460,7 +460,8 @@ static inline bool evsel__is_clock(const struct evsel *evsel) > evsel__match(evsel, SOFTWARE, SW_TASK_CLOCK); > } > > -bool evsel__fallback(struct evsel *evsel, int err, char *msg, size_t msgsize); > +bool evsel__fallback(struct evsel *evsel, struct target *target, int err, > + char *msg, size_t msgsize); > int evsel__open_strerror(struct evsel *evsel, struct target *target, > int err, char *msg, size_t size); > > -- > 2.43.0.rc1.413.gea7ed67945-goog > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v2] perf evsel: Fallback to task-clock when not system wide 2023-12-04 16:02 ` Ian Rogers @ 2023-12-04 22:55 ` Namhyung Kim 0 siblings, 0 replies; 6+ messages in thread From: Namhyung Kim @ 2023-12-04 22:55 UTC (permalink / raw) To: Ian Rogers Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter, Kan Liang, Yang Jihong, Leo Yan, Sandipan Das, Ravi Bangoria, linux-perf-users, linux-kernel, Ajay Kaher, Alexey Makhalov On Mon, Dec 4, 2023 at 8:02 AM Ian Rogers <irogers@google.com> wrote: > > On Mon, Nov 20, 2023 at 4:04 PM Ian Rogers <irogers@google.com> wrote: > > > > When the cycles event isn't available evsel will fallback to the > > cpu-clock software event. task-clock is similar to cpu-clock but only > > runs when the process is running. Falling back to cpu-clock when not > > system wide leads to confusion, by falling back to task-clock it is > > hoped the confusion is less. > > > > Pass the target to determine if task-clock is more appropriate. Update > > a nearby comment and debug string for the change. > > > > --- > > v2. Use target__has_cpu as suggested by Namhyung. > > https://lpc.events/event/17/contributions/1556/ > > > > Signed-off-by: Ian Rogers <irogers@google.com> Acked-by: Namhyung Kim <namhyung@kernel.org> Thanks, Namhyung ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v2] perf evsel: Fallback to task-clock when not system wide 2023-11-21 0:04 [RFC PATCH v2] perf evsel: Fallback to task-clock when not system wide Ian Rogers 2023-12-04 16:02 ` Ian Rogers @ 2023-12-05 11:21 ` Athira Rajeev 2023-12-05 15:25 ` Arnaldo Carvalho de Melo 1 sibling, 1 reply; 6+ messages in thread From: Athira Rajeev @ 2023-12-05 11:21 UTC (permalink / raw) To: Ian Rogers, Namhyung Kim, Arnaldo Carvalho de Melo Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Adrian Hunter, Kan Liang, Yang Jihong, Leo Yan, Sandipan Das, Ravi Bangoria, linux-perf-users, LKML, Ajay Kaher, Alexey Makhalov > On 21-Nov-2023, at 5:34 AM, Ian Rogers <irogers@google.com> wrote: > > When the cycles event isn't available evsel will fallback to the > cpu-clock software event. task-clock is similar to cpu-clock but only > runs when the process is running. Falling back to cpu-clock when not > system wide leads to confusion, by falling back to task-clock it is > hoped the confusion is less. > > Pass the target to determine if task-clock is more appropriate. Update > a nearby comment and debug string for the change. > > --- > v2. Use target__has_cpu as suggested by Namhyung. > https://lpc.events/event/17/contributions/1556/ > > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/perf/builtin-record.c | 2 +- > tools/perf/builtin-stat.c | 2 +- > tools/perf/builtin-top.c | 2 +- > tools/perf/util/evsel.c | 18 ++++++++++-------- > tools/perf/util/evsel.h | 3 ++- > 5 files changed, 15 insertions(+), 12 deletions(-) > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > index 8ec818568662..d8bb59511fdd 100644 > --- a/tools/perf/builtin-record.c > +++ b/tools/perf/builtin-record.c > @@ -1350,7 +1350,7 @@ static int record__open(struct record *rec) > evlist__for_each_entry(evlist, pos) { > try_again: > if (evsel__open(pos, pos->core.cpus, pos->core.threads) < 0) { > - if (evsel__fallback(pos, errno, msg, sizeof(msg))) { > + if (evsel__fallback(pos, &opts->target, errno, msg, sizeof(msg))) { Hi Ian Tested this with perf record and I could find the code fallback to using task-clock ./perf record -v ls Warning: The cycles event is not supported, trying to fall back to task-clock But in case of “perf stat”, in my environment, found that the code path won't invoke “evsel__fallback”. Snippet for builtin-stat.c if (errno == EINVAL || errno == ENOSYS || errno == ENOENT || errno == EOPNOTSUPP || errno == ENXIO) { if (verbose > 0) 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; if ((evsel__leader(counter) != counter) || !(counter->core.leader->nr_members > 1)) return COUNTER_SKIP; } else if (evsel__fallback(counter, &target, errno, msg, sizeof(msg))) { if (verbose > 0) ui__warning("%s\n", msg); return COUNTER_RETRY; So if the perf_event_open returns ENOENT, we won’t do a fallback in builtin-stat.c Should we address cycles differently here ? Any comments ? Thanks Athira > > if (verbose > 0) > ui__warning("%s\n", msg); > goto try_again; > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c > index a3af805a1d57..d8e5d6f7a87a 100644 > --- a/tools/perf/builtin-stat.c > +++ b/tools/perf/builtin-stat.c > @@ -653,7 +653,7 @@ static enum counter_recovery stat_handle_error(struct evsel *counter) > if ((evsel__leader(counter) != counter) || > !(counter->core.leader->nr_members > 1)) > return COUNTER_SKIP; > - } else if (evsel__fallback(counter, errno, msg, sizeof(msg))) { > + } else if (evsel__fallback(counter, &target, errno, msg, sizeof(msg))) { > if (verbose > 0) > ui__warning("%s\n", msg); > return COUNTER_RETRY; > diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c > index ea8c7eca5eee..1e42bd1c7d5a 100644 > --- a/tools/perf/builtin-top.c > +++ b/tools/perf/builtin-top.c > @@ -1044,7 +1044,7 @@ static int perf_top__start_counters(struct perf_top *top) > perf_top_overwrite_fallback(top, counter)) > goto try_again; > > - if (evsel__fallback(counter, errno, msg, sizeof(msg))) { > + if (evsel__fallback(counter, &opts->target, errno, msg, sizeof(msg))) { > if (verbose > 0) > ui__warning("%s\n", msg); > goto try_again; > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > index a5da74e3a517..532f34d9fcb5 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -2853,7 +2853,8 @@ u64 evsel__intval_common(struct evsel *evsel, struct perf_sample *sample, const > > #endif > > -bool evsel__fallback(struct evsel *evsel, int err, char *msg, size_t msgsize) > +bool evsel__fallback(struct evsel *evsel, struct target *target, int err, > + char *msg, size_t msgsize) > { > int paranoid; > > @@ -2861,18 +2862,19 @@ bool evsel__fallback(struct evsel *evsel, int err, char *msg, size_t msgsize) > evsel->core.attr.type == PERF_TYPE_HARDWARE && > evsel->core.attr.config == PERF_COUNT_HW_CPU_CYCLES) { > /* > - * If it's cycles then fall back to hrtimer based > - * cpu-clock-tick sw counter, which is always available even if > - * no PMU support. > + * If it's cycles then fall back to hrtimer based cpu-clock sw > + * counter, which is always available even if no PMU support. > * > * PPC returns ENXIO until 2.6.37 (behavior changed with commit > * b0a873e). > */ > - scnprintf(msg, msgsize, "%s", > -"The cycles event is not supported, trying to fall back to cpu-clock-ticks"); > - > evsel->core.attr.type = PERF_TYPE_SOFTWARE; > - evsel->core.attr.config = PERF_COUNT_SW_CPU_CLOCK; > + evsel->core.attr.config = target__has_cpu(target) > + ? PERF_COUNT_SW_CPU_CLOCK > + : PERF_COUNT_SW_TASK_CLOCK; > + scnprintf(msg, msgsize, > + "The cycles event is not supported, trying to fall back to %s", > + target__has_cpu(target) ? "cpu-clock" : "task-clock"); > > zfree(&evsel->name); > return true; > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h > index f19ac9f027ef..efbb6e848287 100644 > --- a/tools/perf/util/evsel.h > +++ b/tools/perf/util/evsel.h > @@ -460,7 +460,8 @@ static inline bool evsel__is_clock(const struct evsel *evsel) > evsel__match(evsel, SOFTWARE, SW_TASK_CLOCK); > } > > -bool evsel__fallback(struct evsel *evsel, int err, char *msg, size_t msgsize); > +bool evsel__fallback(struct evsel *evsel, struct target *target, int err, > + char *msg, size_t msgsize); > int evsel__open_strerror(struct evsel *evsel, struct target *target, > int err, char *msg, size_t size); > > -- > 2.43.0.rc1.413.gea7ed67945-goog > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v2] perf evsel: Fallback to task-clock when not system wide 2023-12-05 11:21 ` Athira Rajeev @ 2023-12-05 15:25 ` Arnaldo Carvalho de Melo 2023-12-06 6:11 ` Athira Rajeev 0 siblings, 1 reply; 6+ messages in thread From: Arnaldo Carvalho de Melo @ 2023-12-05 15:25 UTC (permalink / raw) To: Athira Rajeev Cc: Ian Rogers, Namhyung Kim, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter, Kan Liang, Yang Jihong, Leo Yan, Sandipan Das, Ravi Bangoria, linux-perf-users, LKML, Ajay Kaher, Alexey Makhalov Em Tue, Dec 05, 2023 at 04:51:01PM +0530, Athira Rajeev escreveu: > > > > On 21-Nov-2023, at 5:34 AM, Ian Rogers <irogers@google.com> wrote: > > > > When the cycles event isn't available evsel will fallback to the > > cpu-clock software event. task-clock is similar to cpu-clock but only > > runs when the process is running. Falling back to cpu-clock when not > > system wide leads to confusion, by falling back to task-clock it is > > hoped the confusion is less. > > > > Pass the target to determine if task-clock is more appropriate. Update > > a nearby comment and debug string for the change. > > > > --- > > v2. Use target__has_cpu as suggested by Namhyung. > > https://lpc.events/event/17/contributions/1556/ > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > tools/perf/builtin-record.c | 2 +- > > tools/perf/builtin-stat.c | 2 +- > > tools/perf/builtin-top.c | 2 +- > > tools/perf/util/evsel.c | 18 ++++++++++-------- > > tools/perf/util/evsel.h | 3 ++- > > 5 files changed, 15 insertions(+), 12 deletions(-) > > > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > > index 8ec818568662..d8bb59511fdd 100644 > > --- a/tools/perf/builtin-record.c > > +++ b/tools/perf/builtin-record.c > > @@ -1350,7 +1350,7 @@ static int record__open(struct record *rec) > > evlist__for_each_entry(evlist, pos) { > > try_again: > > if (evsel__open(pos, pos->core.cpus, pos->core.threads) < 0) { > > - if (evsel__fallback(pos, errno, msg, sizeof(msg))) { > > + if (evsel__fallback(pos, &opts->target, errno, msg, sizeof(msg))) { > > Hi Ian > > Tested this with perf record and I could find the code fallback to using task-clock > > ./perf record -v ls > Warning: > The cycles event is not supported, trying to fall back to task-clock Ok, so I'll take that as a Tested-by: you, ok? The "perf stat" part can be addressed in a follow up patch, when that error handling is researched to remember why we have that ->supported, ->errored thing. - Arnaldo > But in case of “perf stat”, in my environment, found that the code path won't invoke “evsel__fallback”. > > Snippet for builtin-stat.c > if (errno == EINVAL || errno == ENOSYS || > errno == ENOENT || errno == EOPNOTSUPP || > errno == ENXIO) { > if (verbose > 0) > 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; > > if ((evsel__leader(counter) != counter) || > !(counter->core.leader->nr_members > 1)) > return COUNTER_SKIP; > } else if (evsel__fallback(counter, &target, errno, msg, sizeof(msg))) { > if (verbose > 0) > ui__warning("%s\n", msg); > return COUNTER_RETRY; > > So if the perf_event_open returns ENOENT, we won’t do a fallback in builtin-stat.c > Should we address cycles differently here ? Any comments ? > > Thanks > Athira > > > > if (verbose > 0) > > ui__warning("%s\n", msg); > > goto try_again; > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c > > index a3af805a1d57..d8e5d6f7a87a 100644 > > --- a/tools/perf/builtin-stat.c > > +++ b/tools/perf/builtin-stat.c > > @@ -653,7 +653,7 @@ static enum counter_recovery stat_handle_error(struct evsel *counter) > > if ((evsel__leader(counter) != counter) || > > !(counter->core.leader->nr_members > 1)) > > return COUNTER_SKIP; > > - } else if (evsel__fallback(counter, errno, msg, sizeof(msg))) { > > + } else if (evsel__fallback(counter, &target, errno, msg, sizeof(msg))) { > > if (verbose > 0) > > ui__warning("%s\n", msg); > > return COUNTER_RETRY; > > diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c > > index ea8c7eca5eee..1e42bd1c7d5a 100644 > > --- a/tools/perf/builtin-top.c > > +++ b/tools/perf/builtin-top.c > > @@ -1044,7 +1044,7 @@ static int perf_top__start_counters(struct perf_top *top) > > perf_top_overwrite_fallback(top, counter)) > > goto try_again; > > > > - if (evsel__fallback(counter, errno, msg, sizeof(msg))) { > > + if (evsel__fallback(counter, &opts->target, errno, msg, sizeof(msg))) { > > if (verbose > 0) > > ui__warning("%s\n", msg); > > goto try_again; > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > > index a5da74e3a517..532f34d9fcb5 100644 > > --- a/tools/perf/util/evsel.c > > +++ b/tools/perf/util/evsel.c > > @@ -2853,7 +2853,8 @@ u64 evsel__intval_common(struct evsel *evsel, struct perf_sample *sample, const > > > > #endif > > > > -bool evsel__fallback(struct evsel *evsel, int err, char *msg, size_t msgsize) > > +bool evsel__fallback(struct evsel *evsel, struct target *target, int err, > > + char *msg, size_t msgsize) > > { > > int paranoid; > > > > @@ -2861,18 +2862,19 @@ bool evsel__fallback(struct evsel *evsel, int err, char *msg, size_t msgsize) > > evsel->core.attr.type == PERF_TYPE_HARDWARE && > > evsel->core.attr.config == PERF_COUNT_HW_CPU_CYCLES) { > > /* > > - * If it's cycles then fall back to hrtimer based > > - * cpu-clock-tick sw counter, which is always available even if > > - * no PMU support. > > + * If it's cycles then fall back to hrtimer based cpu-clock sw > > + * counter, which is always available even if no PMU support. > > * > > * PPC returns ENXIO until 2.6.37 (behavior changed with commit > > * b0a873e). > > */ > > - scnprintf(msg, msgsize, "%s", > > -"The cycles event is not supported, trying to fall back to cpu-clock-ticks"); > > - > > evsel->core.attr.type = PERF_TYPE_SOFTWARE; > > - evsel->core.attr.config = PERF_COUNT_SW_CPU_CLOCK; > > + evsel->core.attr.config = target__has_cpu(target) > > + ? PERF_COUNT_SW_CPU_CLOCK > > + : PERF_COUNT_SW_TASK_CLOCK; > > + scnprintf(msg, msgsize, > > + "The cycles event is not supported, trying to fall back to %s", > > + target__has_cpu(target) ? "cpu-clock" : "task-clock"); > > > > zfree(&evsel->name); > > return true; > > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h > > index f19ac9f027ef..efbb6e848287 100644 > > --- a/tools/perf/util/evsel.h > > +++ b/tools/perf/util/evsel.h > > @@ -460,7 +460,8 @@ static inline bool evsel__is_clock(const struct evsel *evsel) > > evsel__match(evsel, SOFTWARE, SW_TASK_CLOCK); > > } > > > > -bool evsel__fallback(struct evsel *evsel, int err, char *msg, size_t msgsize); > > +bool evsel__fallback(struct evsel *evsel, struct target *target, int err, > > + char *msg, size_t msgsize); > > int evsel__open_strerror(struct evsel *evsel, struct target *target, > > int err, char *msg, size_t size); > > > > -- > > 2.43.0.rc1.413.gea7ed67945-goog > > > -- - Arnaldo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v2] perf evsel: Fallback to task-clock when not system wide 2023-12-05 15:25 ` Arnaldo Carvalho de Melo @ 2023-12-06 6:11 ` Athira Rajeev 0 siblings, 0 replies; 6+ messages in thread From: Athira Rajeev @ 2023-12-06 6:11 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Ian Rogers, Namhyung Kim, Adrian Hunter Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa, Kan Liang, Yang Jihong, Leo Yan, Sandipan Das, Ravi Bangoria, linux-perf-users, LKML, Ajay Kaher, Alexey Makhalov > On 05-Dec-2023, at 8:55 PM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > Em Tue, Dec 05, 2023 at 04:51:01PM +0530, Athira Rajeev escreveu: >> >> >>> On 21-Nov-2023, at 5:34 AM, Ian Rogers <irogers@google.com> wrote: >>> >>> When the cycles event isn't available evsel will fallback to the >>> cpu-clock software event. task-clock is similar to cpu-clock but only >>> runs when the process is running. Falling back to cpu-clock when not >>> system wide leads to confusion, by falling back to task-clock it is >>> hoped the confusion is less. >>> >>> Pass the target to determine if task-clock is more appropriate. Update >>> a nearby comment and debug string for the change. >>> >>> --- >>> v2. Use target__has_cpu as suggested by Namhyung. >>> https://lpc.events/event/17/contributions/1556/ >>> >>> Signed-off-by: Ian Rogers <irogers@google.com> >>> --- >>> tools/perf/builtin-record.c | 2 +- >>> tools/perf/builtin-stat.c | 2 +- >>> tools/perf/builtin-top.c | 2 +- >>> tools/perf/util/evsel.c | 18 ++++++++++-------- >>> tools/perf/util/evsel.h | 3 ++- >>> 5 files changed, 15 insertions(+), 12 deletions(-) >>> >>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c >>> index 8ec818568662..d8bb59511fdd 100644 >>> --- a/tools/perf/builtin-record.c >>> +++ b/tools/perf/builtin-record.c >>> @@ -1350,7 +1350,7 @@ static int record__open(struct record *rec) >>> evlist__for_each_entry(evlist, pos) { >>> try_again: >>> if (evsel__open(pos, pos->core.cpus, pos->core.threads) < 0) { >>> - if (evsel__fallback(pos, errno, msg, sizeof(msg))) { >>> + if (evsel__fallback(pos, &opts->target, errno, msg, sizeof(msg))) { >> >> Hi Ian >> >> Tested this with perf record and I could find the code fallback to using task-clock >> >> ./perf record -v ls >> Warning: >> The cycles event is not supported, trying to fall back to task-clock > > Ok, so I'll take that as a Tested-by: you, ok? Hi Arnaldo, Please add my Tested-by. Tested-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> > > The "perf stat" part can be addressed in a follow up patch, when that > error handling is researched to remember why we have that ->supported, > ->errored thing. Sure, I will take a look at why we have that difference in “perf stat” Thanks Athira > > - Arnaldo > >> But in case of “perf stat”, in my environment, found that the code path won't invoke “evsel__fallback”. >> >> Snippet for builtin-stat.c >> if (errno == EINVAL || errno == ENOSYS || >> errno == ENOENT || errno == EOPNOTSUPP || >> errno == ENXIO) { >> if (verbose > 0) >> 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; >> >> if ((evsel__leader(counter) != counter) || >> !(counter->core.leader->nr_members > 1)) >> return COUNTER_SKIP; >> } else if (evsel__fallback(counter, &target, errno, msg, sizeof(msg))) { >> if (verbose > 0) >> ui__warning("%s\n", msg); >> return COUNTER_RETRY; >> >> So if the perf_event_open returns ENOENT, we won’t do a fallback in builtin-stat.c >> Should we address cycles differently here ? Any comments ? >> >> Thanks >> Athira >>> >>> if (verbose > 0) >>> ui__warning("%s\n", msg); >>> goto try_again; >>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c >>> index a3af805a1d57..d8e5d6f7a87a 100644 >>> --- a/tools/perf/builtin-stat.c >>> +++ b/tools/perf/builtin-stat.c >>> @@ -653,7 +653,7 @@ static enum counter_recovery stat_handle_error(struct evsel *counter) >>> if ((evsel__leader(counter) != counter) || >>> !(counter->core.leader->nr_members > 1)) >>> return COUNTER_SKIP; >>> - } else if (evsel__fallback(counter, errno, msg, sizeof(msg))) { >>> + } else if (evsel__fallback(counter, &target, errno, msg, sizeof(msg))) { >>> if (verbose > 0) >>> ui__warning("%s\n", msg); >>> return COUNTER_RETRY; >>> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c >>> index ea8c7eca5eee..1e42bd1c7d5a 100644 >>> --- a/tools/perf/builtin-top.c >>> +++ b/tools/perf/builtin-top.c >>> @@ -1044,7 +1044,7 @@ static int perf_top__start_counters(struct perf_top *top) >>> perf_top_overwrite_fallback(top, counter)) >>> goto try_again; >>> >>> - if (evsel__fallback(counter, errno, msg, sizeof(msg))) { >>> + if (evsel__fallback(counter, &opts->target, errno, msg, sizeof(msg))) { >>> if (verbose > 0) >>> ui__warning("%s\n", msg); >>> goto try_again; >>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c >>> index a5da74e3a517..532f34d9fcb5 100644 >>> --- a/tools/perf/util/evsel.c >>> +++ b/tools/perf/util/evsel.c >>> @@ -2853,7 +2853,8 @@ u64 evsel__intval_common(struct evsel *evsel, struct perf_sample *sample, const >>> >>> #endif >>> >>> -bool evsel__fallback(struct evsel *evsel, int err, char *msg, size_t msgsize) >>> +bool evsel__fallback(struct evsel *evsel, struct target *target, int err, >>> + char *msg, size_t msgsize) >>> { >>> int paranoid; >>> >>> @@ -2861,18 +2862,19 @@ bool evsel__fallback(struct evsel *evsel, int err, char *msg, size_t msgsize) >>> evsel->core.attr.type == PERF_TYPE_HARDWARE && >>> evsel->core.attr.config == PERF_COUNT_HW_CPU_CYCLES) { >>> /* >>> - * If it's cycles then fall back to hrtimer based >>> - * cpu-clock-tick sw counter, which is always available even if >>> - * no PMU support. >>> + * If it's cycles then fall back to hrtimer based cpu-clock sw >>> + * counter, which is always available even if no PMU support. >>> * >>> * PPC returns ENXIO until 2.6.37 (behavior changed with commit >>> * b0a873e). >>> */ >>> - scnprintf(msg, msgsize, "%s", >>> -"The cycles event is not supported, trying to fall back to cpu-clock-ticks"); >>> - >>> evsel->core.attr.type = PERF_TYPE_SOFTWARE; >>> - evsel->core.attr.config = PERF_COUNT_SW_CPU_CLOCK; >>> + evsel->core.attr.config = target__has_cpu(target) >>> + ? PERF_COUNT_SW_CPU_CLOCK >>> + : PERF_COUNT_SW_TASK_CLOCK; >>> + scnprintf(msg, msgsize, >>> + "The cycles event is not supported, trying to fall back to %s", >>> + target__has_cpu(target) ? "cpu-clock" : "task-clock"); >>> >>> zfree(&evsel->name); >>> return true; >>> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h >>> index f19ac9f027ef..efbb6e848287 100644 >>> --- a/tools/perf/util/evsel.h >>> +++ b/tools/perf/util/evsel.h >>> @@ -460,7 +460,8 @@ static inline bool evsel__is_clock(const struct evsel *evsel) >>> evsel__match(evsel, SOFTWARE, SW_TASK_CLOCK); >>> } >>> >>> -bool evsel__fallback(struct evsel *evsel, int err, char *msg, size_t msgsize); >>> +bool evsel__fallback(struct evsel *evsel, struct target *target, int err, >>> + char *msg, size_t msgsize); >>> int evsel__open_strerror(struct evsel *evsel, struct target *target, >>> int err, char *msg, size_t size); >>> >>> -- >>> 2.43.0.rc1.413.gea7ed67945-goog >>> >> > > -- > > - Arnaldo ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-12-06 6:12 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-11-21 0:04 [RFC PATCH v2] perf evsel: Fallback to task-clock when not system wide Ian Rogers 2023-12-04 16:02 ` Ian Rogers 2023-12-04 22:55 ` Namhyung Kim 2023-12-05 11:21 ` Athira Rajeev 2023-12-05 15:25 ` Arnaldo Carvalho de Melo 2023-12-06 6:11 ` Athira Rajeev
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).