TODO, plus a number of other fixes
authorSøren Sandmann Pedersen <ssp@redhat.com>
Tue, 8 Sep 2009 23:35:03 +0000 (19:35 -0400)
committerSøren Sandmann Pedersen <ssp@redhat.com>
Tue, 8 Sep 2009 23:35:03 +0000 (19:35 -0400)
TODO
collector.c
tracker.c

diff --git a/TODO b/TODO
index 90c6ee4..8d0e5f5 100644 (file)
--- 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
index d428007..02967a8 100644 (file)
@@ -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);
     
@@ -376,6 +375,12 @@ counter_enable (counter_t *counter)
 }
 
 static void
+counter_disable (counter_t *counter)
+{
+    ioctl (counter->fd, PERF_COUNTER_IOC_DISABLE);
+}
+
+static void
 counter_free (counter_t *counter)
 {
     munmap (counter->mmap_page, (N_PAGES + 1) * get_page_size());
@@ -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;
 }
index 92f2ab2..a94650d 100644 (file)
--- 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 : "<unknown>");
        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;