Re: [PATCH v5] perf tools: add support for libpfm4

Re: [PATCH v5] perf tools: add support for libpfm4

[Jiri Olsa]

On Mon, Mar 23, 2020 at 04:58:46PM -0700, Ian Rogers wrote:

SNIP

> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 10107747b361..31ed184566c8 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -37,6 +37,11 @@
>  #include "util/evsel_config.h"
>  #include "util/event.h"
>  
> +#ifdef HAVE_LIBPFM
> +#include <perfmon/pfmlib_perf_event.h>
> +static void print_libpfm_events(bool name_only);
> +#endif
> +
>  #define MAX_NAME_LEN 100
>  
>  #ifdef PARSER_DEBUG
> @@ -2794,6 +2799,10 @@ void print_events(const char *event_glob, bool name_only, bool quiet_flag,
>  	print_sdt_events(NULL, NULL, name_only);
>  
>  	metricgroup__print(true, true, NULL, name_only, details_flag);
> +
> +#ifdef HAVE_LIBPFM
> +	print_libpfm_events(name_only);


we should make some effort to fit this into our current list shape,
currently it looks like:

	$ perf list
	...

	TopdownL1_SMT:
	  Backend_Bound_SMT
	       [This category represents fraction of slots where no uops are being delivered due to a lack of required resources for accepting new uops in the Backend>
	  Bad_Speculation_SMT
	       [This category represents fraction of slots wasted due to incorrect speculations. SMT version; use when SMT is enabled and measuring per logical CPU]
	  Frontend_Bound_SMT
	       [This category represents fraction of slots where the processor's Frontend undersupplies its Backend. SMT version; use when SMT is enabled and measurin>
	  Retiring_SMT
	       [This category represents fraction of slots utilized by useful work i.e. issued uops that eventually get retired. SMT version; use when SMT is enabled >


	Name  : UNHALTED_CORE_CYCLES
	PMU   : ix86arch
	Desc  : count core clock cycles whenever the clock signal on the specific core is running (not halted)
	Equiv : None
	Code  : 0x3c
	Modif : PMU: [e] : edge level (may require counter-mask >= 1) (boolean)
	Modif : PMU: [i] : invert (boolean)
	Modif : PMU: [c] : counter-mask in range [0-255] (integer)
	Modif : PMU: [t] : measure any thread (boolean)
	Modif : PMU: [intx] : monitor only inside transactional memory region (boolean)
	Modif : PMU: [intxcp] : do not count occurrences inside aborted transactional memory region (boolean)
	Modif : perf_event: [u] : monitor at user level (boolean)
	Modif : perf_event: [k] : monitor at kernel level (boolean)
	Modif : perf_event: [period] : sampling period (integer)
	Modif : perf_event: [freq] : sampling frequency (Hz) (integer)
	Modif : perf_event: [excl] : exclusive access (boolean)
	Modif : perf_event: [mg] : monitor guest execution (boolean)
	Modif : perf_event: [mh] : monitor host execution (boolean)
	Modif : perf_event: [cpu] : CPU to program (integer)
	Modif : perf_event: [pinned] : pin event to counters (boolean)

it needs some header like 'libpfm events:' and then
probably just name and doc for basic list and more
verbose for -v

jirka

Re: [PATCH v5] perf tools: add support for libpfm4

[Jiri Olsa]

On Mon, Mar 23, 2020 at 04:58:46PM -0700, Ian Rogers wrote:

SNIP

> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 616fbda7c3fc..11421f5dc9cb 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -869,6 +869,17 @@ static struct perf_pmu *pmu_find(const char *name)
>  	return NULL;
>  }
>  
> +struct perf_pmu *perf_pmu__find_by_type(unsigned int type)
> +{
> +	struct perf_pmu *pmu;
> +
> +	list_for_each_entry(pmu, &pmus, list)
> +		if (pmu->type == type)
> +			return pmu;
> +
> +	return NULL;
> +}

please move this to separate patch

thanks,
jirka

> +
>  struct perf_pmu *perf_pmu__scan(struct perf_pmu *pmu)
>  {
>  	/*
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index 5fb3f16828df..de3b868d912c 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -65,6 +65,7 @@ struct perf_pmu_alias {
>  };
>  
>  struct perf_pmu *perf_pmu__find(const char *name);
> +struct perf_pmu *perf_pmu__find_by_type(unsigned int type);
>  int perf_pmu__config(struct perf_pmu *pmu, struct perf_event_attr *attr,
>  		     struct list_head *head_terms,
>  		     struct parse_events_error *error);
> -- 
> 2.25.1.696.g5e7596f4ac-goog
> 

Re: [PATCH v5] perf tools: add support for libpfm4

[Jiri Olsa]

On Mon, Mar 23, 2020 at 04:58:46PM -0700, Ian Rogers wrote:

SNIP

> +		/* no event */
> +		if (*q == '\0')
> +			continue;
> +
> +		memset(&attr, 0, sizeof(attr));
> +		event_attr_init(&attr);
> +
> +		ret = parse_libpfm_event(q, &attr);
> +		if (ret != PFM_SUCCESS) {
> +			fprintf(stderr, "failed to parse event %s : %s\n", str, pfm_strerror(ret));
> +			goto error;
> +		}
> +
> +		evsel = perf_evsel__new_idx(&attr, evlist->core.nr_entries);
> +		if (evsel == NULL)
> +			goto error;
> +
> +		evsel->name = strdup(q);
> +		if (!evsel->name) {
> +			evsel__delete(evsel);
> +			goto error;
> +		}
> +		evsel->is_libpfm_event = true;
> +
> +		pmu = perf_pmu__find_by_type((unsigned)attr.type);
> +		if (pmu)
> +			evsel->core.own_cpus = perf_cpu_map__get(pmu->cpus);

I think you need to do more setup in here, like in __add_event function
would be great to factor those bits from __add_event function and call
it from here, so it's all in one place

jirka

Re: [PATCH v5] perf tools: add support for libpfm4

[Jiri Olsa]

On Mon, Mar 23, 2020 at 04:58:46PM -0700, Ian Rogers wrote:

SNIP

>  INTERACTIVE PROMPTING KEYS
>  --------------------------
> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> index 80e55e796be9..571aa6b1af40 100644
> --- a/tools/perf/Makefile.config
> +++ b/tools/perf/Makefile.config
> @@ -999,6 +999,18 @@ ifdef LIBCLANGLLVM
>    endif
>  endif
>  
> +ifndef NO_LIBPFM4
> +  ifeq ($(feature-libpfm4), 1)
> +    CFLAGS += -DHAVE_LIBPFM
> +    EXTLIBS += -lpfm
> +    ASCIIDOC_EXTRA = -aHAVE_LIBPFM=1
> +    $(call detected,CONFIG_LIBPFM4)

you don't use CONFIG_LIBPFM4

but I was wondering if we should put it all in separate
object like util/pfm.c to get rid of some of those ifdefs

jirka

Re: [PATCH v5] perf tools: add support for libpfm4

[Jiri Olsa]

On Mon, Mar 23, 2020 at 04:58:46PM -0700, Ian Rogers wrote:

SNIP

> diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile
> index 7ac0d8088565..573072d32545 100644
> --- a/tools/build/feature/Makefile
> +++ b/tools/build/feature/Makefile
> @@ -67,7 +67,9 @@ FILES=                                          \
>           test-llvm.bin				\
>           test-llvm-version.bin			\
>           test-libaio.bin			\
> -         test-libzstd.bin
> +         test-libzstd.bin			\
> +         test-libpfm4.bin
> +
>  
>  FILES := $(addprefix $(OUTPUT),$(FILES))
>  
> @@ -321,6 +323,9 @@ $(OUTPUT)test-libaio.bin:
>  $(OUTPUT)test-libzstd.bin:
>  	$(BUILD) -lzstd
>  
> +$(OUTPUT)test-libpfm4.bin:
> +	$(BUILD) -lpfm

also please move the libpfm4 detection into separated patch

thanks,
jirka

Re: [PATCH v5] perf tools: add support for libpfm4

[Jiri Olsa]

On Mon, Mar 23, 2020 at 04:58:46PM -0700, Ian Rogers wrote:

SNIP

> +
> +int parse_libpfm_events_option(const struct option *opt, const char *str,
> +			int unset __maybe_unused)
> +{
> +	struct evlist *evlist = *(struct evlist **)opt->value;
> +	struct perf_event_attr attr;
> +	struct perf_pmu *pmu;
> +	struct evsel *evsel, *grp_leader = NULL;
> +	char *p, *q, *p_orig;
> +	const char *sep;
> +	int grp_evt = -1;
> +	int ret;
> +
> +	p_orig = p = strdup(str);
> +	if (!p)
> +		return -1;
> +	/*
> +	 * force loading of the PMU list
> +	 */
> +	perf_pmu__scan(NULL);
> +
> +	for (q = p; strsep(&p, ",{}"); q = p) {

it seems like this code could be really easily testtable,
could you please write simple test for this?

thanks,
jirka

Re: [PATCH v5] perf tools: add support for libpfm4

[Jiri Olsa]

On Mon, Mar 23, 2020 at 04:58:46PM -0700, Ian Rogers wrote:
> This patch links perf with the libpfm4 library if it is available and
> NO_LIBPFM4 isn't passed to the build. The libpfm4 library contains hardware
> event tables for all processors supported by perf_events. It is a helper
> library that helps convert from a symbolic event name to the event
> encoding required by the underlying kernel interface. This
> library is open-source and available from: http://perfmon2.sf.net.
> 
> With this patch, it is possible to specify full hardware events
> by name. Hardware filters are also supported. Events must be
> specified via the --pfm-events and not -e option. Both options
> are active at the same time and it is possible to mix and match:
> 
> $ perf stat --pfm-events inst_retired:any_p:c=1:i -e cycles ....
> 
> v5 is a rebase.
> v4 is a rebase on git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
>    branch perf/core and re-adds the tools/build/feature/test-libpfm4.c
>    missed in v3.
> v3 is against acme/perf/core and removes a diagnostic warning.
> v2 of this patch makes the --pfm-events man page documentation
> conditional on libpfm4 behing configured. It tidies some of the
> documentation and adds the feature test missed in the v1 patch.
> 
> Author: Stephane Eranian <eranian@google.com>
> Signed-off-by: Ian Rogers <irogers@google.com>

I still have some conflicts, but I merged it by hand


	patching file tools/build/Makefile.feature
	patching file tools/build/feature/Makefile
	patching file tools/build/feature/test-libpfm4.c
	patching file tools/perf/Documentation/Makefile
	patching file tools/perf/Documentation/perf-record.txt
	patching file tools/perf/Documentation/perf-stat.txt
	patching file tools/perf/Documentation/perf-top.txt
	patching file tools/perf/Makefile.config
	patching file tools/perf/Makefile.perf
	Hunk #3 FAILED at 834.
	1 out of 3 hunks FAILED -- saving rejects to file tools/perf/Makefile.perf.rej
	patching file tools/perf/builtin-list.c
	patching file tools/perf/builtin-record.c
	patching file tools/perf/builtin-stat.c
	patching file tools/perf/builtin-top.c
	Hunk #2 succeeded at 1549 (offset 2 lines).
	Hunk #3 succeeded at 1567 (offset 2 lines).
	patching file tools/perf/util/evsel.c
	patching file tools/perf/util/evsel.h
	patching file tools/perf/util/parse-events.c
	patching file tools/perf/util/parse-events.h
	patching file tools/perf/util/pmu.c
	Hunk #1 succeeded at 869 (offset 5 lines).
	patching file tools/perf/util/pmu.h
	Hunk #1 succeeded at 65 (offset 1 line).

jirka

Re: [PATCH v5] perf tools: add support for libpfm4

[Ian Rogers]

On Tue, Mar 24, 2020 at 3:28 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Mar 23, 2020 at 04:58:46PM -0700, Ian Rogers wrote:
> > This patch links perf with the libpfm4 library if it is available and
> > NO_LIBPFM4 isn't passed to the build. The libpfm4 library contains hardware
> > event tables for all processors supported by perf_events. It is a helper
> > library that helps convert from a symbolic event name to the event
> > encoding required by the underlying kernel interface. This
> > library is open-source and available from: http://perfmon2.sf.net.
> >
> > With this patch, it is possible to specify full hardware events
> > by name. Hardware filters are also supported. Events must be
> > specified via the --pfm-events and not -e option. Both options
> > are active at the same time and it is possible to mix and match:
> >
> > $ perf stat --pfm-events inst_retired:any_p:c=1:i -e cycles ....
> >
> > v5 is a rebase.
> > v4 is a rebase on git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
> >    branch perf/core and re-adds the tools/build/feature/test-libpfm4.c
> >    missed in v3.
> > v3 is against acme/perf/core and removes a diagnostic warning.
> > v2 of this patch makes the --pfm-events man page documentation
> > conditional on libpfm4 behing configured. It tidies some of the
> > documentation and adds the feature test missed in the v1 patch.
> >
> > Author: Stephane Eranian <eranian@google.com>
> > Signed-off-by: Ian Rogers <irogers@google.com>
>
> I still have some conflicts, but I merged it by hand
>
>
>         patching file tools/build/Makefile.feature
>         patching file tools/build/feature/Makefile
>         patching file tools/build/feature/test-libpfm4.c
>         patching file tools/perf/Documentation/Makefile
>         patching file tools/perf/Documentation/perf-record.txt
>         patching file tools/perf/Documentation/perf-stat.txt
>         patching file tools/perf/Documentation/perf-top.txt
>         patching file tools/perf/Makefile.config
>         patching file tools/perf/Makefile.perf
>         Hunk #3 FAILED at 834.
>         1 out of 3 hunks FAILED -- saving rejects to file tools/perf/Makefile.perf.rej
>         patching file tools/perf/builtin-list.c
>         patching file tools/perf/builtin-record.c
>         patching file tools/perf/builtin-stat.c
>         patching file tools/perf/builtin-top.c
>         Hunk #2 succeeded at 1549 (offset 2 lines).
>         Hunk #3 succeeded at 1567 (offset 2 lines).
>         patching file tools/perf/util/evsel.c
>         patching file tools/perf/util/evsel.h
>         patching file tools/perf/util/parse-events.c
>         patching file tools/perf/util/parse-events.h
>         patching file tools/perf/util/pmu.c
>         Hunk #1 succeeded at 869 (offset 5 lines).
>         patching file tools/perf/util/pmu.h
>         Hunk #1 succeeded at 65 (offset 1 line).
>
> jirka

Thanks! I did a clone of acme's linux.git branch perf/core and applied
the change with git am, then built and tested. Perhaps you are using a
different tree or branch? Anyway, hopefully this is resolved now :-)

Thanks again,
Ian

Re: [PATCH v5] perf tools: add support for libpfm4

[Ian Rogers]

On Tue, Mar 24, 2020 at 3:28 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Mar 23, 2020 at 04:58:46PM -0700, Ian Rogers wrote:
> > This patch links perf with the libpfm4 library if it is available and
> > NO_LIBPFM4 isn't passed to the build. The libpfm4 library contains hardware
> > event tables for all processors supported by perf_events. It is a helper
> > library that helps convert from a symbolic event name to the event
> > encoding required by the underlying kernel interface. This
> > library is open-source and available from: http://perfmon2.sf.net.
> >
> > With this patch, it is possible to specify full hardware events
> > by name. Hardware filters are also supported. Events must be
> > specified via the --pfm-events and not -e option. Both options
> > are active at the same time and it is possible to mix and match:
> >
> > $ perf stat --pfm-events inst_retired:any_p:c=1:i -e cycles ....
> >
> > v5 is a rebase.
> > v4 is a rebase on git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
> >    branch perf/core and re-adds the tools/build/feature/test-libpfm4.c
> >    missed in v3.
> > v3 is against acme/perf/core and removes a diagnostic warning.
> > v2 of this patch makes the --pfm-events man page documentation
> > conditional on libpfm4 behing configured. It tidies some of the
> > documentation and adds the feature test missed in the v1 patch.
> >
> > Author: Stephane Eranian <eranian@google.com>
> > Signed-off-by: Ian Rogers <irogers@google.com>
>
> I still have some conflicts, but I merged it by hand
>
>
>         patching file tools/build/Makefile.feature
>         patching file tools/build/feature/Makefile
>         patching file tools/build/feature/test-libpfm4.c
>         patching file tools/perf/Documentation/Makefile
>         patching file tools/perf/Documentation/perf-record.txt
>         patching file tools/perf/Documentation/perf-stat.txt
>         patching file tools/perf/Documentation/perf-top.txt
>         patching file tools/perf/Makefile.config
>         patching file tools/perf/Makefile.perf
>         Hunk #3 FAILED at 834.
>         1 out of 3 hunks FAILED -- saving rejects to file tools/perf/Makefile.perf.rej
>         patching file tools/perf/builtin-list.c
>         patching file tools/perf/builtin-record.c
>         patching file tools/perf/builtin-stat.c
>         patching file tools/perf/builtin-top.c
>         Hunk #2 succeeded at 1549 (offset 2 lines).
>         Hunk #3 succeeded at 1567 (offset 2 lines).
>         patching file tools/perf/util/evsel.c
>         patching file tools/perf/util/evsel.h
>         patching file tools/perf/util/parse-events.c
>         patching file tools/perf/util/parse-events.h
>         patching file tools/perf/util/pmu.c
>         Hunk #1 succeeded at 869 (offset 5 lines).
>         patching file tools/perf/util/pmu.h
>         Hunk #1 succeeded at 65 (offset 1 line).
>
> jirka

Thanks for looking at this! I sent a new version (v7):
https://lore.kernel.org/lkml/20200407064018.158555-1-irogers@google.com/T/#u
that adds Stephane's improvements for handling fallback when
perf_event_open fails. Please let us know if there are issues we can
address with the patch set.

Thanks,
Ian