rthreads: plugging mem leaks

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

rthreads: plugging mem leaks

Otto Moerbeek
Hi,

One of the large missing pieces of the rthreads puzzle is cleaning up
after a thread exits.

This is all more complicated that you'd think since a thread may be
referenced by pthread_join() or pthread_detach() _after_ it has exited.

Cleaning up stacks is even more nasty: the stack is needed to call
threxit(). Cleaning up the stack cannot be done in the regular cleanup
code, sice the we do not know if the threxit() has actually been
completed.

Anyway, here's a diff to fix all this. It's nice to see the
pthread_specific regress test first eat a lot of memory, and then
giving it back piece by piece.

Thanks to Marco Hyman for providing some valuable hints.

        -Otto


Index: Makefile
===================================================================
RCS file: /cvs/src/lib/librthread/Makefile,v
retrieving revision 1.7
diff -u -p -r1.7 Makefile
--- Makefile 1 Jan 2006 19:32:30 -0000 1.7
+++ Makefile 3 Jan 2006 09:11:59 -0000
@@ -7,7 +7,8 @@ CFLAGS+=-Wall -g -Werror -Wshadow
 
 .PATH: ${.CURDIR}/arch/${MACHINE_ARCH}
 SRCS= rthread.c rthread_attr.c rthread_sched.c rthread_sync.c rthread_tls.c \
- rthread_sig.c rthread_np.c rthread_debug.c rthread_stack.c
+ rthread_sig.c rthread_np.c rthread_debug.c rthread_stack.c \
+ rthread_reaper.c
 
 OBJS+= _atomic_lock.o rfork_thread.o
 
Index: rthread.c
===================================================================
RCS file: /cvs/src/lib/librthread/rthread.c,v
retrieving revision 1.26
diff -u -p -r1.26 rthread.c
--- rthread.c 1 Jan 2006 19:32:30 -0000 1.26
+++ rthread.c 3 Jan 2006 09:11:59 -0000
@@ -21,6 +21,7 @@
  */
 
 #include <sys/param.h>
+#include <sys/event.h>
 #include <sys/mman.h>
 #include <sys/wait.h>
 
@@ -45,6 +46,8 @@ struct listhead _thread_list = LIST_HEAD
 _spinlock_lock_t _thread_lock = _SPINLOCK_UNLOCKED;
 struct pthread _initial_thread;
 
+static pthread_t reaper_thread;
+
 int rfork_thread(int, void *, void (*)(void *), void *);
 
 /*
@@ -103,14 +106,43 @@ _rthread_init(void)
  thread->tid = getthrid();
  thread->donesem.lock = _SPINLOCK_UNLOCKED;
  thread->flags |= THREAD_CANCEL_ENABLE|THREAD_CANCEL_DEFERRED;
+ thread->flags_lock = _SPINLOCK_UNLOCKED;
  strlcpy(thread->name, "Main process", sizeof(thread->name));
  LIST_INSERT_HEAD(&_thread_list, thread, threads);
+ _rthread_kq = kqueue();
+ if (_rthread_kq == -1)
+ return (errno);
  _threads_ready = 1;
  __isthreaded = 1;
 
  return (0);
 }
 
+static void
+_rthread_free(pthread_t thread)
+{
+ /* catch wrongdoers for the moment */
+ memset(thread, 0xd0, sizeof(*thread));
+ if (thread != &_initial_thread)
+ free(thread);
+}
+
+static void
+_rthread_setflag(pthread_t thread, int flag)
+{
+ _spinlock(&thread->flags_lock);
+ thread->flags |= flag;
+ _spinunlock(&thread->flags_lock);
+}
+
+static void
+_rthread_clearflag(pthread_t thread, int flag)
+{
+ _spinlock(&thread->flags_lock);
+ thread->flags &= ~flag;
+ _spinunlock(&thread->flags_lock);
+}
+
 /*
  * real pthread functions
  */
