[PATCH v5 0/4] Add support for parametrized events

[PATCH v5 0/4] Add support for parametrized events

[Sukadev Bhattiprolu]

Description of "event parameters" from the documentation patch:

    Event parameters are a basic way for partial events to be specified in
    sysfs with per-event names given to the fields that need to be filled in
    when using a particular event.

    It is intended for supporting cases where the single 'cpu' parameter is
    insufficient. For example, POWER 8 has events for physical
    sockets/cores/cpus that are accessible from with virtual machines. To
    keep using the single 'cpu' parameter we'd need to perform a mapping
    between Linux's cpus and the physical machine's cpus (in this case
    Linux is running under a hypervisor). This isn't possible because
    bindings between our cpus and physical cpus may not be fixed, and we
    probably won't have a "cpu" on each physical cpu.

Description of the sysfs contents when events are parameterized (copied from an
included patch):

	Examples:

		domain=0x1,offset=0x8,starting_index=$core

	In the case of the last example, a value replacing "$core" would need
	to be provided by the user selecting the particular event. This is
	refered to as "event parameterization". All non-numerical values
	indicate an event parameter.

Notes on how perf-list displays parameterized events

	PARAMETERIZED EVENTS
	--------------------

	Some pmu events listed by 'perf list' will be displayed with '$xyz' in
	them. For example:

	  hv_24x7/HPM_THREAD_NAP_CCYC__PHYS_CORE,starting_index=$core/

	This means that when provided as an event, a value for $core must also
	be supplied. For example:

	  perf stat  -e \
	  	'hv_24x7/HPM_THREAD_NAP_CCYC__PHYS_CORE,starting_index=2'
		...

Changelog[v5]
	- [Jiri Olsa, Peter Zijlstra] Use '$arg' notation rather than ?
	  to indicate event parameters.

	- [Michael Ellerman] Separate the kernel and tool patches in the
	  patchset into different patchsets.

Changelog[v4]
	- [Jiri Olsa] Rebase to perf/core tree (fix small merge conflict)

Changelog[v3]
	- [Jiri Olsa] Changed the event parameters are specified. If
	  event file specifes 'param=val' make the usage 'param=123'
	  rather than 'val=123'. (patch 1,2/10)
	- Shortened event names using "PHYS" and "VCPU" (patch 4/10)
	- Print help message if invalid parameter is specified or required
	  parameter is missing.
	- Moved 3 patches that are unrelated to parametrized events into
	  a separate patchset.
	- Reordered patches so code changes come first.

Changelog[v2]
	- [Joe Perches, David Laight] Use beNN_to_cpu() instead of guessing
	  the size from type.
	- Use kmem_cache_free() to free page allocated with kmem_cache_alloc().
	- Rebase to recent kernel


Cody P Schafer (4):
  tools/perf: support parsing parameterized events
  tools/perf: extend format_alias() to include event parameters
  perf Documentation: add event parameters
  tools/perf: Document parameterized and symbolic events

 .../testing/sysfs-bus-event_source-devices-events  |  6 ++
 tools/perf/Documentation/perf-list.txt             | 13 +++
 tools/perf/Documentation/perf-record.txt           | 12 +++
 tools/perf/Documentation/perf-stat.txt             | 20 ++++-
 tools/perf/util/parse-events.h                     |  1 +
 tools/perf/util/pmu.c                              | 92 +++++++++++++++++++---
 6 files changed, 128 insertions(+), 16 deletions(-)

-- 
1.8.3.1

[PATCH v5 1/4] tools/perf: support parsing parameterized events

[Sukadev Bhattiprolu]

From: Cody P Schafer <cody@linux.vnet.ibm.com>

Enable event specification like:

	pmu/event_name,param1=0x1,param2=0x4/

Assuming that

	/sys/bus/event_source/devices/pmu/events/event_name

Contains something like

	param2=$foo,bar=1,param1=$baz

Changelog[v4]:
	[Jiri Olsa] Merge to recent perf-core and fix a small conflict.

