Re: [PATCH 1/2] perf-tools: Fix tracepoint event type recording
From: Arnaldo Carvalho de Melo
Date: Mon Jan 17 2011 - 15:28:17 EST
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);
}
--
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/