* Re: [PATCH v8 4/4] perf tools: add support for libpfm4 [not found] ` <20200411074631.9486-5-irogers@google.com> @ 2020-04-14 15:21 ` Jiri Olsa 2020-04-14 18:14 ` Ian Rogers 0 siblings, 1 reply; 3+ messages in thread From: Jiri Olsa @ 2020-04-14 15:21 UTC (permalink / raw) To: Ian Rogers Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Yonghong Song, Andrii Nakryiko, Greg Kroah-Hartman, Thomas Gleixner, Igor Lubashev, Alexey Budankov, Florian Fainelli, Adrian Hunter, Andi Kleen, Jiwei Sun, yuzhouji On Sat, Apr 11, 2020 at 12:46:31AM -0700, Ian Rogers wrote: SNIP > TAG_FOLDERS= . ../lib ../include > TAG_FILES= ../../include/uapi/linux/perf_event.h > diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c > index 965ef017496f..7b64cd34266e 100644 > --- a/tools/perf/builtin-list.c > +++ b/tools/perf/builtin-list.c > @@ -18,6 +18,10 @@ > #include <subcmd/parse-options.h> > #include <stdio.h> > > +#ifdef HAVE_LIBPFM > +#include "util/pfm.h" > +#endif so we have the HAVE_LIBPFM you could do the: #ifdef HAVE_LIBPFM #else #endif in util/pfm.h and add stubs for libpfm_initialize and others in case HAVE_LIBPFM is not defined.. that clear out all the #ifdefs in the change SNIP > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c > index b6322eb0f423..8b323151f22c 100644 > --- a/tools/perf/tests/builtin-test.c > +++ b/tools/perf/tests/builtin-test.c > @@ -313,6 +313,15 @@ static struct test generic_tests[] = { > .desc = "maps__merge_in", > .func = test__maps__merge_in, > }, > + { > + .desc = "Test libpfm4 support", > + .func = test__pfm, > + .subtest = { > + .skip_if_fail = true, > + .get_nr = test__pfm_subtest_get_nr, > + .get_desc = test__pfm_subtest_get_desc, > + } awesome :) SNIP > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > index d23db6755f51..83ad76d3d2be 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -2447,9 +2447,15 @@ bool perf_evsel__fallback(struct evsel *evsel, int err, > const char *sep = ":"; > > /* Is there already the separator in the name. */ > +#ifndef HAVE_LIBPFM > if (strchr(name, '/') || > strchr(name, ':')) > sep = ""; > +#else > + if (strchr(name, '/') || > + (strchr(name, ':') && !evsel->is_libpfm_event)) > + sep = ""; > +#endif ^^^^^^^^ > > if (asprintf(&new_name, "%s%su", name, sep) < 0) > return false; > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h > index 53187c501ee8..397d335d5e24 100644 > --- a/tools/perf/util/evsel.h > +++ b/tools/perf/util/evsel.h > @@ -76,6 +76,9 @@ struct evsel { > bool ignore_missing_thread; > bool forced_leader; > bool use_uncore_alias; > +#ifdef HAVE_LIBPFM > + bool is_libpfm_event; > +#endif perhaps we could had this one in unconditionaly, because I think we have some members like that for aux tracing.. and that would remove the #ifdef above SNIP > > +#ifdef HAVE_LIBPFM > +struct evsel *parse_events__pfm_add_event(int idx, struct perf_event_attr *attr, > + char *name, struct perf_pmu *pmu) > +{ > + return __add_event(NULL, &idx, attr, false, name, pmu, NULL, false, > + NULL); > +} > +#endif could you instead add parse_events__add_event and call it from pfm code? SNIP > + pmu = perf_pmu__find_by_type((unsigned int)attr.type); > + evsel = parse_events__pfm_add_event(evlist->core.nr_entries, > + &attr, q, pmu); > + if (evsel == NULL) > + goto error; > + > + evsel->is_libpfm_event = true; > + > + evlist__add(evlist, evsel); > + > + if (grp_evt == 0) > + grp_leader = evsel; > + > + if (grp_evt > -1) { > + evsel->leader = grp_leader; > + grp_leader->core.nr_members++; > + grp_evt++; > + } > + > + if (*sep == '}') { > + if (grp_evt < 0) { > + ui__error("cannot close a non-existing event group\n"); > + goto error; > + } > + evlist->nr_groups++; > + grp_leader = NULL; > + grp_evt = -1; > + } > + evsel->is_libpfm_event = true; seems to be set twice in here > + } > + return 0; > +error: > + free(p_orig); > + return -1; > +} > + > +static const char *srcs[PFM_ATTR_CTRL_MAX] = { > + [PFM_ATTR_CTRL_UNKNOWN] = "???", > + [PFM_ATTR_CTRL_PMU] = "PMU", > + [PFM_ATTR_CTRL_PERF_EVENT] = "perf_event", > +}; SNIP thanks, jirka ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v8 4/4] perf tools: add support for libpfm4 2020-04-14 15:21 ` [PATCH v8 4/4] perf tools: add support for libpfm4 Jiri Olsa @ 2020-04-14 18:14 ` Ian Rogers 0 siblings, 0 replies; 3+ messages in thread From: Ian Rogers @ 2020-04-14 18:14 UTC (permalink / raw) To: Jiri Olsa Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Yonghong Song, Andrii Nakryiko, Greg Kroah-Hartman, Thomas Gleixner, Igor Lubashev, Alexey Budankov, Florian Fainelli, Adrian Hunter, Andi Kleen, Jiwei Sun, yuzhouji On Tue, Apr 14, 2020 at 8:21 AM Jiri Olsa <jolsa@redhat.com> wrote: > > On Sat, Apr 11, 2020 at 12:46:31AM -0700, Ian Rogers wrote: > > SNIP > > > TAG_FOLDERS= . ../lib ../include > > TAG_FILES= ../../include/uapi/linux/perf_event.h > > diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c > > index 965ef017496f..7b64cd34266e 100644 > > --- a/tools/perf/builtin-list.c > > +++ b/tools/perf/builtin-list.c > > @@ -18,6 +18,10 @@ > > #include <subcmd/parse-options.h> > > #include <stdio.h> > > > > +#ifdef HAVE_LIBPFM > > +#include "util/pfm.h" > > +#endif > > so we have the HAVE_LIBPFM you could do the: > > #ifdef HAVE_LIBPFM > #else > #endif > > in util/pfm.h and add stubs for libpfm_initialize and others > in case HAVE_LIBPFM is not defined.. that clear out all the > #ifdefs in the change > > > SNIP > > > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c > > index b6322eb0f423..8b323151f22c 100644 > > --- a/tools/perf/tests/builtin-test.c > > +++ b/tools/perf/tests/builtin-test.c > > @@ -313,6 +313,15 @@ static struct test generic_tests[] = { > > .desc = "maps__merge_in", > > .func = test__maps__merge_in, > > }, > > + { > > + .desc = "Test libpfm4 support", > > + .func = test__pfm, > > + .subtest = { > > + .skip_if_fail = true, > > + .get_nr = test__pfm_subtest_get_nr, > > + .get_desc = test__pfm_subtest_get_desc, > > + } > > awesome :) > > SNIP > > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > > index d23db6755f51..83ad76d3d2be 100644 > > --- a/tools/perf/util/evsel.c > > +++ b/tools/perf/util/evsel.c > > @@ -2447,9 +2447,15 @@ bool perf_evsel__fallback(struct evsel *evsel, int err, > > const char *sep = ":"; > > > > /* Is there already the separator in the name. */ > > +#ifndef HAVE_LIBPFM > > if (strchr(name, '/') || > > strchr(name, ':')) > > sep = ""; > > +#else > > + if (strchr(name, '/') || > > + (strchr(name, ':') && !evsel->is_libpfm_event)) > > + sep = ""; > > +#endif > > > ^^^^^^^^ > > > > > if (asprintf(&new_name, "%s%su", name, sep) < 0) > > return false; > > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h > > index 53187c501ee8..397d335d5e24 100644 > > --- a/tools/perf/util/evsel.h > > +++ b/tools/perf/util/evsel.h > > @@ -76,6 +76,9 @@ struct evsel { > > bool ignore_missing_thread; > > bool forced_leader; > > bool use_uncore_alias; > > +#ifdef HAVE_LIBPFM > > + bool is_libpfm_event; > > +#endif > > perhaps we could had this one in unconditionaly, > because I think we have some members like that > for aux tracing.. and that would remove the #ifdef > above > > > SNIP > > > > > +#ifdef HAVE_LIBPFM > > +struct evsel *parse_events__pfm_add_event(int idx, struct perf_event_attr *attr, > > + char *name, struct perf_pmu *pmu) > > +{ > > + return __add_event(NULL, &idx, attr, false, name, pmu, NULL, false, > > + NULL); > > +} > > +#endif > > could you instead add parse_events__add_event and call it from pfm code? I wasn't clear whether this was just a name change given the different arguments on existing functions. Hopefully everything is addressed in the v9 set: https://lore.kernel.org/lkml/20200414181054.22435-2-irogers@google.com/T/#m32fc3e3605e49b01e12418f59ef3977cab0561ed Thanks, Ian > SNIP > > > + pmu = perf_pmu__find_by_type((unsigned int)attr.type); > > + evsel = parse_events__pfm_add_event(evlist->core.nr_entries, > > + &attr, q, pmu); > > + if (evsel == NULL) > > + goto error; > > + > > + evsel->is_libpfm_event = true; > > + > > + evlist__add(evlist, evsel); > > + > > + if (grp_evt == 0) > > + grp_leader = evsel; > > + > > + if (grp_evt > -1) { > > + evsel->leader = grp_leader; > > + grp_leader->core.nr_members++; > > + grp_evt++; > > + } > > + > > + if (*sep == '}') { > > + if (grp_evt < 0) { > > + ui__error("cannot close a non-existing event group\n"); > > + goto error; > > + } > > + evlist->nr_groups++; > > + grp_leader = NULL; > > + grp_evt = -1; > > + } > > + evsel->is_libpfm_event = true; > > seems to be set twice in here > > > > + } > > + return 0; > > +error: > > + free(p_orig); > > + return -1; > > +} > > + > > +static const char *srcs[PFM_ATTR_CTRL_MAX] = { > > + [PFM_ATTR_CTRL_UNKNOWN] = "???", > > + [PFM_ATTR_CTRL_PMU] = "PMU", > > + [PFM_ATTR_CTRL_PERF_EVENT] = "perf_event", > > +}; > > SNIP > > thanks, > jirka > ^ permalink raw reply [flat|nested] 3+ messages in thread
[parent not found: <20200411074631.9486-3-irogers@google.com>]
* Re: [PATCH v8 2/4] tools feature: add support for detecting libpfm4 [not found] ` <20200411074631.9486-3-irogers@google.com> @ 2020-04-14 15:21 ` Jiri Olsa 0 siblings, 0 replies; 3+ messages in thread From: Jiri Olsa @ 2020-04-14 15:21 UTC (permalink / raw) To: Ian Rogers Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Yonghong Song, Andrii Nakryiko, Greg Kroah-Hartman, Thomas Gleixner, Igor Lubashev, Alexey Budankov, Florian Fainelli, Adrian Hunter, Andi Kleen, Jiwei Sun, yuzhouji On Sat, Apr 11, 2020 at 12:46:29AM -0700, Ian Rogers wrote: > From: Stephane Eranian <eranian@google.com> > > libpfm4 provides an alternate command line encoding of perf events. > > Signed-off-by: Stephane Eranian <eranian@google.com> > Reviewed-by: Ian Rogers <irogers@google.com> > --- > tools/build/Makefile.feature | 6 ++++-- > tools/build/feature/Makefile | 6 +++++- > tools/build/feature/test-libpfm4.c | 9 +++++++++ > 3 files changed, 18 insertions(+), 3 deletions(-) > create mode 100644 tools/build/feature/test-libpfm4.c > > diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature > index 3e0c019ef297..0b651171476f 100644 > --- a/tools/build/Makefile.feature > +++ b/tools/build/Makefile.feature > @@ -73,7 +73,8 @@ FEATURE_TESTS_BASIC := \ > libaio \ > libzstd \ > disassembler-four-args \ > - file-handle > + file-handle \ > + libpfm4 let's treat this the same way as libbpf and do not include it in the basic check, which will fail for omst users jirka ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-04-14 18:14 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20200411074631.9486-1-irogers@google.com>
[not found] ` <20200411074631.9486-5-irogers@google.com>
2020-04-14 15:21 ` [PATCH v8 4/4] perf tools: add support for libpfm4 Jiri Olsa
2020-04-14 18:14 ` Ian Rogers
[not found] ` <20200411074631.9486-3-irogers@google.com>
2020-04-14 15:21 ` [PATCH v8 2/4] tools feature: add support for detecting libpfm4 Jiri Olsa
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).