diff --git a/ChangeLog b/ChangeLog index 52d7fb70..48187e66 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +Sat Aug 12 16:13:05 2006 Søren Sandmann + + * 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 * TODO: Updates diff --git a/TODO b/TODO index 828d4648..27cbdcde 100644 --- a/TODO +++ b/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: diff --git a/module/sysprof-module.c b/module/sysprof-module.c index d3ef07a6..0ab68dc1 100644 --- a/module/sysprof-module.c +++ b/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; } diff --git a/stackstash.c b/stackstash.c index 21e2f6b6..25e18599 100644 --- a/stackstash.c +++ b/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) {