Changelog[v3]:
	[Jiri Olsa] If the sysfs event file specifies 'param=val', make the
	usage 'hv_24x7/event,param=123/' rather than 'hv_24x7/event,val=123/'.

CC: Haren Myneni <hbabu@us.ibm.com>
CC: Cody P Schafer <dev@codyps.com>
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>

Conflicts:
	tools/perf/util/pmu.c
---
 tools/perf/util/parse-events.h |  1 +
 tools/perf/util/pmu.c          | 65 +++++++++++++++++++++++++++++++++++-------
 2 files changed, 55 insertions(+), 11 deletions(-)

diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index db2cf78..ca226ce 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -71,6 +71,7 @@ struct parse_events_term {
 	int type_val;
 	int type_term;
 	struct list_head list;
+	bool used;
 };
 
 struct parse_events_evlist {
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 5c9c494..cb516dd 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -551,31 +551,68 @@ static void pmu_format_value(unsigned long *format, __u64 value, __u64 *v,
 }
 
 /*
+ * Term is a string term, and might be a param-term. Try to look up it's value
+ * in the remaining terms.
+ * - We have a term like "base-or-format-term=param-term",
+ * - We need to find the value supplied for "param-term" (with param-term named
+ *   in a config string) later on in the term list.
+ */
+static int pmu_resolve_param_term(struct parse_events_term *term,
+				  struct list_head *head_terms,
+				  __u64 *value)
+{
+	struct parse_events_term *t;
+
+	list_for_each_entry(t, head_terms, list) {
+		if (t->type_val == PARSE_EVENTS__TERM_TYPE_NUM) {
+			if (!strcmp(t->config, term->config)) {
+				t->used = true;
+				*value = t->val.num;
+				return 0;
+			}
+		}
+	}
+
+	if (verbose)
+		printf("Required parameter '%s' not specified\n", term->config);
+
+	return -1;
+}
+
+/*
  * Setup one of config[12] attr members based on the
  * user input data - term parameter.
  */
 static int pmu_config_term(struct list_head *formats,
 			   struct perf_event_attr *attr,
 			   struct parse_events_term *term,
+			   struct list_head *head_terms,
 			   bool zero)
 {
 	struct perf_pmu_format *format;
 	__u64 *vp;
+	__u64 val;
+
+	/*
+	 * If this is a parameter we've already used for parameterized-eval,
+	 * skip it in normal eval.
+	 */
+	if (term->used)
+		return 0;
 
 	/*
-	 * Support only for hardcoded and numnerial terms.
 	 * Hardcoded terms should be already in, so nothing
 	 * to be done for them.
 	 */
 	if (parse_events__is_hardcoded_term(term))
 		return 0;
 
-	if (term->type_val != PARSE_EVENTS__TERM_TYPE_NUM)
-		return -EINVAL;
-
 	format = pmu_find_format(formats, term->config);
-	if (!format)
+	if (!format) {
+		if (verbose)
+			printf("Invalid event/parameter '%s'\n", term->config);
 		return -EINVAL;
+	}
 
 	switch (format->value) {
 	case PERF_PMU_FORMAT_VALUE_CONFIG:
@@ -592,11 +629,16 @@ static int pmu_config_term(struct list_head *formats,
 	}
 
 	/*
-	 * XXX If we ever decide to go with string values for
-	 * non-hardcoded terms, here's the place to translate
-	 * them into value.
+	 * Either directly use a numeric term, or try to translate string terms
+	 * using event parameters.
 	 */
-	pmu_format_value(format->bits, term->val.num, vp, zero);
+	if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
+		val = term->val.num;
+	else
+		if (pmu_resolve_param_term(term, head_terms, &val))
+			return -EINVAL;
+
+	pmu_format_value(format->bits, val, vp, zero);
 	return 0;
 }
 
@@ -607,9 +649,10 @@ int perf_pmu__config_terms(struct list_head *formats,
 {
 	struct parse_events_term *term;
 
-	list_for_each_entry(term, head_terms, list)
-		if (pmu_config_term(formats, attr, term, zero))
+	list_for_each_entry(term, head_terms, list) {
+		if (pmu_config_term(formats, attr, term, head_terms, zero))
 			return -EINVAL;
+	}
 
 	return 0;
 }
-- 
1.8.3.1

[PATCH v5 2/4] tools/perf: extend format_alias() to include event parameters

[Sukadev Bhattiprolu]

From: Cody P Schafer <cody@linux.vnet.ibm.com>

This causes `perf list pmu` to show parameters for parameterized events
like:

  pmu/event_name,param1=$param1,param2=$param2/ [Kernel PMU event]

Example:

  hv_24x7/HPM_TLBIE__PHYS_CORE,starting_index=$core/ [Kernel PMU event]

Changelog[v5]
	[Jiri Olsa, Peter Zijlstra] Use '$' to prefix parameterized events.

Changelog[v4]
	[Jiri Olsa] If the parameter for an event in sysfs is 'param=val',
	have perf-list show the event as 'param=?' rather than 'val=?'.

CC: Haren Myneni <hbabu@us.ibm.com>
CC: Cody P Schafer <dev@codyps.com>
Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 tools/perf/util/pmu.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index cb516dd..f8674c1 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -810,10 +810,35 @@ void perf_pmu__set_format(unsigned long *bits, long from, long to)
 		set_bit(b, bits);
 }
 
+static int sub_non_neg(int a, int b)
+{
+	if (b > a)
+		return 0;
+	return a - b;
+}
+
 static char *format_alias(char *buf, int len, struct perf_pmu *pmu,
 			  struct perf_pmu_alias *alias)
 {
-	snprintf(buf, len, "%s/%s/", pmu->name, alias->name);
+	struct parse_events_term *term;
+	int used = snprintf(buf, len, "%s/%s", pmu->name, alias->name);
+
+	list_for_each_entry(term, &alias->terms, list)
+		if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
+			used += snprintf(buf + used, sub_non_neg(len, used),
+					",%s=$%s", term->config,
+					term->val.str);
+
+	if (sub_non_neg(len, used) > 0) {
+		buf[used] = '/';
+		used++;
+	}
+	if (sub_non_neg(len, used) > 0) {
+		buf[used] = '\0';
+		used++;
+	} else
+		buf[len - 1] = '\0';
+
 	return buf;
 }
 
-- 
1.8.3.1

[PATCH v5 3/4] perf Documentation: add event parameters

[Sukadev Bhattiprolu]

From: Cody P Schafer <cody@linux.vnet.ibm.com>

Event parameters are a basic way for partial events to be specified in
sysfs with per-event names given to the fields that need to be filled in
when using a particular event.

It is intended for supporting cases where the single 'cpu' parameter is
insufficient. For example, POWER 8 has events for physical
sockets/cores/cpus that are accessible from with virtual machines. To
keep using the single 'cpu' parameter we'd need to perform a mapping
between Linux's cpus and the physical machine's cpus (in this case
Linux is running under a hypervisor). This isn't possible because
bindings between our cpus and physical cpus may not be fixed, and we
probably won't have a "cpu" on each physical cpu.

CC: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
CC: Haren Myneni <hbabu@us.ibm.com>
CC: Cody P Schafer <dev@codyps.com>
Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
---
 Documentation/ABI/testing/sysfs-bus-event_source-devices-events | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-event_source-devices-events b/Documentation/ABI/testing/sysfs-bus-event_source-devices-events
index 20979f8..f584b16 100644
--- a/Documentation/ABI/testing/sysfs-bus-event_source-devices-events
+++ b/Documentation/ABI/testing/sysfs-bus-event_source-devices-events
@@ -52,12 +52,18 @@ Description:	Per-pmu performance monitoring events specific to the running syste
 			event=0x2abc
 			event=0x423,inv,cmask=0x3
 			domain=0x1,offset=0x8,starting_index=0xffff
+			domain=0x1,offset=0x8,starting_index=$phys_cpu
 
 		Each of the assignments indicates a value to be assigned to a
 		particular set of bits (as defined by the format file
 		corresponding to the <term>) in the perf_event structure passed
 		to the perf_open syscall.
 
+		In the case of the last example, a value replacing "$phys_cpu"
+		would need to be provided by the user selecting the particular
+		event. This is referred to as "event parameterization". All
+		non-numerical values indicate an event parameter.
+
 What: /sys/bus/event_source/devices/<pmu>/events/<event>.unit
 Date: 2014/02/24
 Contact:	Linux kernel mailing list <linux-kernel@vger.kernel.org>
-- 
1.8.3.1

[PATCH v5 4/4] tools/perf: Document parameterized and symbolic events

[Sukadev Bhattiprolu]

From: Cody P Schafer <cody@linux.vnet.ibm.com>

Changelog[v6]:
	- [Sukadev Bhattiprolu]: Update documentation of perf-list and
	  perf-record; Added documentation for perf-stat.

CC: Haren Myneni <hbabu@us.ibm.com>
CC: Cody P Schafer <dev@codyps.com>
Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 tools/perf/Documentation/perf-list.txt   | 13 +++++++++++++
 tools/perf/Documentation/perf-record.txt | 12 ++++++++++++
 tools/perf/Documentation/perf-stat.txt   | 20 ++++++++++++++++----
 3 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/tools/perf/Documentation/perf-list.txt b/tools/perf/Documentation/perf-list.txt
index cbb4f74..8e9a1121 100644
--- a/tools/perf/Documentation/perf-list.txt
+++ b/tools/perf/Documentation/perf-list.txt
@@ -89,6 +89,19 @@ raw encoding of 0x1A8 can be used:
 You should refer to the processor specific documentation for getting these
 details. Some of them are referenced in the SEE ALSO section below.
 
+PARAMETERIZED EVENTS
+--------------------
+
+Some pmu events listed by 'perf-list' will be displayed with '$x' in them. For
+example:
+
+  hv_gpci/dtbp_ptitc,starting_index=$core/
+
+This means that when provided as an event, a value for '$core' must
+also be supplied. For example:
+
+  perf stat -C 0 -e 'hv_gpci/dtbp_ptitc,starting_index=0x2/' ...
+
 OPTIONS
 -------
 
diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index af9a54e..acdcf3b 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -33,6 +33,18 @@ OPTIONS
         - a raw PMU event (eventsel+umask) in the form of rNNN where NNN is a
 	  hexadecimal event descriptor.
 
+	- a symbolically formed PMU event like 'pmu/param1=0x3,param2/' where
+	  'param1', 'param2', etc are defined as formats for the PMU in
+	  /sys/bus/event_sources/devices/<pmu>/format/*.
+
+	- a symbolically formed event like 'pmu/config=M,config1=N,config3=K/'
+
+          where M, N, K are numbers (in decimal, hex, octal format). Acceptable
+          values for each of 'config', 'config1' and 'config2' are defined by
+          corresponding entries in /sys/bus/event_sources/devices/<pmu>/format/*
+          param1 and param2 are defined as formats for the PMU in:
+	  /sys/bus/event_sources/devices/<pmu>/format/*
+
         - a hardware breakpoint event in the form of '\mem:addr[:access]'
           where addr is the address in memory you want to break in.
           Access is the memory access type (read, write, execute) it can
diff --git a/tools/perf/Documentation/perf-stat.txt b/tools/perf/Documentation/perf-stat.txt
index 29ee857..04e150d 100644
--- a/tools/perf/Documentation/perf-stat.txt
+++ b/tools/perf/Documentation/perf-stat.txt
@@ -25,10 +25,22 @@ OPTIONS
 
 -e::
 --event=::
-	Select the PMU event. Selection can be a symbolic event name
-	(use 'perf list' to list all events) or a raw PMU
-	event (eventsel+umask) in the form of rNNN where NNN is a
-	 hexadecimal event descriptor.
+	Select the PMU event. Selection can be:
+
+	- a symbolic event name (use 'perf list' to list all events)
+
+	- a raw PMU event (eventsel+umask) in the form of rNNN where NNN is a
+	  hexadecimal event descriptor.
+
+	- a symbolically formed event like 'pmu/param1=0x3,param2/' where
+	  param1 and param2 are defined as formats for the PMU in
+	  /sys/bus/event_sources/devices/<pmu>/format/*
+
+	- a symbolically formed event like 'pmu/config=M,config1=N,config2=K/'
+	  where M, N, K are numbers (in decimal, hex, octal format).
+	  Acceptable values for each of 'config', 'config1' and 'config2'
+	  parameters are defined by corresponding entries in
+	  /sys/bus/event_sources/devices/<pmu>/format/*
 
 -i::
 --no-inherit::
-- 
1.8.3.1

Re: [PATCH v5 1/4] tools/perf: support parsing parameterized events

[Jiri Olsa]

On Tue, Dec 02, 2014 at 06:09:35PM -0800, Sukadev Bhattiprolu wrote:
> From: Cody P Schafer <cody@linux.vnet.ibm.com>
> 
> Enable event specification like:
> 
> 	pmu/event_name,param1=0x1,param2=0x4/
> 
> Assuming that
> 
> 	/sys/bus/event_source/devices/pmu/events/event_name
> 
> Contains something like
> 
> 	param2=$foo,bar=1,param1=$baz

oops.. sorry to be PITA on this one.. I might have missed something
in the previous discussion but I guess I might have finally some
opinion on this ;-)

here's how I think your patchset works:

in /sys/bus/event_source/devices/pmu/events/event_name you can actually have:

   param2=foo,bar=1,param1=baz

notice no '$', thats what you add later in 'perf list' output, right?

Moreover it actually does not matter whats in value 'param2=HERE',
because it's not used in the config code at all apart from the
'perf list' display processing.

So when we discussed the '$' name way, I thought it'd be like:

in /sys/bus/event_source/devices/pmu/events/event_name you have:
  param2=$foo,bar=1,param1=$baz

and on command line you'd use:
  pmu/event_name,foo=0x1,bar=0x4/

to assign directly to the $var, which would justify the $var
syntax I think..

anyway we could assign directly to the param term name as you do,
but I think we just need to mark the term as parametrized, like:

in /sys/bus/event_source/devices/pmu/events/event_name you have:
  param2=?,bar=1,param1=?

and on command line you'd use:
  pmu/event_name,param2=0x1,param1=0x4/

while the config code would check that the param substitution is
done only for terms with '?' in value, like 'param2=?' and not
for all PARSE_EVENTS__TERM_TYPE_STR type terms (as of now)

thanks,
jirka

Re: [PATCH v5 1/4] tools/perf: support parsing parameterized events

[Cody P Schafer]

On Thu, Dec 4, 2014 at 7:44 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Tue, Dec 02, 2014 at 06:09:35PM -0800, Sukadev Bhattiprolu wrote:
>> From: Cody P Schafer <cody@linux.vnet.ibm.com>
>>
>> Enable event specification like:
>>
>>       pmu/event_name,param1=0x1,param2=0x4/
>>
>> Assuming that
>>
>>       /sys/bus/event_source/devices/pmu/events/event_name
>>
>> Contains something like
>>
>>       param2=$foo,bar=1,param1=$baz
>
> oops.. sorry to be PITA on this one.. I might have missed something
> in the previous discussion but I guess I might have finally some
> opinion on this ;-)
>
> here's how I think your patchset works:
>
> in /sys/bus/event_source/devices/pmu/events/event_name you can actually have:
>
>    param2=foo,bar=1,param1=baz
>
> notice no '$', thats what you add later in 'perf list' output, right?
>
> Moreover it actually does not matter whats in value 'param2=HERE',
> because it's not used in the config code at all apart from the
> 'perf list' display processing.
>
> So when we discussed the '$' name way, I thought it'd be like:
>
> in /sys/bus/event_source/devices/pmu/events/event_name you have:
>   param2=$foo,bar=1,param1=$baz
>
> and on command line you'd use:
>   pmu/event_name,foo=0x1,bar=0x4/
>
> to assign directly to the $var, which would justify the $var
> syntax I think..
>

Agreed, what you've described above sounds like a good idea.

Compared to monopolizing all strings (which is what I did when
initialy writing this), using a '$' prefix would allow less pain when
some events suddenly need non-integer parameters.

> anyway we could assign directly to the param term name as you do,
> but I think we just need to mark the term as parametrized, like:
>
> in /sys/bus/event_source/devices/pmu/events/event_name you have:
>   param2=?,bar=1,param1=?
>
> and on command line you'd use:
>   pmu/event_name,param2=0x1,param1=0x4/
>
> while the config code would check that the param substitution is
> done only for terms with '?' in value, like 'param2=?' and not
> for all PARSE_EVENTS__TERM_TYPE_STR type terms (as of now)

I prefer the `foo=0x1` as mentioned previously: it makes the user
interface much less painful as we can have event-specific names for
register/hcall fields.

I'm pretty sure the code used to do this, not sure when it was removed
(haven't been following this patchset closely).

That said: I haven't fiddled with this code in a while (it's Suka's at
this point), and there might be arguments the other way on both of
those.

Re: [PATCH v5 1/4] tools/perf: support parsing parameterized events

[Jiri Olsa]

On Fri, Dec 05, 2014 at 06:05:26PM -0500, Cody P Schafer wrote:
> On Thu, Dec 4, 2014 at 7:44 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> > On Tue, Dec 02, 2014 at 06:09:35PM -0800, Sukadev Bhattiprolu wrote:
> >> From: Cody P Schafer <cody@linux.vnet.ibm.com>
> >>
> >> Enable event specification like:
> >>
> >>       pmu/event_name,param1=0x1,param2=0x4/
> >>
> >> Assuming that
> >>
> >>       /sys/bus/event_source/devices/pmu/events/event_name
> >>
> >> Contains something like
> >>
> >>       param2=$foo,bar=1,param1=$baz
> >
> > oops.. sorry to be PITA on this one.. I might have missed something
> > in the previous discussion but I guess I might have finally some
> > opinion on this ;-)
> >
> > here's how I think your patchset works:
> >
> > in /sys/bus/event_source/devices/pmu/events/event_name you can actually have:
> >
> >    param2=foo,bar=1,param1=baz
> >
> > notice no '$', thats what you add later in 'perf list' output, right?
> >
> > Moreover it actually does not matter whats in value 'param2=HERE',
> > because it's not used in the config code at all apart from the
> > 'perf list' display processing.
> >
> > So when we discussed the '$' name way, I thought it'd be like:
> >
> > in /sys/bus/event_source/devices/pmu/events/event_name you have:
> >   param2=$foo,bar=1,param1=$baz
> >
> > and on command line you'd use:
> >   pmu/event_name,foo=0x1,bar=0x4/
> >
> > to assign directly to the $var, which would justify the $var
> > syntax I think..
> >
> 
> Agreed, what you've described above sounds like a good idea.
> 
> Compared to monopolizing all strings (which is what I did when
> initialy writing this), using a '$' prefix would allow less pain when
> some events suddenly need non-integer parameters.
> 
> > anyway we could assign directly to the param term name as you do,
> > but I think we just need to mark the term as parametrized, like:
> >
> > in /sys/bus/event_source/devices/pmu/events/event_name you have:
> >   param2=?,bar=1,param1=?
> >
> > and on command line you'd use:
> >   pmu/event_name,param2=0x1,param1=0x4/
> >
> > while the config code would check that the param substitution is
> > done only for terms with '?' in value, like 'param2=?' and not
> > for all PARSE_EVENTS__TERM_TYPE_STR type terms (as of now)
> 
> I prefer the `foo=0x1` as mentioned previously: it makes the user
> interface much less painful as we can have event-specific names for
> register/hcall fields.
> 
> I'm pretty sure the code used to do this, not sure when it was removed
> (haven't been following this patchset closely).

right, I recall seeing the 2 indirect assignments earlier,
but it was without the '$' marks

> 
> That said: I haven't fiddled with this code in a while (it's Suka's at
> this point), and there might be arguments the other way on both of
> those.

I guess I'm ok with both ways, maybe slightly inclined to
the '$' variable style one ;-)

jirka

Re: [PATCH v5 1/4] tools/perf: support parsing parameterized events

[Sukadev Bhattiprolu]

Jiri Olsa [jolsa@redhat.com] wrote:

| anyway we could assign directly to the param term name as you do,
| but I think we just need to mark the term as parametrized, like:
| 
| in /sys/bus/event_source/devices/pmu/events/event_name you have:
|   param2=?,bar=1,param1=?

I like the idea of just using a single ? for required parameters, but
the problem I had with this approach can be seen with these two sysfs
entries:

        $ cat HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE
        domain=0x2,offset=0xe0,starting_index=core,lpar=0x0

        $ cat HPM_0THRD_NON_IDLE_CCYC__VCPU_HOME_CORE
        domain=0x3,offset=0xe0,starting_index=vcpu,lpar=sibling_guest_id

The parameter 'starting_index' refers to a core in one event and vcpu in
another event. We were trying to give a hint as to what it refers to.

Given that, 'starting_index' is not very intuitive, how about discarding
starting_index and replacing with what it really means for the event and,
use a simple '?' to indicate required parameter).

        $ cat HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE
        domain=0x2,offset=0xe0,core=?,lpar=0x0

        $ cat HPM_0THRD_NON_IDLE_CCYC__VCPU_HOME_CORE
        domain=0x3,offset=0xe0,vcpu=?,lpar=?

perf list shows these as:

	hv_24x7/HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE,core=?/ 
	hv_24x7/HPM_0THRD_NON_IDLE_CCYC__VCPU_HOME_CHIP,vcpu=?,lpar=?/

command line would be

	-e hv_24x7/HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE,core=2/ 

	or

	-e hv_24x7/HPM_0THRD_NON_IDLE_CCYC__VCPU_HOME_CHIP,vcpu=2,lpar=7/

and would fail if a required parameter is missing.

This would eliminate the need for new strings like 'sibling_guest_id' (or
as Cody calls it monopolizing strings...)

Following quick patch on top of the patchset shows the changes:

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 73d5bfc..a82bc64 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -27,6 +27,8 @@
 #include "hv-24x7-catalog.h"
 #include "hv-common.h"
 
+
+#if 0
 static const char *domain_to_index_string(unsigned domain)
 {
 	switch (domain) {
@@ -40,6 +42,7 @@ static const char *domain_to_index_string(unsigned domain)
 		return "UNKNOWN_DOMAIN_INDEX_STRING";
 	}
 }
+#endif
 
 static const char *event_domain_suffix(unsigned domain)
 {
@@ -114,7 +117,8 @@ static bool catalog_entry_domain_is_valid(unsigned domain)
 /* u3 0-6, one of HV_24X7_PERF_DOMAIN */
 EVENT_DEFINE_RANGE_FORMAT(domain, config, 0, 3);
 /* u16 */
-EVENT_DEFINE_RANGE_FORMAT(starting_index, config, 16, 31);
+EVENT_DEFINE_RANGE_FORMAT(core, config, 16, 31);
+EVENT_DEFINE_RANGE_FORMAT(vcpu, config, 16, 31);
 /* u32, see "data_offset" */
 EVENT_DEFINE_RANGE_FORMAT(offset, config, 32, 63);
 /* u16 */
@@ -127,7 +131,8 @@ EVENT_DEFINE_RANGE(reserved3, config2,  0, 63);
 static struct attribute *format_attrs[] = {
 	&format_attr_domain.attr,
 	&format_attr_offset.attr,
-	&format_attr_starting_index.attr,
+	&format_attr_core.attr,
+	&format_attr_vcpu.attr,
 	&format_attr_lpar.attr,
 	NULL,
 };
@@ -280,19 +285,23 @@ static unsigned core_domains[] = {
 
 static char *event_fmt(struct hv_24x7_event_data *event, unsigned domain)
 {
+	const char *sindex;
 	const char *lpar;
 
-	if (is_physical_domain(domain))
+	if (is_physical_domain(domain)) {
 		lpar = "0x0";
-	else
-		lpar = "$sibling_guest_id";
+		sindex = "core";
+	} else {
+		lpar = "?";
+		sindex = "vcpu";
+	}
 
 	return kasprintf(GFP_KERNEL,
-			"domain=0x%x,offset=0x%x,starting_index=%s,lpar=%s",
+			"domain=0x%x,offset=0x%x,%s=?,lpar=%s",
 			domain,
 			be16_to_cpu(event->event_counter_offs) +
 				be16_to_cpu(event->event_group_record_offs),
-			domain_to_index_string(domain),
+			sindex,
 			lpar);
 }
 
@@ -1061,9 +1070,17 @@ out:
 static unsigned long event_24x7_request(struct perf_event *event, u64 *res,
 		bool success_expected)
 {
+	u16 idx;
+	unsigned domain = event_get_domain(event);
+
+	if (is_physical_domain(domain))
+		idx = event_get_core(event);
+	else
+		idx = event_get_vcpu(event);
+
 	return single_24x7_request(event_get_domain(event),
 				event_get_offset(event),
-				event_get_starting_index(event),
+				idx,
 				event_get_lpar(event),
 				res,
 				success_expected);
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index f8674c1..d208fef 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -826,7 +826,7 @@ static char *format_alias(char *buf, int len, struct perf_pmu *pmu,
 	list_for_each_entry(term, &alias->terms, list)
 		if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
 			used += snprintf(buf + used, sub_non_neg(len, used),
-					",%s=$%s", term->config,
+					",%s=%s", term->config,
 					term->val.str);
 
 	if (sub_non_neg(len, used) > 0) {

Re: [PATCH v5 1/4] tools/perf: support parsing parameterized events

[Jiri Olsa]

On Sat, Dec 06, 2014 at 11:37:24PM -0800, Sukadev Bhattiprolu wrote:
> Jiri Olsa [jolsa@redhat.com] wrote:
> 
> | anyway we could assign directly to the param term name as you do,
> | but I think we just need to mark the term as parametrized, like:
> | 
> | in /sys/bus/event_source/devices/pmu/events/event_name you have:
> |   param2=?,bar=1,param1=?
> 
> I like the idea of just using a single ? for required parameters, but
> the problem I had with this approach can be seen with these two sysfs
> entries:
> 
>         $ cat HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE
>         domain=0x2,offset=0xe0,starting_index=core,lpar=0x0
> 
>         $ cat HPM_0THRD_NON_IDLE_CCYC__VCPU_HOME_CORE
>         domain=0x3,offset=0xe0,starting_index=vcpu,lpar=sibling_guest_id
> 
> The parameter 'starting_index' refers to a core in one event and vcpu in
> another event. We were trying to give a hint as to what it refers to.
> 
> Given that, 'starting_index' is not very intuitive, how about discarding
> starting_index and replacing with what it really means for the event and,
> use a simple '?' to indicate required parameter).
> 
>         $ cat HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE
>         domain=0x2,offset=0xe0,core=?,lpar=0x0
> 
>         $ cat HPM_0THRD_NON_IDLE_CCYC__VCPU_HOME_CORE
>         domain=0x3,offset=0xe0,vcpu=?,lpar=?
> 
> perf list shows these as:
> 
> 	hv_24x7/HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE,core=?/ 
> 	hv_24x7/HPM_0THRD_NON_IDLE_CCYC__VCPU_HOME_CHIP,vcpu=?,lpar=?/
> 
> command line would be
> 
> 	-e hv_24x7/HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE,core=2/ 
> 
> 	or
> 
> 	-e hv_24x7/HPM_0THRD_NON_IDLE_CCYC__VCPU_HOME_CHIP,vcpu=2,lpar=7/
> 
> and would fail if a required parameter is missing.

that sounds good to me

jirka