[sysprof] TODO, plus a number of other fixes

[Søren Sandmann Pedersen <ssp src gnome org>]

commit 09ffea0d57f2081021ea7e3c7409c0006012e9c4
Author: Søren Sandmann Pedersen <ssp redhat com>
Date:   Tue Sep 8 19:35:03 2009 -0400

    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 90c6ee4..8d0e5f5 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 d428007..02967a8 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);
     
@@ -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;
 }
diff --git a/tracker.c b/tracker.c
index 92f2ab2..a94650d 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 : "<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;