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

[PATCH v6 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,core=?

	In the case of the last example, a value replacing "?" 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,core=?/

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

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

Changelog[v6]

	[Jiri Olsa, Sukadev Bhattiprolu] Rather than display
	'starting_index=$core' in perf.list and sysfs and expect user to
	specify a value for 'starting_index', replace 'starting_index' with
	what it really means for the event. i.e for an event, if starting_index
	refers to 'core' then display 'core=?' in both perf list and sysfs (see
	examples above).

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 v6 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=?,bar=1,param1=?

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>
---
 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 v6 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=?,param2=?/ [Kernel PMU event]

An example:

  hv_24x7/HPM_TLBIE__PHYS_CORE,core=?/ [Kernel PMU event]

Changelog[v6]
	[Jir Olsa, Sukadev Bhattiprolu] Drop the '$' sign and go back to
	just printing whatevever sysfs provides (which is '=?') to identify
	required parameters. sysfs also now uses parameters like 'core'
	and 'vcpu' rather than 'starting_index'.

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..d208fef 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 v6 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.

Changelog[v6]
	Update event description to explain how required parameters
	are displayed.

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..47ad2a1 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,core=?
 
 		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 "?" 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 v6 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..d8be6fa 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,phys_processor_idx=?/
+
+This means that when provided as an event, a value for '?' must
+also be supplied. For example:
+
+  perf stat -C 0 -e 'hv_gpci/dtbp_ptitc,phys_processor_idx=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 v6 1/4] tools/perf: support parsing parameterized events

[Jiri Olsa]

On Sun, Dec 21, 2014 at 11:49:24PM -0800, Sukadev Bhattiprolu wrote:

SNIP

> +	}
>  
>  	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;
> +

I'm ok with the change logic, but I'm missing here check for the 'term'
string value to be '?', so we force subst terms to have '?' as value..
I believe thats what we decided in the previous set discussion, right?

I guess the it'd be nice to parse it directly in the bison code like
below (could be done later), but I'd be ok with simple check on this
place for now.

thanks,
jirka


---
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 93c4c9fbc922..7e021c64d5cc 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -484,6 +484,14 @@ PE_TERM '=' PE_VALUE
 	$$ = term;
 }
 |
