Make n_samples per-cpu. Add an atomic variable in_timer_notify and use it

Sat Aug 12 16:13:05 2006  Søren Sandmann  <sandmann@redhat.com>

	* module/sysprof-module.c: Make n_samples per-cpu. Add an atomic
	variable in_timer_notify and use it to lock out simultaneous timer
	interrupts.

	* stackstash.c (decorate_node): Make decorate_node() static

	* TODO

ChangeLog

@@ -1,3 +1,13 @@
+Sat Aug 12 16:13:05 2006  Søren Sandmann  <sandmann@redhat.com>
+
+	* module/sysprof-module.c: Make n_samples per-cpu. Add an atomic
+	variable in_timer_notify and use it to lock out simultaneous timer
+	interrupts.
+
+	* stackstash.c (decorate_node): Make decorate_node() static
+
+	* TODO
+
 Fri Aug 11 11:41:15 2006  Søren Sandmann  <sandmann@redhat.com>
 
 	* TODO: Updates

TODO

@@ -1,4 +1,4 @@
-Before 1.0.3:
+Before 1.0.4:
 
 * See if we can reproduce the problem where libraries didn't get correctly
   reloaded after new versions were installed.
@@ -32,14 +32,16 @@ Before 1.0.3:
 
 Before 1.2:
 
-* Rename stack_stash_foreach_by_address() to stack_stash_foreach_unique()
+* Rename stack_stash_foreach_by_address() to stack_stash_foreach_unique(),
+  or maybe not ...
 
 * Make it compilable against a non-running kernel.
 
 * commandline version should check that the output file is writable
   before starting the profiling.
 
-* Maybe report idle time?
+* Maybe report idle time? Although this would come for free with the
+  timelines.
 
 * Fix (deleted) problem. But more generally, whenever we can't display a
   symbol, display an error message instead, ie., 
@@ -366,6 +368,11 @@ When the interrupt happens,
 
 Later:
 
+- On SMP systems interrupts happen unpredictably, including when another
+  one is running. Right now we are ignoring any interrupts that happen
+  when another one is running, but we should probably just save the data
+  in a per-cpu buffer.
+
 - Find out if sysprof accurately reports time spent handling pagefaults.
   There is evidence that it doesn't:
 	- a version of sysprof took 10 seconds to load a certain profile.
@@ -553,7 +560,26 @@ Later:
 
 
 
-DONE:
+-=-=-=-=-=-=-=-=-=-=-=-=-=-=- ALREADY DONE -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
+
+* It is apparently possible to get another timer interrupt in the middle
+  of timer_notify. If this happens the circular buffer gets screwed up and
+  you get crashes. Note this gets much worse on SMP (in fact how did this
+  work at all previously?)
+
+  Possible fixes
+	- have a "in timer notify" variable, then simply reject nested
+	  interrupts
+	- keep a "ghost head" that timers can use to allocate new traces,
+	  then update the real head whenever one of them completes. Note
+	  though, that the traces will get generated in the wrong order
+	  due to the nesting. In fact, only the outermost timernotify
+	  can update the real head, and it should update it to ghost_head.
+	- do correct locking? Nah, that's crazy talk
+  Also note: a race is a race, and on SMP we probably can't even make it
+  unlikely enough to not matter.
+
+  Fixed by ignoring the nested interrupts using an atomic variable.
 
 * When you load a file from the commandline, there is a weird flash of the toolbar. 
   What's going on is this:

module/sysprof-module.c

@@ -107,6 +107,8 @@ read_frame (void *frame_pointer, StackFrame *frame)
 	return 0;
 }
 
+DEFINE_PER_CPU (int, n_samples);
+
 #ifdef OLD_PROFILE
 static int timer_notify(struct notifier_block * self, unsigned long val, void * data)
 #else
@@ -118,23 +120,28 @@ timer_notify (struct pt_regs *regs)
 	struct pt_regs * regs = (struct pt_regs *)data;
 #endif
 	void *frame_pointer;
-	static int n_samples;
 	SysprofStackTrace *trace = head;
 	int i;
 	int is_user;
 	StackFrame frame;
 	int result;
+	static atomic_t in_timer_notify = ATOMIC_INIT(1);
 
-	if ((++n_samples % INTERVAL) != 0)
+	/* 0: locked, 1: unlocked */
+	
+	if (((++get_cpu_var(n_samples)) % INTERVAL) != 0)
 		return 0;
 	
+	if (!atomic_dec_and_test (&in_timer_notify))
+		goto out;
+
 	is_user = user_mode(regs);
 
 	if (!current || current->pid == 0 || !current->mm)
-		return 0;
+		goto out;
 	
 	if (is_user && current->state != TASK_RUNNING)
-		return 0;
+		goto out;
 
 	memset(trace, 0, sizeof (SysprofStackTrace));
 	
@@ -170,7 +177,9 @@ timer_notify (struct pt_regs *regs)
 		head = &stack_traces[0];
 	
 	wake_up (&wait_for_trace);
-	
+
+out:
+	atomic_inc (&in_timer_notify);
 	return 0;
 }
 

stackstash.c

@@ -62,7 +62,7 @@ stack_stash_new (GDestroyNotify destroy)
     return create_stack_stash (destroy);
 }
 
-void
+static void
 decorate_node (StackStash *stash,
 	       StackNode  *node)
 {