GNOME.org
 

[sysprof/ftrace: 7/16] A number of cleanups.

commit 440d2cef87cde5733129dbb9073dc3fe6a6722e1
Author: Søren Sandmann Pedersen <sandmann daimi au dk>
Date:   Fri Aug 14 01:31:55 2009 -0400

    A number of cleanups.

 TODO                    |    2 +
 collector.c             |  246 ++++++++++++-----------------------------------
 module/sysprof-module.h |   16 ---
 sysprof-text.c          |    1 -
 sysprof.c               |    1 -
 5 files changed, 63 insertions(+), 203 deletions(-)
---
diff --git a/TODO b/TODO
index 67fcb33..f0462ce 100644
--- a/TODO
+++ b/TODO
@@ -26,6 +26,8 @@ Before 1.2:
 * Find out why gtk_tree_view_columns_autosize() apparently doesn't
   work on empty tree views.
 
+* Get rid of O(n^2) string handling in on_read()
+
 * Hack to disable recursion for binaries without symbols causes the
   symbols to not work the way other symbols do.  A better approach is
   probably to simply generate a new symbol for every appearance except
diff --git a/collector.c b/collector.c
index af9bec2..79d470d 100644
--- a/collector.c
+++ b/collector.c
@@ -19,7 +19,6 @@
 
 #include "stackstash.h"
 #include "collector.h"
-#include "module/sysprof-module.h"
 #include "watch.h"
 #include "process.h"
 #include "elfparser.h"
@@ -37,10 +36,34 @@
 typedef struct CPUInfo CPUInfo;
 
 #define N_CPU 8		/* FIXME: Arbitrary number of CPU's */
-		    
-static void set_no_module_error (GError **err);
-static void set_cant_open_error (GError **err, int eno);
 
+#define SYSPROF_FILE "/sys/kernel/debug/tracing/trace_pipe"
+
+enum
+{
+    BEGIN_TRACE		= 0,
+    KERNEL_ADDRESS	= 1,
+    USER_ADDRESS	= 2,
+    END_TRACE		= 3
+};
+
+typedef struct SysprofStackTrace SysprofStackTrace;
+
+#define SYSPROF_N_TRACES 64
+#define SYSPROF_MAX_ADDRESSES 126
+
+struct SysprofStackTrace
+{
+    void *kernel_stack[SYSPROF_MAX_ADDRESSES];
+    void *addresses[SYSPROF_MAX_ADDRESSES];
+    int	n_kernel_words;
+    int n_addresses;      /* note: this can be 1 if the process was compiled
+			   * with -fomit-frame-pointer or is otherwise weird
+			   */
+    int	pid;		/* -1 if in kernel */
+    int truncated;
+};
+		    
 struct CPUInfo
 {
     int			user_pos;
@@ -164,21 +187,9 @@ add_trace_to_stash (const SysprofStackTrace *trace,
 	}
     }
 
-#if 0
-    0xc04068d7
-#endif
-
     /* Add process */
     addrs[a++] = (gulong)process;
 
-#if 0
-    if (a != n_addresses)
- 	g_print ("a: %d, n_addresses: %d, kernel words: %d\n trace->nad %d",
-		 a, n_addresses, trace->n_kernel_words, trace->n_addresses);
-    
-    g_assert (a == n_addresses);
-#endif
-    
     stack_stash_add_trace (stash, addrs, a, 1);
     
     if (addrs != addrs_stack)
@@ -201,91 +212,12 @@ in_dead_period (Collector *collector)
     return FALSE;
 }
 
