memprof + threads fix ...

View: New views
4 Messages — Rating Filter:   Alert me  

memprof + threads fix ...

by michael meeks :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

So,

        A bit more poking - and it turns out all the memprof thread stuff
assumes that 'getpid' returns something different for each thread ;-) of
course, with NPTL [ ie. any recent system ] that just isn't so.

        Using a __thread variable instead makes it really solid.

        I attach a new patch; Owen - who maintains this now ? - and/or can one
just commit such things that fix clear brokenness without overmuch
review ? :-)

        Thanks,

                Michael.

--
 michael.meeks@...  <><, Pseudo Engineer, itinerant idiot

Index: ChangeLog
===================================================================
RCS file: /cvs/gnome/memprof/ChangeLog,v
retrieving revision 1.129
diff -u -r1.129 ChangeLog
--- ChangeLog 1 Apr 2005 23:09:59 -0000 1.129
+++ ChangeLog 14 Jun 2005 10:50:00 -0000
@@ -1,3 +1,23 @@
+2005-06-14  Michael Meeks  <michael.meeks@...>
+
+ * intercept.c (allocate_thread, find_thread):
+ update signatures and re-work for NPTL which gives
+ the same getpid() for each process: fixes all the
+ threading problems.
+
+ * process.c (input_func): reset the info structure
+ between messages; defensively.
+
+2005-06-13  Michael Meeks  <michael.meeks@...>
+
+ * server.c (ensure_cleanup, mp_server_init),
+ * process.c (process_start_input): use a fuller set
+ of G_IO_ flags for various conditions.
+
+ * intercept.c (exit_wait, mi_write_stack): use atomic
+ increment for seqno - if we get out of step process.c
+ bombs with an ever-growing sorted GList.
+
 2005-04-01  Steve Murphy  <murf@...>
 
         * configure.in: Added "rw" to ALL_LINGUAS.
