From f4c187e980a13e2b00e199eadcbd0bd1b945a10e Mon Sep 17 00:00:00 2001 From: zeromus Date: Mon, 9 Jan 2017 16:46:35 -0600 Subject: [PATCH 1/6] - port slock and scond fixes from desmume - make sthread_isself return false for null test thread full disclosure: this work was done months ago. I can't be 100% sure I've merged it correctly with recent rthreads.c changes --- libretro-common/rthreads/rthreads.c | 146 ++++++++++++++++++++++++---- 1 file changed, 127 insertions(+), 19 deletions(-) diff --git a/libretro-common/rthreads/rthreads.c b/libretro-common/rthreads/rthreads.c index 1744705642..8048438079 100644 --- a/libretro-common/rthreads/rthreads.c +++ b/libretro-common/rthreads/rthreads.c @@ -87,7 +87,32 @@ struct slock struct scond { #ifdef USE_WIN32_THREADS - HANDLE event; + + /* The syntax we'll use is mind-bending unless we use a struct. Plus, we might want to store more info later */ + /* This will be used as a linked list immplementing a queue of waiting threads */ + struct QueueEntry + { + struct QueueEntry* next; + }; + + /* With this implementation of scond, we don't have any way of waking (or even identifying) specific threads */ + /* But we need to wake them in the order indicated by the queue. */ + /* This potato token will get get passed around every waiter. The bearer can test whether he's next, and hold onto the potato if he is. */ + /* When he's done he can then put it back into play to progress the queue further */ + HANDLE hot_potato; + + /* The primary signalled event. Hot potatoes are passed until this is set. */ + HANDLE event; + + /* the head of the queue; NULL if queue is empty */ + struct QueueEntry* head; + + /* equivalent to the queue length */ + int waiters; + + /* how many waiters in the queue have been conceptually wakened by signals (even if we haven't managed to actually wake them yet */ + int wakens; + #else pthread_cond_t cond; #endif @@ -211,6 +236,9 @@ void sthread_join(sthread_t *thread) */ bool sthread_isself(sthread_t *thread) { + /* This thread can't possibly be a null thread */ + if (!thread) return false; + #ifdef USE_WIN32_THREADS return GetCurrentThread() == thread->thread; #else @@ -228,23 +256,25 @@ bool sthread_isself(sthread_t *thread) **/ slock_t *slock_new(void) { + bool mutex_created = false; slock_t *lock = (slock_t*)calloc(1, sizeof(*lock)); if (!lock) return NULL; #ifdef USE_WIN32_THREADS lock->lock = CreateMutex(NULL, FALSE, NULL); - if (!lock->lock) - goto error; + mutex_created = !!lock->lock; #else - if ((pthread_mutex_init(&lock->lock, NULL) < 0)) - goto error; + mutex_created = (pthread_mutex_init(&lock->lock, NULL) == 0); #endif + if (!mutex_created) + goto error; + return lock; error: - slock_free(lock); + free(lock); return NULL; } @@ -314,21 +344,33 @@ void slock_unlock(slock_t *lock) **/ scond_t *scond_new(void) { - bool event_created = false; scond_t *cond = (scond_t*)calloc(1, sizeof(*cond)); if (!cond) return NULL; #ifdef USE_WIN32_THREADS - cond->event = CreateEvent(NULL, FALSE, FALSE, NULL); - event_created = !!cond->event; -#else - event_created = (pthread_cond_init(&cond->cond, NULL) == 0); -#endif - - if (!event_created) + /* This is very complex because recreating condition variable semantics with win32 parts is not easy */ + /* The main problem is that a condition variable can be used to wake up a thread, but only if the thread is already waiting; */ + /* whereas a win32 event will 'wake up' a thread in advance (the event will be set in advance, so a 'waiter' wont even have to wait on it) */ + /* So at the very least, we need to do something clever. But there's bigger problems. */ + /* We don't even have a straightforward way in win32 to satisfy pthread_cond_wait's atomicity requirement. The bulk of this algorithm is solving that. */ + /* Note: We might could simplify this using vista+ condition variables, but we wanted an XP compatible solution. */ + cond->event = CreateEvent(NULL, FALSE, FALSE, NULL); + if(!cond->event) goto error; + cond->hot_potato = CreateEvent(NULL, FALSE, FALSE, NULL); + if(!cond->hot_potato) + { + CloseHandle(cond->event); goto error; + } + cond->waiters = cond->wakens = 0; + cond->head = NULL; + +#else + if(pthread_cond_init(&cond->cond, NULL) != 0) + goto error; +#endif return cond; @@ -350,6 +392,7 @@ void scond_free(scond_t *cond) #ifdef USE_WIN32_THREADS CloseHandle(cond->event); + CloseHandle(cond->hot_potato); #else pthread_cond_destroy(&cond->cond); #endif @@ -366,10 +409,56 @@ void scond_free(scond_t *cond) void scond_wait(scond_t *cond, slock_t *lock) { #ifdef USE_WIN32_THREADS - WaitForSingleObject(cond->event, 0); + + /* add ourselves to a queue of waiting threads */ + struct QueueEntry myentry; + struct QueueEntry** ptr = &cond->head; + while(*ptr) /* walk to the end of the linked list */ + ptr = &((*ptr)->next); + *ptr = &myentry; + myentry.next = NULL; + cond->waiters++; + + /* now the conceptual lock release and condition block are supposed to be atomic. */ + /* we can't do that in windows, but we can simulate the effects by using the queue, by the following analysis: */ + /* What happens if they aren't atomic? */ + /* 1. a signaller can rush in and signal, expecting a waiter to get it; but the waiter wouldn't, because he isn't blocked yet */ + /* solution: win32 events make this easy. the event will sit there enabled */ + /* 2. a signaller can rush in and signal, and then turn right around and wait */ + /* solution: the signaller will get queued behind the waiter, who's enqueued before he releases the mutex */ + + /* It's my turn if I'm the head of the queue. Check to see if it's my turn. */ + while (cond->head != &myentry) + { + /* As long as someone is even going to be able to wake up when they receive the potato, keep it going round */ + if (cond->wakens > 0) + SetEvent(cond->hot_potato); + + /* Wait to catch the hot potato before checking for my turn again */ + SignalObjectAndWait(lock->lock, cond->hot_potato, INFINITE, FALSE); + slock_lock(lock); + } + + /* It's my turn now -- I hold the potato */ SignalObjectAndWait(lock->lock, cond->event, INFINITE, FALSE); slock_lock(lock); + + /* Remove ourselves from the queue */ + cond->head = myentry.next; + cond->waiters--; + + /* If any other wakenings are pending, go ahead and set it up */ + /* There may actually be no waiters. That's OK. The first waiter will come in, find it's his turn, and immediately get the signaled event */ + cond->wakens--; + if(cond->wakens>0) + { + SetEvent(cond->event); + + /* Progress the queue: Put the hot potato back into play. It'll be tossed around until next in line gets it */ + SetEvent(cond->hot_potato); + } + #else pthread_cond_wait(&cond->cond, &lock->lock); #endif @@ -385,9 +474,17 @@ void scond_wait(scond_t *cond, slock_t *lock) int scond_broadcast(scond_t *cond) { #ifdef USE_WIN32_THREADS - /* FIXME _- check how this function should differ - * from scond_signal implementation. */ - SetEvent(cond->event); + + /* remember: we currently have mutex */ + if(cond->waiters == 0) return 0; + + /* awaken everything which is currently queued up */ + if(cond->wakens == 0) SetEvent(cond->event); + cond->wakens = cond->waiters; + + /* Since there is now at least one pending waken, the potato must be in play */ + SetEvent(cond->hot_potato); + return 0; #else return pthread_cond_broadcast(&cond->cond); @@ -404,7 +501,17 @@ int scond_broadcast(scond_t *cond) void scond_signal(scond_t *cond) { #ifdef USE_WIN32_THREADS - SetEvent(cond->event); + + /* remember: we currently have mutex */ + if(cond->waiters == 0) return; + + /* wake up the next thing in the queue */ + if(cond->wakens == 0) SetEvent(cond->event); + cond->wakens++; + + /* Since there is now at least one pending waken, the potato must be in play */ + SetEvent(cond->hot_potato); + #else pthread_cond_signal(&cond->cond); #endif @@ -427,6 +534,7 @@ bool scond_wait_timeout(scond_t *cond, slock_t *lock, int64_t timeout_us) #ifdef USE_WIN32_THREADS DWORD ret; + /* TODO: this is woefully inadequate. It needs to be solved with the newer approach used above */ WaitForSingleObject(cond->event, 0); ret = SignalObjectAndWait(lock->lock, cond->event, (DWORD)(timeout_us) / 1000, FALSE); From 10157c58319ddade5d1209edcf292d96c9e0adc6 Mon Sep 17 00:00:00 2001 From: zeromus Date: Sat, 21 Jan 2017 16:49:10 -0600 Subject: [PATCH 2/6] apply feedback re: PR #4392 --- libretro-common/rthreads/rthreads.c | 58 ++++++++++++++++++++--------- 1 file changed, 40 insertions(+), 18 deletions(-) diff --git a/libretro-common/rthreads/rthreads.c b/libretro-common/rthreads/rthreads.c index 8048438079..e9a12a8b14 100644 --- a/libretro-common/rthreads/rthreads.c +++ b/libretro-common/rthreads/rthreads.c @@ -92,7 +92,7 @@ struct scond /* This will be used as a linked list immplementing a queue of waiting threads */ struct QueueEntry { - struct QueueEntry* next; + struct QueueEntry *next; }; /* With this implementation of scond, we don't have any way of waking (or even identifying) specific threads */ @@ -102,16 +102,16 @@ struct scond HANDLE hot_potato; /* The primary signalled event. Hot potatoes are passed until this is set. */ - HANDLE event; + HANDLE event; /* the head of the queue; NULL if queue is empty */ - struct QueueEntry* head; + struct QueueEntry *head; /* equivalent to the queue length */ - int waiters; + int waiters; /* how many waiters in the queue have been conceptually wakened by signals (even if we haven't managed to actually wake them yet */ - int wakens; + int wakens; #else pthread_cond_t cond; @@ -189,7 +189,7 @@ error: * @thread : pointer to thread object * * Detach a thread. When a detached thread terminates, its - * resource sare automatically released back to the system + * resources are automatically released back to the system * without the need for another thread to join with the * terminated thread. * @@ -351,15 +351,17 @@ scond_t *scond_new(void) #ifdef USE_WIN32_THREADS /* This is very complex because recreating condition variable semantics with win32 parts is not easy */ - /* The main problem is that a condition variable can be used to wake up a thread, but only if the thread is already waiting; */ - /* whereas a win32 event will 'wake up' a thread in advance (the event will be set in advance, so a 'waiter' wont even have to wait on it) */ + /* The main problem is that a condition variable can't be used to "pre-wake" a thread (it will get wakened only after it's waited) */ + /* whereas a win32 event can pre-wake a thread (the event will be set in advance, so a 'waiter' won't even have to wait on it) */ + /* Keep in mind a condition variable can apparently pre-wake a thread, insofar as spurious wakeups are always possible, */ + /* but nobody will be expecting this and it does not need to be simulated. */ /* So at the very least, we need to do something clever. But there's bigger problems. */ /* We don't even have a straightforward way in win32 to satisfy pthread_cond_wait's atomicity requirement. The bulk of this algorithm is solving that. */ /* Note: We might could simplify this using vista+ condition variables, but we wanted an XP compatible solution. */ cond->event = CreateEvent(NULL, FALSE, FALSE, NULL); - if(!cond->event) goto error; + if (!cond->event) goto error; cond->hot_potato = CreateEvent(NULL, FALSE, FALSE, NULL); - if(!cond->hot_potato) + if (!cond->hot_potato) { CloseHandle(cond->event); goto error; @@ -410,9 +412,11 @@ void scond_wait(scond_t *cond, slock_t *lock) { #ifdef USE_WIN32_THREADS + /* Reminder: `lock` is held before this is called */ + /* add ourselves to a queue of waiting threads */ struct QueueEntry myentry; - struct QueueEntry** ptr = &cond->head; + struct QueueEntry **ptr = &cond->head; while(*ptr) /* walk to the end of the linked list */ ptr = &((*ptr)->next); *ptr = &myentry; @@ -431,18 +435,36 @@ void scond_wait(scond_t *cond, slock_t *lock) /* It's my turn if I'm the head of the queue. Check to see if it's my turn. */ while (cond->head != &myentry) { + /* It isn't my turn: */ + /* As long as someone is even going to be able to wake up when they receive the potato, keep it going round */ if (cond->wakens > 0) SetEvent(cond->hot_potato); - /* Wait to catch the hot potato before checking for my turn again */ - SignalObjectAndWait(lock->lock, cond->hot_potato, INFINITE, FALSE); - slock_lock(lock); + /* Let someone else go */ + ReleaseMutex(lock->lock); + + /* Wait a while to catch the hot potato.. someone else should get a chance to go */ + /* After all, it isn't my turn (and it must be someone else's */ + Sleep(0); + WaitForSingleObject(cond->hot_potato, INFINITE); + + /* I should come out of here with the main lock taken */ + WaitForSingleObject(lock->lock, INFINITE); } - + /* It's my turn now -- I hold the potato */ - SignalObjectAndWait(lock->lock, cond->event, INFINITE, FALSE); - slock_lock(lock); + + /* I still have the main lock, in any case */ + /* I need to release it so that someone can set the event */ + ReleaseMutex(lock->lock); + + /* Wait for someone to actually signal this condition */ + /* We're the only waiter waiting on the event right now -- everyone else is waiting on something different */ + WaitForSingleObject(cond->event, INFINITE); + + /* Take the main lock so we can do work. Nobody else waits on this lock for very long, so even though it's GO TIME we won't have to wait long */ + WaitForSingleObject(lock->lock, INFINITE); /* Remove ourselves from the queue */ cond->head = myentry.next; @@ -451,7 +473,7 @@ void scond_wait(scond_t *cond, slock_t *lock) /* If any other wakenings are pending, go ahead and set it up */ /* There may actually be no waiters. That's OK. The first waiter will come in, find it's his turn, and immediately get the signaled event */ cond->wakens--; - if(cond->wakens>0) + if (cond->wakens > 0) { SetEvent(cond->event); From 3e8e02c9538be27330766bb05b08aa27ef932012 Mon Sep 17 00:00:00 2001 From: zeromus Date: Sat, 21 Jan 2017 17:09:38 -0600 Subject: [PATCH 3/6] change the main lock mutex to a critical section, and update copyrights year --- libretro-common/rthreads/rthreads.c | 30 ++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/libretro-common/rthreads/rthreads.c b/libretro-common/rthreads/rthreads.c index e9a12a8b14..fc6f14743c 100644 --- a/libretro-common/rthreads/rthreads.c +++ b/libretro-common/rthreads/rthreads.c @@ -1,4 +1,4 @@ -/* Copyright (C) 2010-2016 The RetroArch team +/* Copyright (C) 2010-2017 The RetroArch team * * --------------------------------------------------------------------------------------- * The following license statement only applies to this file (rthreads.c). @@ -78,7 +78,7 @@ struct sthread struct slock { #ifdef USE_WIN32_THREADS - HANDLE lock; + CRITICAL_SECTION lock; #else pthread_mutex_t lock; #endif @@ -262,10 +262,10 @@ slock_t *slock_new(void) return NULL; #ifdef USE_WIN32_THREADS - lock->lock = CreateMutex(NULL, FALSE, NULL); - mutex_created = !!lock->lock; + InitializeCriticalSection(&lock->lock); + mutex_created = true; #else - mutex_created = (pthread_mutex_init(&lock->lock, NULL) == 0); + mutex_created = (pthread_mutex_init(&lock->lock, NULL) == 0); #endif if (!mutex_created) @@ -290,7 +290,7 @@ void slock_free(slock_t *lock) return; #ifdef USE_WIN32_THREADS - CloseHandle(lock->lock); + DeleteCriticalSection(&lock->lock); #else pthread_mutex_destroy(&lock->lock); #endif @@ -310,7 +310,7 @@ void slock_lock(slock_t *lock) if (!lock) return; #ifdef USE_WIN32_THREADS - WaitForSingleObject(lock->lock, INFINITE); + EnterCriticalSection(&lock->lock); #else pthread_mutex_lock(&lock->lock); #endif @@ -327,7 +327,7 @@ void slock_unlock(slock_t *lock) if (!lock) return; #ifdef USE_WIN32_THREADS - ReleaseMutex(lock->lock); + LeaveCriticalSection(&lock->lock); #else pthread_mutex_unlock(&lock->lock); #endif @@ -442,7 +442,7 @@ void scond_wait(scond_t *cond, slock_t *lock) SetEvent(cond->hot_potato); /* Let someone else go */ - ReleaseMutex(lock->lock); + LeaveCriticalSection(&lock->lock); /* Wait a while to catch the hot potato.. someone else should get a chance to go */ /* After all, it isn't my turn (and it must be someone else's */ @@ -450,21 +450,21 @@ void scond_wait(scond_t *cond, slock_t *lock) WaitForSingleObject(cond->hot_potato, INFINITE); /* I should come out of here with the main lock taken */ - WaitForSingleObject(lock->lock, INFINITE); + EnterCriticalSection(&lock->lock); } /* It's my turn now -- I hold the potato */ /* I still have the main lock, in any case */ /* I need to release it so that someone can set the event */ - ReleaseMutex(lock->lock); + LeaveCriticalSection(&lock->lock); /* Wait for someone to actually signal this condition */ /* We're the only waiter waiting on the event right now -- everyone else is waiting on something different */ WaitForSingleObject(cond->event, INFINITE); /* Take the main lock so we can do work. Nobody else waits on this lock for very long, so even though it's GO TIME we won't have to wait long */ - WaitForSingleObject(lock->lock, INFINITE); + EnterCriticalSection(&lock->lock); /* Remove ourselves from the queue */ cond->head = myentry.next; @@ -558,10 +558,10 @@ bool scond_wait_timeout(scond_t *cond, slock_t *lock, int64_t timeout_us) /* TODO: this is woefully inadequate. It needs to be solved with the newer approach used above */ WaitForSingleObject(cond->event, 0); - ret = SignalObjectAndWait(lock->lock, cond->event, - (DWORD)(timeout_us) / 1000, FALSE); + LeaveCriticalSection(&lock->lock); + ret = WaitForSingleObject(cond->event,(DWORD)(timeout_us) / 1000); - slock_lock(lock); + EnterCriticalSection(&lock->lock); return ret == WAIT_OBJECT_0; #else int ret; From 168de31fb9d5d4c3caba6099a5986f64f316e359 Mon Sep 17 00:00:00 2001 From: zeromus Date: Sat, 21 Jan 2017 22:44:31 -0600 Subject: [PATCH 4/6] make scond_signal slightly more pthreads compliant (and other tidying). re: PR #4392 --- libretro-common/rthreads/rthreads.c | 47 +++++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/libretro-common/rthreads/rthreads.c b/libretro-common/rthreads/rthreads.c index fc6f14743c..e6ae676625 100644 --- a/libretro-common/rthreads/rthreads.c +++ b/libretro-common/rthreads/rthreads.c @@ -113,6 +113,9 @@ struct scond /* how many waiters in the queue have been conceptually wakened by signals (even if we haven't managed to actually wake them yet */ int wakens; + /* used to control access to this scond, in case the user fails */ + CRITICAL_SECTION cs; + #else pthread_cond_t cond; #endif @@ -350,11 +353,14 @@ scond_t *scond_new(void) return NULL; #ifdef USE_WIN32_THREADS + /* This is very complex because recreating condition variable semantics with win32 parts is not easy */ /* The main problem is that a condition variable can't be used to "pre-wake" a thread (it will get wakened only after it's waited) */ /* whereas a win32 event can pre-wake a thread (the event will be set in advance, so a 'waiter' won't even have to wait on it) */ /* Keep in mind a condition variable can apparently pre-wake a thread, insofar as spurious wakeups are always possible, */ /* but nobody will be expecting this and it does not need to be simulated. */ + /* Moreover, we won't be doing this, because it counts as a spurious wakeup -- someone else with a genuine claim must get wakened, in any case. + /* Therefore we choose to wake only one of the correct waiting threads. */ /* So at the very least, we need to do something clever. But there's bigger problems. */ /* We don't even have a straightforward way in win32 to satisfy pthread_cond_wait's atomicity requirement. The bulk of this algorithm is solving that. */ /* Note: We might could simplify this using vista+ condition variables, but we wanted an XP compatible solution. */ @@ -366,6 +372,8 @@ scond_t *scond_new(void) CloseHandle(cond->event); goto error; } + + InitializeCriticalSection(&cond->cs); cond->waiters = cond->wakens = 0; cond->head = NULL; @@ -395,6 +403,7 @@ void scond_free(scond_t *cond) #ifdef USE_WIN32_THREADS CloseHandle(cond->event); CloseHandle(cond->hot_potato); + DeleteCriticalSection(&cond->cs); #else pthread_cond_destroy(&cond->cond); #endif @@ -412,11 +421,15 @@ void scond_wait(scond_t *cond, slock_t *lock) { #ifdef USE_WIN32_THREADS - /* Reminder: `lock` is held before this is called */ + struct QueueEntry myentry; + struct QueueEntry **ptr; + + /* Reminder: `lock` is held before this is called. */ + /* however, someone else may have called scond_signal without the lock. soo... */ + EnterCriticalSection(&cond->cs); /* add ourselves to a queue of waiting threads */ - struct QueueEntry myentry; - struct QueueEntry **ptr = &cond->head; + ptr = &cond->head; while(*ptr) /* walk to the end of the linked list */ ptr = &((*ptr)->next); *ptr = &myentry; @@ -443,14 +456,16 @@ void scond_wait(scond_t *cond, slock_t *lock) /* Let someone else go */ LeaveCriticalSection(&lock->lock); + LeaveCriticalSection(&cond->cs); /* Wait a while to catch the hot potato.. someone else should get a chance to go */ - /* After all, it isn't my turn (and it must be someone else's */ + /* After all, it isn't my turn (and it must be someone else's) */ Sleep(0); WaitForSingleObject(cond->hot_potato, INFINITE); /* I should come out of here with the main lock taken */ EnterCriticalSection(&lock->lock); + EnterCriticalSection(&cond->cs); } /* It's my turn now -- I hold the potato */ @@ -458,6 +473,7 @@ void scond_wait(scond_t *cond, slock_t *lock) /* I still have the main lock, in any case */ /* I need to release it so that someone can set the event */ LeaveCriticalSection(&lock->lock); + LeaveCriticalSection(&cond->cs); /* Wait for someone to actually signal this condition */ /* We're the only waiter waiting on the event right now -- everyone else is waiting on something different */ @@ -465,6 +481,7 @@ void scond_wait(scond_t *cond, slock_t *lock) /* Take the main lock so we can do work. Nobody else waits on this lock for very long, so even though it's GO TIME we won't have to wait long */ EnterCriticalSection(&lock->lock); + EnterCriticalSection(&cond->cs); /* Remove ourselves from the queue */ cond->head = myentry.next; @@ -481,6 +498,8 @@ void scond_wait(scond_t *cond, slock_t *lock) SetEvent(cond->hot_potato); } + LeaveCriticalSection(&cond->cs); + #else pthread_cond_wait(&cond->cond, &lock->lock); #endif @@ -524,13 +543,29 @@ void scond_signal(scond_t *cond) { #ifdef USE_WIN32_THREADS + /* Unfortunately, pthread_cond_signal does not require that the lock be held in advance */ + /* To avoid stomping on the condvar from other threads, we need to control access to it with this */ + EnterCriticalSection(&cond->cs); + /* remember: we currently have mutex */ - if(cond->waiters == 0) return; + if(cond->waiters == 0) + { + LeaveCriticalSection(&cond->cs); + return; + } /* wake up the next thing in the queue */ if(cond->wakens == 0) SetEvent(cond->event); cond->wakens++; + /* The data structure is done being modified.. I think we can leave the CS now. */ + /* This would prevent some other thread from receiving the hot potato and then + /* immediately stalling for the critical section. */ + /* But remember, we were trying to replicate a semantic where this entire scond_signal call + /* was controlled (by the user) by a lock. */ + /* So in case there's trouble with this, we can move it after SetEvent() */ + LeaveCriticalSection(&cond->cs); + /* Since there is now at least one pending waken, the potato must be in play */ SetEvent(cond->hot_potato); @@ -556,7 +591,7 @@ bool scond_wait_timeout(scond_t *cond, slock_t *lock, int64_t timeout_us) #ifdef USE_WIN32_THREADS DWORD ret; - /* TODO: this is woefully inadequate. It needs to be solved with the newer approach used above */ + /* TODO: this is woefully inadequate. It needs to be solved with the newer approach used above. */ WaitForSingleObject(cond->event, 0); LeaveCriticalSection(&lock->lock); ret = WaitForSingleObject(cond->event,(DWORD)(timeout_us) / 1000); From 233c13228e2f1aec70c513711205c650bd6455fa Mon Sep 17 00:00:00 2001 From: zeromus Date: Fri, 3 Feb 2017 21:52:52 -0600 Subject: [PATCH 5/6] attempt win32 scond_wait_timeout --- libretro-common/rthreads/rthreads.c | 135 +++++++++++++++++++++------- 1 file changed, 103 insertions(+), 32 deletions(-) diff --git a/libretro-common/rthreads/rthreads.c b/libretro-common/rthreads/rthreads.c index e6ae676625..32a306cfb1 100644 --- a/libretro-common/rthreads/rthreads.c +++ b/libretro-common/rthreads/rthreads.c @@ -38,6 +38,7 @@ #else #define WIN32_LEAN_AND_MEAN #include +#include #endif #elif defined(GEKKO) #include "gx_pthread.h" @@ -84,17 +85,18 @@ struct slock #endif }; +#ifdef USE_WIN32_THREADS +/* The syntax we'll use is mind-bending unless we use a struct. Plus, we might want to store more info later */ +/* This will be used as a linked list immplementing a queue of waiting threads */ +struct QueueEntry +{ + struct QueueEntry *next; +}; +#endif + struct scond { #ifdef USE_WIN32_THREADS - - /* The syntax we'll use is mind-bending unless we use a struct. Plus, we might want to store more info later */ - /* This will be used as a linked list immplementing a queue of waiting threads */ - struct QueueEntry - { - struct QueueEntry *next; - }; - /* With this implementation of scond, we don't have any way of waking (or even identifying) specific threads */ /* But we need to wake them in the order indicated by the queue. */ /* This potato token will get get passed around every waiter. The bearer can test whether he's next, and hold onto the potato if he is. */ @@ -110,7 +112,7 @@ struct scond /* equivalent to the queue length */ int waiters; - /* how many waiters in the queue have been conceptually wakened by signals (even if we haven't managed to actually wake them yet */ + /* how many waiters in the queue have been conceptually wakened by signals (even if we haven't managed to actually wake them yet) */ int wakens; /* used to control access to this scond, in case the user fails */ @@ -359,7 +361,7 @@ scond_t *scond_new(void) /* whereas a win32 event can pre-wake a thread (the event will be set in advance, so a 'waiter' won't even have to wait on it) */ /* Keep in mind a condition variable can apparently pre-wake a thread, insofar as spurious wakeups are always possible, */ /* but nobody will be expecting this and it does not need to be simulated. */ - /* Moreover, we won't be doing this, because it counts as a spurious wakeup -- someone else with a genuine claim must get wakened, in any case. + /* Moreover, we won't be doing this, because it counts as a spurious wakeup -- someone else with a genuine claim must get wakened, in any case. */ /* Therefore we choose to wake only one of the correct waiting threads. */ /* So at the very least, we need to do something clever. But there's bigger problems. */ /* We don't even have a straightforward way in win32 to satisfy pthread_cond_wait's atomicity requirement. The bulk of this algorithm is solving that. */ @@ -410,24 +412,33 @@ void scond_free(scond_t *cond) free(cond); } -/** - * scond_wait: - * @cond : pointer to condition variable object - * @lock : pointer to mutex object - * - * Block on a condition variable (i.e. wait on a condition). - **/ -void scond_wait(scond_t *cond, slock_t *lock) -{ #ifdef USE_WIN32_THREADS - +static bool _scond_wait_win32(scond_t *cond, slock_t *lock, DWORD dwMilliseconds) +{ + static bool beginPeriod = false; + struct QueueEntry myentry; struct QueueEntry **ptr; + DWORD tsBegin; + DWORD dwFinalTimeout = dwMilliseconds; /* Careful! in case we begin in the head, we don't do the hot potato stuff, so this timeout needs presetting */ + DWORD waitResult; /* Reminder: `lock` is held before this is called. */ /* however, someone else may have called scond_signal without the lock. soo... */ EnterCriticalSection(&cond->cs); + /* since this library is meant for realtime game software I have no problem setting this to 1 and forgetting about it. */ + if(!beginPeriod) + { + beginPeriod = true; + timeBeginPeriod(1); + } + + /* Now we can take a good timestamp for use in faking the timeout ourselves. */ + /* But don't bother unless we need to (to save a little time) */ + if(dwMilliseconds != INFINITE) + tsBegin = timeGetTime(); + /* add ourselves to a queue of waiting threads */ ptr = &cond->head; while(*ptr) /* walk to the end of the linked list */ @@ -450,10 +461,25 @@ void scond_wait(scond_t *cond, slock_t *lock) { /* It isn't my turn: */ + DWORD timeout = INFINITE; + /* As long as someone is even going to be able to wake up when they receive the potato, keep it going round */ if (cond->wakens > 0) SetEvent(cond->hot_potato); + /* Assess the remaining timeout time */ + if(dwMilliseconds != INFINITE) + { + DWORD now = timeGetTime(); + DWORD elapsed = now - tsBegin; + if(elapsed > dwMilliseconds) + { + /* Try one last time with a zero timeout (keeps the code simpler) */ + elapsed = dwMilliseconds; + } + timeout = dwMilliseconds - elapsed; + } + /* Let someone else go */ LeaveCriticalSection(&lock->lock); LeaveCriticalSection(&cond->cs); @@ -461,14 +487,39 @@ void scond_wait(scond_t *cond, slock_t *lock) /* Wait a while to catch the hot potato.. someone else should get a chance to go */ /* After all, it isn't my turn (and it must be someone else's) */ Sleep(0); - WaitForSingleObject(cond->hot_potato, INFINITE); + waitResult = WaitForSingleObject(cond->hot_potato, timeout); /* I should come out of here with the main lock taken */ EnterCriticalSection(&lock->lock); EnterCriticalSection(&cond->cs); + + if(waitResult == WAIT_TIMEOUT) + { + /* Out of time! Now, let's think about this. I do have the potato now--maybe it's my turn, and I have the event? */ + /* If that's the case, I could proceed right now without aborting due to timeout. */ + /* However.. I DID wait a real long time. The caller was willing to wait that long. */ + /* I choose to give him one last chance with a zero timeout in the next step */ + if(cond->head == &myentry) + { + dwFinalTimeout = 0; + break; + } + else + { + /* It's not our turn and we're out of time. Give up. */ + /* Remove ourself from the queue and bail. */ + struct QueueEntry* curr = cond->head; + while(curr->next != &myentry) curr = curr->next; + curr->next = myentry.next; + cond->waiters--; + LeaveCriticalSection(&cond->cs); + return false; + } + } + } - /* It's my turn now -- I hold the potato */ + /* It's my turn now -- and I hold the potato */ /* I still have the main lock, in any case */ /* I need to release it so that someone can set the event */ @@ -477,7 +528,7 @@ void scond_wait(scond_t *cond, slock_t *lock) /* Wait for someone to actually signal this condition */ /* We're the only waiter waiting on the event right now -- everyone else is waiting on something different */ - WaitForSingleObject(cond->event, INFINITE); + waitResult = WaitForSingleObject(cond->event, dwFinalTimeout); /* Take the main lock so we can do work. Nobody else waits on this lock for very long, so even though it's GO TIME we won't have to wait long */ EnterCriticalSection(&lock->lock); @@ -487,8 +538,16 @@ void scond_wait(scond_t *cond, slock_t *lock) cond->head = myentry.next; cond->waiters--; + if(waitResult == WAIT_TIMEOUT) + { + /* Oops! ran out of time in the final wait. Just bail. */ + LeaveCriticalSection(&cond->cs); + return false; + } + /* If any other wakenings are pending, go ahead and set it up */ /* There may actually be no waiters. That's OK. The first waiter will come in, find it's his turn, and immediately get the signaled event */ + printf("%d\n",cond->wakens); cond->wakens--; if (cond->wakens > 0) { @@ -499,7 +558,21 @@ void scond_wait(scond_t *cond, slock_t *lock) } LeaveCriticalSection(&cond->cs); + return true; +} +#endif +/** + * scond_wait: + * @cond : pointer to condition variable object + * @lock : pointer to mutex object + * + * Block on a condition variable (i.e. wait on a condition). + **/ +void scond_wait(scond_t *cond, slock_t *lock) +{ +#ifdef USE_WIN32_THREADS + _scond_wait_win32(cond, lock, INFINITE); #else pthread_cond_wait(&cond->cond, &lock->lock); #endif @@ -589,15 +662,13 @@ void scond_signal(scond_t *cond) bool scond_wait_timeout(scond_t *cond, slock_t *lock, int64_t timeout_us) { #ifdef USE_WIN32_THREADS - DWORD ret; - - /* TODO: this is woefully inadequate. It needs to be solved with the newer approach used above. */ - WaitForSingleObject(cond->event, 0); - LeaveCriticalSection(&lock->lock); - ret = WaitForSingleObject(cond->event,(DWORD)(timeout_us) / 1000); - - EnterCriticalSection(&lock->lock); - return ret == WAIT_OBJECT_0; + /* How to convert a us timeout to ms? */ + /* Someone asking for a 0 timeout clearly wants no timeout. */ + /* Someone asking for a 1 timeout clearly wants an actual timeout of the minimum length */ + /* Someone asking for 1000 or 1001 timeout shouldn't accidentally get 2ms. */ + DWORD dwMilliseconds = timeout_us/1000; + if(timeout_us < 1000) dwMilliseconds = 1; + return _scond_wait_win32(cond,lock,dwMilliseconds); #else int ret; int64_t seconds, remainder; From 0ef09a0ed46d87c137f8b18c3cc278e2e46479d4 Mon Sep 17 00:00:00 2001 From: zeromus Date: Sat, 4 Feb 2017 16:24:14 -0600 Subject: [PATCH 6/6] win32 scond_wait_timeout: refine 0 timeout logic --- libretro-common/rthreads/rthreads.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/libretro-common/rthreads/rthreads.c b/libretro-common/rthreads/rthreads.c index 32a306cfb1..c61ca1096a 100644 --- a/libretro-common/rthreads/rthreads.c +++ b/libretro-common/rthreads/rthreads.c @@ -547,7 +547,6 @@ static bool _scond_wait_win32(scond_t *cond, slock_t *lock, DWORD dwMilliseconds /* If any other wakenings are pending, go ahead and set it up */ /* There may actually be no waiters. That's OK. The first waiter will come in, find it's his turn, and immediately get the signaled event */ - printf("%d\n",cond->wakens); cond->wakens--; if (cond->wakens > 0) { @@ -663,11 +662,18 @@ bool scond_wait_timeout(scond_t *cond, slock_t *lock, int64_t timeout_us) { #ifdef USE_WIN32_THREADS /* How to convert a us timeout to ms? */ - /* Someone asking for a 0 timeout clearly wants no timeout. */ + /* Someone asking for a 0 timeout clearly wants immediate timeout. */ /* Someone asking for a 1 timeout clearly wants an actual timeout of the minimum length */ /* Someone asking for 1000 or 1001 timeout shouldn't accidentally get 2ms. */ DWORD dwMilliseconds = timeout_us/1000; - if(timeout_us < 1000) dwMilliseconds = 1; + if(timeout_us == 0) { + /* The implementation of a 0 timeout here with pthreads is sketchy. */ + /* It isn't clear what happens if pthread_cond_timedwait is called with NOW. */ + /* Moreover, it is possible that this thread gets pre-empted after the clock_gettime but before the pthread_cond_timedwait. */ + /* In order to help smoke out problems caused by this strange usage, let's treat a 0 timeout as always timing out. */ + return false; + } + else if(timeout_us < 1000) dwMilliseconds = 1; return _scond_wait_win32(cond,lock,dwMilliseconds); #else int ret;