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
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) {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
