Re: [msysGit] Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER

> On Tue, Oct 25, 2011 at 10:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> Am 25.10.2011 17:42, schrieb Erik Faye-Lund:
>>> 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.
>>
>> No, it is not, and it took me a train ride to see what's wrong. It has
>> nothing to do with autoinit, but with all the other memory locations
>> that are written. See here, with pthread_mutex_init() inlined:
>>
>>  if (mutex->autoinit) {
>>
>> Assume two threads enter this block.
>>
>>     if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {
>>
>> Only one thread, A, say on CPU A, will enter this block.
>>
>>        InitializeCriticalSection(&mutex->cs);
>>
>> Thread A writes some values. Note that there are no memory barriers
>> involved here. Not that I know of or that they would be documented.
>>
>>        mutex->autoinit = 0;
>>
>> And it writes another one. Thread A continues below to contend for the
>> mutex it just initialized.
>>
>>     } else
>>
>> Meanwhile, thread B, say on CPU B, spins in this loop:
>>
>>        while (mutex->autoinit != 0)
>>           ; /* wait for other thread */
>>
>> When thread B arrives here, it sees the value of autoinit that thread A
>> has written above.
>>
>> HOWEVER, when it continues, there is NO [*] guarantee that it will also
>> see the values that InitializeCriticalSection() has written, because
>> there were no memory barriers involved. When it continues, there is a
>> chance that it calls EnterCriticalSection() with uninitialized values!
>>
>
> Thanks for pointing this out, I completely forgot about write re-ordering.
>
> This is indeed a problem. So, shouldn't replacing "mutex->autoinit =
> 0;" with "InterlockedExchange(&mutex->autoinit, 0)" solve the problem?
> InterlockedExchange generates a full memory barrier:
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms683590(v=vs.85).aspx