From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Tue, 25 Oct 2011 15:42:53 GMT
Subject: Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER
Message-ID: <CABPQNSZ8wesy-px-n1LYbVwFT3gBNcrHfe+_553sinTferqsog@mail.gmail.com>
In-Reply-To: <4EA6D594.90402@viscovery.net>

On Tue, Oct 25, 2011 at 5:28 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 10/25/2011 16:55, schrieb Erik Faye-Lund:
>> +int pthread_mutex_lock(pthread_mutex_t *mutex)
>> +{
>> +     if (mutex->autoinit) {
>> +             if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {
>> +                     pthread_mutex_init(mutex, NULL);
>> +                     mutex->autoinit = 0;
>> +             } else
>> +                     while (mutex->autoinit != 0)
>> +                             ; /* wait for other thread */
>> +     }
>
> The double-checked locking idiom. Very suspicious. Can you explain why it
> works in this case? Why are no Interlocked functions needed for the other
> accesses of autoinit? ("It is volatile" is the wrong answer to this last
> question, BTW.)

I agree that it should look a bit suspicious; I'm generally skeptical
whenever I see 'volatile' in threading-code myself. But I think it's
the right answer in this case. "volatile" means that the compiler
cannot optimize away accesses, which is sufficient in this case.

Basically, the thread that gets the original 1 returned from
InterlockedCompareExchange is the only one who writes to
mutex->autoinit. All other threads only read the value, and the
volatile should make sure they actually do. Since all 32-bit reads and
writes are atomic on Windows (see
http://msdn.microsoft.com/en-us/library/windows/desktop/ms684122(v=vs.85).aspx
"Simple reads and writes to properly-aligned 32-bit variables are
atomic operations.") and mutex->autoinit is a LONG, this should be
safe AFAICT. In fact, Windows specifically does not have any
explicitly atomic writes exactly for this reason.

The only ways mutex->autoinit can be updated is:
- InterlockedCompareExchange compares it to 1, finds it's identical
and inserts -1
- intialization is done
Both these updates happens from the same thread.

Yes, details like this should probably go into the commit message ;)

If there's something I'm missing, please let me know.