|
View:
New views
4 Messages
—
Rating Filter:
Alert me
|
|
|
memprof + threads fix ...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 ...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 ? :-) 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 |
|
|
Re: memprof + threads fix ...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 ...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 ? 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 |
| Free embeddable forum powered by Nabble | Forum Help |