updates

Fri Apr 15 16:37:45 2005  Soeren Sandmann  <sandmann@redhat.com>

	* TODO: updates

	* sysprof.c (sorry): If you hit profile when the module isn't
	loaded, pop up an annoying dialog.

	* sysprof-module.c: Clean-ups, remove various unused abstractions.

ChangeLog

@@ -1,3 +1,12 @@
+Fri Apr 15 16:37:45 2005  Soeren Sandmann  <sandmann@redhat.com>
+
+	* TODO: updates
+
+	* sysprof.c (sorry): If you hit profile when the module isn't
+	loaded, pop up an annoying dialog.
+
+	* sysprof-module.c: Clean-ups, remove various unused abstractions.
+
 Sat Apr  9 17:49:13 2005  Søren Sandmann  <sandmann@redhat.com>
 
 	* COPYING: Add a copy of the GPL

TODO

@@ -103,8 +103,6 @@ Later:
 - Find out how to hack around gtk+ bug causing multiple double clicks 
   to get eaten.
 
-- Figure out what a 'disk profiler' is.
-
 - Consider what it would take to take stacktraces of other languages
 
 	- perl, 
@@ -121,7 +119,33 @@ Later:
 
   This function would behave essentially like a signal handler: couldn't
   call malloc(), couldn't call printf(), etc. 
-  
+
+- Consider this usecase:
+	Someone is considering replacing malloc()/free() with a freelist
+	for a certain data structure. All use of this data structure is 
+	confined to one function, foo(). It is now interesting to know
+	how much time that particular function spends on malloc() and free()
+	combined.
+
+	Possible UI:
+
+		- Select foo(), 
+		- find an instance of malloc()
+		- shift-click on it,
+		-       all traces with malloc are removed
+		- 	a new item "..." appears immeidately below foo()
+		-       malloc is added below "..."
+  		- same for free
+		- at this point, the desired data can be read at comulative
+		  for "..." 
+
+	Actually, with this UI, you could potentially get rid of the
+	caller list: Just present the call tree under an <everything> root, 
+	and use ... to  single out the stuff you are interested in.
+
+	Maybe also get rid of 'callers' by having a new "show details"
+	dialog or something.
+		
 - figure out a way to deal with both disk and CPU. Need to make sure that
   things that are UNINTERRUPTIBLE while there are RUNNING tasks are not
   considered bad.
@@ -143,7 +167,7 @@ Later:
   traces has the same diskaccesses.
 
   Or visualize a set of squares with a color that is more saturated depending
-  on the number of unique stack traces that access it. The look for the
+  on the number of unique stack traces that access it. Then look for the
   lightly saturated ones. 
 
   The input to the profiler would basically be

sysprof-module.c

@@ -37,6 +37,7 @@
 #include "sysprof-module.h"
 
 #include <linux/version.h>
+#include <linux/kallsyms.h>
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Soeren Sandmann (sandmann@daimi.au.dk)");
@@ -51,34 +52,11 @@ static SysprofStackTrace *	tail = &stack_traces[0];
 DECLARE_WAIT_QUEUE_HEAD (wait_for_trace);
 DECLARE_WAIT_QUEUE_HEAD (wait_for_exit);
 
-typedef void (* TimeoutFunc)(unsigned long data);
+static int exiting;
+
 static void on_timer(unsigned long);
-
-/*
- * Wrappers to cover up kernel differences in timer handling
- */
 static struct timer_list timer;
 
