[PATCH/RFC v1 0/1] Platform-specific event support in the perf

[PATCH/RFC v1 0/1] Platform-specific event support in the perf

[Tomasz Fujak]

Hi,

The folowing patch upgrades the perf with extended performance event description, as proposed in a separate RFC.
You can find it here: http://article.gmane.org/gmane.linux.ports.arm.kernel/72822

It has been tested on a ARMv7 Cortex-A8 incarnation.
Event names can be specified to -e switch, and are reported in the list command.

The perf-predefined commands take precedence (if a collision occurs) over the platform-specific ones.
A workaround for that is though - specify the raw event, as seen provided by the system.
Currently it resides in the sysfs, but I've already been pointed to a sysfs note disclaiming mixed types, multiple lines and fancy formatting. Might swith over to debugfs.


Thanks,
-- 
Tomasz Fujak

[PATCH v1 1/1] Extended events (platform-specific) support in perf

[Tomasz Fujak]

Signed-off-by: Tomasz Fujak <t.fujak@samsung.com>
Reviewed-by: Pawel Osciak <p.osciak@samsung.com>
Reviewed-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Kyungmin Park <kuyngmin.park@samsung.com>

---
 tools/perf/util/parse-events.c |  224 ++++++++++++++++++++++++++++++++++++++--
 1 files changed, 213 insertions(+), 11 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 05d0c5c..3e32198 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -9,6 +9,9 @@
 #include "header.h"
 #include "debugfs.h"
 
+#define			EXTENDED_EVENT_PATH	\
+			"/sys/devices/system/cpu/perf_events/extevents"
+
 int				nr_counters;
 
 struct perf_event_attr		attrs[MAX_COUNTERS];