Index: intercept.c
===================================================================
RCS file: /cvs/gnome/memprof/intercept.c,v
retrieving revision 1.5
diff -u -r1.5 intercept.c
--- intercept.c 3 Sep 2004 22:29:47 -0000 1.5
+++ intercept.c 14 Jun 2005 10:50:01 -0000
@@ -46,7 +46,9 @@
  */
 
 typedef struct {
+#ifdef NPTL
  uint32_t ref_count;
+#endif
  pid_t pid;
  int outfd;
  pid_t clone_pid; /* See comments in clone_thunk */
@@ -72,10 +74,10 @@
 
 static ThreadInfo threads[MAX_THREADS];
 static char *socket_path = NULL;
 static char socket_buf[64];
-static unsigned int seqno = 0;
+static uint32_t seqno = 0;
 
 #undef ENABLE_DEBUG
 
 #ifdef ENABLE_DEBUG
 #define MI_DEBUG(arg) mi_debug arg
@@ -87,72 +89,85 @@
 #define MI_DEBUG(arg) (void)0
 #endif /* ENABLE_DEBUG */
 
+/*
+ * For the new NPTL implementation, all threads return the
+ * same 'getpid' - so instead we use a thread variable.
+ */
+#define NPTL
+
+#ifdef NPTL
+static __thread ThreadInfo global_per_thread = { 0, };
+#endif
+
 static ThreadInfo *
-allocate_thread (pid_t pid)
+allocate_thread (void)
 {
+ ThreadInfo *thread = NULL;
+#ifdef NPTL
+ thread = &global_per_thread;
+#else
  int i;
 
  for (i = 0; i < MAX_THREADS; i++) {
  if (threads[i].ref_count == 0) {
  unsigned int new_count = mi_atomic_increment (&threads[i].ref_count);
  if (new_count == 1) {
- threads[i].pid = pid;
- threads[i].clone_pid = 0;
- return &threads[i];
+ thread = &threads[i];
+ break;
  } else
  mi_atomic_decrement (&threads[i].ref_count);
  }
  }
-
- mi_debug ("Can't find free thread slot");
- tracing = MI_FALSE;
- _exit(1);
+ if (!thread) {
+ mi_debug ("Can't find free thread slot");
+ tracing = MI_FALSE;
+ _exit(1);
+ }
+#endif
+ thread->pid = getpid();
+ thread->clone_pid = 0;
 
- return NULL;
+ return thread;
 }
 
 static void
 release_thread (ThreadInfo *thread)
 {
  thread->pid = 0;
+#ifndef NPTL
  mi_atomic_decrement (&thread->ref_count);
+#endif
 }
 
 static ThreadInfo *
-find_thread (pid_t pid)
+find_thread (void)
 {
- int i;
-#if 1
  ThreadInfo *thread;
+#ifdef NPTL
+ thread = &global_per_thread;
 #else
- /* Over-optimized GCC/GLibc extension happy version
- * Requires gcc-3.2 and glibc-2.3.
- */
- static __thread ThreadInfo *thread = NULL;
  int i;
-
-a if (__builtin_expect (thread == NULL, 0))
- return thread;
-#endif
+ pid_t pid = getpid();
 
  for (i=0; i < MAX_THREADS; i++)
  if (threads[i].pid == pid) {
  thread = &threads[i];
-
- if (thread->clone_pid) {
- /* See comments in clone_thunk() */
- new_process (thread, thread->clone_pid, MI_CLONE);
- thread->clone_pid = 0;
- }
-
- return thread;
+ break;
  }
+#endif
+ if (!thread) {
+ mi_debug ("Thread not found\n");
+ tracing = MI_FALSE;
+ _exit(1);
+ }
 
- mi_debug ("Thread not found\n");
- tracing = MI_FALSE;
- _exit(1);
-
- return NULL;
+ if (thread->clone_pid) {
+ /* See comments in clone_thunk() */
+ new_process (thread, thread->clone_pid, MI_CLONE);
+ thread->clone_pid = 0;
+ }
+
+ return thread;
 }
 
 static void
@@ -249,7 +264,7 @@
  info.fork.seqno = 0;
 
  if (!thread)
- thread = allocate_thread (info.fork.new_pid);
+ thread = allocate_thread ();
 
  thread->outfd = outfd;
 
@@ -331,9 +346,9 @@
 
  info->alloc.stack_size = n_frames;
  info->alloc.pid = getpid();
- info->alloc.seqno = seqno++;
+ info->alloc.seqno = mi_atomic_increment (&seqno) - 1;
 
- thread = find_thread (info->alloc.pid);
+ thread = find_thread ();
 
  if (!mi_write (thread->outfd, info, sizeof (MIInfo)) ||
     !mi_write (thread->outfd, frames, n_frames * sizeof(void *)))
@@ -352,7 +367,7 @@
  int pid;
  int old_pid = getpid();
 
- find_thread (old_pid); /* Make sure we're registered */
+ find_thread (); /* Make sure we're registered */
 
  pid = (*old_fork) ();
 
@@ -380,7 +395,7 @@
  int pid;
  int old_pid = getpid();
 
- find_thread (old_pid); /* Make sure we're registered */
+ find_thread (); /* Make sure we're registered */
 
  pid = (*old_vfork) ();
 
@@ -434,7 +449,7 @@
  * So, we simply allocate the structure for the thread and delay
  * the initialization.
  */
- thread = allocate_thread (getpid());
+ thread = allocate_thread ();
  thread->clone_pid = data->pid;
  data->started = 1;
 
@@ -452,14 +467,14 @@
 
  if (!mi_check_init ())
  abort_unitialized ("__clone");
-
+
  if (tracing) {
  data.started = 0;
  data.fn = fn;
  data.arg = arg;
  data.pid = getpid();
 
- find_thread (data.pid); /* Make sure we're registered */
+ find_thread (); /* Make sure we're registered */
 
  pid = (*old_clone) (clone_thunk, child_stack, flags, (void *)&data, xarg1, xarg2, xarg3, xarg4);
 
@@ -490,12 +505,12 @@
  int count;
  char response;
  info.any.operation = MI_EXIT;
- info.any.seqno = seqno++;
+ info.any.seqno = mi_atomic_increment (&seqno) - 1;
  info.any.pid = getpid();
 
  mi_stop ();
 
- thread = find_thread (info.any.pid);
+ thread = find_thread ();
 
  if (mi_write (thread->outfd, &info, sizeof (MIInfo)))
  /* Wait for a response before really exiting
Index: process.c
===================================================================
RCS file: /cvs/gnome/memprof/process.c,v
retrieving revision 1.37
diff -u -r1.37 process.c
--- process.c 13 Sep 2004 18:43:52 -0000 1.37
+++ process.c 14 Jun 2005 10:50:02 -0000
@@ -600,6 +600,8 @@
  MPProcess *process = NULL;
   
  do {
+ memset (&info, 0, sizeof (info));
+ info.operation = -1;
  count = 0;
  if (g_io_channel_get_flags(source) & G_IO_FLAG_IS_READABLE)
  g_io_channel_read_chars (source, (char *)&info, sizeof(info), &count, NULL);
@@ -621,7 +623,9 @@
     info.operation == MI_TIME) {
  void **stack_buffer = NULL;
  StackStash *stash = get_stack_stash (input_process);
-
+
+ g_assert (count >= sizeof (MIInfoAlloc));
+
  stack_buffer = g_alloca (sizeof (void *) * info.alloc.stack_size);
  g_io_channel_read_chars (source, (char *)stack_buffer, sizeof(void *) * info.alloc.stack_size, &count, NULL);
  stack = stack_stash_store (stash, stack_buffer, info.alloc.stack_size);
@@ -667,7 +672,7 @@
  if (!process->input_tag && process->input_channel)
  process->input_tag = g_io_add_watch_full (process->input_channel,
   G_PRIORITY_LOW,
-  G_IO_IN | G_IO_HUP,
+  G_IO_IN | G_IO_PRI | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
   input_func, process, NULL);
 }
 
Index: server.c
===================================================================
RCS file: /cvs/gnome/memprof/server.c,v
retrieving revision 1.14
diff -u -r1.14 server.c
--- server.c 20 Sep 2003 21:02:18 -0000 1.14
+++ server.c 14 Jun 2005 10:50:03 -0000
@@ -164,7 +164,8 @@
  channel = g_io_channel_unix_new (server->socket_fd);
  g_io_channel_set_encoding (channel, NULL, NULL);
 
- server->control_watch = g_io_add_watch (channel, G_IO_IN | G_IO_HUP, control_func, server);
+ server->control_watch = g_io_add_watch (channel, G_IO_IN | G_IO_PRI | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
+ control_func, server);
  g_io_channel_unref (channel);
 
  g_object_ref (G_OBJECT (server));
@@ -366,7 +367,7 @@
  channel = g_io_channel_unix_new (terminate_pipe[0]);
  g_io_channel_set_encoding (channel, NULL, NULL);
 
- g_io_add_watch (channel, G_IO_IN, terminate_io_handler, NULL);
+ g_io_add_watch (channel, G_IO_IN | G_IO_PRI, terminate_io_handler, NULL);
  g_io_channel_unref (channel);
 
  added_cleanup = TRUE;

_______________________________________________
memprof-list mailing list
memprof-list@...
http://mail.gnome.org/mailman/listinfo/memprof-list

Re: memprof + threads fix ...

by Owen Taylor :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Tue, 2005-06-14 at 11:47 +0100, michael meeks wrote:

> So,
>
> A bit more poking - and it turns out all the memprof thread stuff
> assumes that 'getpid' returns something different for each thread ;-) of
> course, with NPTL [ ie. any recent system ] that just isn't so.
>
> Using a __thread variable instead makes it really solid.
>
> I attach a new patch; Owen - who maintains this now ? - and/or can one
> just commit such things that fix clear brokenness without overmuch
> review ? :-)
I'm certainly not maintaining memprof at the moment. If John Myers has a
chance to review the patch, that would be great.

Otherwise, you can just go ahead and commit. Memprof hasn't been working
in recent quick tests I've done even for single threaded apps, but I
haven't had time to try and figure out why. I think it's libc changes,
as usual.

Using thread primitives in memprof is something that I've always been
very wary of, because memprof is hooking in at the clone level *before*
the new thread is fully established. But if the use of a per-thread
variable works, I guess it works.

Regards,
                                                Owen

(I don't think adding adding random conditions to g_io_channel_add_watch
is correct, from a glance at the patch. There's no way that a channel
will suddenly start reporting G_IO_PRI unless you've coded that into
the structure of your app. No way to get G_IO_NVAL unless you have
a bug in your app. Etc.)


_______________________________________________
memprof-list mailing list
memprof-list@...
http://mail.gnome.org/mailman/listinfo/memprof-list

signature.asc (196 bytes) Download Attachment

Re: memprof + threads fix ...

by michael meeks :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message


On Tue, 2005-06-14 at 11:33 -0400, Owen Taylor wrote:
> Otherwise, you can just go ahead and commit. Memprof hasn't been working
> in recent quick tests I've done even for single threaded apps, but I
> haven't had time to try and figure out why. I think it's libc changes,
> as usual.

        Interesting.

> Using thread primitives in memprof is something that I've always been
> very wary of, because memprof is hooking in at the clone level *before*
> the new thread is fully established. But if the use of a per-thread
> variable works, I guess it works.

        So - just out of interest; why don't we hook the callback passed to
clone and then do our setup inside the new thread before handing control
to the child function ?

> (I don't think adding adding random conditions to g_io_channel_add_watch
> is correct, from a glance at the patch. There's no way that a channel
> will suddenly start reporting G_IO_PRI unless you've coded that into
> the structure of your app. No way to get G_IO_NVAL unless you have
> a bug in your app. Etc.)

        Ah ;-) and I like the random conditions :-)

        Thanks,

                Michael.

--
 michael.meeks@...  <><, Pseudo Engineer, itinerant idiot

_______________________________________________
memprof-list mailing list
memprof-list@...
http://mail.gnome.org/mailman/listinfo/memprof-list

Re: memprof + threads fix ...

by Owen Taylor :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Thu, 2005-06-23 at 10:43 +0100, michael meeks wrote:

> On Tue, 2005-06-14 at 11:33 -0400, Owen Taylor wrote:
> > Otherwise, you can just go ahead and commit. Memprof hasn't been working
> > in recent quick tests I've done even for single threaded apps, but I
> > haven't had time to try and figure out why. I think it's libc changes,
> > as usual.
>
> Interesting.
>
> > Using thread primitives in memprof is something that I've always been
> > very wary of, because memprof is hooking in at the clone level *before*
> > the new thread is fully established. But if the use of a per-thread
> > variable works, I guess it works.
>
> So - just out of interest; why don't we hook the callback passed to
> clone and then do our setup inside the new thread before handing control
> to the child function ?
The point is that we aren't hooking pthread_create(), we are hooking
clone(), and libc itself will do additional setup inside the child
function of clone() before calling the user's function to start the
operation of the thread.

Regards,
                                        Owen



_______________________________________________
memprof-list mailing list
memprof-list@...
http://mail.gnome.org/mailman/listinfo/memprof-list

signature.asc (196 bytes) Download Attachment