-#if 0
-static void
-collect_traces (Collector *collector)
-{
-    gboolean first;
-    
-    /* After a reset we ignore samples for a short period so that
-     * a reset will actually cause 'samples' to become 0
-     */
-    if (in_dead_period (collector))
-    {
-	collector->current = collector->map_area->head;
-	return;
-    }
-
-    first = collector->n_samples == 0;
-    
-    while (collector->current != collector->map_area->head)
-    {
-	const SysprofStackTrace *trace;
-
-	trace = &(collector->map_area->traces[collector->current]);
-
-#if 0
-	{
-	    int i;
-	    g_print ("pid: %d (%d)\n", trace->pid, trace->n_addresses);
-	    for (i=0; i < trace->n_addresses; ++i)
-		g_print ("rd: %08x\n", trace->addresses[i]);
-	    g_print ("-=-\n");
-	}
-#endif
-#if 0
-	{
-	    int i;
-	    g_print ("pid: %d (%d)\n", trace->pid, trace->n_addresses);
-	    for (i=0; i < trace->n_kernel_words; ++i)
-		g_print ("rd: %08x\n", trace->kernel_stack[i]);
-	    g_print ("-=-\n");
-	}
-#endif
-	
-	add_trace_to_stash (trace, collector->stash);
-	
-	collector->current++;
-	if (collector->current >= SYSPROF_N_TRACES)
-	    collector->current = 0;
-	
-	collector->n_samples++;
-    }
-	
-    if (collector->callback)
-	collector->callback (first, collector->data);
-}
-#endif
-
-static gboolean
-load_module (void)
-{
-    int exit_status = -1;
-    char *dummy1, *dummy2;
-    
-    if (g_spawn_command_line_sync ("/sbin/modprobe sysprof-module",
-				   &dummy1, &dummy2,
-				   &exit_status,
-				   NULL))
-    {
-	if (WIFEXITED (exit_status))
-	    exit_status = WEXITSTATUS (exit_status);
-	
-	g_free (dummy1);
-	g_free (dummy2);
-    }
-    
-    return (exit_status == 0);
-}
-
 static void
 warn (const char *text)
 {
     g_warning ("%s: (%s)\n", text, strerror (errno));
 }
 
-#define SYSPROF_FILE "/sys/kernel/debug/trace_pipe"
-
 static gboolean
 write_text (const char *file, const char *text)
 {
@@ -319,28 +251,6 @@ collector_set_tracing (Collector *collector,
 	write_text ("/sys/kernel/debug/tracing/tracing_enabled", "0\n");
 }
 
-/* Format of a trace entry:
- *
- *
- *  pid			4 bytes
- *  cpu			1 byte
- *  t			8 bytes
- *  arg1 (type)		4 bytes on 32 bit, 8 bytes on 64
- *  arg2 (address)	4 bytes on 32 bit, 8 bytes on 64
- *  arg3 (location)	4 bytes on 32 bit, 8 bytes on 64
- *
- *  TOTAL: 25 bytes on x86
- */
-#define ENTRY_SIZE  (							\
-	sizeof (guint32) +  /* pid */					\
-	sizeof (guint32) +   /* cpu */					\
-	sizeof (guint64) +  /* timestamp */				\
-	sizeof (gulong) +   /* type */					\
-	sizeof (gulong) +   /* address */				\
-	sizeof (gulong)     /* location */)
-
-#define N_ENTRIES 1024
-
 typedef struct Entry
 {
     guint32 pid;
@@ -351,21 +261,23 @@ typedef struct Entry
     gulong location;
 } Entry;
 
-static int
-read4 (const guchar *line)
-{
-    return (line[0] << 0) | (line[1] << 8) | (line[2] << 16) | (line[3] << 24);
-}
-
-static guint64
-read8 (const guchar *line)
+static void
+dump_entry (Entry *entry)
 {
-    guint64 rl, rh;
-
-    rl = read4 (line);
-    rh = read4 (line);
-
-    return (rh << 32) | rl;
+    int i;
+	
+    g_printerr ("Entry:\n");
+    g_printerr ("- pid: %d (%x)\n", entry->pid, entry->pid);
+    g_printerr ("- cpu: %d\n", entry->cpu);
+    g_printerr ("- timestamp: %lld\n", entry->timestamp);
+    g_printerr ("- type: %ld\n", entry->type);
+    g_printerr ("- address: %ld\n", entry->address);
+    g_printerr ("- location: %ld\n", entry->location);
+
+    g_printerr ("\n");
+    for (i = 0; i < sizeof (Entry); ++i)
+	g_printerr ("%2x ", ((unsigned char *)entry)[i]);
+    g_printerr ("\n");
 }
 
 static void
