| Message ID | 1417572578-9051-2-git-send-email-sukadev@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Michael Ellerman |
| Headers | show |
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
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.
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
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; }