Re: [PATCH v5 1/4] tools/perf: support parsing parameterized events
On Sat, Dec 06, 2014 at 11:37:24PM -0800, Sukadev Bhattiprolu wrote: > Jiri Olsa [jo...@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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 1/4] tools/perf: support parsing parameterized events
Jiri Olsa [jo...@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
per
Re: [PATCH v5 1/4] tools/perf: support parsing parameterized events
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 wrote: > > On Tue, Dec 02, 2014 at 06:09:35PM -0800, Sukadev Bhattiprolu wrote: > >> From: Cody P Schafer > >> > >> 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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 1/4] tools/perf: support parsing parameterized events
On Thu, Dec 4, 2014 at 7:44 AM, Jiri Olsa wrote: > On Tue, Dec 02, 2014 at 06:09:35PM -0800, Sukadev Bhattiprolu wrote: >> From: Cody P Schafer >> >> 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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 1/4] tools/perf: support parsing parameterized events
On Tue, Dec 02, 2014 at 06:09:35PM -0800, Sukadev Bhattiprolu wrote: > From: Cody P Schafer > > 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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v5 1/4] tools/perf: support parsing parameterized events
From: Cody P Schafer
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
CC: Cody P Schafer
Signed-off-by: Sukadev Bhattiprolu
Signed-off-by: Cody P Schafer
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
___
