Re: [PATCH v9 4/4] perf tools: add support for libpfm4

Re: [PATCH v9 4/4] perf tools: add support for libpfm4

[Jiri Olsa]

On Wed, Apr 15, 2020 at 11:35:51PM -0700, Ian Rogers wrote:

SNIP

> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> index 12a8204d63c6..26167ad38a47 100644
> --- a/tools/perf/Makefile.config
> +++ b/tools/perf/Makefile.config
> @@ -1012,6 +1012,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)
> +  else
> +    msg := $(warning libpfm4 not found, disables libpfm4 support. Please install libpfm4-dev);
> +    NO_LIBPFM4 := 1
> +  endif
> +endif

now when it's in FEATURE_TESTS_EXTRA it will not get detected,
unless you add the change below.. I wonder how come it was
still being detected for you.. might be bug in feature detection
stuff

jirka


---
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index 26167ad38a47..b45c5d370b42 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -1013,6 +1013,7 @@ ifdef LIBCLANGLLVM
 endif
 
 ifndef NO_LIBPFM4
+  $(call feature_check,libpfm4)
   ifeq ($(feature-libpfm4), 1)
     CFLAGS += -DHAVE_LIBPFM
     EXTLIBS += -lpfm

Re: [PATCH v9 4/4] perf tools: add support for libpfm4

[Jiri Olsa]

On Wed, Apr 15, 2020 at 11:35:51PM -0700, Ian Rogers wrote:
> From: Stephane Eranian <eranian@google.com>
> 
> 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 ....
> 
> Signed-off-by: Stephane Eranian <eranian@google.com>
> Reviewed-by: Ian Rogers <irogers@google.com>

	# perf list
	...
	perf_raw pfm-events
	  r0000
	    [perf_events raw event syntax: r[0-9a-fA-F]+]

	skl pfm-events
	  UNHALTED_CORE_CYCLES
	    [Count core clock cycles whenever the clock signal on the specific core is running (not halted)]
	  UNHALTED_REFERENCE_CYCLES

please add ':' behind the '* pfm-events' label

jirka

Re: [PATCH v9 4/4] perf tools: add support for libpfm4

[Jiri Olsa]

On Wed, Apr 15, 2020 at 11:35:51PM -0700, Ian Rogers wrote:

SNIP

> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 1ab349abe904..80ac598f125b 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -43,6 +43,7 @@
>  #include "util/time-utils.h"
>  #include "util/units.h"
>  #include "util/bpf-event.h"
> +#include "util/pfm.h"
>  #include "asm/bug.h"
>  #include "perf.h"
>  
> @@ -64,6 +65,9 @@
>  #include <linux/zalloc.h>
>  #include <linux/bitmap.h>
>  
> +
> +
> +

extra new lines..

jirka