@@ -134,12 +166,11 @@ void
 pthread_exit(void *retval)
 {
  struct rthread_cleanup_fn *clfn;
+ struct stack *stack;
  pthread_t thread = pthread_self();
 
  thread->retval = retval;
- thread->flags |= THREAD_DONE;
 
- _sem_post(&thread->donesem);
  for (clfn = thread->cleanup_fns; clfn; ) {
  struct rthread_cleanup_fn *oclfn = clfn;
  clfn = clfn->next;
@@ -150,10 +181,19 @@ pthread_exit(void *retval)
  _spinlock(&_thread_lock);
  LIST_REMOVE(thread, threads);
  _spinunlock(&_thread_lock);
-#if 0
+
+ _sem_post(&thread->donesem);
+
+ stack = thread->stack;
  if (thread->flags & THREAD_DETACHED)
- free(thread);
-#endif
+ _rthread_free(thread);
+ else
+ _rthread_setflag(thread, THREAD_DONE);
+
+ if (thread->tid != _initial_thread.tid &&
+    thread->tid != reaper_thread->tid)
+ _rthread_add_to_reaper(thread->tid, stack);
+
  threxit(0);
  for(;;);
 }
@@ -162,25 +202,38 @@ int
 pthread_join(pthread_t thread, void **retval)
 {
 
+ if (thread->flags & THREAD_DETACHED)
+ return EINVAL;
+
  _sem_wait(&thread->donesem, 0, 0);
  if (retval)
  *retval = thread->retval;
 
+ /* We should be the last having a ref to this thread, but
+ * someone stupid or evil might haved detached it;
+ * in that case the thread will cleanup itself */
+ if ((thread->flags & THREAD_DETACHED) == 0)
+ _rthread_free(thread);
  return (0);
 }
 
 int
 pthread_detach(pthread_t thread)
 {
- _spinlock(&_thread_lock);
-#if 0
- if (thread->flags & THREAD_DONE)
- free(thread);
- else
-#endif
+ int rc = 0;
+
+ _spinlock(&thread->flags_lock);
+ if (thread->flags & THREAD_DETACHED) {
+ rc = EINVAL;
+ _spinunlock(&thread->flags_lock);
+ } else if (thread->flags & THREAD_DONE) {
+ _spinunlock(&thread->flags_lock);
+ _rthread_free(thread);
+ } else {
  thread->flags |= THREAD_DETACHED;
- _spinunlock(&_thread_lock);
- return (0);
+ _spinunlock(&thread->flags_lock);
+ }
+ return (rc);
 }
 
 int
@@ -200,6 +253,7 @@ pthread_create(pthread_t *threadp, const
  return (errno);
  memset(thread, 0, sizeof(*thread));
  thread->donesem.lock = _SPINLOCK_UNLOCKED;
+ thread->flags_lock = _SPINLOCK_UNLOCKED;
  thread->fn = start_routine;
  thread->arg = arg;
  if (attr)
@@ -238,6 +292,11 @@ pthread_create(pthread_t *threadp, const
  kill(thread->tid, SIGSTOP);
 
  _spinunlock(&_thread_lock);
+ if (reaper_thread == NULL) {
+ pthread_create(&reaper_thread, NULL, _rthread_reaper, NULL);
+ strlcpy(reaper_thread->name, "reaper",
+    sizeof(reaper_thread->name));
+ }
 
  return (0);
 
@@ -246,7 +305,7 @@ fail2:
  LIST_REMOVE(thread, threads);
 fail1:
  _spinunlock(&_thread_lock);
- free(thread);
+ _rthread_free(thread);
 
  return (rc);
 }
@@ -267,7 +326,7 @@ int
 pthread_cancel(pthread_t thread)
 {
 
- thread->flags |= THREAD_CANCELLED;
+ _rthread_setflag(thread, THREAD_CANCELLED);
  return (0);
 }
 
@@ -289,10 +348,10 @@ pthread_setcancelstate(int state, int *o
  oldstate = self->flags & THREAD_CANCEL_ENABLE ?
     PTHREAD_CANCEL_ENABLE : PTHREAD_CANCEL_DISABLE;
  if (state == PTHREAD_CANCEL_ENABLE) {
- self->flags |= THREAD_CANCEL_ENABLE;
+ _rthread_setflag(self, THREAD_CANCEL_ENABLE);
  pthread_testcancel();
  } else if (state == PTHREAD_CANCEL_DISABLE) {
- self->flags &= ~THREAD_CANCEL_ENABLE;
+ _rthread_clearflag(self, THREAD_CANCEL_ENABLE);
  } else {
  return (EINVAL);
  }
@@ -311,10 +370,10 @@ pthread_setcanceltype(int type, int *old
  oldtype = self->flags & THREAD_CANCEL_DEFERRED ?
     PTHREAD_CANCEL_DEFERRED : PTHREAD_CANCEL_ASYNCHRONOUS;
  if (type == PTHREAD_CANCEL_DEFERRED) {
- self->flags |= THREAD_CANCEL_DEFERRED;
+ _rthread_setflag(self, THREAD_CANCEL_DEFERRED);
  pthread_testcancel();
  } else if (type == PTHREAD_CANCEL_ASYNCHRONOUS) {
- self->flags &= ~THREAD_CANCEL_DEFERRED;
+ _rthread_clearflag(self, THREAD_CANCEL_DEFERRED);
  } else {
  return (EINVAL);
  }
Index: rthread.h
===================================================================
RCS file: /cvs/src/lib/librthread/rthread.h,v
retrieving revision 1.15
diff -u -p -r1.15 rthread.h
--- rthread.h 1 Jan 2006 19:32:30 -0000 1.15
+++ rthread.h 3 Jan 2006 09:11:59 -0000
@@ -35,6 +35,7 @@ struct stack {
  void *guard;
  size_t guardsize;
  size_t len;
+ int user_allocated;
 };
 
 typedef struct semaphore {
@@ -107,6 +108,7 @@ struct pthread {
  struct semaphore donesem;
  pid_t tid;
  unsigned int flags;
+ _spinlock_lock_t flags_lock;
  void *retval;
  void *(*fn)(void *);
  void *arg;
@@ -130,6 +132,7 @@ extern int _threads_ready;
 extern LIST_HEAD(listhead, pthread) _thread_list;
 extern struct pthread _initial_thread;
 extern _spinlock_lock_t _thread_lock;
+extern int _rthread_kq;
 
 void _spinlock(_spinlock_lock_t *);
 void _spinunlock(_spinlock_lock_t *);
@@ -144,6 +147,10 @@ void _rthread_free_stack(struct stack *)
 void _rthread_tls_destructors(pthread_t);
 void _rthread_debug(int, const char *, ...)
  __attribute__((__format__ (printf, 2, 3)));
+void _rthread_init_reaper(void);
+void _rthread_add_to_reaper(pid_t, struct stack *);
+void * _rthread_reaper(void *);
+
 
 void _thread_dump_info(void);
 
Index: rthread_reaper.c
===================================================================
RCS file: rthread_reaper.c
diff -N rthread_reaper.c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ rthread_reaper.c 3 Jan 2006 09:11:59 -0000
@@ -0,0 +1,60 @@
+/* $OpenBSD$ */
+/*
+ * Copyright (c) 2006 Otto Moerbeek <[hidden email]>
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include <sys/types.h>
+#include <sys/event.h>
+
+#include <machine/spinlock.h>
+#include <pthread.h>
+
+#include "rthread.h"
+
+int _rthread_kq;
+
+void
+_rthread_add_to_reaper(pid_t t, struct stack *s)
+{
+ struct kevent kc;
+ int n;
+
+ _rthread_debug(1, "Adding %d to reaper\n", t);
+ EV_SET(&kc, t, EVFILT_PROC, EV_ADD|EV_CLEAR, NOTE_EXIT, 0, s);
+ n = kevent(_rthread_kq, &kc, 1, NULL, 0, NULL);
+ if (n)
+ _rthread_debug(0, "_rthread_add_to_reaper(): kevent %d", n);
+}
+
+/* ARGSUSED */
+void *
+_rthread_reaper(void *arg)
+{
+ struct kevent ke;
+ int  n;
+
+ for (;;) {
+ n = kevent(_rthread_kq, NULL, 0, &ke, 1, NULL);
+ if (n < 0)
+ _rthread_debug(0, "_rthread_reaper(): kevent %d", n);
+ else {
+ _rthread_debug(1, "_rthread_reaper(): %d died\n",
+    ke.ident);
+ /* XXX check error conditions */
+ _rthread_free_stack(ke.udata);
+ }
+
+ }
+}
Index: rthread_stack.c
===================================================================
RCS file: /cvs/src/lib/librthread/rthread_stack.c,v
retrieving revision 1.1
diff -u -p -r1.1 rthread_stack.c
--- rthread_stack.c 1 Jan 2006 19:32:30 -0000 1.1
+++ rthread_stack.c 3 Jan 2006 09:11:59 -0000
@@ -68,7 +68,8 @@ _rthread_alloc_stack(pthread_t thread)
  if (!stack) {
  mprotect(guard, thread->attr.guard_size,
  PROT_EXEC | PROT_READ | PROT_WRITE);
- munmap(base, size);
+ if (thread->attr.stack_addr == NULL)
+ munmap(base, size);
  return (NULL);
  }
  stack->sp = start;
@@ -76,6 +77,7 @@ _rthread_alloc_stack(pthread_t thread)
  stack->guard = guard;
  stack->guardsize = thread->attr.guard_size;
  stack->len = size;
+ stack->user_allocated = thread->attr.stack_addr != NULL;
  return (stack);
  }
  return (NULL);
@@ -86,7 +88,8 @@ _rthread_free_stack(struct stack *stack)
 {
  mprotect(stack->guard, stack->guardsize,
  PROT_EXEC | PROT_READ | PROT_WRITE);
- munmap(stack->base, stack->len);
+ if (!stack->user_allocated)
+ munmap(stack->base, stack->len);
  free(stack);
 }

Reply | Threaded
Open this post in threaded view
|

Re: rthreads: plugging mem leaks

Otto Moerbeek
On Tue, 3 Jan 2006, Otto Moerbeek wrote:

> Hi,
>
> One of the large missing pieces of the rthreads puzzle is cleaning up
> after a thread exits.
>
> This is all more complicated that you'd think since a thread may be
> referenced by pthread_join() or pthread_detach() _after_ it has exited.
>
> Cleaning up stacks is even more nasty: the stack is needed to call
> threxit(). Cleaning up the stack cannot be done in the regular cleanup
> code, sice the we do not know if the threxit() has actually been
> completed.
>
> Anyway, here's a diff to fix all this. It's nice to see the
> pthread_specific regress test first eat a lot of memory, and then
> giving it back piece by piece.
>
> Thanks to Marco Hyman for providing some valuable hints.

A different version has been committed, not neading a separate reaper thread.

        -Otto