+PE_TERM '=' PE_SUBST
+{
+	struct parse_events_term *term;
+
+	ABORT_ON(parse_events_term__subst(&term, (int)$1, NULL, NULL));
+	$$ = term;
+}
+|
 PE_TERM
 {
 	struct parse_events_term *term;

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

[Jiri Olsa]

On Sun, Dec 21, 2014 at 11:49:26PM -0800, Sukadev Bhattiprolu wrote:
> 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.
> 
> Changelog[v6]
> 	Update event description to explain how required parameters
> 	are displayed.
> 
> 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..47ad2a1 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,core=?
>  
>  		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 "?" 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.

I see.. here's the glitch ;-) I thought we agreed on forcing '?'
as the value for param events, not 'All non-numerical values'

thanks,
jirka

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

[Jiri Olsa]

On Sun, Dec 21, 2014 at 11:49:27PM -0800, Sukadev Bhattiprolu wrote:
> 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..d8be6fa 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:

s/$x/?/                                                         ^^^^

> +
> +  hv_gpci/dtbp_ptitc,phys_processor_idx=?/
> +
> +This means that when provided as an event, a value for '?' must
> +also be supplied. For example:
> +
> +  perf stat -C 0 -e 'hv_gpci/dtbp_ptitc,phys_processor_idx=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.

SNIP

> +
> +	- 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/*

   ^^^^ misaligned tab

> +
>          - 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
>  

thanks,
jirka

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

[Sukadev Bhattiprolu]

Jiri Olsa [jolsa@redhat.com] wrote:
| On Sun, Dec 21, 2014 at 11:49:24PM -0800, Sukadev Bhattiprolu wrote:
| 
| SNIP
| 
| > +	}
| >  
| >  	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;
| > +
| 
| I'm ok with the change logic, but I'm missing here check for the 'term'
| string value to be '?', so we force subst terms to have '?' as value..
| I believe thats what we decided in the previous set discussion, right?

The =? is not a user input, so I did not think of validating that.

perf tool expects kernel/sysfs to show entries like 'core=?'. Are you
saying that we should error out if kernel mistakenly displays 'core=$val'
or 'core=?val' ? 

If a required parameter is missing, we catch that in pmu_resolve_param_term().
If a bogus parameter is specified we catch that above in pmu_config_term().


| 
| I guess the it'd be nice to parse it directly in the bison code like
| below (could be done later), but I'd be ok with simple check on this
| place for now.
| 
| thanks,
| jirka
| 
| 
| ---
| diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
| index 93c4c9fbc922..7e021c64d5cc 100644
| --- a/tools/perf/util/parse-events.y
| +++ b/tools/perf/util/parse-events.y
| @@ -484,6 +484,14 @@ PE_TERM '=' PE_VALUE
|  	$$ = term;
|  }
|  |
| +PE_TERM '=' PE_SUBST
| +{
| +	struct parse_events_term *term;
| +
| +	ABORT_ON(parse_events_term__subst(&term, (int)$1, NULL, NULL));
| +	$$ = term;
| +}
| +|
|  PE_TERM
|  {
|  	struct parse_events_term *term;

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

[Sukadev Bhattiprolu]

Jiri Olsa [jolsa@redhat.com] wrote:
| On Sun, Dec 21, 2014 at 11:49:26PM -0800, Sukadev Bhattiprolu wrote:
| > From: Cody P Schafer <cody@linux.vnet.ibm.com>
| > +		In the case of the last example, a value replacing "?" 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.
| 
| I see.. here's the glitch ;-) I thought we agreed on forcing '?'
| as the value for param events, not 'All non-numerical values'

Yes, it is currently more broad than needed, but it is not really
user input - we are just parsing sysfs entries that developer specified
in the kernel. If necessary, we can tighten that independently ?
| 
| thanks,
| jirka

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

[Sukadev Bhattiprolu]

Jiri Olsa [jolsa@redhat.com] wrote:

| > +          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/*
| 
|    ^^^^ misaligned tab

Thanks, Fixed the typos and alignment below.
---
>From 1fdb5012081903172c11a41f9edb0d51525ba500 Mon Sep 17 00:00:00 2001
From: Cody P Schafer <cody@linux.vnet.ibm.com>
Date: Wed, 24 Sep 2014 12:27:23 -0700
Subject: [PATCH v6 4/4] tools/perf: Document parameterized and symbolic events

Changelog[v6]:
	- [Sukadev Bhattiprolu]: Update documentation of perf-list and
	  perf-record; Added documentation for perf-stat.
	- [Jiri Olsa] Fix some typos/formatting

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..3e2aec9 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 '?' in them. For
+example:
+
+  hv_gpci/dtbp_ptitc,phys_processor_idx=?/
+
+This means that when provided as an event, a value for '?' must
+also be supplied. For example:
+
+  perf stat -C 0 -e 'hv_gpci/dtbp_ptitc,phys_processor_idx=0x2/' ...
+
 OPTIONS
 -------
 
diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index af9a54e..7d8df2e 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 v6 3/4] perf Documentation: add event parameters

[Jiri Olsa]

On Mon, Dec 22, 2014 at 11:34:36AM -0800, Sukadev Bhattiprolu wrote:
> Jiri Olsa [jolsa@redhat.com] wrote:
> | On Sun, Dec 21, 2014 at 11:49:26PM -0800, Sukadev Bhattiprolu wrote:
> | > From: Cody P Schafer <cody@linux.vnet.ibm.com>
> | > +		In the case of the last example, a value replacing "?" 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.
> | 
> | I see.. here's the glitch ;-) I thought we agreed on forcing '?'
> | as the value for param events, not 'All non-numerical values'
> 
> Yes, it is currently more broad than needed, but it is not really
> user input - we are just parsing sysfs entries that developer specified
> in the kernel. If necessary, we can tighten that independently ?

I think it's better to tighten it up from the beginning, so when
we decide later for other string usage, we will not breake the
'current behaviour'.

Like if now we allow users (kernel pmu modules) to put anything
as param event's value, we will brake their expectations/code if
we later decide for another usage of that string value.

As Cody wrong in last version thread:
---
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.
---

thanks,
jirka

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

[Jiri Olsa]

On Mon, Dec 22, 2014 at 11:30:45AM -0800, Sukadev Bhattiprolu wrote:
> Jiri Olsa [jolsa@redhat.com] wrote:
> | On Sun, Dec 21, 2014 at 11:49:24PM -0800, Sukadev Bhattiprolu wrote:
> | 
> | SNIP
> | 
> | > +	}
> | >  
> | >  	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;
> | > +
> | 
> | I'm ok with the change logic, but I'm missing here check for the 'term'
> | string value to be '?', so we force subst terms to have '?' as value..
> | I believe thats what we decided in the previous set discussion, right?
> 
> The =? is not a user input, so I did not think of validating that.
> 
> perf tool expects kernel/sysfs to show entries like 'core=?'. Are you
> saying that we should error out if kernel mistakenly displays 'core=$val'
> or 'core=?val' ? 

I think the we should at least try to have interface unambiguous
from the beginning

> If a required parameter is missing, we catch that in pmu_resolve_param_term().
> If a bogus parameter is specified we catch that above in pmu_config_term().

but the value of that param is unspecified, and if we later want to
add another type of string values we would be screwed.. as I described
in the previous reply for your other patch.

jirka

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

[Sukadev Bhattiprolu]

>From a24b480ff54381a7e092597864cf615162afdd60 Mon Sep 17 00:00:00 2001
From: Cody P Schafer <cody@linux.vnet.ibm.com>
Date: Wed, 24 Sep 2014 12:27:15 -0700
Subject: [PATCH 1/4] tools/perf: support parsing parameterized events

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=?,bar=1,param1=?

Changelog[v6]:
	[Jiri Olsa] Add a check to make sure that sysfs entries with
	parameters exactly match '=?'.

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>
---
 tools/perf/util/parse-events.h |  1 +
 tools/perf/util/pmu.c          | 74 +++++++++++++++++++++++++++++++++++-------
 2 files changed, 64 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..bfbecf7 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,25 @@ 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 (term->type_val == PARSE_EVENTS__TERM_TYPE_STR) {
+		if (strcmp(term->val.str, "?")) {
+			if (verbose)
+				pr_info("Invalid sysfs entry %s=%s\n",
+						term->config, term->val.str);
+			return -EINVAL;
+		}
+
+		if (pmu_resolve_param_term(term, head_terms, &val))
+			return -EINVAL;
+	} else
+		return -EINVAL;
+
+	pmu_format_value(format->bits, val, vp, zero);
 	return 0;
 }
 
@@ -607,9 +658,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

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

[Sukadev Bhattiprolu]

>From bfc20fd2161060cc51f3f24aa91663da39e97750 Mon Sep 17 00:00:00 2001
From: Cody P Schafer <cody@linux.vnet.ibm.com>
Date: Wed, 24 Sep 2014 12:27:22 -0700
Subject: [PATCH 3/4] perf Documentation: add event parameters

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.

Changelog[v6]
	Update event description to explain how required parameters
	are displayed.

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..505f080 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,core=?
 
 		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 "?" would
+		need to be provided by the user selecting the particular event.
+		This is referred to as "event parameterization". Event
+		parameters have the format 'param=?'.
+
 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

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

[Jiri Olsa]

On Sun, Dec 21, 2014 at 11:49:25PM -0800, Sukadev Bhattiprolu wrote:
> 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=?,param2=?/ [Kernel PMU event]
> 
> An example:
> 
>   hv_24x7/HPM_TLBIE__PHYS_CORE,core=?/ [Kernel PMU event]
> 
> Changelog[v6]
> 	[Jir Olsa, Sukadev Bhattiprolu] Drop the '$' sign and go back to
> 	just printing whatevever sysfs provides (which is '=?') to identify
> 	required parameters. sysfs also now uses parameters like 'core'
> 	and 'vcpu' rather than 'starting_index'.
> 
> 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..d208fef 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);

why not display PARSE_EVENTS__TERM_TYPE_NUM as well?

jirka

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

[Jiri Olsa]

On Sun, Dec 21, 2014 at 11:49:25PM -0800, Sukadev Bhattiprolu wrote:
> 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=?,param2=?/ [Kernel PMU event]
> 
> An example:
> 
>   hv_24x7/HPM_TLBIE__PHYS_CORE,core=?/ [Kernel PMU event]
> 
> Changelog[v6]
> 	[Jir Olsa, Sukadev Bhattiprolu] Drop the '$' sign and go back to
> 	just printing whatevever sysfs provides (which is '=?') to identify
> 	required parameters. sysfs also now uses parameters like 'core'
> 	and 'vcpu' rather than 'starting_index'.
> 
> 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..d208fef 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);

also please enclose above loop and if in {}

jirka

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

[Jiri Olsa]

On Tue, Dec 23, 2014 at 11:58:50AM -0800, Sukadev Bhattiprolu wrote:
> From a24b480ff54381a7e092597864cf615162afdd60 Mon Sep 17 00:00:00 2001
> From: Cody P Schafer <cody@linux.vnet.ibm.com>
> Date: Wed, 24 Sep 2014 12:27:15 -0700
> Subject: [PATCH 1/4] tools/perf: support parsing parameterized events
> 
> 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=?,bar=1,param1=?
> 
> Changelog[v6]:
> 	[Jiri Olsa] Add a check to make sure that sysfs entries with
> 	parameters exactly match '=?'.
> 
> 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/'.

Acked-by: Jiri Olsa <jolsa@kernel.org>

I'm ok with whole patchset, with some nits for patch 2
that I've already sent out..

Would you mind sending v7 so we avoid confusion for Arnaldo
what to pick up?

thanks,
jirka

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

[Arnaldo Carvalho de Melo]

Em Tue, Jan 06, 2015 at 10:42:20AM +0100, Jiri Olsa escreveu:
> On Tue, Dec 23, 2014 at 11:58:50AM -0800, Sukadev Bhattiprolu wrote:
> > From a24b480ff54381a7e092597864cf615162afdd60 Mon Sep 17 00:00:00 2001
> > From: Cody P Schafer <cody@linux.vnet.ibm.com>
> > Date: Wed, 24 Sep 2014 12:27:15 -0700
> > Subject: [PATCH 1/4] tools/perf: support parsing parameterized events
> > 
> > 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=?,bar=1,param1=?
> > 
> > Changelog[v6]:
> > 	[Jiri Olsa] Add a check to make sure that sysfs entries with
> > 	parameters exactly match '=?'.
> > 
> > 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/'.
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>
> 
> I'm ok with whole patchset, with some nits for patch 2
> that I've already sent out..
> 
> Would you mind sending v7 so we avoid confusion for Arnaldo
> what to pick up?

Yes, please, I'm trying to pick up what has accumulated over the
holidays,

- Arnaldo

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

[Sukadev Bhattiprolu]

Jiri Olsa [jolsa@redhat.com] wrote:
| On Sun, Dec 21, 2014 at 11:49:25PM -0800, Sukadev Bhattiprolu wrote:
| > 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=?,param2=?/ [Kernel PMU event]
| > 
| > An example:
| > 
| >   hv_24x7/HPM_TLBIE__PHYS_CORE,core=?/ [Kernel PMU event]
| > 
| > Changelog[v6]
| > 	[Jir Olsa, Sukadev Bhattiprolu] Drop the '$' sign and go back to
| > 	just printing whatevever sysfs provides (which is '=?') to identify
| > 	required parameters. sysfs also now uses parameters like 'core'
| > 	and 'vcpu' rather than 'starting_index'.
| > 
| > 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..d208fef 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);
| 
| why not display PARSE_EVENTS__TERM_TYPE_NUM as well?

Well, we are only trying to list the attributes that user needs to specify:

	hv_24x7/HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE,core=?/

We could print the PARSE_EVENTS__TERM_TYPE_NUM terms as well,

	hv_24x7/HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE,domain=2,offset=e0,core=?,lpar=0/

but it would be needlessly verbose. The attributes with numeric values
like domain and offset, have fixed values for the event and user doesn't
need to know. If they do really need to know, they could just look up in
sysfs ?

Sukadev