@@ -60,6 +63,10 @@ static struct event_symbol event_symbols[] = {
 #define PERF_EVENT_TYPE(config)	__PERF_EVENT_FIELD(config, TYPE)
 #define PERF_EVENT_ID(config)		__PERF_EVENT_FIELD(config, EVENT)
 
+static struct event_symbol *extended_event_symbols;
+static unsigned int 	extended_event_count;
+static int 			extended_events_initialized;
+
 static const char *hw_event_names[] = {
 	"cycles",
 	"instructions",
@@ -241,6 +248,157 @@ static const char *tracepoint_id_to_name(u64 config)
 	return buf;
 }
 
+static unsigned count_lines(const char *str, unsigned size)
+{
+	unsigned count = 0;
+
+	while (size--)
+		count += ('\n' == *str++);
+	return count;
+}
+
+#define	BYTES_PER_CHUNK 4096
+/* returns the number of lines read;
+   on fail the return is negative and no memory is allocated
+   otherwise the *buf contains a memory chunk of which first
+   *size bytes are used for file contents
+ */
+static int read_file(int fd, char **buf, unsigned *size)
+{
+	unsigned bytes = BYTES_PER_CHUNK;
+	int lines = 0;
+	char *ptr = malloc(bytes);
+
+	*size = 0;
+	do {
+		int ret = read(fd, ptr + *size, bytes - *size);
+		if (ret < 0) {
+			if (EINTR == errno)
+				continue;
+			else {
+				free(ptr);
+				return -1;
+			}
+		}
+
+		if (!ret)
+			break;
+
+		lines += count_lines(ptr + *size, ret);
+		*size += ret;
+
+		if (*size == bytes) {
+			char *tmp = realloc(ptr, bytes <<= 1);
+			if (!tmp) {
+				free(ptr);
+				return -1;
+			}
+			ptr = tmp;
+		}
+	} while (1);
+
+	*buf = ptr;
+	return lines;
+}
+
+static int parse_extevent_file(struct event_symbol *symbols,
+			unsigned lines, char *buf)
+{
+	char *name, *description, *ptr, *end;
+	unsigned offset = 0, count = 0;
+	int items, eaten;
+	unsigned long long discard;
+
+/* each line format should be "0x%llx\t%s\t%lld-%lld\t%s\n" */
+	while (lines--) {
+		items = sscanf(buf + offset + 2, "%llx",
+				&symbols[count].config);
+		if (1 != items)
+			continue;
+
+		/* skip 0x%llx\t */
+		ptr = strchr(buf + offset, '\t') + 1;
+
+		name = description = NULL;
+
+		end = strchr(ptr, '\t');
+		if (end) {
+			name = strndup(ptr, end - ptr);
+			ptr = end + 1;
+		}
+
+		ptr = end;
+		items = sscanf(ptr, "%lld-%lld\t%n", &discard, &discard,
+			 &eaten);
+		if (2 != items)
+			continue;
+
+		ptr += eaten;
+		end = strchr(ptr, '\n');
+		if (end) {
+			description = strndup(ptr, end - ptr);
+			offset = end - buf + 1;
+		} else
+			break;
+
+		if (name && description) {
+			symbols[count].symbol = name;
+			symbols[count].alias = "";
+			++count;
+			/* description gets lost here */
+			free(description);
+		} else {
+			free(description);
+			free(name);
+		}
+	}
+
+	return count;
+}
+
+static int load_extended_events(const char *extevent_path)
+{
+	int fd;
+	int lines = 0;
+	unsigned size = 0;
+	char *buf = NULL;
+
+	fd = open(extevent_path, O_RDONLY);
+	if (fd < 0)
+		return fd;
+
+	lines = read_file(fd, &buf, &size);
+	close(fd);
+
+	if (0 < lines) {
+		struct event_symbol *symbols = (struct event_symbol *)
+				calloc(sizeof(struct event_symbol), lines);
+		if (symbols) {
+			int parsed_symbols = parse_extevent_file(symbols,
+							lines, buf);
+			if (0 < parsed_symbols) {
+				extended_event_symbols = symbols;
+				extended_event_count = parsed_symbols;
+			} else
+				free(symbols);
+		} else
+			lines = -1;
+	}
+	free(buf);
+
+	return lines;
+}
+
+static struct event_symbol *extevent_find_config(u64 config)
+{
+	unsigned int i;
+	for (i = 0; i < extended_event_count; ++i)
+		if (extended_event_symbols[i].config == config)
+			return &extended_event_symbols[i];
+
+	return NULL;
+}
+
 static int is_cache_op_valid(u8 cache_type, u8 cache_op)
 {
 	if (hw_cache_stat[cache_type] & COP(cache_op))
@@ -283,10 +441,16 @@ const char *__event_name(int type, u64 config)
 	}
 
 	switch (type) {
-	case PERF_TYPE_HARDWARE:
+	case PERF_TYPE_HARDWARE: {
+		const struct event_symbol *event;
+
 		if (config < PERF_COUNT_HW_MAX)
 			return hw_event_names[config];
+		event = extevent_find_config(config);
+		if (event)
+			return event->symbol;
 		return "unknown-hardware";
+	}
 
 	case PERF_TYPE_HW_CACHE: {
 		u8 cache_type, cache_op, cache_result;
@@ -611,33 +775,34 @@ parse_breakpoint_event(const char **strp, struct perf_event_attr *attr)
 	return EVT_HANDLED;
 }
 
-static int check_events(const char *str, unsigned int i)
+static int check_event(const char *str, const struct event_symbol *event)
 {
 	int n;
 
-	n = strlen(event_symbols[i].symbol);
-	if (!strncmp(str, event_symbols[i].symbol, n))
+	n = strlen(event->symbol);
+	if (!strncmp(str, event->symbol, n))
 		return n;
 
-	n = strlen(event_symbols[i].alias);
+	n = strlen(event->alias);
 	if (n)
-		if (!strncmp(str, event_symbols[i].alias, n))
+		if (!strncmp(str, event->alias, n))
 			return n;
 	return 0;
 }
 
 static enum event_result
-parse_symbolic_event(const char **strp, struct perf_event_attr *attr)
+do_parse_symbolic_event(const char **strp, struct perf_event_attr *attr,
+			const struct event_symbol *symbols, unsigned int count)
 {
 	const char *str = *strp;
 	unsigned int i;
 	int n;
 
-	for (i = 0; i < ARRAY_SIZE(event_symbols); i++) {
-		n = check_events(str, i);
+	for (i = 0; i < count; ++i) {
+		n = check_event(str, &symbols[i]);
 		if (n > 0) {
-			attr->type = event_symbols[i].type;
-			attr->config = event_symbols[i].config;
+			attr->type = symbols[i].type;
+			attr->config = symbols[i].config;
 			*strp = str + n;
 			return EVT_HANDLED;
 		}
@@ -646,6 +811,27 @@ parse_symbolic_event(const char **strp, struct perf_event_attr *attr)
 }
 
 static enum event_result
+parse_symbolic_event(const char **strp, struct perf_event_attr *attr)
+{
+	return do_parse_symbolic_event(strp, attr,
+			event_symbols, ARRAY_SIZE(event_symbols));
+}
+
+static enum event_result
+parse_extended_event(const char **strp, struct perf_event_attr *attr)
+{
+	if (!extended_events_initialized)
+		extended_events_initialized =
+			load_extended_events(EXTENDED_EVENT_PATH);
+
+	if (extended_events_initialized < 0)
+		return EVT_FAILED;
+
+	return do_parse_symbolic_event(strp, attr, extended_event_symbols,
+				extended_event_count);
+}
+
+static enum event_result
 parse_raw_event(const char **strp, struct perf_event_attr *attr)
 {
 	const char *str = *strp;
@@ -744,6 +930,10 @@ parse_event_symbols(const char **str, struct perf_event_attr *attr)
 	if (ret != EVT_FAILED)
 		goto modifier;
 
+	ret = parse_extended_event(str, attr);
+	if (ret != EVT_FAILED)
+		goto modifier;
+
 	ret = parse_breakpoint_event(str, attr);
 	if (ret != EVT_FAILED)
 		goto modifier;
@@ -932,6 +1122,18 @@ void print_events(void)
 		}
 	}
 
+    if (!extended_events_initialized)
+		extended_events_initialized =
+			load_extended_events(EXTENDED_EVENT_PATH);
+
+    if (0 < extended_events_initialized) {
+		for (i = 0; i < extended_event_count; ++i)
+			printf("  %-42s [%s]\n",
+				extended_event_symbols[i].symbol,
+				"Hardware platform-specific event");
+	}
+
+
 	printf("\n");
 	printf("  %-42s [%s]\n",
 		"rNNN", event_type_descriptors[PERF_TYPE_RAW]);
-- 
1.5.4.3

[PATCH v1 1/1] Extended events (platform-specific) support in perf

[Arnaldo Carvalho de Melo]

Em Fri, Jan 22, 2010 at 01:08:59PM +0100, Tomasz Fujak escreveu:
> Signed-off-by: Tomasz Fujak <t.fujak@samsung.com>
> Reviewed-by: Pawel Osciak <p.osciak@samsung.com>
> Reviewed-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reviewed-by: Kyungmin Park <kuyngmin.park@samsung.com>

"Extended" seems vague, I think in this context "platform_" would be a
better namespace specifier.
 
> ---
>  tools/perf/util/parse-events.c |  224 ++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 213 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 05d0c5c..3e32198 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -9,6 +9,9 @@
>  #include "header.h"
>  #include "debugfs.h"
>  
> +#define			EXTENDED_EVENT_PATH	\
> +			"/sys/devices/system/cpu/perf_events/extevents"
> +
>  int				nr_counters;
>  
>  struct perf_event_attr		attrs[MAX_COUNTERS];
> @@ -60,6 +63,10 @@ static struct event_symbol event_symbols[] = {
>  #define PERF_EVENT_TYPE(config)	__PERF_EVENT_FIELD(config, TYPE)
>  #define PERF_EVENT_ID(config)		__PERF_EVENT_FIELD(config, EVENT)
>  
> +static struct event_symbol *extended_event_symbols;
> +static unsigned int 	extended_event_count;
> +static int 			extended_events_initialized;
> +
>  static const char *hw_event_names[] = {
>  	"cycles",
>  	"instructions",
> @@ -241,6 +248,157 @@ static const char *tracepoint_id_to_name(u64 config)
>  	return buf;
>  }
>  
> +static unsigned count_lines(const char *str, unsigned size)
> +{
> +	unsigned count = 0;
> +
> +	while (size--)
> +		count += ('\n' == *str++);
> +	return count;
> +}
> +
> +#define	BYTES_PER_CHUNK 4096
> +/* returns the number of lines read;
> +   on fail the return is negative and no memory is allocated
> +   otherwise the *buf contains a memory chunk of which first
> +   *size bytes are used for file contents
> + */
> +static int read_file(int fd, char **buf, unsigned *size)
> +{
> +	unsigned bytes = BYTES_PER_CHUNK;
> +	int lines = 0;
> +	char *ptr = malloc(bytes);

malloc can fail... Also why is that you can't process the lines one by
one instead of reading the whole (albeit small) file at once?

> +	*size = 0;
> +	do {
> +		int ret = read(fd, ptr + *size, bytes - *size);
> +		if (ret < 0) {
> +			if (EINTR == errno)
> +				continue;
> +			else {
> +				free(ptr);
> +				return -1;
> +			}
> +		}
> +
> +		if (!ret)
> +			break;
> +
> +		lines += count_lines(ptr + *size, ret);
> +		*size += ret;
> +
> +		if (*size == bytes) {
> +			char *tmp = realloc(ptr, bytes <<= 1);
> +			if (!tmp) {
> +				free(ptr);
> +				return -1;
> +			}
> +			ptr = tmp;
> +		}
> +	} while (1);
> +
> +	*buf = ptr;
> +	return lines;
> +}
> +
> +static int parse_extevent_file(struct event_symbol *symbols,
> +			unsigned lines, char *buf)
> +{
> +	char *name, *description, *ptr, *end;
> +	unsigned offset = 0, count = 0;
> +	int items, eaten;
> +	unsigned long long discard;
> +
> +/* each line format should be "0x%llx\t%s\t%lld-%lld\t%s\n" */
> +	while (lines--) {
> +		items = sscanf(buf + offset + 2, "%llx",
> +				&symbols[count].config);
> +		if (1 != items)
> +			continue;
> +
> +		/* skip 0x%llx\t */
> +		ptr = strchr(buf + offset, '\t') + 1;
> +
> +		name = description = NULL;
> +
> +		end = strchr(ptr, '\t');
> +		if (end) {
> +			name = strndup(ptr, end - ptr);
> +			ptr = end + 1;
> +		}
> +
> +		ptr = end;
> +		items = sscanf(ptr, "%lld-%lld\t%n", &discard, &discard,
> +			 &eaten);
> +		if (2 != items)
> +			continue;
> +
> +		ptr += eaten;
> +		end = strchr(ptr, '\n');
> +		if (end) {
> +			description = strndup(ptr, end - ptr);
> +			offset = end - buf + 1;
> +		} else
> +			break;
> +
> +		if (name && description) {
> +			symbols[count].symbol = name;
> +			symbols[count].alias = "";
> +			++count;
> +			/* description gets lost here */
> +			free(description);
> +		} else {
> +			free(description);
> +			free(name);
> +		}

Having "free(description);" in both cases seems funny :-)

- Arnaldo

[PATCH v1 1/1] Extended events (platform-specific) support in perf

[Tomasz Fujak]

> -----Original Message-----
> From: linux-arm-kernel-bounces at lists.infradead.org [mailto:linux-arm-
> kernel-bounces at lists.infradead.org] On Behalf Of Arnaldo Carvalho de
> Melo
> Sent: Friday, January 22, 2010 1:26 PM
> To: Tomasz Fujak
> Cc: jpihet at mvista.com; peterz at infradead.org; p.osciak at samsung.com;
> jamie.iles at picochip.com; will.deacon at arm.com; linux-
> kernel at vger.kernel.org; kyungmin.park at samsung.com; mingo at elte.hu;
> linux-arm-kernel at lists.infradead.org; m.szyprowski at samsung.com
> Subject: Re: [PATCH v1 1/1] Extended events (platform-specific) support
> in perf
> 
> Em Fri, Jan 22, 2010 at 01:08:59PM +0100, Tomasz Fujak escreveu:
> > Signed-off-by: Tomasz Fujak <t.fujak@samsung.com>
> > Reviewed-by: Pawel Osciak <p.osciak@samsung.com>
> > Reviewed-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Reviewed-by: Kyungmin Park <kuyngmin.park@samsung.com>
> 
> "Extended" seems vague, I think in this context "platform_" would be a
> better namespace specifier.

Right.

> > +#define	BYTES_PER_CHUNK 4096
> > +/* returns the number of lines read;
> > +   on fail the return is negative and no memory is allocated
> > +   otherwise the *buf contains a memory chunk of which first
> > +   *size bytes are used for file contents
> > + */
> > +static int read_file(int fd, char **buf, unsigned *size)
> > +{
> > +	unsigned bytes = BYTES_PER_CHUNK;
> > +	int lines = 0;
> > +	char *ptr = malloc(bytes);
> 
> malloc can fail... Also why is that you can't process the lines one by
> one instead of reading the whole (albeit small) file at once?

Right, no malloc check.
The purpose of not processing line-by-line is that then the collection needs
to be dynamic.

Since the file buffer gets dropped after it's processed, the overallocated
memory is returned to the system.
In case of the collection (here an array and a counter) it'd require tricks
not to overallocate, and still be O(#events).

But you're right I didn't do it right - in case the a line does not parse,
memory reserved for it is not freed. Fixing.

> > +
> > +		if (name && description) {
> > +			symbols[count].symbol = name;
> > +			symbols[count].alias = "";
> > +			++count;
> > +			/* description gets lost here */
> > +			free(description);
> > +		} else {
> > +			free(description);
> > +			free(name);
> > +		}
> 
> Having "free(description);" in both cases seems funny :-)

I wasn't so bold as to upgrade the perf with human-readable event
description.
Otherwise yes, moving it outside.

> 
> - Arnaldo
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 1/1] Extended events (platform-specific) support in perf

[Peter Zijlstra]

On Fri, 2010-01-22 at 13:08 +0100, Tomasz Fujak wrote:

there's supposed to be a changelog here...

> Signed-off-by: Tomasz Fujak <t.fujak@samsung.com>
> Reviewed-by: Pawel Osciak <p.osciak@samsung.com>
> Reviewed-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reviewed-by: Kyungmin Park <kuyngmin.park@samsung.com>

Surely one of you knew that ;-)

Anyway, I still think it stinks, and as pointed out, it violates the
one-value-per-file sysfs rule.

I really see no reason why you cannot do this in userspace, have
tools/perf/ provide a library that does this for all supported platforms
with a common interface or something.