@@ -375,22 +287,10 @@ read_entry (const guchar *input, Entry *entry)
     
     if (entry->type < -1 || entry->type > 3 || entry->pid < 0)
     {
-	int i;
-	
-	g_printerr ("bad entry: %ld\n", entry->type);
-	g_printerr ("Entry:\n");
-	g_printerr ("- pid: %d (%x)\n", entry->pid, entry->pid);
-	g_printerr ("- cpu: %d\n", entry->cpu);
-	g_printerr ("- timestamp: %lld\n", entry->timestamp);
-	g_printerr ("- type: %ld\n", entry->type);
-	g_printerr ("- address: %ld\n", entry->address);
-	g_printerr ("- location: %ld\n", entry->location);
-
-	g_printerr ("%p: \n", input);
-	for (i = 0; i < ENTRY_SIZE; ++i)
-	    g_printerr ("%2x ", (unsigned char)input[i]);
-	g_printerr ("\n");
+	g_printerr ("bad entry: %ld (pid: %d)\n", entry->type, entry->pid);
 
+	dump_entry (entry);
+	
 	g_assert (FALSE);
     }
 }
@@ -423,7 +323,7 @@ process_entry (Collector *collector, Entry *entry)
      */
     switch (entry->type)
     {
-    case 0: /* Beginning of stack trace */
+    case BEGIN_TRACE:
 	if ((info = find_cpu_info (collector, -1)))
 	{
 	    info->trace.pid = entry->pid;
@@ -433,7 +333,7 @@ process_entry (Collector *collector, Entry *entry)
 	}
 	break;
 	
-    case 1: /* Kernel address */
+    case KERNEL_ADDRESS:
 	if ((info = find_cpu_info (collector, entry->pid)))
 	{
 	    if (info->kernel_pos < SYSPROF_MAX_ADDRESSES)
@@ -443,7 +343,7 @@ process_entry (Collector *collector, Entry *entry)
 	}
 	break;
 	
-    case 2: /* User address */
+    case USER_ADDRESS:
 	if ((info = find_cpu_info (collector, entry->pid)))
 	{
 	    if (info->user_pos < SYSPROF_MAX_ADDRESSES)
@@ -453,7 +353,7 @@ process_entry (Collector *collector, Entry *entry)
 	}
 	break;
 	
-    case 3: /* End of stack trace */
+    case END_TRACE:
 	if ((info = find_cpu_info (collector, entry->pid)))
 	{
 	    info->trace.n_kernel_words = info->kernel_pos;
@@ -472,12 +372,13 @@ process_entry (Collector *collector, Entry *entry)
     }
 }
 
-/* */
+#define N_ENTRIES 128
+
 static void
 on_read (gpointer data)
 {
     Collector *collector = data;
-    char input [N_ENTRIES * ENTRY_SIZE];
+    char input [N_ENTRIES * sizeof (Entry)];
     int n_bytes;
 
     while ((n_bytes = read (collector->fd, input, sizeof (input))) > 0)
@@ -496,15 +397,15 @@ on_read (gpointer data)
 
 	    len = collector->input->len;
 	    s = (const guchar *)collector->input->str;
-	    while (len >= ENTRY_SIZE)
+	    while (len >= sizeof (Entry))
 	    {
 		Entry entry;
 
 		read_entry (s, &entry);
 		process_entry (collector, &entry);
 
-		len -= ENTRY_SIZE;
-		s += ENTRY_SIZE;
+		len -= sizeof (Entry);
+		s += sizeof (Entry);
 	    }
 	    g_string_erase (collector->input, 0, collector->input->len - len);
 	    
@@ -518,8 +419,8 @@ on_read (gpointer data)
 }
 
 static gboolean
