Re: [PATCH v4 01/10] tools/perf: support parsing parameterized events
On Thu, Sep 25, 2014 at 07:25:20PM -0700, Sukadev Bhattiprolu wrote: Jiri Olsa [jo...@redhat.com] wrote: | On Wed, Sep 24, 2014 at 12:27:15PM -0700, Sukadev Bhattiprolu wrote: | From: Cody P Schafer c...@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 | | hum, so what happened to the '?' ... AFAIU from out last discussion, | you wanted to mark terms which are mandatory and user must provide | values for them.. and I thought the decision was to have following | alias record: | | $ cat /sys/bus/event_source/devices/pmu/events/event_name | param2=?,bar=1,param1=? | | while perf would scream if any of param1/2 wasnt filled like for: | pmu/event_name,param1=0x1/ Sorry, I meant to make perf list consistent with sysfs. Consider 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 In the first one, starting_index refers to a 'core' while in the second it refers to a vcpu. This serves as a hint for the parameter's meaning. By replacing both with 'starting_index=?' we lose that hint. Should we fix both sysfs and 'perf list' to say starting_index=?core Peter, Ingo, any opinions on this? Overall explanation is in here: http://marc.info/?l=linux-kernelm=141158688307356w=2 thanks, jirka ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 01/10] tools/perf: support parsing parameterized events
On Wed, Oct 01, 2014 at 11:06:27AM +0200, Jiri Olsa wrote: On Thu, Sep 25, 2014 at 07:25:20PM -0700, Sukadev Bhattiprolu wrote: Jiri Olsa [jo...@redhat.com] wrote: | On Wed, Sep 24, 2014 at 12:27:15PM -0700, Sukadev Bhattiprolu wrote: | From: Cody P Schafer c...@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 | | hum, so what happened to the '?' ... AFAIU from out last discussion, | you wanted to mark terms which are mandatory and user must provide | values for them.. and I thought the decision was to have following | alias record: | | $ cat /sys/bus/event_source/devices/pmu/events/event_name | param2=?,bar=1,param1=? | | while perf would scream if any of param1/2 wasnt filled like for: | pmu/event_name,param1=0x1/ Sorry, I meant to make perf list consistent with sysfs. Consider 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 In the first one, starting_index refers to a 'core' while in the second it refers to a vcpu. This serves as a hint for the parameter's meaning. By replacing both with 'starting_index=?' we lose that hint. Should we fix both sysfs and 'perf list' to say starting_index=?core Peter, Ingo, any opinions on this? Overall explanation is in here: http://marc.info/?l=linux-kernelm=141158688307356w=2 Consistency is good, and you indeed need to indicate it is a parameter. I'm not entirely sure about ?core, but I suppose its easy to parse and clear enough to read. So the typical optional argument syntax would be like $arg or type like. But overall I have no objection as long as you keep the lot consistent and parsable. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 01/10] tools/perf: support parsing parameterized events
On Wed, Sep 24, 2014 at 12:27:15PM -0700, Sukadev Bhattiprolu wrote: From: Cody P Schafer c...@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 hum, so what happened to the '?' ... AFAIU from out last discussion, you wanted to mark terms which are mandatory and user must provide values for them.. and I thought the decision was to have following alias record: $ cat /sys/bus/event_source/devices/pmu/events/event_name param2=?,bar=1,param1=? while perf would scream if any of param1/2 wasnt filled like for: pmu/event_name,param1=0x1/ thanks, jirka ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 01/10] tools/perf: support parsing parameterized events
Jiri Olsa [jo...@redhat.com] wrote: | On Wed, Sep 24, 2014 at 12:27:15PM -0700, Sukadev Bhattiprolu wrote: | From: Cody P Schafer c...@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 | | hum, so what happened to the '?' ... AFAIU from out last discussion, | you wanted to mark terms which are mandatory and user must provide | values for them.. and I thought the decision was to have following | alias record: | | $ cat /sys/bus/event_source/devices/pmu/events/event_name | param2=?,bar=1,param1=? | | while perf would scream if any of param1/2 wasnt filled like for: | pmu/event_name,param1=0x1/ Sorry, I meant to make perf list consistent with sysfs. Consider 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 In the first one, starting_index refers to a 'core' while in the second it refers to a vcpu. This serves as a hint for the parameter's meaning. By replacing both with 'starting_index=?' we lose that hint. Should we fix both sysfs and 'perf list' to say starting_index=?core Sukadev ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v4 01/10] tools/perf: support parsing parameterized events
From: Cody P Schafer c...@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 hb...@us.ibm.com
CC: Cody P Schafer d...@codyps.com
Signed-off-by: Sukadev Bhattiprolu suka...@linux.vnet.ibm.com
Signed-off-by: Cody P Schafer c...@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 df094b4..9d7d2d5 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -59,6 +59,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 22a4ad5..67e59b9 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -511,31 +511,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:
@@ -552,11 +589,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;
}
@@ -567,9 +609,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
