From 8f8cba46b89ad16bac2ce58d554c25b54b0e7c7f Mon Sep 17 00:00:00 2001 From: Alexandre Oliva Date: Tue, 7 Jul 2020 02:47:53 -0300 Subject: [PATCH] Rework the condition variables support for VxWorks This change reworks the condition variables support for VxWorks to address the very legit points raised by Rasmus in https://gcc.gnu.org/pipermail/gcc/2020-May/232524.html While some of the issues were taken care of by the use of semFlush, a few others were indeed calling for adjustments. We first considered resorting to the condvarLib library available in VxWorks7. Unfortunately, it is vx7 only and we wanted something working for at least vx 6.9 as well. It also turned out requiring the use of recursive mutexes for condVarWait, which seemed unnecessarily constraining. Instead, this change corrects the sequencing logic in a few places and leverages the semExchange API to ensure the key atomicity requirement on cond_wait operations. 2020-10-14 Alexandre Oliva libgcc/ * config/gthr-vxworks-thread.c: Include stdlib.h. (tls_delete_hook): Prototype it. (__gthread_cond_signal): Return early if no waiters. Consume signal in case the semaphore got full. Use semInfoGet instead of kernel-mode-only semInfo. (__gthread_cond_timedwait): Use semExchange. Always take the mutex again before returning. * config/gthr-vxworks-cond.c (__ghtread_cond_wait): Likewise. --- libgcc/config/gthr-vxworks-cond.c | 6 ++-- libgcc/config/gthr-vxworks-thread.c | 53 ++++++++++++++++++++++++----- 2 files changed, 47 insertions(+), 12 deletions(-) diff --git a/libgcc/config/gthr-vxworks-cond.c b/libgcc/config/gthr-vxworks-cond.c index d65d07aebc3..65f0a6af183 100644 --- a/libgcc/config/gthr-vxworks-cond.c +++ b/libgcc/config/gthr-vxworks-cond.c @@ -66,13 +66,11 @@ __gthread_cond_wait (__gthread_cond_t *cond, if (!mutex) return ERROR; - __RETURN_ERRNO_IF_NOT_OK (semGive (*mutex)); - - __RETURN_ERRNO_IF_NOT_OK (semTake (*cond, WAIT_FOREVER)); + int ret = __CHECK_RESULT (semExchange (*mutex, *cond, WAIT_FOREVER)); __RETURN_ERRNO_IF_NOT_OK (semTake (*mutex, WAIT_FOREVER)); - return OK; + return ret; } int diff --git a/libgcc/config/gthr-vxworks-thread.c b/libgcc/config/gthr-vxworks-thread.c index 8544b03dd18..8e26d855931 100644 --- a/libgcc/config/gthr-vxworks-thread.c +++ b/libgcc/config/gthr-vxworks-thread.c @@ -29,6 +29,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see #include "gthr.h" #include +#include #define __TIMESPEC_TO_NSEC(timespec) \ ((long long)timespec.tv_sec * 1000000000 + (long long)timespec.tv_nsec) @@ -38,7 +39,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see / 1000000000) #ifdef __RTP__ - void tls_delete_hook (); + void tls_delete_hook (void); #define __CALL_DELETE_HOOK(tcb) tls_delete_hook() #else /* In kernel mode, we need to pass the TCB to task_delete_hook. The TCB is @@ -47,17 +48,55 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see #define __CALL_DELETE_HOOK(tcb) tls_delete_hook((WIND_TCB *) ((tcb)->task_id)) #endif -/* -------------------- Timed Condition Variables --------------------- */ - int __gthread_cond_signal (__gthread_cond_t *cond) { if (!cond) return ERROR; - return __CHECK_RESULT (semGive (*cond)); + /* If nobody is waiting, skip the semGive altogether: no one can get + in line while we hold the mutex associated with *COND. We could + skip this test altogether, but it's presumed cheaper than going + through the give and take below, and that a signal without a + waiter occurs often enough for the test to be worth it. */ + SEM_INFO info; + memset (&info, 0, sizeof (info)); + __RETURN_ERRNO_IF_NOT_OK (semInfoGet (*cond, &info)); + if (info.numTasks == 0) + return OK; + + int ret = __CHECK_RESULT (semGive (*cond)); + + /* It might be the case, however, that when we called semInfo, there + was a waiter just about to timeout, and by the time we called + semGive, it had already timed out, so our semGive would leave the + *cond semaphore full, so the next caller of wait would pass + through. We don't want that. So, make sure we leave the + semaphore empty. Despite the window in which the semaphore will + be full, this works because: + + - we're holding the mutex, so nobody else can semGive, and any + pending semTakes are actually within semExchange. there might + be others blocked to acquire the mutex, but those are not + relevant for the analysis. + + - if there was another non-timed out waiter, semGive will wake it + up immediately instead of leaving the semaphore full, so the + semTake below will time out, and the semantics are as expected + + - otherwise, if all waiters timed out before the semGive (or if + there weren't any to begin with), our semGive completed leaving + the semaphore full, and our semTake below will consume it + before any other waiter has a change to reach the semExchange, + because we're holding the mutex. */ + if (ret == OK) + semTake (*cond, NO_WAIT); + + return ret; } +/* -------------------- Timed Condition Variables --------------------- */ + int __gthread_cond_timedwait (__gthread_cond_t *cond, __gthread_mutex_t *mutex, @@ -93,13 +132,11 @@ __gthread_cond_timedwait (__gthread_cond_t *cond, if (waiting_ticks > INT_MAX) waiting_ticks = INT_MAX; - __RETURN_ERRNO_IF_NOT_OK (semGive (*mutex)); - - __RETURN_ERRNO_IF_NOT_OK (semTake (*cond, waiting_ticks)); + int ret = __CHECK_RESULT (semExchange (*mutex, *cond, waiting_ticks)); __RETURN_ERRNO_IF_NOT_OK (semTake (*mutex, WAIT_FOREVER)); - return OK; + return ret; } /* --------------------------- Timed Mutexes ------------------------------ */