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

[Arnaldo Carvalho de Melo <acme@ghostprotocols.net>]

Em Mon, Jan 17, 2011 at 05:50:34PM -0200, Arnaldo Carvalho de Melo escreveu:
> > +	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.

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

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index df6064a..fcd29e8 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -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));
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 1f4cfe5..bc2732e 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -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);
 	}