>  struct switch_output {
>  	bool		 enabled;
>  	bool		 signal;
> @@ -2421,6 +2425,11 @@ static struct option __record_options[] = {
>  #endif
>  	OPT_CALLBACK(0, "max-size", &record.output_max_size,
>  		     "size", "Limit the maximum size of the output file", parse_output_max_size),
> +#ifdef HAVE_LIBPFM
> +	OPT_CALLBACK(0, "pfm-events", &record.evlist, "event",
> +		"libpfm4 event selector. use 'perf list' to list available events",
> +		parse_libpfm_events_option),
> +#endif
>  	OPT_END()
>  };

SNIP

Re: [PATCH v9 4/4] perf tools: add support for libpfm4

[Ian Rogers]

On Thu, Apr 16, 2020 at 2:55 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, Apr 15, 2020 at 11:35:51PM -0700, Ian Rogers wrote:
> > From: Stephane Eranian <eranian@google.com>
> >
> > 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 ....
> >
> > Signed-off-by: Stephane Eranian <eranian@google.com>
> > Reviewed-by: Ian Rogers <irogers@google.com>
>
>         # perf list
>         ...
>         perf_raw pfm-events
>           r0000
>             [perf_events raw event syntax: r[0-9a-fA-F]+]
>
>         skl pfm-events
>           UNHALTED_CORE_CYCLES
>             [Count core clock cycles whenever the clock signal on the specific core is running (not halted)]
>           UNHALTED_REFERENCE_CYCLES
>
> please add ':' behind the '* pfm-events' label

Thanks! Not sure I follow here. skl here is the pmu. pfm-events is
here just to make it clearer these are --pfm-events. The event is
selected with '--pfm-events UNHALTED_CORE_CYCLES'. Will putting
skl:pfm-events here make it look like that is part of the event
encoding?

Thanks,
Ian

> jirka
>

Re: [PATCH v9 4/4] perf tools: add support for libpfm4

[Jiri Olsa]

On Thu, Apr 16, 2020 at 09:02:54AM -0700, Ian Rogers wrote:
> On Thu, Apr 16, 2020 at 2:55 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Wed, Apr 15, 2020 at 11:35:51PM -0700, Ian Rogers wrote:
> > > From: Stephane Eranian <eranian@google.com>
> > >
> > > 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 ....
> > >
> > > Signed-off-by: Stephane Eranian <eranian@google.com>
> > > Reviewed-by: Ian Rogers <irogers@google.com>
> >
> >         # perf list
> >         ...
> >         perf_raw pfm-events
> >           r0000
> >             [perf_events raw event syntax: r[0-9a-fA-F]+]
> >
> >         skl pfm-events
> >           UNHALTED_CORE_CYCLES
> >             [Count core clock cycles whenever the clock signal on the specific core is running (not halted)]
> >           UNHALTED_REFERENCE_CYCLES
> >
> > please add ':' behind the '* pfm-events' label
> 
> Thanks! Not sure I follow here. skl here is the pmu. pfm-events is
> here just to make it clearer these are --pfm-events. The event is
> selected with '--pfm-events UNHALTED_CORE_CYCLES'. Will putting
> skl:pfm-events here make it look like that is part of the event
> encoding?

aah I might have misunderstood the output here then, we have preceeding
output like:

cache:
  l1d.replacement                                   
       [L1D data line replacements]

so I thought the 'skl pfm-events' is just a label


how about we use the first current label in the middle like:

	# perf list
	List of pre-defined events (to be used in -e):

	  current events stuff

	List of pfm events (to be used in --pfm-xxx):

	  pfm events stuff

or maybe put it under 'perf list --pfm', thoughts?

jirka

Re: [PATCH v9 4/4] perf tools: add support for libpfm4

[Ian Rogers]

On Thu, Apr 16, 2020 at 1:10 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Thu, Apr 16, 2020 at 09:02:54AM -0700, Ian Rogers wrote:
> > On Thu, Apr 16, 2020 at 2:55 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Wed, Apr 15, 2020 at 11:35:51PM -0700, Ian Rogers wrote:
> > > > From: Stephane Eranian <eranian@google.com>
> > > >
> > > > 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 ....
> > > >
> > > > Signed-off-by: Stephane Eranian <eranian@google.com>
> > > > Reviewed-by: Ian Rogers <irogers@google.com>
> > >
> > >         # perf list
> > >         ...
> > >         perf_raw pfm-events
> > >           r0000
> > >             [perf_events raw event syntax: r[0-9a-fA-F]+]
> > >
> > >         skl pfm-events
> > >           UNHALTED_CORE_CYCLES
> > >             [Count core clock cycles whenever the clock signal on the specific core is running (not halted)]
> > >           UNHALTED_REFERENCE_CYCLES
> > >
> > > please add ':' behind the '* pfm-events' label
> >
> > Thanks! Not sure I follow here. skl here is the pmu. pfm-events is
> > here just to make it clearer these are --pfm-events. The event is
> > selected with '--pfm-events UNHALTED_CORE_CYCLES'. Will putting
> > skl:pfm-events here make it look like that is part of the event
> > encoding?
>
> aah I might have misunderstood the output here then, we have preceeding
> output like:
>
> cache:
>   l1d.replacement
>        [L1D data line replacements]
>
> so I thought the 'skl pfm-events' is just a label
>
>
> how about we use the first current label in the middle like:
>
>         # perf list
>         List of pre-defined events (to be used in -e):
>
>           current events stuff
>
>         List of pfm events (to be used in --pfm-xxx):
>
>           pfm events stuff
>
> or maybe put it under 'perf list --pfm', thoughts?

We decided on the former which is in the new patch set. However, the
output isn't conditional on the pager being used, which it is in the
regular event case.
https://lore.kernel.org/lkml/20200416221457.46710-1-irogers@google.com/T/#t

Let me know if there is more to address. Thanks!
Ian

> jirka
>