[2/2] S390: Use generic spinlock code.
Commit Message
This is an updated version of the patch, which adjusts the s390 specific
atomic-macros in the same way as in include/atomic.h.
Thus passing a volatile int pointer is fine, too.
Okay to commit?
Bye.
Stefan
ChangeLog:
* sysdeps/s390/atomic-machine.h:
(__arch_compare_and_exchange_val_32_acq):
Cast type to omit volatile qualifier.
(__arch_compare_and_exchange_val_64_acq): Likewise.
(atomic_exchange_acq): Likewise.
* sysdeps/s390/nptl/pthread_spin_init.c: Delete File.
* sysdeps/s390/nptl/pthread_spin_unlock.c: Likewise.
* sysdeps/s390/nptl/pthread_spin_lock.c:
(SPIN_LOCK_READS_BETWEEN_CMPXCHG): New define.
Use generic spinlock code.
* sysdeps/s390/nptl/pthread_spin_trylock.c:
(SPIN_TRYLOCK_USE_CMPXCHG_INSTEAD_OF_XCHG): New define.
Use generic spinlock code.
Comments
On Wed, 2017-02-08 at 15:49 +0100, Stefan Liebler wrote:
> This is an updated version of the patch, which adjusts the s390 specific
> atomic-macros in the same way as in include/atomic.h.
> Thus passing a volatile int pointer is fine, too.
The general direction of this is okay.
Some of my comments for patch 1/2 apply here as well (eg, volatile vs.
atomics).
What I don't like is the choice of 1000 for
SPIN_LOCK_READS_BETWEEN_CMPXCHG. Have you run benchmarks to come up
with this value, or is it a guess? Why isn't it documented how you end
up with this number?
We can keep these with a choice such as this, but then we need to have a
FIXME comment in the code, explaining that this is just an arbitrary
choice.
I would guess that just spinning forever is sufficient, and that we
don't need to throw in a CAS every now and then; using randomized
exponential back-off might be more important. This is something that we
would be in a better position to answer if you'd provide a
microbenchmark for this choice too.
At the end of 2016, I've posted a draft of a microbenchmark for rwlocks.
Maybe you can use this as a start and run a few experiments?
Also, I'm not quite sure whether this number is really
spinlock-specific, and I would like to find a better place for these.
IMO, they should be in some header that contains default tuning
parameters for synchronization code, which is provided by each
architecture that uses the generic spinlock; we'd have no #ifdef for the
tuning parameters, so we'd catch typos in those headers.
commit 472bccee286f2f40522b8f670c9b54b7ef58425c
Author: Stefan Liebler <stli@linux.vnet.ibm.com>
Date: Wed Feb 8 15:24:14 2017 +0100
S390: Use generic spinlock code.
This patch removes the s390 specific implementation of spinlock code
and is now using the generic one.
For pthread_spin_trylock an explicit load and test before executing
compare and swap instruction is done as it is an interlock update even
if the lock is already acquired.
The macros in s390 specific atomic-machine.h are aligned in order to
omit the storing to and loading from stack.
ChangeLog:
* sysdeps/s390/atomic-machine.h:
(__arch_compare_and_exchange_val_32_acq):
Cast type to omit volatile qualifier.
(__arch_compare_and_exchange_val_64_acq): Likewise.
(atomic_exchange_acq): Likewise.
* sysdeps/s390/nptl/pthread_spin_init.c: Delete File.
* sysdeps/s390/nptl/pthread_spin_unlock.c: Likewise.
* sysdeps/s390/nptl/pthread_spin_lock.c:
(SPIN_LOCK_READS_BETWEEN_CMPXCHG): New define.
Use generic spinlock code.
* sysdeps/s390/nptl/pthread_spin_trylock.c:
(SPIN_TRYLOCK_USE_CMPXCHG_INSTEAD_OF_XCHG): New define.
Use generic spinlock code.
@@ -54,7 +54,7 @@ typedef uintmax_t uatomic_max_t;
#define __arch_compare_and_exchange_val_32_acq(mem, newval, oldval) \
({ __typeof (mem) __archmem = (mem); \
- __typeof (*mem) __archold = (oldval); \
+ __typeof ((__typeof (*(mem))) *(mem)) __archold = (oldval); \
__asm__ __volatile__ ("cs %0,%2,%1" \
: "+d" (__archold), "=Q" (*__archmem) \
: "d" (newval), "m" (*__archmem) : "cc", "memory" ); \
@@ -64,7 +64,7 @@ typedef uintmax_t uatomic_max_t;
# define __HAVE_64B_ATOMICS 1
# define __arch_compare_and_exchange_val_64_acq(mem, newval, oldval) \
({ __typeof (mem) __archmem = (mem); \
- __typeof (*mem) __archold = (oldval); \
+ __typeof ((__typeof (*(mem))) *(mem)) __archold = (oldval); \
__asm__ __volatile__ ("csg %0,%2,%1" \
: "+d" (__archold), "=Q" (*__archmem) \
: "d" ((long) (newval)), "m" (*__archmem) : "cc", "memory" ); \
@@ -86,8 +86,8 @@ typedef uintmax_t uatomic_max_t;
#ifdef __s390x__
# define atomic_exchange_acq(mem, newvalue) \
({ __typeof (mem) __atg5_memp = (mem); \
- __typeof (*(mem)) __atg5_oldval = *__atg5_memp; \
- __typeof (*(mem)) __atg5_value = (newvalue); \
+ __typeof ((__typeof (*(mem))) *(mem)) __atg5_oldval = *__atg5_memp; \
+ __typeof ((__typeof (*(mem))) *(mem)) __atg5_value = (newvalue); \
if (sizeof (*mem) == 4) \
__asm__ __volatile__ ("0: cs %0,%2,%1\n" \
" jl 0b" \
@@ -106,8 +106,8 @@ typedef uintmax_t uatomic_max_t;
#else
# define atomic_exchange_acq(mem, newvalue) \
({ __typeof (mem) __atg5_memp = (mem); \
- __typeof (*(mem)) __atg5_oldval = *__atg5_memp; \
- __typeof (*(mem)) __atg5_value = (newvalue); \
+ __typeof ((__typeof (*(mem))) *(mem)) __atg5_oldval = *__atg5_memp; \
+ __typeof ((__typeof (*(mem))) *(mem)) __atg5_value = (newvalue); \
if (sizeof (*mem) == 4) \
__asm__ __volatile__ ("0: cs %0,%2,%1\n" \
" jl 0b" \
deleted file mode 100644
@@ -1,19 +0,0 @@
-/* Copyright (C) 2003-2017 Free Software Foundation, Inc.
- This file is part of the GNU C Library.
- Contributed by Martin Schwidefsky <schwidefsky@de.ibm.com>, 2003.
-
- 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/>. */
-
-/* Not needed. pthread_spin_init is an alias for pthread_spin_unlock. */
@@ -16,17 +16,9 @@
License along with the GNU C Library; if not, see
<http://www.gnu.org/licenses/>. */
-#include "pthreadP.h"
+#define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000
-int
-pthread_spin_lock (pthread_spinlock_t *lock)
-{
- int oldval;
-
- __asm__ __volatile__ ("0: lhi %0,0\n"
- " cs %0,%2,%1\n"
- " jl 0b"
- : "=&d" (oldval), "=Q" (*lock)
- : "d" (1), "m" (*lock) : "cc" );
- return 0;
-}
+/* We can't use the normal "#include <nptl/pthread_spin_lock.c>" because
+ it will resolve to this very file. Using "sysdeps/.." as reference to the
+ top level directory does the job. */
+#include <sysdeps/../nptl/pthread_spin_lock.c>
@@ -16,17 +16,9 @@
License along with the GNU C Library; if not, see
<http://www.gnu.org/licenses/>. */
-#include <errno.h>
-#include "pthreadP.h"
+#define SPIN_TRYLOCK_USE_CMPXCHG_INSTEAD_OF_XCHG 1
-int
-pthread_spin_trylock (pthread_spinlock_t *lock)
-{
- int old;
-
- __asm__ __volatile__ ("cs %0,%3,%1"
- : "=d" (old), "=Q" (*lock)
- : "0" (0), "d" (1), "m" (*lock) : "cc" );
-
- return old != 0 ? EBUSY : 0;
-}
+/* We can't use the normal "#include <nptl/pthread_spin_trylock.c>" because
+ it will resolve to this very file. Using "sysdeps/.." as reference to the
+ top level directory does the job. */
+#include <sysdeps/../nptl/pthread_spin_trylock.c>
deleted file mode 100644
@@ -1,32 +0,0 @@
-/* Copyright (C) 2003-2017 Free Software Foundation, Inc.
- This file is part of the GNU C Library.
- Contributed by Martin Schwidefsky <schwidefsky@de.ibm.com>, 2003.
-
- 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/>. */
-
-/* Ugly hack to avoid the declaration of pthread_spin_init. */
-#define pthread_spin_init pthread_spin_init_XXX
-#include "pthreadP.h"
-#undef pthread_spin_init
-
-int
-pthread_spin_unlock (pthread_spinlock_t *lock)
-{
- __asm__ __volatile__ (" xc %O0(4,%R0),%0\n"
- " bcr 15,0"
- : "=Q" (*lock) : "m" (*lock) : "cc" );
- return 0;
-}
-strong_alias (pthread_spin_unlock, pthread_spin_init)