-start_tracing (Collector *collector,
-	       GError **err)
+start_tracing (Collector  *collector,
+	       GError    **err)
 {
     int fd;
     int i;
@@ -563,15 +464,13 @@ start_tracing (Collector *collector,
 	return FALSE;
     }
 
-#if 0
     if (!write_text ("/sys/kernel/debug/tracing/sysprof_sample_period", "10000\n"))
     {
 	g_print ("Failed to set sample period\n");
 	return FALSE;
     }
-#endif
 
-    fd = open ("/sys/kernel/debug/tracing/trace_pipe", O_RDONLY | O_NONBLOCK);
+    fd = open (SYSPROF_FILE, O_RDONLY | O_NONBLOCK);
     if (fd < 0)
     {
 	g_print ("Failed to open trace pipe\n");
@@ -586,7 +485,9 @@ start_tracing (Collector *collector,
 	return FALSE;
     }
     else
+    {
 	g_print ("enabled\n");
+    }
 
     fd_add_watch (fd, collector);
     
@@ -827,31 +728,6 @@ collector_create_profile (Collector *collector)
     return profile;
 }
 
-static void
-set_no_module_error (GError **err)
-{
-    g_set_error (err,
-		 COLLECTOR_ERROR,
-		 COLLECTOR_ERROR_CANT_OPEN_FILE,
-		 "Can't open " SYSPROF_FILE ". You need to insert "
-		 "the sysprof kernel module. Run\n"
-		 "\n"
-		 "       modprobe sysprof-module\n"
-		 "\n"
-		 "as root");
-}
-
-static void
-set_cant_open_error (GError **err,
-		     int      eno)
-{
-    g_set_error (err,
-		 COLLECTOR_ERROR,
-		 COLLECTOR_ERROR_CANT_OPEN_FILE,
-		 "Can't open " SYSPROF_FILE ": %s",
-		 g_strerror (eno));
-}
-
 GQuark
 collector_error_quark (void)
 {
diff --git a/module/sysprof-module.h b/module/sysprof-module.h
index 40bbade..6a1286f 100644
--- a/module/sysprof-module.h
+++ b/module/sysprof-module.h
@@ -20,25 +20,9 @@
 #ifndef SYSPROF_MODULE_H
 #define SYSPROF_MODULE_H
 
-typedef struct SysprofStackTrace SysprofStackTrace;
 typedef struct SysprofStackInfo SysprofStackInfo;
 typedef struct SysprofMmapArea SysprofMmapArea;
 
-#define SYSPROF_N_TRACES 64
-#define SYSPROF_MAX_ADDRESSES 126
-
-struct SysprofStackTrace
-{
-    void *kernel_stack[SYSPROF_MAX_ADDRESSES];
-    void *addresses[SYSPROF_MAX_ADDRESSES];
-    int	n_kernel_words;
-    int n_addresses;      /* note: this can be 1 if the process was compiled
-			   * with -fomit-frame-pointer or is otherwise weird
-			   */
-    int	pid;		/* -1 if in kernel */
-    int truncated;
-};
-
 struct SysprofMmapArea
 {
     SysprofStackTrace	traces[SYSPROF_N_TRACES];
diff --git a/sysprof-text.c b/sysprof-text.c
index 19be1f7..a295107 100644
--- a/sysprof-text.c
+++ b/sysprof-text.c
@@ -28,7 +28,6 @@
 #include <stdio.h>
 
 #include "stackstash.h"
-#include "module/sysprof-module.h"
 #include "profile.h"
 #include "process.h"
 #include "watch.h"
diff --git a/sysprof.c b/sysprof.c
index 6778d23..9271d4d 100644
--- a/sysprof.c
+++ b/sysprof.c
@@ -28,7 +28,6 @@
 #include "treeviewutils.h"
 #include "profile.h"
 #include "collector.h"
-#include "module/sysprof-module.h"
 
 /* FIXME - not10 */
 #define _(a) a
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]