Skip to content

Commit d67d169

Browse files
committed
Use atomics for system_working global
Although it almost certainly works in this case, volatile is best not used for multi-threaded code. Using atomics instead avoids warnings from TSan. This also simplifies some logic, as system_working was previously only ever assigned to 1, so --system_working <= 0 should always return true (unless it underflowed).
1 parent d845da0 commit d67d169

4 files changed

Lines changed: 19 additions & 24 deletions

File tree

misc/tsan_suppressions.txt

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,6 @@ race_top:RUBY_VM_INTERRUPTED_ANY
3030
race_top:unblock_function_set
3131
race_top:threadptr_get_interrupts
3232

33-
# system_working needs to be converted to atomic
34-
race:system_working
35-
3633
# It's already crashing. We're doing our best
3734
signal:rb_vm_bugreport
3835
race:check_reserved_signal_

thread.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ static int hrtime_update_expire(rb_hrtime_t *, const rb_hrtime_t);
148148
NORETURN(static void async_bug_fd(const char *mesg, int errno_arg, int fd));
149149
MAYBE_UNUSED(static int consume_communication_pipe(int fd));
150150

151-
static volatile int system_working = 1;
151+
static rb_atomic_t system_working = 1;
152152
static rb_internal_thread_specific_key_t specific_key_count;
153153

154154
/********************************************************************************/

thread_pthread.c

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2574,7 +2574,7 @@ rb_thread_wakeup_timer_thread(int sig)
25742574
timer_thread_wakeup_force();
25752575

25762576
// interrupt main thread if main thread is available
2577-
if (system_working) {
2577+
if (RUBY_ATOMIC_LOAD(system_working)) {
25782578
rb_vm_t *vm = GET_VM();
25792579
rb_thread_t *main_th = vm->ractor.main_thread;
25802580

@@ -3005,12 +3005,12 @@ timer_thread_func(void *ptr)
30053005

30063006
RUBY_DEBUG_LOG("started%s", "");
30073007

3008-
while (system_working) {
3008+
while (RUBY_ATOMIC_LOAD(system_working)) {
30093009
timer_thread_check_signal(vm);
30103010
timer_thread_check_timeout(vm);
30113011
ubf_wakeup_all_threads();
30123012

3013-
RUBY_DEBUG_LOG("system_working:%d", system_working);
3013+
RUBY_DEBUG_LOG("system_working:%d", RUBY_ATOMIC_LOAD(system_working));
30143014
timer_thread_polling(vm);
30153015
}
30163016

@@ -3124,18 +3124,16 @@ rb_thread_create_timer_thread(void)
31243124
static int
31253125
native_stop_timer_thread(void)
31263126
{
3127-
int stopped;
3128-
stopped = --system_working <= 0;
3127+
RUBY_ATOMIC_SET(system_working, 0);
31293128

3130-
if (stopped) {
3131-
RUBY_DEBUG_LOG("wakeup send %d", timer_th.comm_fds[1]);
3132-
timer_thread_wakeup_force();
3133-
RUBY_DEBUG_LOG("wakeup sent");
3134-
pthread_join(timer_th.pthread_id, NULL);
3135-
}
3129+
RUBY_DEBUG_LOG("wakeup send %d", timer_th.comm_fds[1]);
3130+
timer_thread_wakeup_force();
3131+
RUBY_DEBUG_LOG("wakeup sent");
3132+
pthread_join(timer_th.pthread_id, NULL);
31363133

31373134
if (TT_DEBUG) fprintf(stderr, "stop timer thread\n");
3138-
return stopped;
3135+
3136+
return 1;
31393137
}
31403138

31413139
static void

thread_win32.c

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -798,14 +798,14 @@ rb_thread_create_timer_thread(void)
798798
static int
799799
native_stop_timer_thread(void)
800800
{
801-
int stopped = --system_working <= 0;
802-
if (stopped) {
803-
SetEvent(timer_thread.lock);
804-
native_thread_join(timer_thread.id);
805-
CloseHandle(timer_thread.lock);
806-
timer_thread.lock = 0;
807-
}
808-
return stopped;
801+
RUBY_ATOMIC_SET(system_working, 0);
802+
803+
SetEvent(timer_thread.lock);
804+
native_thread_join(timer_thread.id);
805+
CloseHandle(timer_thread.lock);
806+
timer_thread.lock = 0;
807+
808+
return 1;
809809
}
810810

811811
static void

0 commit comments

Comments
 (0)