perf-tools: Fix tracepoint event type recording

PATCH In review Franck Bui-Huu 3 participants 5 messages Jan 17, 2011 – Jan 26, 2011 perf events

lore

Share & tools

lore

Franck Bui-Huu

author

2011-01-17 18:13 UTC #0 lore raw

PATCH 1/2 perf-tools: Fix tracepoint event type recording

From: Franck Bui-Huu <[email protected]>

Tracepoint event registering was done by store_event_type() which took the full event name (sys + ':' + event) as argument.

However commit f006d25a15216a483cec71e886786874f66f9452 broke this by only passing a subset of this full name, that is the substring following the colon.

The consequence of this is that no more tracepoint event type was valid, hence making the trace info section empty.

This patch fixes this by merging store_event_type() into parse_single_tracepoint_event(), so a tracepoint type event is registered when parsed.

Signed-off-by Franck Bui-Huu <[email protected]>

---
 tools/perf/util/header.c       |   15 ++++++++++-----
 tools/perf/util/header.h       |    2 +-
 tools/perf/util/parse-events.c |   38 ++++++--------------------------------
 3 files changed, 17 insertions(+), 38 deletions(-)

tools/perf/util/header.c +10 -5

index 989fa2d..35b707c 100644
@@ -104,10 +104,12 @@ int perf_header__add_attr(struct perf_header *self,
 static int event_count;
 static struct perf_trace_event_type *events;
 
-int perf_header__push_event(u64 id, const char *name)
+int perf_header__push_event(u64 id, const char *name, size_t len)
 {
-	if (strlen(name) > MAX_EVENT_NAME)
+	if (len > MAX_EVENT_NAME - 1) {
 		pr_warning("Event %s will be truncated\n", name);
+		len = MAX_EVENT_NAME - 1;
+	}
 
 	if (!events) {
 		events = malloc(sizeof(struct perf_trace_event_type));

@@ -123,7 +125,8 @@ int perf_header__push_event(u64 id, const char *name)
 	}
 	memset(&events[event_count], 0, sizeof(struct perf_trace_event_type));
 	events[event_count].event_id = id;
-	strncpy(events[event_count].name, name, MAX_EVENT_NAME - 1);
+	strncpy(events[event_count].name, name, len);
+	events[event_count].name[len] = '\0';
 	event_count++;
 	return 0;
 }

@@ -1126,8 +1129,10 @@ int event__synthesize_event_types(event__handler_t process,
 int event__process_event_type(event_t *self,
 			      struct perf_session *session __unused)
 {
-	if (perf_header__push_event(self->event_type.event_type.event_id,
-				    self->event_type.event_type.name) < 0)
+	struct perf_trace_event_type *event_type = &self->event_type.event_type;
+
+	if (perf_header__push_event(event_type->event_id,
+				    event_type->name, strlen(event_type->name)) < 0)
 		return -ENOMEM;
 
 	return 0;

tools/perf/util/header.h +1 -1

index 33f16be..0603a02 100644
@@ -72,7 +72,7 @@ int perf_header__write_pipe(int fd);
 int perf_header__add_attr(struct perf_header *self,
 			  struct perf_header_attr *attr);
 
-int perf_header__push_event(u64 id, const char *name);
+int perf_header__push_event(u64 id, const char *name, size_t name_len);
 char *perf_header__find_event(u64 id);
 
 struct perf_header_attr *perf_header_attr__new(struct perf_event_attr *attr);

tools/perf/util/parse-events.c +6 ~~-32~~

index 5cb6f4b..a58407e 100644
@@ -417,6 +417,7 @@ parse_single_tracepoint_event(char *sys_name,
 	char id_buf[4];
 	u64 id;
 	int fd;
+	size_t len;
 
 	snprintf(evt_path, MAXPATHLEN, "%s/%s/%s/id", debugfs_path,
 		 sys_name, evt_name);

@@ -434,7 +435,6 @@ parse_single_tracepoint_event(char *sys_name,
 	id = atoll(id_buf);
 	attr->config = id;
 	attr->type = PERF_TYPE_TRACEPOINT;
-	*strp += strlen(sys_name) + evt_length + 1; /* + 1 for the ':' */
 
 	attr->sample_type |= PERF_SAMPLE_RAW;
 	attr->sample_type |= PERF_SAMPLE_TIME;

@@ -442,7 +442,11 @@ parse_single_tracepoint_event(char *sys_name,
 
 	attr->sample_period = 1;
 
+	len = strlen(sys_name) + evt_length + 1; /* + 1 for the ':' */
+	if (perf_header__push_event(id, *strp, len) < 0)
+		return EVT_FAILED;
 
+	*strp += len;
 	return EVT_HANDLED;
 }

@@ -490,32 +494,6 @@ parse_multiple_tracepoint_event(char *sys_name, const char *evt_exp,
 	return EVT_HANDLED_ALL;
 }
 
-static int store_event_type(const char *orgname)
-{
-	char filename[PATH_MAX], *c;
-	FILE *file;
-	int id, n;
-
-	sprintf(filename, "%s/", debugfs_path);
-	strncat(filename, orgname, strlen(orgname));
-	strcat(filename, "/id");
-
-	c = strchr(filename, ':');
-	if (c)
-		*c = '/';
-
-	file = fopen(filename, "r");
-	if (!file)
-		return 0;
-	n = fscanf(file, "%i", &id);
-	fclose(file);
-	if (n < 1) {
-		pr_err("cannot store event ID\n");
-		return -EINVAL;
-	}
-	return perf_header__push_event(id, orgname);
-}
-
 static enum event_result parse_tracepoint_event(const char **strp,
 				    struct perf_event_attr *attr)
 {

@@ -558,13 +536,9 @@ static enum event_result parse_tracepoint_event(const char **strp,
 		*strp += strlen(sys_name) + evt_length;
 		return parse_multiple_tracepoint_event(sys_name, evt_name,
 						       flags);
-	} else {
-		if (store_event_type(evt_name) < 0)
-			return EVT_FAILED;
-
+	} else
 		return parse_single_tracepoint_event(sys_name, evt_name,
 						     evt_length, attr, strp);
-	}
 }
 
 static enum event_result

Signature · 1 line

1.7.3.2

Arnaldo Carvalho de Melo

2011-01-17 19:50 UTC #1 lore raw

Em Mon, Jan 17, 2011 at 07:13:11PM +0100, Franck Bui-Huu escreveu:

Quoted 9 lines

From: Franck Bui-Huu <[email protected]>
Tracepoint event registering was done by store_event_type() which took the full event name (sys + ':' + event) as argument.
However commit f006d25a15216a483cec71e886786874f66f9452 broke this by only passing a subset of this full name, that is the substring following the colon.

Quoted 6 lines

The consequence of this is that no more tracepoint event type was valid, hence making the trace info section empty.
This patch fixes this by merging store_event_type() into parse_single_tracepoint_event(), so a tracepoint type event is registered when parsed.

Please try not to do multiple, unrelated changes in a single patch, see below:

Quoted 27 lines

Signed-off-by Franck Bui-Huu <[email protected]>

---
 tools/perf/util/header.c       |   15 ++++++++++-----
 tools/perf/util/header.h       |    2 +-
 tools/perf/util/parse-events.c |   38 ++++++--------------------------------
 3 files changed, 17 insertions(+), 38 deletions(-)

tools/perf/util/header.c +4 -2

index 989fa2d..35b707c 100644
@@ -104,10 +104,12 @@ int perf_header__add_attr(struct perf_header *self,
 static int event_count;
 static struct perf_trace_event_type *events;
 
-int perf_header__push_event(u64 id, const char *name)
+int perf_header__push_event(u64 id, const char *name, size_t len)
 {
-	if (strlen(name) > MAX_EVENT_NAME)
+	if (len > MAX_EVENT_NAME - 1) {
 		pr_warning("Event %s will be truncated\n", name);
+		len = MAX_EVENT_NAME - 1;
+	}
 
 	if (!events) {
 		events = malloc(sizeof(struct perf_trace_event_type));

tools/perf/util/header.c int perf_header__push_event(u64 id, const char *name) · line 129

Quoted 6 lines

tools/perf/util/header.c +2 -1

@@ -123,7 +125,8 @@ int perf_header__push_event(u64 id, const char *name)
 	}
 	memset(&events[event_count], 0, sizeof(struct perf_trace_event_type));
 	events[event_count].event_id = id;
-	strncpy(events[event_count].name, name, MAX_EVENT_NAME - 1);
+	strncpy(events[event_count].name, name, len);
+	events[event_count].name[len] = '\0';

Is this change related to what you describe in the changelog comment? It is a cleanup, can be done as a follow up patch, for perf/core.

The problem you describe deserves perf/urgent treatment, please redo this patch with just the essential bits for that fix.

Quoted 4 lines

tools/perf/util/header.c +0 -0
 	event_count++;
 	return 0;
 }

tools/perf/util/header.c int event__synthesize_event_types(event__handler_t process, · line 1136

Quoted 10 lines

tools/perf/util/header.c +4 -2

@@ -1126,8 +1129,10 @@ int event__synthesize_event_types(event__handler_t process,
 int event__process_event_type(event_t *self,
 			      struct perf_session *session __unused)
 {
-	if (perf_header__push_event(self->event_type.event_type.event_id,
-				    self->event_type.event_type.name) < 0)
+	struct perf_trace_event_type *event_type = &self->event_type.event_type;
+
+	if (perf_header__push_event(event_type->event_id,
+				    event_type->name, strlen(event_type->name)) < 0)
 		return -ENOMEM;

Ditto.

Quoted 35 lines

tools/perf/util/header.c +0 -0

 	return 0;

tools/perf/util/header.h +1 -1

index 33f16be..0603a02 100644
@@ -72,7 +72,7 @@ int perf_header__write_pipe(int fd);
 int perf_header__add_attr(struct perf_header *self,
 			  struct perf_header_attr *attr);
 
-int perf_header__push_event(u64 id, const char *name);
+int perf_header__push_event(u64 id, const char *name, size_t name_len);
 char *perf_header__find_event(u64 id);
 
 struct perf_header_attr *perf_header_attr__new(struct perf_event_attr *attr);

tools/perf/util/parse-events.c +1 -1

index 5cb6f4b..a58407e 100644
@@ -417,6 +417,7 @@ parse_single_tracepoint_event(char *sys_name,
 	char id_buf[4];
 	u64 id;
 	int fd;
+	size_t len;
 
 	snprintf(evt_path, MAXPATHLEN, "%s/%s/%s/id", debugfs_path,
 		 sys_name, evt_name);

@@ -434,7 +435,6 @@ parse_single_tracepoint_event(char *sys_name,
 	id = atoll(id_buf);
 	attr->config = id;
 	attr->type = PERF_TYPE_TRACEPOINT;
-	*strp += strlen(sys_name) + evt_length + 1; /* + 1 for the ':' */
 
 	attr->sample_type |= PERF_SAMPLE_RAW;
 	attr->sample_type |= PERF_SAMPLE_TIME;

tools/perf/util/parse-events.c parse_single_tracepoint_event(char *sys_name, · line 447

Quoted 6 lines

tools/perf/util/parse-events.c +3 -0

@@ -442,7 +442,11 @@ parse_single_tracepoint_event(char *sys_name,
 
 	attr->sample_period = 1;
 
+	len = strlen(sys_name) + evt_length + 1; /* + 1 for the ':' */
+	if (perf_header__push_event(id, *strp, len) < 0)
+		return EVT_FAILED;

Since you're changing the point where perf_header__push_event() is called, consider doing it _after_ parse_events() finishes, by traversing the evsel_list, that way, as a bonus, later, in perf/core we can kill some more global variables:

static int event_count;
static struct perf_trace_event_type *events;

These variables should be in 'struct perf_header', but anyway, this is for later, I'm digressing, please just try to do it after parse_events(), traversing the evsel_list and getting the tracepoint name in string form using event_name(evsel) (that also uses an static variagle, argh, another fix to do).

Doing it that way and excluding the strnlen cleanup, the patch should be much smaller.

We also untie event parsing from header handling, that are only together in record, not in, say, top.

- Arnaldo

Arnaldo Carvalho de Melo

2011-01-17 20:28 UTC #2 lore raw

Em Mon, Jan 17, 2011 at 05:50:34PM -0200, Arnaldo Carvalho de Melo escreveu:

Quoted 17 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (perf_header__push_event(id, *strp, len) < 0)
+		return EVT_FAILED;
Since you're changing the point where perf_header__push_event() is called, consider doing it _after_ parse_events() finishes, by traversing the evsel_list, that way, as a bonus, later, in perf/core we can kill some more global variables:
static int event_count;
static struct perf_trace_event_type *events;
These variables should be in 'struct perf_header', but anyway, this is for later, I'm digressing, please just try to do it after parse_events(), traversing the evsel_list and getting the tracepoint name in string form using event_name(evsel) (that also uses an static variagle, argh, another fix to do).

Quoted 5 lines

Doing it that way and excluding the strnlen cleanup, the patch should be much smaller.
We also untie event parsing from header handling, that are only together in record, not in, say, top.

So here is the minimal patch, tested with:

[root@felicio l]# perf record -a -e sched:* sleep 2
^C[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 2.180 MB perf.data (~95232 samples) ] [root@felicio l]# hexdump -s 0x6e8 -C perf.data -n 512

000006e8  39 00 00 00 00 00 00 00  73 63 68 65 64 3a 73 63  |9.......sched:sc|
000006f8  68 65 64 5f 6b 74 68 72  65 61 64 5f 73 74 6f 70  |hed_kthread_stop|
00000708  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000728  00 00 00 00 00 00 00 00  38 00 00 00 00 00 00 00  |........8.......|
00000738  73 63 68 65 64 3a 73 63  68 65 64 5f 6b 74 68 72  |sched:sched_kthr|
00000748  65 61 64 5f 73 74 6f 70  5f 72 65 74 00 00 00 00  |ead_stop_ret....|
00000758  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000778  37 00 00 00 00 00 00 00  73 63 68 65 64 3a 73 63  |7.......sched:sc|
00000788  68 65 64 5f 77 61 6b 65  75 70 00 00 00 00 00 00  |hed_wakeup......|
00000798  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
000007b8  00 00 00 00 00 00 00 00  36 00 00 00 00 00 00 00  |........6.......|
000007c8  73 63 68 65 64 3a 73 63  68 65 64 5f 77 61 6b 65  |sched:sched_wake|
000007d8  75 70 5f 6e 65 77 00 00  00 00 00 00 00 00 00 00  |up_new..........|
000007e8  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000808  35 00 00 00 00 00 00 00  73 63 68 65 64 3a 73 63  |5.......sched:sc|
00000818  68 65 64 5f 73 77 69 74  63 68 00 00 00 00 00 00  |hed_switch......|
00000828  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000848  00 00 00 00 00 00 00 00  34 00 00 00 00 00 00 00  |........4.......|
00000858  73 63 68 65 64 3a 73 63  68 65 64 5f 6d 69 67 72  |sched:sched_migr|
00000868  61 74 65 5f 74 61 73 6b  00 00 00 00 00 00 00 00  |ate_task........|
00000878  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000898  33 00 00 00 00 00 00 00  73 63 68 65 64 3a 73 63  |3.......sched:sc|
000008a8  68 65 64 5f 70 72 6f 63  65 73 73 5f 66 72 65 65  |hed_process_free|
000008b8  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|

*
000008d8 00 00 00 00 00 00 00 00 32 00 00 00 00 00 00 00 |........2.......| 000008e8
[root@felicio l]# cat /sys/kernel/debug/tracing/events/sched/sched_kthread_stop/id 57
[root@felicio l]# cat /sys/kernel/debug/tracing/events/sched/sched_kthread_stop_ret/id 56
[root@felicio l]# cat /sys/kernel/debug/tracing/events/sched/sched_wakeup/id 55
[root@felicio l]# cat /sys/kernel/debug/tracing/events/sched/sched_wakeup_new/id 54
[root@felicio l]# cat /sys/kernel/debug/tracing/events/sched/sched_wakeup_switch/id
cat: /sys/kernel/debug/tracing/events/sched/sched_wakeup_switch/id: No such file or directory [root@felicio l]# cat /sys/kernel/debug/tracing/events/sched/sched_switch/id 53
[root@felicio l]# cat /sys/kernel/debug/tracing/events/sched/sched_migrate_task/id 52
[root@felicio l]# cat /sys/kernel/debug/tracing/events/sched/sched_process_free/id 51
[root@felicio l]#

See the 0x39, 0x38, 0x37, 0x36, just before the tracepoint names? :-)

It trows all this perf_header stuff out of the parsing routines and moves it to record, where it belongs. No need to re-open the /sys/ file again to get something we already have in perf_evsel->attr.config and that was parsed, opening the /sys file in parse_single_tracepoint_event.

Please let me know if you see any hole in it.

- Arnaldo

tools/perf/builtin-record.c +2 -0

index df6064a..fcd29e8 100644
@@ -936,6 +936,8 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
 	list_for_each_entry(pos, &evsel_list, node) {
 		if (perf_evsel__alloc_fd(pos, cpus->nr, threads->nr) < 0)
 			goto out_free_fd;
+		if (perf_header__push_event(pos->attr.config, event_name(pos)))
+			goto out_free_fd;
 	}
 	event_array = malloc((sizeof(struct pollfd) * MAX_NR_CPUS *
 			      MAX_COUNTERS * threads->nr));

tools/perf/util/parse-events.c +0 ~~-29~~

index 1f4cfe5..bc2732e 100644
@@ -490,32 +490,6 @@ parse_multiple_tracepoint_event(char *sys_name, const char *evt_exp,
 	return EVT_HANDLED_ALL;
 }
 
-static int store_event_type(const char *orgname)
-{
-	char filename[PATH_MAX], *c;
-	FILE *file;
-	int id, n;
-
-	sprintf(filename, "%s/", debugfs_path);
-	strncat(filename, orgname, strlen(orgname));
-	strcat(filename, "/id");
-
-	c = strchr(filename, ':');
-	if (c)
-		*c = '/';
-
-	file = fopen(filename, "r");
-	if (!file)
-		return 0;
-	n = fscanf(file, "%i", &id);
-	fclose(file);
-	if (n < 1) {
-		pr_err("cannot store event ID\n");
-		return -EINVAL;
-	}
-	return perf_header__push_event(id, orgname);
-}
-
 static enum event_result parse_tracepoint_event(const char **strp,
 				    struct perf_event_attr *attr)
 {

@@ -559,9 +533,6 @@ static enum event_result parse_tracepoint_event(const char **strp,
 		return parse_multiple_tracepoint_event(sys_name, evt_name,
 						       flags);
 	} else {
-		if (store_event_type(evt_name) < 0)
-			return EVT_FAILED;
-
 		return parse_single_tracepoint_event(sys_name, evt_name,
 						     evt_length, attr, strp);
 	}

tip-bot for Arnaldo Carvalho de Melo

2011-01-18 08:49 UTC #3 lore raw

[tip:perf/urgent] perf tools: Fix tracepoint id to string perf.data header table

Commit-ID:  ad7f4e3f7b966ac09c8f98dbc5024813a1685775
Gitweb:     http://git.kernel.org/tip/ad7f4e3f7b966ac09c8f98dbc5024813a1685775
Author:     Arnaldo Carvalho de Melo <[email protected]>
AuthorDate: Mon, 17 Jan 2011 18:28:13 -0200
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 17 Jan 2011 18:28:13 -0200

perf tools: Fix tracepoint id to string perf.data header table

It was broken by f006d25 that passed just the event name, not the complete sys:event that it expected to open the /sys/.../sys/sys:event/id file to get the id.

Fix it by moving it to after parse_events in cmd_record, as at that point we can just traverse the evsel_list and use evsel->attr.config + event_name(evsel) instead of re-opening the /id file.

Reported-by Franck Bui-Huu <[email protected]>

Cc Franck Bui-Huu <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Han Pingtian <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Stephane Eranian <[email protected]>

Cc Tom Zanussi <[email protected]>

LKML-Reference: <[email protected]>

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/builtin-record.c    |    2 ++
 tools/perf/util/parse-events.c |   29 -----------------------------
 2 files changed, 2 insertions(+), 29 deletions(-)

tools/perf/builtin-record.c +2 -0

index df6064a..fcd29e8 100644
@@ -936,6 +936,8 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
 	list_for_each_entry(pos, &evsel_list, node) {
 		if (perf_evsel__alloc_fd(pos, cpus->nr, threads->nr) < 0)
 			goto out_free_fd;
+		if (perf_header__push_event(pos->attr.config, event_name(pos)))
+			goto out_free_fd;
 	}
 	event_array = malloc((sizeof(struct pollfd) * MAX_NR_CPUS *
 			      MAX_COUNTERS * threads->nr));

tools/perf/util/parse-events.c +0 ~~-29~~

index 1f4cfe5..bc2732e 100644
@@ -490,32 +490,6 @@ parse_multiple_tracepoint_event(char *sys_name, const char *evt_exp,
 	return EVT_HANDLED_ALL;
 }
 
-static int store_event_type(const char *orgname)
-{
-	char filename[PATH_MAX], *c;
-	FILE *file;
-	int id, n;
-
-	sprintf(filename, "%s/", debugfs_path);
-	strncat(filename, orgname, strlen(orgname));
-	strcat(filename, "/id");
-
-	c = strchr(filename, ':');
-	if (c)
-		*c = '/';
-
-	file = fopen(filename, "r");
-	if (!file)
-		return 0;
-	n = fscanf(file, "%i", &id);
-	fclose(file);
-	if (n < 1) {
-		pr_err("cannot store event ID\n");
-		return -EINVAL;
-	}
-	return perf_header__push_event(id, orgname);
-}
-
 static enum event_result parse_tracepoint_event(const char **strp,
 				    struct perf_event_attr *attr)
 {

@@ -559,9 +533,6 @@ static enum event_result parse_tracepoint_event(const char **strp,
 		return parse_multiple_tracepoint_event(sys_name, evt_name,
 						       flags);
 	} else {
-		if (store_event_type(evt_name) < 0)
-			return EVT_FAILED;
-
 		return parse_single_tracepoint_event(sys_name, evt_name,
 						     evt_length, attr, strp);
 	}

Franck Bui-Huu

author

2011-01-26 21:57 UTC #4 lore raw

In reply to Arnaldo Carvalho de Melo

Hello,

Sorry for my (very) late answer...

Quoted 1 line · Arnaldo Carvalho de Melo

Em Mon, Jan 17, 2011 at 07:13:11PM +0100, Franck Bui-Huu escreveu:

[...]

Quoted 3 lines

Please try not to do multiple, unrelated changes in a single patch, see below:

Well, they're not unrelated because of the place I choose to put perf_header__push_event().

But I agree with you, placing it after parse_events() is much better.

Quoted 18 lines

tools/perf/util/header.c +4 -2

index 989fa2d..35b707c 100644
@@ -104,10 +104,12 @@ int perf_header__add_attr(struct perf_header *self,
 static int event_count;
 static struct perf_trace_event_type *events;
 
-int perf_header__push_event(u64 id, const char *name)
+int perf_header__push_event(u64 id, const char *name, size_t len)
 {
-	if (strlen(name) > MAX_EVENT_NAME)
+	if (len > MAX_EVENT_NAME - 1) {
 		pr_warning("Event %s will be truncated\n", name);
+		len = MAX_EVENT_NAME - 1;
+	}

Quoted 12 lines

tools/perf/util/header.c int perf_header__push_event(u64 id, const char *name) · line 129
Quoted 9 lines
tools/perf/util/header.c +2 -1
 	if (!events) {
 		events = malloc(sizeof(struct perf_trace_event_type));
@@ -123,7 +125,8 @@ int perf_header__push_event(u64 id, const char *name)
 	}
 	memset(&events[event_count], 0, sizeof(struct perf_trace_event_type));
 	events[event_count].event_id = id;
-	strncpy(events[event_count].name, name, MAX_EVENT_NAME - 1);
+	strncpy(events[event_count].name, name, len);
+	events[event_count].name[len] = '\0';
Is this change related to what you describe in the changelog comment? It is a cleanup, can be done as a follow up patch, for perf/core.

It's not really unrelated if you place perf_header__push_event() where I did.

BTW it's not a cleanup too, it's rather a fix since the original code is broken if the name is truncated since the null byte is missing in this case. [...]

Quoted 14 lines

Since you're changing the point where perf_header__push_event() is called, consider doing it _after_ parse_events() finishes, by traversing the evsel_list, that way, as a bonus, later, in perf/core we can kill some more global variables:
static int event_count;
static struct perf_trace_event_type *events;
These variables should be in 'struct perf_header', but anyway, this is for later, I'm digressing, please just try to do it after parse_events(), traversing the evsel_list and getting the tracepoint name in string form using event_name(evsel) (that also uses an static variagle, argh, another fix to do).

Quoted 5 lines

Doing it that way and excluding the strnlen cleanup, the patch should be much smaller.
We also untie event parsing from header handling, that are only together in record, not in, say, top.

Agreed.

Franck