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

[Sukadev Bhattiprolu]

Jiri Olsa [jolsa@xxxxxxxxxx] 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/