diff options
author | Ivan Maidanski <ivmai@mail.ru> | 2022-04-17 15:18:43 +0300 |
---|---|---|
committer | Ivan Maidanski <ivmai@mail.ru> | 2022-04-17 15:19:14 +0300 |
commit | 5d2775378c29cb71a1f2f0076bca6ff584babbc6 (patch) | |
tree | f1309bc37ce9c9a82becc13b4680c3f724c71361 | |
parent | 82c8af1a73c54768aba43648849eb8592cbae361 (diff) | |
download | bdwgc-5d2775378c29cb71a1f2f0076bca6ff584babbc6.tar.gz |
Fix hang on sem_wait in GC_suspend_thread if thread was resumed recently
If GC_resume_thread(t) is followed by GC_suspend_thread(t) immediately,
then there is a risk of the loop (which had been entered before
GC_resume_thread(t) is called) in GC_suspend_self_inner() continue
iterating, thus the suspend signal handler stays pending, and thus
the relevant sem_post() will not be called.
The solution is to replace a boolean flag (suspended_ext) with
a counter (thread_stop_info.ext_suspend_cnt) with the lowest bit
indicating whether the thread is suspended manually.
* include/private/pthread_stop_world.h [SIGNAL_BASED_STOP_WORLD
&& GC_ENABLE_SUSPEND_THREAD] (thread_stop_info.ext_suspend_cnt): Add
field; add comment.
* include/private/pthread_support.h [SIGNAL_BASED_STOP_WORLD
&& GC_ENABLE_SUSPEND_THREAD] (GC_thread.suspended_ext): Remove field.
* include/private/pthread_support.h [SIGNAL_BASED_STOP_WORLD
&& GC_ENABLE_SUSPEND_THREAD] (GC_suspend_self_inner): Add suspend_cnt
argument.
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL
&& GC_ENABLE_SUSPEND_THREAD] (GC_suspend_self_inner): Likewise.
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL
&& GC_ENABLE_SUSPEND_THREAD] (GC_suspend_handler_inner): Declare
suspend_cnt local variable, set it from me->stop_info.ext_suspend_cnt,
pass it GC_suspend_self_inner().
* pthread_support.c [GC_ENABLE_SUSPEND_THREAD
&& SIGNAL_BASED_STOP_WORLD] (GC_do_blocking_inner,
GC_suspend_self_blocked, GC_call_with_gc_active): Likewise.
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL
&& GC_ENABLE_SUSPEND_THREAD] (GC_suspend_handler_inner): Replace
me->suspended_ext with suspend_cnt&1.
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL
&& GC_ENABLE_SUSPEND_THREAD] (GC_suspend_self_inner): Add assertion
that suspend_cnt is odd; replace me->suspended_ext loop condition to
me->stop_info.ext_suspend_cnt==suspend_cnt.
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL
&& GC_ENABLE_SUSPEND_THREAD] (GC_suspend_thread, GC_resume_thread):
Declare/use suspend_cnt local variable; increment
t->stop_info.ext_suspend_cnt to indicate suspend/resume; add comment.
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL
&& GC_ENABLE_SUSPEND_THREAD] (GC_suspend_thread, GC_resume_thread,
GC_is_thread_suspended, GC_suspend_all, GC_stop_world): Replace
t->suspended_ext to t->stop_info.ext_suspend_cnt&1.
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL && E2K
&& GC_ENABLE_SUSPEND_THREAD] (GC_push_all_stacks): Likewise.
* pthread_support.c [GC_ENABLE_SUSPEND_THREAD
&& SIGNAL_BASED_STOP_WORLD] (GC_do_blocking_inner,
GC_suspend_self_blocked, GC_call_with_gc_active,
GC_register_my_thread): Likewise.
-rw-r--r-- | include/private/pthread_stop_world.h | 8 | ||||
-rw-r--r-- | include/private/pthread_support.h | 6 | ||||
-rw-r--r-- | pthread_stop_world.c | 47 | ||||
-rw-r--r-- | pthread_support.c | 22 |
4 files changed, 56 insertions, 27 deletions
diff --git a/include/private/pthread_stop_world.h b/include/private/pthread_stop_world.h index cdaae7c8..828a37dc 100644 --- a/include/private/pthread_stop_world.h +++ b/include/private/pthread_stop_world.h @@ -25,6 +25,14 @@ struct thread_stop_info { volatile AO_t last_stop_count; /* The value of GC_stop_count when the thread */ /* last successfully handled a suspend signal. */ +# ifdef GC_ENABLE_SUSPEND_THREAD + volatile AO_t ext_suspend_cnt; + /* An odd value means thread was suspended */ + /* externally. Incremented on every call of */ + /* GC_suspend_thread() and GC_resume_thread(). */ + /* Updated with the GC lock held, but could be */ + /* read from a signal handler. */ +# endif # endif ptr_t stack_ptr; /* Valid only when stopped. */ diff --git a/include/private/pthread_support.h b/include/private/pthread_support.h index c7053b64..b8dd30aa 100644 --- a/include/private/pthread_support.h +++ b/include/private/pthread_support.h @@ -66,10 +66,6 @@ typedef struct GC_Thread_Rep { /* Extra bookkeeping information the stopping code uses */ struct thread_stop_info stop_info; -# if defined(GC_ENABLE_SUSPEND_THREAD) && defined(SIGNAL_BASED_STOP_WORLD) - volatile AO_t suspended_ext; /* Thread was suspended externally. */ -# endif - unsigned char flags; /* Protected by GC lock. */ # define FINISHED 1 /* Thread has exited. */ # define DETACHED 2 /* Thread is treated as detached. */ @@ -164,7 +160,7 @@ GC_INNER GC_thread GC_lookup_thread(pthread_t id); #endif #if defined(GC_ENABLE_SUSPEND_THREAD) && defined(SIGNAL_BASED_STOP_WORLD) - GC_INNER void GC_suspend_self_inner(GC_thread me); + GC_INNER void GC_suspend_self_inner(GC_thread me, word suspend_cnt); GC_INNER void GC_suspend_self_blocked(ptr_t thread_me, void *context); /* Wrapper over GC_suspend_self_inner. */ diff --git a/pthread_stop_world.c b/pthread_stop_world.c index 2f85457f..9cc4c84f 100644 --- a/pthread_stop_world.c +++ b/pthread_stop_world.c @@ -330,6 +330,9 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy GC_ATTR_UNUSED, size_t stack_size; # endif IF_CANCEL(int cancel_state;) +# ifdef GC_ENABLE_SUSPEND_THREAD + word suspend_cnt; +# endif AO_t my_stop_count = ao_load_acquire_async(&GC_stop_count); /* After the barrier, this thread should see */ /* the actual content of GC_threads. */ @@ -354,7 +357,8 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy GC_ATTR_UNUSED, me = GC_lookup_thread_async(self); # ifdef GC_ENABLE_SUSPEND_THREAD - if (ao_load_async(&me->suspended_ext)) { + suspend_cnt = (word)ao_load_async(&(me -> stop_info.ext_suspend_cnt)); + if ((suspend_cnt & 1) != 0) { GC_store_stack_ptr(me); # ifdef E2K GC_ASSERT(NULL == me -> backing_store_end); @@ -363,7 +367,7 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy GC_ATTR_UNUSED, me -> backing_store_ptr = bs_lo + stack_size; # endif sem_post(&GC_suspend_ack_sem); - GC_suspend_self_inner(me); + GC_suspend_self_inner(me, suspend_cnt); # ifdef DEBUG_THREADS GC_log_printf("Continuing %p on GC_resume_thread\n", (void *)self); # endif @@ -627,8 +631,10 @@ STATIC void GC_restart_handler(int sig) (void)select(0, 0, 0, 0, &tv); } - GC_INNER void GC_suspend_self_inner(GC_thread me) { - while (ao_load_acquire_async(&me->suspended_ext)) { + GC_INNER void GC_suspend_self_inner(GC_thread me, word suspend_cnt) { + GC_ASSERT((suspend_cnt & 1) != 0); + while ((word)ao_load_acquire_async(&(me -> stop_info.ext_suspend_cnt)) + == suspend_cnt) { /* TODO: Use sigsuspend() instead. */ GC_brief_async_signal_safe_sleep(); } @@ -637,24 +643,30 @@ STATIC void GC_restart_handler(int sig) GC_API void GC_CALL GC_suspend_thread(GC_SUSPEND_THREAD_ID thread) { GC_thread t; AO_t saved_stop_count; + word suspend_cnt; IF_CANCEL(int cancel_state;) DCL_LOCK_STATE; LOCK(); t = GC_lookup_thread((pthread_t)thread); - if (t == NULL || t -> suspended_ext) { + if (NULL == t) { + UNLOCK(); + return; + } + suspend_cnt = (word)(t -> stop_info.ext_suspend_cnt); + if ((suspend_cnt & 1) != 0) /* already suspended? */ { UNLOCK(); return; } if ((t -> flags & (FINISHED | DO_BLOCKING)) != 0) { - t -> suspended_ext = TRUE; + t -> stop_info.ext_suspend_cnt = (AO_t)(suspend_cnt | 1); /* suspend */ /* Terminated but not joined yet, or in do-blocking state. */ UNLOCK(); return; } if (THREAD_EQUAL((pthread_t)thread, pthread_self())) { - t -> suspended_ext = TRUE; + t -> stop_info.ext_suspend_cnt = (AO_t)(suspend_cnt | 1); GC_with_callee_saves_pushed(GC_suspend_self_blocked, (ptr_t)t); UNLOCK(); return; @@ -687,7 +699,8 @@ STATIC void GC_restart_handler(int sig) AO_store(&GC_stop_count, saved_stop_count + THREAD_RESTARTED); /* Set the flag making the change visible to the signal handler. */ - AO_store_release(&t->suspended_ext, TRUE); + AO_store_release(&(t -> stop_info.ext_suspend_cnt), + (AO_t)(suspend_cnt | 1)); /* TODO: Support GC_retry_signals (not needed for TSan) */ switch (raise_signal(t, GC_sig_suspend)) { @@ -719,8 +732,14 @@ STATIC void GC_restart_handler(int sig) LOCK(); t = GC_lookup_thread((pthread_t)thread); - if (t != NULL) - AO_store(&t->suspended_ext, FALSE); + if (t != NULL) { + word suspend_cnt = (word)(t -> stop_info.ext_suspend_cnt); + + if ((suspend_cnt & 1) != 0) /* is suspended? */ { + /* Mark the thread as not suspended - it will be resumed shortly. */ + AO_store(&(t -> stop_info.ext_suspend_cnt), (AO_t)(suspend_cnt + 1)); + } + } UNLOCK(); } @@ -731,7 +750,7 @@ STATIC void GC_restart_handler(int sig) LOCK(); t = GC_lookup_thread((pthread_t)thread); - if (t != NULL && t -> suspended_ext) + if (t != NULL && (t -> stop_info.ext_suspend_cnt & 1) != 0) is_suspended = (int)TRUE; UNLOCK(); return is_suspended; @@ -845,7 +864,7 @@ GC_INNER void GC_push_all_stacks(void) if ((GC_stop_count & THREAD_RESTARTED) != 0 && (p -> flags & DO_BLOCKING) == 0 # ifdef GC_ENABLE_SUSPEND_THREAD - && !p->suspended_ext + && (p -> stop_info.ext_suspend_cnt & 1) == 0 # endif && !THREAD_EQUAL(p -> id, self)) continue; /* procedure stack buffer has already been freed */ @@ -903,7 +922,7 @@ STATIC int GC_suspend_all(void) if ((p -> flags & (FINISHED | DO_BLOCKING)) != 0) continue; # ifndef GC_OPENBSD_UTHREADS # ifdef GC_ENABLE_SUSPEND_THREAD - if (p -> suspended_ext) continue; + if ((p -> stop_info.ext_suspend_cnt & 1) != 0) continue; # endif if (AO_load(&p->stop_info.last_stop_count) == GC_stop_count) continue; /* matters only if GC_retry_signals */ @@ -1236,7 +1255,7 @@ GC_INNER void GC_stop_world(void) if ((p -> flags & (FINISHED | DO_BLOCKING)) != 0) continue; # ifndef GC_OPENBSD_UTHREADS # ifdef GC_ENABLE_SUSPEND_THREAD - if (p -> suspended_ext) continue; + if ((p -> stop_info.ext_suspend_cnt & 1) != 0) continue; # endif if (GC_retry_signals && AO_load(&p->stop_info.last_stop_count) == GC_stop_count) diff --git a/pthread_support.c b/pthread_support.c index c1264fff..1f32bc4d 100644 --- a/pthread_support.c +++ b/pthread_support.c @@ -1596,9 +1596,12 @@ GC_INNER void GC_do_blocking_inner(ptr_t data, void * context GC_ATTR_UNUSED) /* Note: this code cannot be moved into do_blocking_leave() */ /* otherwise there could be a static analysis tool warning */ /* (false positive) about unlock without a matching lock. */ - while (EXPECT(me -> suspended_ext, FALSE)) { + while (EXPECT((me -> stop_info.ext_suspend_cnt & 1) != 0, FALSE)) { + word suspend_cnt = (word)(me -> stop_info.ext_suspend_cnt); + /* read suspend counter (number) before unlocking */ + UNLOCK(); - GC_suspend_self_inner(me); + GC_suspend_self_inner(me, suspend_cnt); LOCK(); } # endif @@ -1616,11 +1619,13 @@ GC_INNER void GC_do_blocking_inner(ptr_t data, void * context GC_ATTR_UNUSED) GC_bool topOfStackUnset = do_blocking_enter(me); DCL_LOCK_STATE; - while (me -> suspended_ext) { + do { + word suspend_cnt = (word)(me -> stop_info.ext_suspend_cnt); + UNLOCK(); - GC_suspend_self_inner(me); + GC_suspend_self_inner(me, suspend_cnt); LOCK(); - } + } while ((me -> stop_info.ext_suspend_cnt & 1) != 0); do_blocking_leave(me, topOfStackUnset); } #endif @@ -1725,9 +1730,10 @@ GC_API void * GC_CALL GC_call_with_gc_active(GC_fn_type fn, } # if defined(GC_ENABLE_SUSPEND_THREAD) && defined(SIGNAL_BASED_STOP_WORLD) - while (EXPECT(me -> suspended_ext, FALSE)) { + while (EXPECT((me -> stop_info.ext_suspend_cnt & 1) != 0, FALSE)) { + word suspend_cnt = (word)(me -> stop_info.ext_suspend_cnt); UNLOCK(); - GC_suspend_self_inner(me); + GC_suspend_self_inner(me, suspend_cnt); LOCK(); } # endif @@ -2063,7 +2069,7 @@ GC_API int GC_CALL GC_register_my_thread(const struct GC_stack_base *sb) # endif # if defined(GC_ENABLE_SUSPEND_THREAD) \ && defined(SIGNAL_BASED_STOP_WORLD) - if (me -> suspended_ext) { + if ((me -> stop_info.ext_suspend_cnt & 1) != 0) { GC_with_callee_saves_pushed(GC_suspend_self_blocked, (ptr_t)me); } # endif |