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

[Maxim Kuvyrkov <maxim@codesourcery.com>]

On 26/07/2012, at 6:13 AM, Roland McGrath wrote:

> Here I think the reasonable thing to do is:
> 
> /* A machine-specific version can define SPIN_LOCK_READS_BETWEEN_CMPXCHG
>   to the number of plain reads that it's optimal to spin on between uses
>   of atomic_compare_and_exchange_val_acq.  If spinning forever is optimal
>   then use -1.  If no plain reads here would ever be optimal, use 0.  */
> #ifndef SPIN_LOCK_READS_BETWEEN_CMPXCHG
> # warning machine-dependent file should define SPIN_LOCK_READS_BETWEEN_CMPXCHG
> # define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000
> #endif
> 
> Then ARM et al can do:
> 
> /* Machine-dependent rationale about the selection of this value.  */
> #define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000
> #include <nptl/pthread_spin_lock.c>
> 
> while Teil will use -1.
> 
>> +      if (PTHREAD_SPIN_LOCK_WAIT)
> 
> Don't use implicit boolean coercion.
> Use "if (SPIN_LOCK_READS_BETWEEN_CMPXCHG >= 0)".
> 
>> +	{
>> +	  int wait = PTHREAD_SPIN_LOCK_WAIT;
>> +
>> +	  while (*lock != 0 && --wait)
>> +	    ;
> 
> Write it:
> 	while (wait > 0 && *lock != 0)
> 	  --wait;
> 
> That handles the SPIN_LOCK_READS_BETWEEN_CMPXCHG==0 case implicitly,
> avoids the ugly empty statement, and doesn't use implicit coercion.

OK, the updated patch attached.  Is this what you had in mind?

As before the patch touches ARM, HPPA, M68K and MIPS.

The patch also adds another optimization to use atomic_exchange_acq for the first attempt at acquiring the lock.  For a free lock atomic_exchange is, generally, faster; while atomic_compare_and_exchange is better for waiting on a contended lock.  Same rationale applies to pthread_spin_trylock.

This patch builds on MIPS and regression testing is in progress.  OK to apply is tests are fine?

Thank you,

--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics


Add generic versions of pthread_spin_lock and pthread_spin_trylock.

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

	* nptl/pthread_spin_lock.c: New file.
	* nptl/pthread_spin_trylock.c: New file.

	ports/
	* sysdeps/arm/nptl/pthread_spin_lock.c: Use generic code.
	* sysdeps/arm/nptl/pthread_spin_trylock.c: Remove, use generic version.

	* sysdeps/hppa/nptl/pthread_spin_lock.c: Use generic code.
	* sysdeps/hppa/nptl/pthread_spin_trylock.c: Remove, use generic version.

	* sysdeps/m68k/nptl/pthread_spin_lock.c: Use generic code.
	* sysdeps/m68k/nptl/pthread_spin_trylock.c: Remove, use generic version.

	* sysdeps/mips/nptl/pthread_spin_lock.S: Remove, use generic version.
	* sysdeps/mips/nptl/pthread_spin_lock.c: New file.
	* sysdeps/mips/nptl/pthread_spin_trylock.S: Remove, use generic version.
---
 nptl/pthread_spin_lock.c                       |   69 ++++++++++++++++++++++++
 nptl/pthread_spin_trylock.c                    |   27 +++++++++
 ports/sysdeps/arm/nptl/pthread_spin_lock.c     |   16 +-----
 ports/sysdeps/arm/nptl/pthread_spin_trylock.c  |   26 ---------
 ports/sysdeps/hppa/nptl/pthread_spin_lock.c    |   24 +-------
 ports/sysdeps/hppa/nptl/pthread_spin_trylock.c |   33 -----------
 ports/sysdeps/m68k/nptl/pthread_spin_lock.c    |   16 +-----
 ports/sysdeps/m68k/nptl/pthread_spin_trylock.c |   27 ---------
 ports/sysdeps/mips/nptl/pthread_spin_lock.S    |   36 ------------
 ports/sysdeps/mips/nptl/pthread_spin_lock.c    |   19 +++++++
 ports/sysdeps/mips/nptl/pthread_spin_trylock.S |   40 --------------
 11 files changed, 124 insertions(+), 209 deletions(-)
 create mode 100644 nptl/pthread_spin_lock.c
 create mode 100644 nptl/pthread_spin_trylock.c
 delete mode 100644 ports/sysdeps/arm/nptl/pthread_spin_trylock.c
 delete mode 100644 ports/sysdeps/hppa/nptl/pthread_spin_trylock.c
 delete mode 100644 ports/sysdeps/m68k/nptl/pthread_spin_trylock.c
 delete mode 100644 ports/sysdeps/mips/nptl/pthread_spin_lock.S
 create mode 100644 ports/sysdeps/mips/nptl/pthread_spin_lock.c
 delete mode 100644 ports/sysdeps/mips/nptl/pthread_spin_trylock.S

diff --git a/nptl/pthread_spin_lock.c b/nptl/pthread_spin_lock.c
new file mode 100644
index 0000000..91733fb
--- /dev/null
+++ b/nptl/pthread_spin_lock.c
@@ -0,0 +1,69 @@
+/* pthread_spin_lock -- lock a spin lock.  Generic version.
+   Copyright (C) 2012 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <atomic.h>
+#include "pthreadP.h"
+
+/* A machine-specific version can define SPIN_LOCK_READS_BETWEEN_CMPXCHG
+  to the number of plain reads that it's optimal to spin on between uses
+  of atomic_compare_and_exchange_val_acq.  If spinning forever is optimal
+  then use -1.  If no plain reads here would ever be optimal, use 0.  */
+#ifndef SPIN_LOCK_READS_BETWEEN_CMPXCHG
+# warning machine-dependent file should define SPIN_LOCK_READS_BETWEEN_CMPXCHG
+# define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000
+#endif
+
+int
+pthread_spin_lock (pthread_spinlock_t *lock)
+{
+  /* atomic_exchange usually takes less instructions than
+     atomic_compare_and_exchange.  On the other hand,
+     atomic_compare_and_exchange potentially generates less bus traffic
+     when the lock is locked.
+     We assume that the first try mostly will be successful, and we use
+     atomic_exchange.  For the subsequent tries we use
+     atomic_compare_and_exchange.  */
+  if (atomic_exchange_acq (lock, 1) == 0)
+    return 0;
+
+  do
+    {
+      /* 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 (SPIN_LOCK_READS_BETWEEN_CMPXCHG >= 0)
+	{
+	  int wait = SPIN_LOCK_READS_BETWEEN_CMPXCHG;
+
+	  while (*lock != 0 && wait > 0)
+	    --wait;
+	}
+      else
+	{
+	  while (*lock != 0)
+	    ;
+	}
+    }
+  while (atomic_compare_and_exchange_val_acq (lock, 1, 0) != 0);
+
+  return 0;
+}
diff --git a/nptl/pthread_spin_trylock.c b/nptl/pthread_spin_trylock.c
new file mode 100644
index 0000000..db9372c
--- /dev/null
+++ b/nptl/pthread_spin_trylock.c
@@ -0,0 +1,27 @@
+/* pthread_spin_trylock -- trylock a spin lock.  Generic version.
+   Copyright (C) 2012 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <atomic.h>
+#include "pthreadP.h"
+
+int
+pthread_spin_trylock (pthread_spinlock_t *lock)
+{
+  return atomic_exchange_acq (lock, 1) ? EBUSY : 0;
+}
diff --git a/ports/sysdeps/arm/nptl/pthread_spin_lock.c b/ports/sysdeps/arm/nptl/pthread_spin_lock.c
index 3a23bd3..7ccfbfa 100644
--- a/ports/sysdeps/arm/nptl/pthread_spin_lock.c
+++ b/ports/sysdeps/arm/nptl/pthread_spin_lock.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 2008 Free Software Foundation, Inc.
+/* Copyright (C) 2008-2012 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -15,15 +15,5 @@
    License along with the GNU C Library.  If not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <atomic.h>
-#include "pthreadP.h"
-
-int
-pthread_spin_lock (pthread_spinlock_t *lock)
-{
-  while (atomic_compare_and_exchange_val_acq (lock, 1, 0) != 0)
-   while (*lock != 0)
-    ;
-
-  return 0;
-}
+#define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000
+#include_next <nptl/pthread_spin_lock.c>
diff --git a/ports/sysdeps/arm/nptl/pthread_spin_trylock.c b/ports/sysdeps/arm/nptl/pthread_spin_trylock.c
deleted file mode 100644
index 7d31180..0000000
--- a/ports/sysdeps/arm/nptl/pthread_spin_trylock.c
+++ /dev/null
@@ -1,26 +0,0 @@
-/* Copyright (C) 2008 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library.  If not, see
-   <http://www.gnu.org/licenses/>.  */
-
-#include <errno.h>
-#include <atomic.h>
-#include "pthreadP.h"
-
-int
-pthread_spin_trylock (pthread_spinlock_t *lock)
-{
-  return atomic_compare_and_exchange_val_acq (lock, 1, 0) ? EBUSY : 0;
-}
diff --git a/ports/sysdeps/hppa/nptl/pthread_spin_lock.c b/ports/sysdeps/hppa/nptl/pthread_spin_lock.c
index bcf2240..7c86768 100644
--- a/ports/sysdeps/hppa/nptl/pthread_spin_lock.c
+++ b/ports/sysdeps/hppa/nptl/pthread_spin_lock.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 2005 Free Software Foundation, Inc.
+/* Copyright (C) 2005-2012 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -15,23 +15,5 @@
    License along with the GNU C Library.  If not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <atomic.h>
-#include "pthreadP.h"
-
-int
-pthread_spin_lock (pthread_spinlock_t *lock)
-{
-#if 0
-  volatile unsigned int *addr = __ldcw_align (lock);
-
-  while (__ldcw (addr) == 0)
-    while (*addr == 0) ;
-
-  return 0;
-#endif
-
-  while (atomic_compare_and_exchange_val_acq(lock, 1, 0) == 1)
-    while (*lock == 1);
-  
-  return 0;
-}
+#define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000
+#include_next <nptl/pthread_spin_lock.c>
diff --git a/ports/sysdeps/hppa/nptl/pthread_spin_trylock.c b/ports/sysdeps/hppa/nptl/pthread_spin_trylock.c
deleted file mode 100644
index a802861..0000000
--- a/ports/sysdeps/hppa/nptl/pthread_spin_trylock.c
+++ /dev/null
@@ -1,33 +0,0 @@
-/* Copyright (C) 2005 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library.  If not, see
-   <http://www.gnu.org/licenses/>.  */
-
-#include <errno.h>
-#include <atomic.h>
-#include "pthreadP.h"
-
-int
-pthread_spin_trylock (pthread_spinlock_t *lock)
-{
-#if 0
-  volatile unsigned int *a = __ldcw_align (lock);
-
-  return __ldcw (a) ? 0 : EBUSY;
-#endif
-
-  return atomic_compare_and_exchange_val_acq(lock, 1, 0) ? EBUSY : 0;
-
-}
diff --git a/ports/sysdeps/m68k/nptl/pthread_spin_lock.c b/ports/sysdeps/m68k/nptl/pthread_spin_lock.c
index 90a8262..710e48b 100644
--- a/ports/sysdeps/m68k/nptl/pthread_spin_lock.c
+++ b/ports/sysdeps/m68k/nptl/pthread_spin_lock.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 2010 Free Software Foundation, Inc.
+/* Copyright (C) 2010-2012 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Maxim Kuvyrkov <maxim@codesourcery.com>, 2010.
 
@@ -16,15 +16,5 @@
    License along with the GNU C Library.  If not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <atomic.h>
-#include "pthreadP.h"
-
-int
-pthread_spin_lock (pthread_spinlock_t *lock)
-{
-  while (atomic_compare_and_exchange_val_acq(lock, 1, 0) != 0)
-   while (*lock != 0)
-    ;
-
-  return 0;
-}
+#define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000
+#include_next <nptl/pthread_spin_lock.c>
diff --git a/ports/sysdeps/m68k/nptl/pthread_spin_trylock.c b/ports/sysdeps/m68k/nptl/pthread_spin_trylock.c
deleted file mode 100644
index f4b0c0d..0000000
--- a/ports/sysdeps/m68k/nptl/pthread_spin_trylock.c
+++ /dev/null
@@ -1,27 +0,0 @@
-/* Copyright (C) 2010 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Maxim Kuvyrkov <maxim@codesourcery.com>, 2010.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library.  If not, see
-   <http://www.gnu.org/licenses/>.  */
-
-#include <errno.h>
-#include <atomic.h>
-#include "pthreadP.h"
-
-int
-pthread_spin_trylock (pthread_spinlock_t *lock)
-{
-  return atomic_compare_and_exchange_val_acq(lock, 1, 0) ? EBUSY : 0;
-}
diff --git a/ports/sysdeps/mips/nptl/pthread_spin_lock.S b/ports/sysdeps/mips/nptl/pthread_spin_lock.S
deleted file mode 100644
index a8504f1..0000000
--- a/ports/sysdeps/mips/nptl/pthread_spin_lock.S
+++ /dev/null
@@ -1,36 +0,0 @@
-/* Copyright (C) 2005 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library.  If not, see
-   <http://www.gnu.org/licenses/>.  */
-
-#include <sys/asm.h>
-#include <sysdep.h>
-#include <sgidefs.h>
-
-ENTRY (pthread_spin_lock)
-	.set	push
-#if _MIPS_SIM == _ABIO32
-	.set	mips2
-#endif
-1:	ll	a2, 0(a0)
-	li	a1, 1
-	bnez	a2, 1b
-	sc	a1, 0(a0)
-	beqz	a1, 1b
-	MIPS_SYNC
-	.set	pop
-	li	v0, 0
-	ret
-PSEUDO_END (pthread_spin_lock)
diff --git a/ports/sysdeps/mips/nptl/pthread_spin_lock.c b/ports/sysdeps/mips/nptl/pthread_spin_lock.c
new file mode 100644
index 0000000..83ade5f
--- /dev/null
+++ b/ports/sysdeps/mips/nptl/pthread_spin_lock.c
@@ -0,0 +1,19 @@
+/* Copyright (C) 2012 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library.  If not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000
+#include_next <nptl/pthread_spin_lock.c>
diff --git a/ports/sysdeps/mips/nptl/pthread_spin_trylock.S b/ports/sysdeps/mips/nptl/pthread_spin_trylock.S
deleted file mode 100644
index 95b55c3..0000000
--- a/ports/sysdeps/mips/nptl/pthread_spin_trylock.S
+++ /dev/null
@@ -1,40 +0,0 @@
-/* Copyright (C) 2005 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library.  If not, see
-   <http://www.gnu.org/licenses/>.  */
-
-#include <sys/asm.h>
-#include <sysdep.h>
-#define _ERRNO_H 1
-#include <bits/errno.h>
-#include <sgidefs.h>
-
-ENTRY (pthread_spin_trylock)
-	.set	push
-#if _MIPS_SIM == _ABIO32
-	.set	mips2
-#endif
-	ll	a2, 0(a0)
-	li	a1, 1
-	bnez	a2, 1f
-	sc	a1, 0(a0)
-	beqz	a1, 1f
-	MIPS_SYNC
-	.set	pop
-	li	v0, 0
-	ret
-1:	li	v0, EBUSY
-	ret
-PSEUDO_END (pthread_spin_trylock)
-- 
1.7.0.4