From 09ffea0d57f2081021ea7e3c7409c0006012e9c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Sandmann=20Pedersen?= Date: Tue, 8 Sep 2009 19:35:03 -0400 Subject: [PATCH] TODO, plus a number of other fixes --- TODO | 32 +++++++++++---- collector.c | 112 +++++++++++++++++++++++++++++++++++++++------------- tracker.c | 31 +++++++++++---- 3 files changed, 132 insertions(+), 43 deletions(-) diff --git a/TODO b/TODO index 90c6ee4b..8d0e5f5d 100644 --- a/TODO +++ b/TODO @@ -23,14 +23,30 @@ Before 1.0.4: Before 1.2: +* How to deal with forks and mmaps seemingly happening after + samples. This can happen when a process migrates between CPUs. + There doesn't seem to be any way to get this information in a + guaranteed correct way. + +* How to deal with processes that exit before we can get their maps? + + Such a process can never cause events itself, but it may fork a + child that does. There is no way to get the maps for that child. A + possible solution would be to optionally generate mmap event after + forks. Userspace would turn this off when it was done with the + initial gathering of processes. + +* Find out why the busy cursor stays active after you hit start + * Kernel binary when available, is better than kallsyms. * Hack around gtk+ bug where it mispositions the window. -* Counters must not be destroyed during tracker setup. They have to exist - but be disabled so that we can track creation of processes. +* Counters must not be destroyed during tracker setup. They have to + exist but be disabled so that we can track creation of processes. -* Check that we don't use too much memory (in particular with the timeline). +* Check that we don't use too much memory (in particular with the + timeline). * Fix names. "new process" is really "exec" @@ -49,8 +65,8 @@ Before 1.2: * Get rid of process.c -* On 32 bit, NMI stackframes are not filtered out which leads to - wrong kernel traces +* On 32 bit, NMI stackframes are not filtered out which leads to wrong + kernel traces * Can we track kernel modules being inserted/removed? @@ -66,7 +82,7 @@ Before 1.2: * Sometimes it get samples for unknown processes. This may be due to forking without execing. -* Give an informative error message if run as root +* Give an informative error message if not run as root * What are the events generated for pid 0 and pid 1? They have no stacktrace, and an eip in the kernel. @@ -75,13 +91,13 @@ Before 1.2: 32 bits vs 64 bits. Or use C++ like behdad does in harfbuzz? * We often get "No map". I suspect this is because the vdso stackframe - is broken. + is strange. * Find out why gtk_tree_view_columns_autosize() apparently doesn't work on empty tree views. * Write my own tree model? There is still performance issues in - footreestore. + FooTreeStore. * Hack to disable recursion for binaries without symbols causes the symbols to not work the way other symbols do. A better approach is diff --git a/collector.c b/collector.c index d4280070..02967a84 100644 --- a/collector.c +++ b/collector.c @@ -48,7 +48,9 @@ typedef struct exit_event_t exit_event_t; typedef struct fork_event_t fork_event_t; typedef union counter_event_t counter_event_t; -static void process_event (Collector *collector, counter_event_t *event); +static void process_event (Collector *collector, + counter_t *counter, + counter_event_t *event); struct counter_t { @@ -254,7 +256,7 @@ on_read (gpointer data) if (header->type == PERF_EVENT_SAMPLE) collector->n_samples++; - process_event (collector, (counter_event_t *)header); + process_event (collector, counter, (counter_event_t *)header); } tail += header->size; @@ -332,12 +334,9 @@ counter_new (Collector *collector, attr.wakeup_events = 100000; attr.disabled = TRUE; - if (cpu == 0) - { - attr.mmap = 1; - attr.comm = 1; - attr.task = 1; - } + attr.mmap = 1; + attr.comm = 1; + attr.task = 1; fd = sysprof_perf_counter_open (&attr, -1, cpu, -1, 0); @@ -375,6 +374,12 @@ counter_enable (counter_t *counter) ioctl (counter->fd, PERF_COUNTER_IOC_ENABLE); } +static void +counter_disable (counter_t *counter) +{ + ioctl (counter->fd, PERF_COUNTER_IOC_DISABLE); +} + static void counter_free (counter_t *counter) { @@ -390,29 +395,64 @@ counter_free (counter_t *counter) /* * Collector */ +static void +enable_counters (Collector *collector) +{ + GList *list; + + for (list = collector->counters; list != NULL; list = list->next) + { + counter_t *counter = list->data; + + counter_enable (counter); + } +} + +static void +disable_counters (Collector *collector) +{ + GList *list; + + for (list = collector->counters; list != NULL; list = list->next) + { + counter_t *counter = list->data; + + counter_disable (counter); + } +} + void collector_reset (Collector *collector) { - gboolean restart = FALSE; - + /* Disable the counters so that we won't track + * the activity of tracker_free()/tracker_new() + * + * They will still record fork/mmap/etc. so + * we can keep an accurate log of process creation + */ if (collector->counters) { - collector_stop (collector); - restart = TRUE; + g_print ("disable counters\n"); + + disable_counters (collector); } if (collector->tracker) { tracker_free (collector->tracker); - collector->tracker = NULL; + collector->tracker = tracker_new (); } collector->n_samples = 0; g_get_current_time (&collector->latest_reset); - if (restart) - collector_start (collector, NULL); + if (collector->counters) + { + g_print ("enable counters\n"); + + enable_counters (collector); + } } /* callback is called whenever a new sample arrives */ @@ -446,7 +486,7 @@ process_mmap (Collector *collector, mmap_event_t *mmap) static void process_comm (Collector *collector, comm_event_t *comm) { - g_print ("comm: pid: %d\n", comm->pid); + g_print ("pid, tid: %d %d", comm->pid, comm->tid); tracker_add_process (collector->tracker, comm->pid, @@ -456,7 +496,8 @@ process_comm (Collector *collector, comm_event_t *comm) static void process_fork (Collector *collector, fork_event_t *fork) { - g_print ("fork: ppid: %d pid: %d\n", fork->ppid, fork->pid); + g_print ("ppid: %d pid: %d ptid: %d tid %d", + fork->ppid, fork->pid, fork->ptid, fork->tid); tracker_add_fork (collector->tracker, fork->ppid, fork->pid); } @@ -464,6 +505,8 @@ process_fork (Collector *collector, fork_event_t *fork) static void process_exit (Collector *collector, exit_event_t *exit) { + g_print ("for %d %d", exit->pid, exit->tid); + tracker_add_exit (collector->tracker, exit->pid); } @@ -474,6 +517,8 @@ process_sample (Collector *collector, uint64_t *ips; int n_ips; + g_print ("pid, tid: %d %d", sample->pid, sample->tid); + if (sample->n_ips == 0) { uint64_t trace[3]; @@ -508,8 +553,27 @@ process_sample (Collector *collector, static void process_event (Collector *collector, + counter_t *counter, counter_event_t *event) { + char *name; + + switch (event->header.type) + { + case PERF_EVENT_MMAP: name = "mmap"; break; + case PERF_EVENT_LOST: name = "lost"; break; + case PERF_EVENT_COMM: name = "comm"; break; + case PERF_EVENT_EXIT: name = "exit"; break; + case PERF_EVENT_THROTTLE: name = "throttle"; break; + case PERF_EVENT_UNTHROTTLE: name = "unthrottle"; break; + case PERF_EVENT_FORK: name = "fork"; break; + case PERF_EVENT_READ: name = "read"; break; + case PERF_EVENT_SAMPLE: name = "samp"; break; + default: name = "unknown"; break; + } + + g_print ("cpu %d :: %s :: ", counter->cpu, name); + switch (event->header.type) { case PERF_EVENT_MMAP: @@ -545,10 +609,11 @@ process_event (Collector *collector, break; default: - g_warning ("Got unknown event: %d (%d)\n", - event->header.type, event->header.size); + g_warning ("unknown event: %d (%d)\n", event->header.type, event->header.size); break; } + + g_print ("\n"); } gboolean @@ -556,7 +621,6 @@ collector_start (Collector *collector, GError **err) { int n_cpus = get_n_cpus (); - GList *list; int i; if (!collector->tracker) @@ -568,14 +632,8 @@ collector_start (Collector *collector, collector->counters = g_list_append (collector->counters, counter); } - - /* Hack to make sure we parse the kernel symbols before - * starting collection, so the parsing doesn't interfere - * with the profiling. - */ - for (list = collector->counters; list != NULL; list = list->next) - counter_enable (list->data); + enable_counters (collector); return TRUE; } diff --git a/tracker.c b/tracker.c index 92f2ab20..a94650d3 100644 --- a/tracker.c +++ b/tracker.c @@ -104,6 +104,7 @@ fake_new_process (tracker_t *tracker, pid_t pid) if (lines[0] && strlen (lines[0]) > 0) { tracker_add_process (tracker, pid, lines[0]); + done = TRUE; } @@ -131,6 +132,9 @@ fake_new_process (tracker_t *tracker, pid_t pid) g_strfreev (lines); } + + if (!done) + g_print ("failed to fake %d\n", pid); } static void @@ -460,6 +464,8 @@ create_process (state_t *state, new_process_t *new_process) process->pid = new_process->pid; process->comm = g_strdup (new_process->command_line); process->maps = g_ptr_array_new (); + + g_print ("new comm process %d\n", new_process->pid); g_hash_table_insert ( state->processes_by_pid, GINT_TO_POINTER (process->pid), process); @@ -482,7 +488,9 @@ process_fork (state_t *state, fork_t *fork) process_t *parent = g_hash_table_lookup ( state->processes_by_pid, GINT_TO_POINTER (fork->pid)); +#if 0 if (parent) +#endif { process_t *process = g_new0 (process_t, 1); int i; @@ -490,26 +498,33 @@ process_fork (state_t *state, fork_t *fork) g_print ("new child %d\n", fork->child_pid); process->pid = fork->child_pid; - process->comm = g_strdup (parent->comm); + process->comm = g_strdup (parent? parent->comm : ""); process->maps = g_ptr_array_new (); - - for (i = 0; i < parent->maps->len; ++i) + + if (parent) { - map_t *map = copy_map (parent->maps->pdata[i]); - - g_ptr_array_add (process->maps, map); + for (i = 0; i < parent->maps->len; ++i) + { + map_t *map = copy_map (parent->maps->pdata[i]); + + g_ptr_array_add (process->maps, map); + } } - + g_hash_table_insert ( state->processes_by_pid, GINT_TO_POINTER (process->pid), process); } +#if 0 else g_print ("no parent for %d\n", fork->child_pid); +#endif } static void process_exit (state_t *state, exit_t *exit) { + g_print ("Exit for %d\n", exit->pid); + /* ignore for now */ } @@ -989,7 +1004,7 @@ tracker_create_profile (tracker_t *tracker) break; case EXIT: - process_exit (state, (exit_t *)exit); + process_exit (state, (exit_t *)event); event += sizeof (exit_t); break;