From 5759fa78f2289c7e891aaded2a54e069b8bdac01 Mon Sep 17 00:00:00 2001 From: Nikodemus Siivola Date: Thu, 6 Sep 2007 13:36:23 +0000 Subject: [PATCH] 1.0.9.39: thread stack memory leaks * Since 1.0.9.30 we pad & align the per-thread areas after allocation -- but we need to still pass the original address and size to os_invalidate(), or else we leak. * Also refactor the freeable_thread_stack stuff slightly for less OAOOM. * Whitespace in tests. --- src/compiler/generic/objdef.lisp | 5 ++++ src/runtime/thread.c | 65 ++++++++++++++++++++-------------------- tests/threads.impure.lisp | 34 ++++++++++----------- version.lisp-expr | 2 +- 4 files changed, 55 insertions(+), 51 deletions(-) diff --git a/src/compiler/generic/objdef.lisp b/src/compiler/generic/objdef.lisp index bf81d29d0..4082bd177 100644 --- a/src/compiler/generic/objdef.lisp +++ b/src/compiler/generic/objdef.lisp @@ -374,6 +374,11 @@ ;; of a symbol is initialized to zero (no-tls-value-marker) (os-thread :c-type "volatile os_thread_t") + ;; This is the original address at which the memory was allocated, + ;; which may have different alignment then what we prefer to use. + ;; Kept here so that when the thread dies we can releast the whole + ;; memory we reserved. + (os-address :c-type "void *" :length #!+alpha 2 #!-alpha 1) (binding-stack-start :c-type "lispobj *" :length #!+alpha 2 #!-alpha 1) (binding-stack-pointer :c-type "lispobj *" :length #!+alpha 2 #!-alpha 1) (control-stack-start :c-type "lispobj *" :length #!+alpha 2 #!-alpha 1) diff --git a/src/runtime/thread.c b/src/runtime/thread.c index e61ecbe99..a838c7d55 100644 --- a/src/runtime/thread.c +++ b/src/runtime/thread.c @@ -70,7 +70,7 @@ struct freeable_stack { struct freeable_stack *next; #endif os_thread_t os_thread; - os_vm_address_t stack; + os_vm_address_t os_address; }; @@ -144,7 +144,8 @@ initial_thread_trampoline(struct thread *th) #define THREAD_STRUCT_SIZE (THREAD_CONTROL_STACK_SIZE + BINDING_STACK_SIZE + \ ALIEN_STACK_SIZE + dynamic_values_bytes + \ - 32 * SIGSTKSZ) + 32 * SIGSTKSZ + \ + BACKEND_PAGE_SIZE) #ifdef LISP_FEATURE_SB_THREAD @@ -153,33 +154,27 @@ initial_thread_trampoline(struct thread *th) static void queue_freeable_thread_stack(struct thread *thread_to_be_cleaned_up) { + struct freeable_stack *new_freeable_stack = 0; if (thread_to_be_cleaned_up) { + /* FIXME: os_validate is mmap -- for small things like these + * malloc would probably perform better. */ + new_freeable_stack = (struct freeable_stack *) + os_validate(0, sizeof(struct freeable_stack)); + new_freeable_stack->next = NULL; + new_freeable_stack->os_thread = thread_to_be_cleaned_up->os_thread; + new_freeable_stack->os_address = thread_to_be_cleaned_up->os_address; pthread_mutex_lock(&freeable_stack_lock); if (freeable_stack_queue) { - struct freeable_stack *new_freeable_stack = 0, *next; + struct freeable_stack *next; next = freeable_stack_queue; while (next->next) { next = next->next; } - new_freeable_stack = (struct freeable_stack *) - os_validate(0, sizeof(struct freeable_stack)); - new_freeable_stack->next = NULL; - new_freeable_stack->os_thread = thread_to_be_cleaned_up->os_thread; - new_freeable_stack->stack = (os_vm_address_t) - thread_to_be_cleaned_up->control_stack_start; next->next = new_freeable_stack; - freeable_stack_count++; } else { - struct freeable_stack *new_freeable_stack = 0; - new_freeable_stack = (struct freeable_stack *) - os_validate(0, sizeof(struct freeable_stack)); - new_freeable_stack->next = NULL; - new_freeable_stack->os_thread = thread_to_be_cleaned_up->os_thread; - new_freeable_stack->stack = (os_vm_address_t) - thread_to_be_cleaned_up->control_stack_start; freeable_stack_queue = new_freeable_stack; - freeable_stack_count++; } + freeable_stack_count++; pthread_mutex_unlock(&freeable_stack_lock); } } @@ -196,7 +191,7 @@ free_freeable_stacks() { freeable_stack_count--; gc_assert(pthread_join(old->os_thread, NULL) == 0); FSHOW((stderr, "freeing thread %x stack\n", old->os_thread)); - os_invalidate(old->stack, THREAD_STRUCT_SIZE); + os_invalidate(old->os_address, THREAD_STRUCT_SIZE); os_invalidate((os_vm_address_t)old, sizeof(struct freeable_stack)); pthread_mutex_unlock(&freeable_stack_lock); } @@ -214,7 +209,7 @@ cleanup_thread(void *arg) gc_assert(pthread_join(freeable->os_thread, NULL) == 0); FSHOW((stderr, "/cleaner thread(%p): free stack %p\n", self, freeable->stack)); - os_invalidate(freeable->stack, THREAD_STRUCT_SIZE); + os_invalidate(freeable->os_address, THREAD_STRUCT_SIZE); free(freeable); pthread_detach(self); @@ -233,8 +228,8 @@ create_cleanup_thread(struct thread *thread_to_be_cleaned_up) malloc(sizeof(struct freeable_stack)); gc_assert(freeable != NULL); freeable->os_thread = thread_to_be_cleaned_up->os_thread; - freeable->stack = - (os_vm_address_t) thread_to_be_cleaned_up->control_stack_start; + freeable->os_address = + (os_vm_address_t) thread_to_be_cleaned_up->os_address; result = pthread_create(&thread, NULL, cleanup_thread, freeable); gc_assert(result == 0); sched_yield(); @@ -250,8 +245,8 @@ free_thread_stack_later(struct thread *thread_to_be_cleaned_up) new_freeable_stack = (struct freeable_stack *) os_validate(0, sizeof(struct freeable_stack)); new_freeable_stack->os_thread = thread_to_be_cleaned_up->os_thread; - new_freeable_stack->stack = (os_vm_address_t) - thread_to_be_cleaned_up->control_stack_start; + new_freeable_stack->os_address = (os_vm_address_t) + thread_to_be_cleaned_up->os_address; } new_freeable_stack = (struct freeable_stack *) swap_lispobjs((lispobj *)(void *)&freeable_stack, @@ -262,7 +257,7 @@ free_thread_stack_later(struct thread *thread_to_be_cleaned_up) * exists and the stack can be safely freed. This is sadly not * mandated by the pthread spec. */ gc_assert(pthread_join(new_freeable_stack->os_thread, NULL) == 0); - os_invalidate(new_freeable_stack->stack, THREAD_STRUCT_SIZE); + os_invalidate(new_freeable_stack->os_address, THREAD_STRUCT_SIZE); os_invalidate((os_vm_address_t) new_freeable_stack, sizeof(struct freeable_stack)); } @@ -351,7 +346,7 @@ free_thread_struct(struct thread *th) if (th->interrupt_data) os_invalidate((os_vm_address_t) th->interrupt_data, (sizeof (struct interrupt_data))); - os_invalidate((os_vm_address_t) th->control_stack_start, + os_invalidate((os_vm_address_t) th->os_address, THREAD_STRUCT_SIZE); } @@ -365,6 +360,7 @@ create_thread_struct(lispobj initial_function) { union per_thread_data *per_thread; struct thread *th=0; /* subdue gcc */ void *spaces=0; + void *aligned_spaces=0; #ifdef LISP_FEATURE_SB_THREAD int i; #endif @@ -380,14 +376,16 @@ create_thread_struct(lispobj initial_function) { * alignment passed from os_validate, since that might assume the * current (e.g. 4k) pagesize, while we calculate with the biggest * (e.g. 64k) pagesize allowed by the ABI. */ - spaces=os_validate(0, THREAD_STRUCT_SIZE + BACKEND_PAGE_SIZE); + spaces=os_validate(0, THREAD_STRUCT_SIZE); if(!spaces) - return NULL; - spaces = (void *)((((unsigned long)(char *)spaces) - + BACKEND_PAGE_SIZE - 1) - & ~(BACKEND_PAGE_SIZE - 1)); + return NULL; + /* Aligning up is safe as THREAD_STRUCT_SIZE has BACKEND_PAGE_SIZE + * padding. */ + aligned_spaces = (void *)((((unsigned long)(char *)spaces) + + BACKEND_PAGE_SIZE - 1) + & ~(BACKEND_PAGE_SIZE - 1)); per_thread=(union per_thread_data *) - (spaces+ + (aligned_spaces+ THREAD_CONTROL_STACK_SIZE+ BINDING_STACK_SIZE+ ALIEN_STACK_SIZE); @@ -422,7 +420,8 @@ create_thread_struct(lispobj initial_function) { #endif th=&per_thread->thread; - th->control_stack_start = spaces; + th->os_address = spaces; + th->control_stack_start = aligned_spaces; th->binding_stack_start= (lispobj*)((void*)th->control_stack_start+THREAD_CONTROL_STACK_SIZE); th->control_stack_end = th->binding_stack_start; diff --git a/tests/threads.impure.lisp b/tests/threads.impure.lisp index cbf5ce6a3..068d68b99 100644 --- a/tests/threads.impure.lisp +++ b/tests/threads.impure.lisp @@ -18,10 +18,10 @@ (defmacro defincf (name accessor &rest args) `(defun ,name (x) (let* ((old (,accessor x ,@args)) - (new (1+ old))) + (new (1+ old))) (loop until (eq old (sb-ext:compare-and-swap (,accessor x ,@args) old new)) do (setf old (,accessor x ,@args) - new (1+ old))) + new (1+ old))) new))) (defstruct cas-struct (slot 0)) @@ -38,30 +38,30 @@ (defun ,name (n) (declare (fixnum n)) (let* ((x ,init) - (run nil) - (threads - (loop repeat 10 - collect (sb-thread:make-thread - (lambda () - (loop until run) - (loop repeat n do (,incf x))))))) - (setf run t) - (dolist (th threads) - (sb-thread:join-thread th)) - (assert (= (,op x) (* 10 n))))) + (run nil) + (threads + (loop repeat 10 + collect (sb-thread:make-thread + (lambda () + (loop until run) + (loop repeat n do (,incf x))))))) + (setf run t) + (dolist (th threads) + (sb-thread:join-thread th)) + (assert (= (,op x) (* 10 n))))) (,name 200000))) (def-test-cas test-cas-car (cons 0 nil) incf-car car) (def-test-cas test-cas-cdr (cons nil 0) incf-cdr cdr) (def-test-cas test-cas-slot (make-cas-struct) incf-slot cas-struct-slot) (def-test-cas test-cas-value (let ((x '.x.)) - (set x 0) - x) + (set x 0) + x) incf-symbol-value symbol-value) (def-test-cas test-cas-svref/0 (vector 0 nil) incf-svref/0 (lambda (x) - (svref x 0))) + (svref x 0))) (def-test-cas test-cas-svref/1 (vector nil 0) incf-svref/1 (lambda (x) - (svref x 1))) + (svref x 1))) (format t "~&compare-and-swap tests done~%") (use-package :test-util) diff --git a/version.lisp-expr b/version.lisp-expr index efffb8716..1e4090cfc 100644 --- a/version.lisp-expr +++ b/version.lisp-expr @@ -17,4 +17,4 @@ ;;; checkins which aren't released. (And occasionally for internal ;;; versions, especially for internal versions off the main CVS ;;; branch, it gets hairier, e.g. "0.pre7.14.flaky4.13".) -"1.0.9.38" +"1.0.9.39" -- 2.11.4.GIT