-static void
-init_timeout (void)
-{
-	init_timer(&timer);
-}
-
-static void
-remove_timeout(void)
-{
-	del_timer (&timer);
-}
-
-static void
-add_timeout(unsigned int interval,
-	    TimeoutFunc  f)
-{
-	timer.function = f;
-	mod_timer(&timer, jiffies + INTERVAL);
-}
-
 typedef struct userspace_reader userspace_reader;
 struct userspace_reader
 {
@@ -108,7 +86,8 @@ init_userspace_reader (userspace_reader *reader,
  * Do not walk the page table directly, use get_user_pages
  */
  
-static int x_access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, int len, int write)
+static int
+x_access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, int len, int write)
 {
 	struct mm_struct *mm;
 	struct vm_area_struct *vma;
@@ -166,50 +145,8 @@ read_user_space (userspace_reader *reader,
 		 unsigned long address,
 		 unsigned long *result)
 {
-#if 0
-	unsigned long user_page = (address & PAGE_MASK);
-	int res;
-#endif
-
 	return x_access_process_vm(reader->task, address, result, 4, 0);
 }
-#if 0
-	struct task_struct *tsk, unsigned long addr, void *buf, int len, int write);
-
-	if (user_page == 0)
-		return 0;
-
-	if (!reader->user_page || user_page != reader->user_page) {
-		int found;
-		struct page *page;
-		pte_t pte;
-		
-		found = get_user_pages (reader->task, reader->task->mm,
-					user_page, 1, 0, 0, &page, NULL);
-
-		if (!found)
-			return 0;
-
-		if (!pte_present (page))
-			return 0;
-		
-		if (reader->user_page)
-			kunmap (reader->page);
-
-		reader->kernel_page = (unsigned long)kmap (page);
-		reader->user_page = user_page;
-		reader->page = page;
-		
-	}
-
-	if (get_user (res, (int *)(reader->kernel_page + (address - user_page))) != 0)
-		return 0;
-
-	*result = res;
-	
-	return 1;
-}
-#endif
 
 static void
 done_userspace_reader (userspace_reader *reader)
