Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API

[Erik Faye-Lund <kusmabite@googlemail.com>]

On Fri, Jan 8, 2010 at 4:32 AM, Dmitry Potapov <dpotapov@gmail.com> wrote:
> On Thu, Jan 07, 2010 at 10:54:57PM +0100, Johannes Sixt wrote:
>> +
>> +int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)
>> +{
>> +     /* serialize access to waiters count */
>> +     EnterCriticalSection(&cond->waiters_lock);
>> +     ++cond->waiters;
>> +     LeaveCriticalSection(&cond->waiters_lock);
>
> InterlockedIncrement(&cond->waiters);
>
>> +
>> +     /*
>> +      * Unlock external mutex and wait for signal.
>> +      * NOTE: we've held mutex locked long enough to increment
>> +      * waiters count above, so there's no problem with
>> +      * leaving mutex unlocked before we wait on semaphore.
>> +      */
>> +     LeaveCriticalSection(mutex);
>> +
>> +     /* let's wait - ignore return value */
>> +     WaitForSingleObject(cond->sema, INFINITE);
>> +
>> +     /* we're done waiting, so make sure we decrease waiters count */
>> +     EnterCriticalSection(&cond->waiters_lock);
>> +     --cond->waiters;
>> +     LeaveCriticalSection(&cond->waiters_lock);
>
> InterlockedDecrement(&cond->waiters);

Nice.

>> +
>> +     /* lock external mutex again */
>> +     EnterCriticalSection(mutex);
>> +
>> +     return 0;
>> +}
>> +
>> +int pthread_cond_signal(pthread_cond_t *cond)
>> +{
>> +     int have_waiters;
>> +
>> +     /* serialize access to waiters count */
>> +     EnterCriticalSection(&cond->waiters_lock);
>> +     have_waiters = cond->waiters > 0;
>> +     LeaveCriticalSection(&cond->waiters_lock);
>
> AFAIK, Win32 API assumes that reading LONG is always atomic, so
> the critical section is not really necesary here, but you need
> to declare 'waiters' as 'volatile':

"Simple reads and writes to properly-aligned 32-bit variables are
atomic operations."
http://msdn.microsoft.com/en-us/library/ms684122(VS.85).aspx

In other words: Yes, you are right.

>> +
>> +int pthread_cond_init(pthread_cond_t *cond, const void *unused)
>> +{
>> +     cond->waiters = 0;
>> +
>> +     InitializeCriticalSection(&cond->waiters_lock);
>
> Is waiters_lock really necessary?

(Yeah, I moved this one to the end)

No, I think you've proven that it isn't.

-- 
Erik "kusma" Faye-Lund