Re: [PATCH] Unify pthread_spin_[try]lock implementations.

[Maxim Kuvyrkov <maxim@codesourcery.com>]

On 11/07/2012, at 11:22 PM, Roland McGrath wrote:

>> OK, but this will be a change on its own in a follow up patch.  I want to
>> keep this patch just a mechanical change.
> 
> I don't want to put anything in the generic code that isn't clearly correct
> or isn't clearly justified or doesn't have thorough comments explaining any
> nonobvious issues.  So get it right first before the generic code goes in.

This code _is_ right.  It may not be optimal for all machines, but it is correct.  I agree that the code is not obvious and the attached patch clears it up.

> Each machine dropping its old code in favor of the new generic code can
> wait as long as each machine maintainer wants.

This is not new code.  It is the exactly same code that ARM, MIPS, HPPA and M68K have all been using for several years, it is just moved into the generic directory.

Attached is the patch based on Andrew's and Chris's comments.  It bounds the wait loop by a reasonable constant and makes atomic_compare_and_exchange update memory on the local processor.  Assuming that cmpxchg is about 100 times more expensive than a simple load, we spend 90% of the wait cycle in "while (*lock != 0)" loop and 10% in atomic_compare_and_exchange to update the memory.

--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics


Bound wait loop in pthread_spin_lock.

	2012-07-12  Maxim Kuvyrkov  <maxim@codesourcery.com>

	* nptl/pthread_spin_lock.c (PTHREAD_SPIN_LOCK_WAIT): New macro.
	(pthread_spin_lock): Bound wait cycle with PTHREAD_SPIN_LOCK_WAIT.
---
 nptl/pthread_spin_lock.c |   27 +++++++++++++++++++++++++--
 1 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/nptl/pthread_spin_lock.c b/nptl/pthread_spin_lock.c
index 1daa43d..ac59117 100644
--- a/nptl/pthread_spin_lock.c
+++ b/nptl/pthread_spin_lock.c
@@ -19,12 +19,35 @@
 #include <atomic.h>
 #include "pthreadP.h"
 
+#ifndef PTHREAD_SPIN_LOCK_WAIT
+# define PTHREAD_SPIN_LOCK_WAIT 1000
+#endif
+
 int
 pthread_spin_lock (pthread_spinlock_t *lock)
 {
   while (atomic_compare_and_exchange_val_acq (lock, 1, 0) != 0)
-    while (*lock != 0)
-      ;
+    /* The lock is contended and we need to wait.  Going straight back
+       to cmpxchg is not a good idea on many targets as that will force
+       expensive memory synchronizations among processors and penalize other
+       running threads.
+       On the other hand, we do want to update memory state on the local core
+       once in a while to avoid spinning indefinitely until some event that
+       will happen to update local memory as a side-effect.  */
+    {
+      if (PTHREAD_SPIN_LOCK_WAIT)
+	{
+	  int wait = PTHREAD_SPIN_LOCK_WAIT;
+
+	  while (*lock != 0 && --wait)
+	    ;
+	}
+      else
+	{
+	  while (*lock != 0)
+	    ;
+	}
+    }
 
   return 0;
 }
-- 
1.7.4.1