@@ -256,7 +193,7 @@ generate_stack_trace(struct task_struct *task,
 	unsigned long addr;
 	userspace_reader reader;
 	int i;
-	
+
 	memset(trace, 0, sizeof (SysprofStackTrace));
 	
 	trace->pid = task->pid;
@@ -292,49 +229,32 @@ static void
 do_generate (void *data)
 {
 	struct task_struct *task = data;
-	struct task_struct *g, *p;
+	
+	generate_stack_trace(task, head);
+	wake_up_process (task);
+		
+	if (head++ == &stack_traces[N_TRACES - 1])
+		head = &stack_traces[0];
+	
+	wake_up (&wait_for_trace);
 
-	/* Make sure the thread still exists */
+	mod_timer(&timer, jiffies + INTERVAL);
-	/* FIXME: this is probably not necessary anymore, now that
-	 * we make the process sleep
-	 */
-	do_each_thread (g, p) {
-		if (p == task) {
-			generate_stack_trace(task, head);
-
-			if (head++ == &stack_traces[N_TRACES - 1])
-				head = &stack_traces[0];
-			
-			wake_up (&wait_for_trace);
-			wake_up_process (task);
-
-			goto out;
-		}
-	} while_each_thread (g, p);
-
- out:
-	add_timeout (INTERVAL, on_timer);
 }
 
 static void
 on_timer(unsigned long dong)
 {
-#if 0
+	if (current && current->state == TASK_RUNNING && current->pid != 0)
-	static const int cpu_profiler = 1; /* set to 0 to profile disk */
+	{
-
-	if (p->state == (cpu_profiler? TASK_RUNNING : TASK_UNINTERRUPTIBLE))
-		;
-#endif
-
-	if (current && current->state == TASK_RUNNING && current->pid != 0) {
-		INIT_WORK (&work, do_generate, current);
-		
 		set_current_state (TASK_UNINTERRUPTIBLE);
 		
+		INIT_WORK (&work, do_generate, current);
+		
 		schedule_work (&work);
 	}
-	else {
+	else
-		add_timeout (INTERVAL, on_timer);
+	{
+		mod_timer(&timer, jiffies + INTERVAL);
 	}
 }
 
@@ -382,9 +302,9 @@ init_module(void)
 	trace_proc_file->proc_fops->poll = procfile_poll;
 	trace_proc_file->size = sizeof (SysprofStackTrace);
 	
-	init_timeout();
+	init_timer(&timer);
-	
+	timer.function = on_timer;
-	add_timeout(INTERVAL, on_timer);
+	mod_timer(&timer, jiffies + INTERVAL);
 	
 	printk(KERN_ALERT "starting sysprof module\n");
 	
@@ -394,8 +314,10 @@ init_module(void)
 void
 cleanup_module(void)
 {
-	remove_timeout();
+	exiting = 1;
-	
+
+	del_timer (&timer);
+
 	remove_proc_entry("sysprof-trace", &proc_root);
 	
 	printk(KERN_ALERT "stopping sysprof module\n");

sysprof.c

@@ -98,13 +98,6 @@ struct Application
 					    */
 };
 
-static void
-disaster (const char *what)
-{
-    fprintf (stderr, what);
-    exit (1);
-}
-
 static gboolean
 show_samples_timeout (gpointer data)
 {
@@ -332,8 +325,10 @@ on_read (gpointer data)
 /* 	    filename = trace.filename; */
 	
 	for (i = 0; i < trace.n_addresses; ++i)
+	{
 	    process_ensure_map (process, trace.pid, 
 				(gulong)trace.addresses[i]);
+	}
 	g_assert (!app->generating_profile);
 	
 	stack_stash_add_trace (
@@ -398,6 +393,31 @@ start_profiling (gpointer data)
     return FALSE;
 }
 
+static void
+sorry (GtkWidget *parent_window,
+       const gchar *format,
+       ...)
+{
+    va_list args;
+    char *message;
+    GtkWidget *dialog;
+    
+    va_start (args, format);
+    g_vasprintf (&message, format, args);
+    va_end (args);
+    
+    dialog = gtk_message_dialog_new (parent_window ? GTK_WINDOW (parent_window) : NULL,
+				     GTK_DIALOG_DESTROY_WITH_PARENT,
+				     GTK_MESSAGE_WARNING,
+				     GTK_BUTTONS_OK, message);
+    g_free (message);
+    
+    gtk_window_set_title (GTK_WINDOW (dialog), APPLICATION_NAME " Warning");
+    
+    gtk_dialog_run (GTK_DIALOG (dialog));
+    gtk_widget_destroy (dialog);
+}
+
 static void
 on_start_toggled (GtkWidget *widget, gpointer data)
 {
@@ -406,6 +426,29 @@ on_start_toggled (GtkWidget *widget, gpointer data)
     if (!gtk_toggle_tool_button_get_active (
 	    GTK_TOGGLE_TOOL_BUTTON (app->start_button)))
 	return;
+
+    if (app->input_fd == -1)
+    {
+	int fd = open ("/proc/sysprof-trace", O_RDONLY);
+	if (fd < 0)
+	{
+	    sorry (app->main_window,
+		   "Can't open /proc/sysprof-trace. You need to insert\n"
+		   "the sysprof kernel module. Type \n"
+		   "\n"
+		   "       insmod sysprof-module.ko\n"
+		   "\n"
+		   "as root.");
+
+	    update_sensitivity (app);
+	    return;
+	}
+
+	app->input_fd = fd;
+	fd_add_watch (app->input_fd, app);
+    }
+    
+    fd_set_read_callback (app->input_fd, on_read);
     
     delete_data (app);
     
@@ -688,9 +731,8 @@ ensure_profile (Application *app)
     if (app->profile)
 	return;
     
-    /* take care of reentrancy */
     app->profile = profile_new (app->stash);
-    
+
     fill_lists (app);
     
     app->state = DISPLAYING;
@@ -734,31 +776,6 @@ on_profile_toggled (GtkWidget *widget, gpointer data)
     }
 }
 
-static void
-sorry (GtkWidget *parent_window,
-       const gchar *format,
-       ...)
-{
-    va_list args;
-    char *message;
-    GtkWidget *dialog;
-    
-    va_start (args, format);
-    g_vasprintf (&message, format, args);
-    va_end (args);
-    
-    dialog = gtk_message_dialog_new (parent_window ? GTK_WINDOW (parent_window) : NULL,
-				     GTK_DIALOG_DESTROY_WITH_PARENT,
-				     GTK_MESSAGE_WARNING,
-				     GTK_BUTTONS_OK, message);
-    free (message);
-    
-    gtk_window_set_title (GTK_WINDOW (dialog), APPLICATION_NAME " Warning");
-    
-    gtk_dialog_run (GTK_DIALOG (dialog));
-    gtk_widget_destroy (dialog);
-}
-
 static void
 on_reset_clicked (gpointer widget, gpointer data)
 {
@@ -1242,21 +1259,6 @@ main (int argc, char **argv)
     
     app = application_new ();
     
-    app->input_fd = open ("/proc/sysprof-trace", O_RDONLY);
-    if (app->input_fd < 0)
-    {
-	disaster ("Can't open /proc/sysprof-trace. You need to insert\n"
-		  "the sysprof kernel module. Type \n"
-		  "\n"
-		  "       insmod sysprof-module.ko\n"
-		  "\n"
-		  "as root.\n"
-		  "\n");
-    }
-    
-    fd_add_watch (app->input_fd, app);
-    fd_set_read_callback (app->input_fd, on_read);
-    
 #if 0
     nice (-19);
     g_timeout_add (10, on_timeout, app);