{"thread":{"id":"28763","subject":"[PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER","startedAt":"2011-10-25T14:55:09Z","lastAt":"2011-10-28T18:35:46Z","messageCount":13,"participants":["Erik Faye-Lund","Johannes Sixt","Kyle Moffett","Atsushi Nakagawa"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"178268","messageId":"1319554509-6532-1-git-send-email-kusmabite@gmail.com","threadId":"28763","inReplyTo":null,"subject":"[PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-10-25T14:55:09Z","receivedAt":"2011-10-25T14:55:09Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n---\nEvery now and then, someone suggest using PTHREAD_MUTEX_INITIALIZER,\nbut some times gets show down by our lack of support on Windows.\n\nI'm working on something myself that could benefit from this, so I\ngave it a stab. The result looks promising to me, but I haven't\nreally debugged it yet.\n\nBut is there a fundamental reason why we haven't done something like\nthis before? :)\n\n compat/win32/pthread.c |   28 +++++++++++++++++++++++++---\n compat/win32/pthread.h |   18 ++++++++++++------\n 2 files changed, 37 insertions(+), 9 deletions(-)\n\ndiff --git a/compat/win32/pthread.c b/compat/win32/pthread.c\nindex 010e875..14f91d5 100644\n--- a/compat/win32/pthread.c\n+++ b/compat/win32/pthread.c\n@@ -57,6 +57,28 @@ pthread_t pthread_self(void)\n \treturn t;\n }\n \n+int pthread_mutex_init(pthread_mutex_t *mutex, const pthread_mutexattr_t *attr)\n+{\n+\tInitializeCriticalSection(&mutex->cs);\n+\tmutex->autoinit = 0;\n+\treturn 0;\n+}\n+\n+int pthread_mutex_lock(pthread_mutex_t *mutex)\n+{\n+\tif (mutex->autoinit) {\n+\t\tif (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {\n+\t\t\tpthread_mutex_init(mutex, NULL);\n+\t\t\tmutex->autoinit = 0;\n+\t\t} else\n+\t\t\twhile (mutex->autoinit != 0)\n+\t\t\t\t; /* wait for other thread */\n+\t}\n+\n+\tEnterCriticalSection(&mutex->cs);\n+\treturn 0;\n+}\n+\n int pthread_cond_init(pthread_cond_t *cond, const void *unused)\n {\n \tcond->waiters = 0;\n@@ -85,7 +107,7 @@ int pthread_cond_destroy(pthread_cond_t *cond)\n \treturn 0;\n }\n \n-int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)\n+int pthread_cond_wait(pthread_cond_t *cond, pthread_mutex_t *mutex)\n {\n \tint last_waiter;\n \n@@ -99,7 +121,7 @@ int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)\n \t * waiters count above, so there's no problem with\n \t * leaving mutex unlocked before we wait on semaphore.\n \t */\n-\tLeaveCriticalSection(mutex);\n+\tLeaveCriticalSection(&mutex->cs);\n \n \t/* let's wait - ignore return value */\n \tWaitForSingleObject(cond->sema, INFINITE);\n@@ -133,7 +155,7 @@ int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)\n \t\t */\n \t}\n \t/* lock external mutex again */\n-\tEnterCriticalSection(mutex);\n+\tEnterCriticalSection(&mutex->cs);\n \n \treturn 0;\n }\ndiff --git a/compat/win32/pthread.h b/compat/win32/pthread.h\nindex 2e20548..647e6d4 100644\n--- a/compat/win32/pthread.h\n+++ b/compat/win32/pthread.h\n@@ -16,14 +16,20 @@\n /*\n  * Defines that adapt Windows API threads to pthreads API\n  */\n-#define pthread_mutex_t CRITICAL_SECTION\n+typedef struct {\n+\tCRITICAL_SECTION cs;\n+\tLONG volatile autoinit;\n+} pthread_mutex_t;\n \n-#define pthread_mutex_init(a,b) (InitializeCriticalSection((a)), 0)\n-#define pthread_mutex_destroy(a) DeleteCriticalSection((a))\n-#define pthread_mutex_lock EnterCriticalSection\n-#define pthread_mutex_unlock LeaveCriticalSection\n+#define PTHREAD_MUTEX_INITIALIZER { { 0 }, 1 }\n \n typedef int pthread_mutexattr_t;\n+\n+int pthread_mutex_init(pthread_mutex_t *, const pthread_mutexattr_t *);\n+#define pthread_mutex_destroy(a) DeleteCriticalSection(&(a)->cs)\n+int pthread_mutex_lock(pthread_mutex_t *);\n+#define pthread_mutex_unlock(a) LeaveCriticalSection(&(a)->cs)\n+\n #define pthread_mutexattr_init(a) (*(a) = 0)\n #define pthread_mutexattr_destroy(a) do {} while (0)\n #define pthread_mutexattr_settype(a, t) 0\n@@ -47,7 +53,7 @@ typedef struct {\n \n extern int pthread_cond_init(pthread_cond_t *cond, const void *unused);\n extern int pthread_cond_destroy(pthread_cond_t *cond);\n-extern int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex);\n+extern int pthread_cond_wait(pthread_cond_t *cond, pthread_mutex_t *mutex);\n extern int pthread_cond_signal(pthread_cond_t *cond);\n extern int pthread_cond_broadcast(pthread_cond_t *cond);\n \n-- \n1.7.7.msysgit.1.1.g7b316\n"},{"id":"178270","messageId":"4EA6D594.90402@viscovery.net","threadId":"28763","inReplyTo":"1319554509-6532-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-10-25T15:28:20Z","receivedAt":"2011-10-25T15:28:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/25/2011 16:55, schrieb Erik Faye-Lund:\n> +int pthread_mutex_lock(pthread_mutex_t *mutex)\n> +{\n> +\tif (mutex->autoinit) {\n> +\t\tif (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {\n> +\t\t\tpthread_mutex_init(mutex, NULL);\n> +\t\t\tmutex->autoinit = 0;\n> +\t\t} else\n> +\t\t\twhile (mutex->autoinit != 0)\n> +\t\t\t\t; /* wait for other thread */\n> +\t}\n\nThe double-checked locking idiom. Very suspicious. Can you explain why it\nworks in this case? Why are no Interlocked functions needed for the other\naccesses of autoinit? (\"It is volatile\" is the wrong answer to this last\nquestion, BTW.)\n\n-- Hannes\n"},{"id":"178273","messageId":"CABPQNSZ8wesy-px-n1LYbVwFT3gBNcrHfe+_553sinTferqsog@mail.gmail.com","threadId":"28763","inReplyTo":"4EA6D594.90402@viscovery.net","subject":"Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-10-25T15:42:53Z","receivedAt":"2011-10-25T15:42:53Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Oct 25, 2011 at 5:28 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Am 10/25/2011 16:55, schrieb Erik Faye-Lund:\n>> +int pthread_mutex_lock(pthread_mutex_t *mutex)\n>> +{\n>> +     if (mutex->autoinit) {\n>> +             if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {\n>> +                     pthread_mutex_init(mutex, NULL);\n>> +                     mutex->autoinit = 0;\n>> +             } else\n>> +                     while (mutex->autoinit != 0)\n>> +                             ; /* wait for other thread */\n>> +     }\n>\n> The double-checked locking idiom. Very suspicious. Can you explain why it\n> works in this case? Why are no Interlocked functions needed for the other\n> accesses of autoinit? (\"It is volatile\" is the wrong answer to this last\n> question, BTW.)\n\nI agree that it should look a bit suspicious; I'm generally skeptical\nwhenever I see 'volatile' in threading-code myself. But I think it's\nthe right answer in this case. \"volatile\" means that the compiler\ncannot optimize away accesses, which is sufficient in this case.\n\nBasically, the thread that gets the original 1 returned from\nInterlockedCompareExchange is the only one who writes to\nmutex->autoinit. All other threads only read the value, and the\nvolatile should make sure they actually do. Since all 32-bit reads and\nwrites are atomic on Windows (see\nhttp://msdn.microsoft.com/en-us/library/windows/desktop/ms684122(v=vs.85).aspx\n\"Simple reads and writes to properly-aligned 32-bit variables are\natomic operations.\") and mutex->autoinit is a LONG, this should be\nsafe AFAICT. In fact, Windows specifically does not have any\nexplicitly atomic writes exactly for this reason.\n\nThe only ways mutex->autoinit can be updated is:\n- InterlockedCompareExchange compares it to 1, finds it's identical\nand inserts -1\n- intialization is done\nBoth these updates happens from the same thread.\n\nYes, details like this should probably go into the commit message ;)\n\nIf there's something I'm missing, please let me know.\n"},{"id":"178294","messageId":"4EA716FC.2010804@kdbg.org","threadId":"28763","inReplyTo":"CABPQNSZ8wesy-px-n1LYbVwFT3gBNcrHfe+_553sinTferqsog@mail.gmail.com","subject":"Re: [msysGit] Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-10-25T20:07:24Z","receivedAt":"2011-10-25T20:07:24Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 25.10.2011 17:42, schrieb Erik Faye-Lund:\n> On Tue, Oct 25, 2011 at 5:28 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n>> Am 10/25/2011 16:55, schrieb Erik Faye-Lund:\n>>> +int pthread_mutex_lock(pthread_mutex_t *mutex)\n>>> +{\n>>> +     if (mutex->autoinit) {\n>>> +             if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {\n>>> +                     pthread_mutex_init(mutex, NULL);\n>>> +                     mutex->autoinit = 0;\n>>> +             } else\n>>> +                     while (mutex->autoinit != 0)\n>>> +                             ; /* wait for other thread */\n>>> +     }\n>>\n>> The double-checked locking idiom. Very suspicious. Can you explain why it\n>> works in this case? Why are no Interlocked functions needed for the other\n>> accesses of autoinit? (\"It is volatile\" is the wrong answer to this last\n>> question, BTW.)\n> \n> I agree that it should look a bit suspicious; I'm generally skeptical\n> whenever I see 'volatile' in threading-code myself. But I think it's\n> the right answer in this case. \"volatile\" means that the compiler\n> cannot optimize away accesses, which is sufficient in this case.\n\nNo, it is not, and it took me a train ride to see what's wrong. It has\nnothing to do with autoinit, but with all the other memory locations\nthat are written. See here, with pthread_mutex_init() inlined:\n\n  if (mutex->autoinit) {\n\nAssume two threads enter this block.\n\n     if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {\n\nOnly one thread, A, say on CPU A, will enter this block.\n\n        InitializeCriticalSection(&mutex->cs);\n\nThread A writes some values. Note that there are no memory barriers\ninvolved here. Not that I know of or that they would be documented.\n\n        mutex->autoinit = 0;\n\nAnd it writes another one. Thread A continues below to contend for the\nmutex it just initialized.\n\n     } else\n\nMeanwhile, thread B, say on CPU B, spins in this loop:\n\n        while (mutex->autoinit != 0)\n           ; /* wait for other thread */\n\nWhen thread B arrives here, it sees the value of autoinit that thread A\nhas written above.\n\nHOWEVER, when it continues, there is NO [*] guarantee that it will also\nsee the values that InitializeCriticalSection() has written, because\nthere were no memory barriers involved. When it continues, there is a\nchance that it calls EnterCriticalSection() with uninitialized values!\n\n  }\n\n\n[*] If you compile this code with MSVC >= 2005, \"No guarantee\" is not\ntrue, it's exactly the opposite because Microsoft extended the meaning\nof 'volatile' to imply a memory barriere. This is *NOT* true for gcc in\ngeneral. It may be true for MinGW gcc, but I do not know.\n\n> Basically, the thread that gets the original 1 returned from\n> InterlockedCompareExchange is the only one who writes to\n> mutex->autoinit. All other threads only read the value, and the\n> volatile should make sure they actually do. Since all 32-bit reads and\n> writes are atomic on Windows (see\n> http://msdn.microsoft.com/en-us/library/windows/desktop/ms684122(v=vs.85).aspx\n> \"Simple reads and writes to properly-aligned 32-bit variables are\n> atomic operations.\") and mutex->autoinit is a LONG, this should be\n> safe AFAICT. In fact, Windows specifically does not have any\n> explicitly atomic writes exactly for this reason.\n\nThere is a difference between atomic and coherent: Yes, 32-bit accesses\nare atomic, but they are not automatically coherent: A 32-bit value\nwritten by one CPU is not instantly visible on the other CPU. 'volatile'\nas per the C lanugage does not add any guarantees that would be of\ninterest here. OTOH, Microsoft's definition of 'volatile' does.\n\n> The only ways mutex->autoinit can be updated is:\n> - InterlockedCompareExchange compares it to 1, finds it's identical\n> and inserts -1\n> - intialization is done\n> Both these updates happens from the same thread.\n> \n> Yes, details like this should probably go into the commit message ;)\n\nA comment in the function is preferred!\n\n-- Hannes\n"},{"id":"178297","messageId":"CABPQNSY6-j7iNagsJc3WKVZ94=yZHdfBswA-v0XY7vH+RxyjYQ@mail.gmail.com","threadId":"28763","inReplyTo":"4EA716FC.2010804@kdbg.org","subject":"Re: [msysGit] Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-10-25T20:51:19Z","receivedAt":"2011-10-25T20:51:19Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Oct 25, 2011 at 10:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n> Am 25.10.2011 17:42, schrieb Erik Faye-Lund:\n>> On Tue, Oct 25, 2011 at 5:28 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n>>> Am 10/25/2011 16:55, schrieb Erik Faye-Lund:\n>>>> +int pthread_mutex_lock(pthread_mutex_t *mutex)\n>>>> +{\n>>>> +     if (mutex->autoinit) {\n>>>> +             if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {\n>>>> +                     pthread_mutex_init(mutex, NULL);\n>>>> +                     mutex->autoinit = 0;\n>>>> +             } else\n>>>> +                     while (mutex->autoinit != 0)\n>>>> +                             ; /* wait for other thread */\n>>>> +     }\n>>>\n>>> The double-checked locking idiom. Very suspicious. Can you explain why it\n>>> works in this case? Why are no Interlocked functions needed for the other\n>>> accesses of autoinit? (\"It is volatile\" is the wrong answer to this last\n>>> question, BTW.)\n>>\n>> I agree that it should look a bit suspicious; I'm generally skeptical\n>> whenever I see 'volatile' in threading-code myself. But I think it's\n>> the right answer in this case. \"volatile\" means that the compiler\n>> cannot optimize away accesses, which is sufficient in this case.\n>\n> No, it is not, and it took me a train ride to see what's wrong. It has\n> nothing to do with autoinit, but with all the other memory locations\n> that are written. See here, with pthread_mutex_init() inlined:\n>\n>  if (mutex->autoinit) {\n>\n> Assume two threads enter this block.\n>\n>     if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {\n>\n> Only one thread, A, say on CPU A, will enter this block.\n>\n>        InitializeCriticalSection(&mutex->cs);\n>\n> Thread A writes some values. Note that there are no memory barriers\n> involved here. Not that I know of or that they would be documented.\n>\n>        mutex->autoinit = 0;\n>\n> And it writes another one. Thread A continues below to contend for the\n> mutex it just initialized.\n>\n>     } else\n>\n> Meanwhile, thread B, say on CPU B, spins in this loop:\n>\n>        while (mutex->autoinit != 0)\n>           ; /* wait for other thread */\n>\n> When thread B arrives here, it sees the value of autoinit that thread A\n> has written above.\n>\n> HOWEVER, when it continues, there is NO [*] guarantee that it will also\n> see the values that InitializeCriticalSection() has written, because\n> there were no memory barriers involved. When it continues, there is a\n> chance that it calls EnterCriticalSection() with uninitialized values!\n>\n\nThanks for pointing this out, I completely forgot about write re-ordering.\n\nThis is indeed a problem. So, shouldn't replacing \"mutex->autoinit =\n0;\" with \"InterlockedExchange(&mutex->autoinit, 0)\" solve the problem?\nInterlockedExchange generates a full memory barrier:\nhttp://msdn.microsoft.com/en-us/library/windows/desktop/ms683590(v=vs.85).aspx\n\n>  }\n>\n>\n> [*] If you compile this code with MSVC >= 2005, \"No guarantee\" is not\n> true, it's exactly the opposite because Microsoft extended the meaning\n> of 'volatile' to imply a memory barriere.\n\nDo you have a source for this? I'm not saying it isn't true, I just\nnever heard of this, and would like to read up on it :)\n\n> This is *NOT* true for gcc in\n> general. It may be true for MinGW gcc, but I do not know.\n>\n>> Basically, the thread that gets the original 1 returned from\n>> InterlockedCompareExchange is the only one who writes to\n>> mutex->autoinit. All other threads only read the value, and the\n>> volatile should make sure they actually do. Since all 32-bit reads and\n>> writes are atomic on Windows (see\n>> http://msdn.microsoft.com/en-us/library/windows/desktop/ms684122(v=vs.85).aspx\n>> \"Simple reads and writes to properly-aligned 32-bit variables are\n>> atomic operations.\") and mutex->autoinit is a LONG, this should be\n>> safe AFAICT. In fact, Windows specifically does not have any\n>> explicitly atomic writes exactly for this reason.\n>\n> There is a difference between atomic and coherent: Yes, 32-bit accesses\n> are atomic, but they are not automatically coherent: A 32-bit value\n> written by one CPU is not instantly visible on the other CPU. 'volatile'\n> as per the C lanugage does not add any guarantees that would be of\n> interest here. OTOH, Microsoft's definition of 'volatile' does.\n>\n\nI never meant to imply this either, I simply forgot about write re-ordering ;)\n\n>> The only ways mutex->autoinit can be updated is:\n>> - InterlockedCompareExchange compares it to 1, finds it's identical\n>> and inserts -1\n>> - intialization is done\n>> Both these updates happens from the same thread.\n>>\n>> Yes, details like this should probably go into the commit message ;)\n>\n> A comment in the function is preferred!\n>\n\nYes, good point. This is code that looks dangerous (and in this case\npotentially was - hopefully eventual future iteration won't be), and\nshould of course be documented inline...\n"},{"id":"178298","messageId":"4EA7267E.1080103@kdbg.org","threadId":"28763","inReplyTo":"CABPQNSY6-j7iNagsJc3WKVZ94=yZHdfBswA-v0XY7vH+RxyjYQ@mail.gmail.com","subject":"Re: [msysGit] Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-10-25T21:13:34Z","receivedAt":"2011-10-25T21:13:34Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 25.10.2011 22:51, schrieb Erik Faye-Lund:\n> On Tue, Oct 25, 2011 at 10:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n>> HOWEVER, when it continues, there is NO [*] guarantee that it will also\n>> see the values that InitializeCriticalSection() has written, because\n>> there were no memory barriers involved. When it continues, there is a\n>> chance that it calls EnterCriticalSection() with uninitialized values!\n>>\n> \n> Thanks for pointing this out, I completely forgot about write re-ordering.\n> \n> This is indeed a problem. So, shouldn't replacing \"mutex->autoinit =\n> 0;\" with \"InterlockedExchange(&mutex->autoinit, 0)\" solve the problem?\n> InterlockedExchange generates a full memory barrier:\n> http://msdn.microsoft.com/en-us/library/windows/desktop/ms683590(v=vs.85).aspx\n\nThat should do it.\n\n>> [*] If you compile this code with MSVC >= 2005, \"No guarantee\" is not\n>> true, it's exactly the opposite because Microsoft extended the meaning\n>> of 'volatile' to imply a memory barriere.\n> \n> Do you have a source for this? I'm not saying it isn't true, I just\n> never heard of this, and would like to read up on it :)\n\nhttp://msdn.microsoft.com/en-us/library/ms686355%28VS.85%29.aspx\n\n-- Hannes\n"},{"id":"178307","messageId":"16520370.401.1319598319120.JavaMail.geo-discussion-forums@prms22","threadId":"28763","inReplyTo":"1319554509-6532-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER","fromName":"Atsushi Nakagawa","fromEmail":"atnak@chejz.com","sentAt":"2011-10-26T03:05:19Z","receivedAt":"2011-10-26T03:05:19Z","isPatch":true,"sender":{"key":"atnak@chejz.com","avatar":null},"body":"On Oct 25, 11:55 pm, Erik Faye-Lund <kusmab...@gmail.com> wrote:\n> [...]\n> +int pthread_mutex_init(pthread_mutex_t *mutex, const pthread_mutexattr_t \n*attr)\n> +{\n> +       InitializeCriticalSection(&mutex->cs);\n> +       mutex->autoinit = 0;\n> +       return 0;\n> +}\n> +\n> +int pthread_mutex_lock(pthread_mutex_t *mutex)\n> +{\n> +       if (mutex->autoinit) {\n> +               if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) \n!= -1) {\n\nI'm making the assumption that mutex->autoinit starts off as 1 before \nthings get multi-threaded..\n\nI've only looked at what's in the patch so I could be missing vital \ncontext..  Anyways, is there a reason why you made this \n\"InterlockedCompareExchange(..., -1, 1) != -1\" and not \n\"InterlockedCompareExchange(..., -1, 1) == 1\"?\n\nIt looks to me the former adds a race condition after \"if (mutex->autoinit) \n{\".  e.g. A second thread could reinitialize mutex->cs after the first \nthread has already entered EnterCriticalSection(...).\n\n> +                       pthread_mutex_init(mutex, NULL);\n> +                       mutex->autoinit = 0;\n> +               } else\n> +                       while (mutex->autoinit != 0)\n> +                               ; /* wait for other thread */\n> +       }\n> +\n> +       EnterCriticalSection(&mutex->cs);\n> +       return 0;\n> +}\n> [...]\n\n-- \nAtsushi Nakagawa\n\n"},{"id":"178305","messageId":"CAGZ=bqJ7k5h_-62M3y-Jype4a7mOTe+FxeU13JreGj0mOnSRSg@mail.gmail.com","threadId":"28763","inReplyTo":"CABPQNSY6-j7iNagsJc3WKVZ94=yZHdfBswA-v0XY7vH+RxyjYQ@mail.gmail.com","subject":"Re: [msysGit] Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER","fromName":"Kyle Moffett","fromEmail":"kyle@moffetthome.net","sentAt":"2011-10-26T03:44:47Z","receivedAt":"2011-10-26T03:44:47Z","isPatch":true,"sender":{"key":"kyle@moffetthome.net","avatar":null},"body":"On Tue, Oct 25, 2011 at 16:51, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n> On Tue, Oct 25, 2011 at 10:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n>> Am 25.10.2011 17:42, schrieb Erik Faye-Lund:\n>>> On Tue, Oct 25, 2011 at 5:28 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n>>>> Am 10/25/2011 16:55, schrieb Erik Faye-Lund:\n>>>>> +int pthread_mutex_lock(pthread_mutex_t *mutex)\n>>>>> +{\n>>>>> +     if (mutex->autoinit) {\n>>>>> +             if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {\n>>>>> +                     pthread_mutex_init(mutex, NULL);\n>>>>> +                     mutex->autoinit = 0;\n>>>>> +             } else\n>>>>> +                     while (mutex->autoinit != 0)\n>>>>> +                             ; /* wait for other thread */\n>>>>> +     }\n>>>>\n>>>> The double-checked locking idiom. Very suspicious. Can you explain why it\n>>>> works in this case? Why are no Interlocked functions needed for the other\n>>>> accesses of autoinit? (\"It is volatile\" is the wrong answer to this last\n>>>> question, BTW.)\n>>>\n>>> I agree that it should look a bit suspicious; I'm generally skeptical\n>>> whenever I see 'volatile' in threading-code myself. But I think it's\n>>> the right answer in this case. \"volatile\" means that the compiler\n>>> cannot optimize away accesses, which is sufficient in this case.\n>>\n>> No, it is not, and it took me a train ride to see what's wrong. It has\n>> nothing to do with autoinit, but with all the other memory locations\n>> that are written. See here, with pthread_mutex_init() inlined:\n>>\n>>  if (mutex->autoinit) {\n>>\n>> Assume two threads enter this block.\n>>\n>>     if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {\n>>\n>> Only one thread, A, say on CPU A, will enter this block.\n>>\n>>        InitializeCriticalSection(&mutex->cs);\n>>\n>> Thread A writes some values. Note that there are no memory barriers\n>> involved here. Not that I know of or that they would be documented.\n>>\n>>        mutex->autoinit = 0;\n>>\n>> And it writes another one. Thread A continues below to contend for the\n>> mutex it just initialized.\n>>\n>>     } else\n>>\n>> Meanwhile, thread B, say on CPU B, spins in this loop:\n>>\n>>        while (mutex->autoinit != 0)\n>>           ; /* wait for other thread */\n>>\n>> When thread B arrives here, it sees the value of autoinit that thread A\n>> has written above.\n>>\n>> HOWEVER, when it continues, there is NO [*] guarantee that it will also\n>> see the values that InitializeCriticalSection() has written, because\n>> there were no memory barriers involved. When it continues, there is a\n>> chance that it calls EnterCriticalSection() with uninitialized values!\n>>\n>\n> Thanks for pointing this out, I completely forgot about write re-ordering.\n>\n> This is indeed a problem. So, shouldn't replacing \"mutex->autoinit =\n> 0;\" with \"InterlockedExchange(&mutex->autoinit, 0)\" solve the problem?\n> InterlockedExchange generates a full memory barrier:\n> http://msdn.microsoft.com/en-us/library/windows/desktop/ms683590(v=vs.85).aspx\n\nNo, I'm afraid that won't solve the issue (at least in GCC, not sure about MSVC)\n\nA write barrier in one thread is only effective if it is paired with a\nread barrier in the other thread.\n\nSince there's no read barrier in the \"while(mutex->autoinit != 0)\",\nyou don't have any guaranteed ordering.\n\nI guess if MSVC assumes that volatile reads imply barriers then it might work...\n\nCheers,\nKyle Moffett\n\n-- \nCurious about my work on the Debian powerpcspe port?\nI'm keeping a blog here: http://pureperl.blogspot.com/\n"},{"id":"178319","messageId":"CABPQNSbuKuZ1yY1+ex3Y=ZY=7cjLwvo3Chbwi7ApoEjUOAErbA@mail.gmail.com","threadId":"28763","inReplyTo":"16520370.401.1319598319120.JavaMail.geo-discussion-forums@prms22","subject":"Re: [msysGit] Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-10-26T13:08:19Z","receivedAt":"2011-10-26T13:08:19Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Oct 26, 2011 at 5:05 AM, Atsushi Nakagawa <atnak@chejz.com> wrote:\n> On Oct 25, 11:55 pm, Erik Faye-Lund <kusmab...@gmail.com> wrote:\n>> [...]\n>> +int pthread_mutex_init(pthread_mutex_t *mutex, const pthread_mutexattr_t\n>> *attr)\n>> +{\n>> +       InitializeCriticalSection(&mutex->cs);\n>> +       mutex->autoinit = 0;\n>> +       return 0;\n>> +}\n>> +\n>> +int pthread_mutex_lock(pthread_mutex_t *mutex)\n>> +{\n>> +       if (mutex->autoinit) {\n>> +               if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) !=\n>> -1) {\n> I'm making the assumption that mutex->autoinit starts off as 1 before things\n> get multi-threaded..\n> I've only looked at what's in the patch so I could be missing vital\n> context..  Anyways, is there a reason why you made this\n> \"InterlockedCompareExchange(..., -1, 1) != -1\" and not\n> \"InterlockedCompareExchange(..., -1, 1) == 1\"?\n\nNo, not really.\n\n> It looks to me the former adds a race condition after \"if (mutex->autoinit)\n> {\".  e.g. A second thread could reinitialize mutex->cs after the first\n> thread has already entered EnterCriticalSection(...).\n\nYou are indeed correct, thanks for spotting :)\n"},{"id":"178320","messageId":"CABPQNSY9LdqOK=1nN_61ZRMv-ieXzSDYgNsQe0w21RAOw_D7yA@mail.gmail.com","threadId":"28763","inReplyTo":"CAGZ=bqJ7k5h_-62M3y-Jype4a7mOTe+FxeU13JreGj0mOnSRSg@mail.gmail.com","subject":"Re: [msysGit] Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-10-26T13:16:03Z","receivedAt":"2011-10-26T13:16:03Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Oct 26, 2011 at 5:44 AM, Kyle Moffett <kyle@moffetthome.net> wrote:\n> On Tue, Oct 25, 2011 at 16:51, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n>> On Tue, Oct 25, 2011 at 10:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n>>> Am 25.10.2011 17:42, schrieb Erik Faye-Lund:\n>>>> On Tue, Oct 25, 2011 at 5:28 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n>>>>> Am 10/25/2011 16:55, schrieb Erik Faye-Lund:\n>>>>>> +int pthread_mutex_lock(pthread_mutex_t *mutex)\n>>>>>> +{\n>>>>>> +     if (mutex->autoinit) {\n>>>>>> +             if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {\n>>>>>> +                     pthread_mutex_init(mutex, NULL);\n>>>>>> +                     mutex->autoinit = 0;\n>>>>>> +             } else\n>>>>>> +                     while (mutex->autoinit != 0)\n>>>>>> +                             ; /* wait for other thread */\n>>>>>> +     }\n>>>>>\n>>>>> The double-checked locking idiom. Very suspicious. Can you explain why it\n>>>>> works in this case? Why are no Interlocked functions needed for the other\n>>>>> accesses of autoinit? (\"It is volatile\" is the wrong answer to this last\n>>>>> question, BTW.)\n>>>>\n>>>> I agree that it should look a bit suspicious; I'm generally skeptical\n>>>> whenever I see 'volatile' in threading-code myself. But I think it's\n>>>> the right answer in this case. \"volatile\" means that the compiler\n>>>> cannot optimize away accesses, which is sufficient in this case.\n>>>\n>>> No, it is not, and it took me a train ride to see what's wrong. It has\n>>> nothing to do with autoinit, but with all the other memory locations\n>>> that are written. See here, with pthread_mutex_init() inlined:\n>>>\n>>>  if (mutex->autoinit) {\n>>>\n>>> Assume two threads enter this block.\n>>>\n>>>     if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {\n>>>\n>>> Only one thread, A, say on CPU A, will enter this block.\n>>>\n>>>        InitializeCriticalSection(&mutex->cs);\n>>>\n>>> Thread A writes some values. Note that there are no memory barriers\n>>> involved here. Not that I know of or that they would be documented.\n>>>\n>>>        mutex->autoinit = 0;\n>>>\n>>> And it writes another one. Thread A continues below to contend for the\n>>> mutex it just initialized.\n>>>\n>>>     } else\n>>>\n>>> Meanwhile, thread B, say on CPU B, spins in this loop:\n>>>\n>>>        while (mutex->autoinit != 0)\n>>>           ; /* wait for other thread */\n>>>\n>>> When thread B arrives here, it sees the value of autoinit that thread A\n>>> has written above.\n>>>\n>>> HOWEVER, when it continues, there is NO [*] guarantee that it will also\n>>> see the values that InitializeCriticalSection() has written, because\n>>> there were no memory barriers involved. When it continues, there is a\n>>> chance that it calls EnterCriticalSection() with uninitialized values!\n>>>\n>>\n>> Thanks for pointing this out, I completely forgot about write re-ordering.\n>>\n>> This is indeed a problem. So, shouldn't replacing \"mutex->autoinit =\n>> 0;\" with \"InterlockedExchange(&mutex->autoinit, 0)\" solve the problem?\n>> InterlockedExchange generates a full memory barrier:\n>> http://msdn.microsoft.com/en-us/library/windows/desktop/ms683590(v=vs.85).aspx\n>\n> No, I'm afraid that won't solve the issue (at least in GCC, not sure about MSVC)\n>\n> A write barrier in one thread is only effective if it is paired with a\n> read barrier in the other thread.\n>\n> Since there's no read barrier in the \"while(mutex->autoinit != 0)\",\n> you don't have any guaranteed ordering.\n>\n> I guess if MSVC assumes that volatile reads imply barriers then it might work...\n\nOK, so I should probably do something like this instead?\n\nwhile (InterlockedCompareExchange(&mutex->autoinit, 0, 0) != 0)\n\t; /* wait for other thread */\n\nI really appreciate getting some extra eyes on this, thanks.\nConcurrent programming is not my strong-suit (as this exercise has\nshown) ;)\n"},{"id":"178387","messageId":"20111028080033.57FC.B013761@chejz.com","threadId":"28763","inReplyTo":"CABPQNSY9LdqOK=1nN_61ZRMv-ieXzSDYgNsQe0w21RAOw_D7yA@mail.gmail.com","subject":"Re: [msysGit] Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER","fromName":"Atsushi Nakagawa","fromEmail":"atnak@chejz.com","sentAt":"2011-10-27T23:00:37Z","receivedAt":"2011-10-27T23:00:37Z","isPatch":true,"sender":{"key":"atnak@chejz.com","avatar":null},"body":"Erik Faye-Lund <kusmabite@gmail.com> wrote:\n> On Wed, Oct 26, 2011 at 5:44 AM, Kyle Moffett <kyle@moffetthome.net> wrote:\n> > On Tue, Oct 25, 2011 at 16:51, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n> >> On Tue, Oct 25, 2011 at 10:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n> >>> Am 25.10.2011 17:42, schrieb Erik Faye-Lund:\n> >>>> On Tue, Oct 25, 2011 at 5:28 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> >>>>> Am 10/25/2011 16:55, schrieb Erik Faye-Lund:\n> >>>>>> +int pthread_mutex_lock(pthread_mutex_t *mutex)\n> >>>>>> +{\n> >>>>>> + [snip]\n> >>>>>\n> >>>>> The double-checked locking idiom. Very suspicious. Can you explain why it\n> >>> [snip]\n> >>>\n> >>>  if (mutex->autoinit) {\n> >>>\n> >>> Assume two threads enter this block.\n> >>>\n> >>>     if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {\n> >>>\n> >>> Only one thread, A, say on CPU A, will enter this block.\n> >>>\n> >>>        InitializeCriticalSection(&mutex->cs);\n> >>>\n> >>> Thread A writes some values. Note that there are no memory barriers\n> >>> involved here. Not that I know of or that they would be documented.\n> >>>\n> >>>        mutex->autoinit = 0;\n> >>>\n> >>> And it writes another one. Thread A continues below to contend for the\n> >>> mutex it just initialized.\n> >>>\n> >>>     } else\n> >>>\n> >>> Meanwhile, thread B, say on CPU B, spins in this loop:\n> >>>\n> >>>        while (mutex->autoinit != 0)\n> >>>           ; /* wait for other thread */\n> >>>\n> >>> When thread B arrives here, it sees the value of autoinit that thread A\n> >>> has written above.\n> >>>\n> >>> [snip]\n> >>>\n> >>\n> >> Thanks for pointing this out, I completely forgot about write re-ordering.\n> >>\n> >> This is indeed a problem. So, shouldn't replacing \"mutex->autoinit =\n> >> 0;\" with \"InterlockedExchange(&mutex->autoinit, 0)\" solve the problem?\n> >> InterlockedExchange generates a full memory barrier:\n> >> http://msdn.microsoft.com/en-us/library/windows/desktop/ms683590(v=vs.85).aspx\n> >\n> > No, I'm afraid that won't solve the issue (at least in GCC, not sure about MSVC)\n> >\n> > A write barrier in one thread is only effective if it is paired with a\n> > read barrier in the other thread.\n> >\n> > Since there's no read barrier in the \"while(mutex->autoinit != 0)\",\n> > you don't have any guaranteed ordering.\n\nOut of curiosity, where could re-ordering be a problem here?  I'm\nthinking probably at \"EnterCriticalSection(&mutex->cs)\" and the contents\nof \"mutex->cs\" not being propagated to the waiting thread.  However,\nshouldn't that be a non-problem, as far as compiler reordering goes,\nbecause it's an external function call and only the address of mutex->cs\nis passed?\n\nThe only other cause I could think of is if ordering at the CPU was\nsomehow different (it could be if there're no special provisions for\ncalling external functions) or if \"InterlockedExchange(&mutex->autoinit,\n0)\" wasn't atomic in updating autoinit and doing the memory barrier.\n\nEither way, I couldn't vouch for the safety of the above logic without\na memory barrier so this question is purely of an academical nature. :)\n\n> > I guess if MSVC assumes that volatile reads imply barriers then it might work...\n> \n> OK, so I should probably do something like this instead?\n> \n> while (InterlockedCompareExchange(&mutex->autoinit, 0, 0) != 0)\n> \t; /* wait for other thread */\n\nTechnically, assuming only the updating of \"mutex->cs\" is in question,\nthe ICE should only be required once after exiting the loop...\n\nThere's a question of the propagation of the value of \"mutex->autoinit\"\nitself, but my take is that the memory barrier on the writing thread\nwill push out the updated value across all CPUs, thus preventing an\ninfinite loop.  The other factors, value caching and loop optimization\nby the compiler, should be prevented by the \"volatile\" keyword even with\ngcc or MSVC 2003.\n\n> I really appreciate getting some extra eyes on this, thanks.\n> Concurrent programming is not my strong-suit (as this exercise has\n> shown) ;)\n\nSo would I. :)\n\n-- \nAtsushi Nakagawa\n<atnak@chejz.com>\nChanges are made when there is inconvenience.\n"},{"id":"178388","messageId":"CAGZ=bqKA7P_FJz447AZA5HjWdghKnZqAWGuKAuvjsGp5bAGC1w@mail.gmail.com","threadId":"28763","inReplyTo":"20111028080033.57FC.B013761@chejz.com","subject":"Re: [msysGit] Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER","fromName":"Kyle Moffett","fromEmail":"kyle@moffetthome.net","sentAt":"2011-10-27T23:20:40Z","receivedAt":"2011-10-27T23:20:40Z","isPatch":true,"sender":{"key":"kyle@moffetthome.net","avatar":null},"body":"On Thu, Oct 27, 2011 at 19:00, Atsushi Nakagawa <atnak@chejz.com> wrote:\n> Erik Faye-Lund <kusmabite@gmail.com> wrote:\n>> On Wed, Oct 26, 2011 at 5:44 AM, Kyle Moffett <kyle@moffetthome.net> wrote:\n>> > On Tue, Oct 25, 2011 at 16:51, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n>> >> Thanks for pointing this out, I completely forgot about write re-ordering.\n>> >>\n>> >> This is indeed a problem. So, shouldn't replacing \"mutex->autoinit =\n>> >> 0;\" with \"InterlockedExchange(&mutex->autoinit, 0)\" solve the problem?\n>> >> InterlockedExchange generates a full memory barrier:\n>> >> http://msdn.microsoft.com/en-us/library/windows/desktop/ms683590(v=vs.85).aspx\n>> >\n>> > No, I'm afraid that won't solve the issue (at least in GCC, not sure about MSVC)\n>> >\n>> > A write barrier in one thread is only effective if it is paired with a\n>> > read barrier in the other thread.\n>> >\n>> > Since there's no read barrier in the \"while(mutex->autoinit != 0)\",\n>> > you don't have any guaranteed ordering.\n>\n> Out of curiosity, where could re-ordering be a problem here?  I'm\n> thinking probably at \"EnterCriticalSection(&mutex->cs)\" and the contents\n> of \"mutex->cs\" not being propagated to the waiting thread.  However,\n> shouldn't that be a non-problem, as far as compiler reordering goes,\n> because it's an external function call and only the address of mutex->cs\n> is passed?\n>\n> The only other cause I could think of is if ordering at the CPU was\n> somehow different (it could be if there're no special provisions for\n> calling external functions) or if \"InterlockedExchange(&mutex->autoinit,\n> 0)\" wasn't atomic in updating autoinit and doing the memory barrier.\n>\n> Either way, I couldn't vouch for the safety of the above logic without\n> a memory barrier so this question is purely of an academical nature. :)\n>\n>> > I guess if MSVC assumes that volatile reads imply barriers then it might work...\n>>\n>> OK, so I should probably do something like this instead?\n>>\n>> while (InterlockedCompareExchange(&mutex->autoinit, 0, 0) != 0)\n>>       ; /* wait for other thread */\n>\n> Technically, assuming only the updating of \"mutex->cs\" is in question,\n> the ICE should only be required once after exiting the loop...\n>\n> There's a question of the propagation of the value of \"mutex->autoinit\"\n> itself, but my take is that the memory barrier on the writing thread\n> will push out the updated value across all CPUs, thus preventing an\n> infinite loop.  The other factors, value caching and loop optimization\n> by the compiler, should be prevented by the \"volatile\" keyword even with\n> gcc or MSVC 2003.\n>\n>> I really appreciate getting some extra eyes on this, thanks.\n>> Concurrent programming is not my strong-suit (as this exercise has\n>> shown) ;)\n>\n> So would I. :)\n\nOk, so here's the race condition:\n\nThread1\t\t\t\tThread2\n\t\t\t\t/* Speculative prefetch */\n\t\t\t\tprefetch(*mutex);\n\nif (mutex->autoinit) {\nif (ICE(&mutex->autoinit, -1, 1) != -1) {\n/* Now mutex->autoinit == -1 */\npthread_mutex_init(mutex, NULL);\n/* This forces writes out to memory */\nICE(&mutex->autoinit, 0, -1);\n\n\t\t\t\tif (mutex->autoinit) {} /* false */\n\t\t\t\t/* No read barrier here */\n\t\t\t\tEnterCriticalSection(&mutex->cs);\n\t\t\t\t/* Use cached mutex->cs from earlier */\n\nEven though you forced the memory write to be ordered in Thread 1 you\ndid not ensure that the read of autoinit occurred before the read of\nmutex->cs in Thread 2.  If the first thing that EnterCriticalSection\ndoes is follow a pointer or read a mutex key value in mutex->cs then\nwon't necessarily get the right answer.\n\nThe rule of memory barriers is the ALWAYS come in pairs.  This simple\nexample guarantees that Thread2 will read \"tmp_a\" == 1 if it\npreviously read \"tmp_b\" == 1, although getting \"tmp_a\" == 1 and\n\"tmp_b\" != 1 is still possible.\n\nThread1:\na = 1;\nwrite_barrier();\nb = 1;\n\nThread2:\ntmp_b = b;\nread_barrier();\ntmp_a = a;\n\nI think there's a Documentation/memory-barriers.txt file in the kernel\nsource code with more helpful info.\n\nCheers,\nKyle Moffett\n\n-- \nCurious about my work on the Debian powerpcspe port?\nI'm keeping a blog here: http://pureperl.blogspot.com/\n"},{"id":"178482","messageId":"20111029033542.5C66.B013761@chejz.com","threadId":"28763","inReplyTo":"CAGZ=bqKA7P_FJz447AZA5HjWdghKnZqAWGuKAuvjsGp5bAGC1w@mail.gmail.com","subject":"Re: [msysGit] Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER","fromName":"Atsushi Nakagawa","fromEmail":"atnak@chejz.com","sentAt":"2011-10-28T18:35:46Z","receivedAt":"2011-10-28T18:35:46Z","isPatch":true,"sender":{"key":"atnak@chejz.com","avatar":null},"body":"Thanks for the explanation. :)\n\nKyle Moffett <kyle@moffetthome.net> wrote:\n> On Thu, Oct 27, 2011 at 19:00, Atsushi Nakagawa <atnak@chejz.com> wrote:\n> > Erik Faye-Lund <kusmabite@gmail.com> wrote:\n> >> On Wed, Oct 26, 2011 at 5:44 AM, Kyle Moffett <kyle@moffetthome.net> wrote:\n> >> > On Tue, Oct 25, 2011 at 16:51, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n> >> >> [...]\n> >> >\n> >> > No, I'm afraid that won't solve the issue (at least in GCC, not sure about MSVC)\n> >> >\n> >> > A write barrier in one thread is only effective if it is paired with a\n> >> > read barrier in the other thread.\n> >> >\n> >> > Since there's no read barrier in the \"while(mutex->autoinit != 0)\",\n> >> > you don't have any guaranteed ordering.\n> >\n> > Out of curiosity, where could re-ordering be a problem here?  I'm\n> > thinking probably at \"EnterCriticalSection(&mutex->cs)\" and the contents\n> > of \"mutex->cs\" not being propagated to the waiting thread.  However,\n> > shouldn't that be a non-problem, as far as compiler reordering goes,\n> > because it's an external function call and only the address of mutex->cs\n> > is passed?\n> >\n> > [...]\n> \n> Ok, so here's the race condition:\n> \n> Thread1\t\t\t\tThread2\n> \t\t\t\t/* Speculative prefetch */\n> \t\t\t\tprefetch(*mutex);\n> \n> if (mutex->autoinit) {\n> if (ICE(&mutex->autoinit, -1, 1) != -1) {\n> /* Now mutex->autoinit == -1 */\n> pthread_mutex_init(mutex, NULL);\n> /* This forces writes out to memory */\n> ICE(&mutex->autoinit, 0, -1);\n> \n> \t\t\t\tif (mutex->autoinit) {} /* false */\n> \t\t\t\t/* No read barrier here */\n> \t\t\t\tEnterCriticalSection(&mutex->cs);\n> \t\t\t\t/* Use cached mutex->cs from earlier */\n\nOk, so there's no way of skimping on that one memory barrier in every\nvisit to pthread_mutex_lock().  Interesting.  Makes me wonder how it\ntrades off to lazy initialization.\n> \n> Even though you forced the memory write to be ordered in Thread 1 you\n> did not ensure that the read of autoinit occurred before the read of\n> mutex->cs in Thread 2.  If the first thing that EnterCriticalSection\n> does is follow a pointer or read a mutex key value in mutex->cs then\n> won't necessarily get the right answer.\n> \n> The rule of memory barriers is the ALWAYS come in pairs.  This simple\n> example guarantees that Thread2 will read \"tmp_a\" == 1 if it\n> previously read \"tmp_b\" == 1, although getting \"tmp_a\" == 1 and\n> \"tmp_b\" != 1 is still possible.\n> \n> Thread1:\n> a = 1;\n> write_barrier();\n> b = 1;\n> \n> Thread2:\n> tmp_b = b;\n> read_barrier();\n> tmp_a = a;\n> \n> I think there's a Documentation/memory-barriers.txt file in the kernel\n> source code with more helpful info.\n\n-- \nAtsushi Nakagawa\n<atnak@chejz.com>\nChanges are made when there is inconvenience.\n"}]}