[v2,4/6] i386: Remove bogus THREAD_ATOMIC_* macros

Message ID 20181228010255.21406-5-adhemerval.zanella@linaro.org
State New
Headers show
Series
  • General fixes and refactor for BZ#12683
Related show

Commit Message

Adhemerval Zanella Dec. 28, 2018, 1:02 a.m.
The x86 defines optimized THREAD_ATOMIC_* macros where reference always
the current thread instead of the one indicated by input 'descr' argument.
It work as long the input is the self thread pointer, however it generates
wrong code is the semantic is to set a bit atomicialy from another thread.

This is not an issue for current GLIBC usage, however the new cancellation
code expects that some synchronization code to atomically set bits from
different threads.

If some usage indeed proves to be a hotspot we can add an extra macro
with a more descriptive name (THREAD_ATOMIC_BIT_SET_SELF for instance)
where i386 might optimize it.

Checked on i686-linux-gnu.

	* sysdeps/i686/nptl/tls.h (THREAD_ATOMIC_CMPXCHG_VAL,
	THREAD_ATOMIC_AND, THREAD_ATOMIC_BIT_SET): Remove macros.
---
 ChangeLog               |  3 +++
 sysdeps/i386/nptl/tls.h | 37 -------------------------------------
 2 files changed, 3 insertions(+), 37 deletions(-)

Comments

Siddhesh Poyarekar Dec. 29, 2018, 2:13 a.m. | #1
On 28/12/18 6:32 AM, Adhemerval Zanella wrote:
> The x86 defines optimized THREAD_ATOMIC_* macros where reference always
> the current thread instead of the one indicated by input 'descr' argument.
> It work as long the input is the self thread pointer, however it generates
> wrong code is the semantic is to set a bit atomicialy from another thread.

"if the semantic is to set a bit atomically"

> 
> This is not an issue for current GLIBC usage, however the new cancellation
> code expects that some synchronization code to atomically set bits from
> different threads.
> 
> If some usage indeed proves to be a hotspot we can add an extra macro
> with a more descriptive name (THREAD_ATOMIC_BIT_SET_SELF for instance)
> where i386 might optimize it.
> 
> Checked on i686-linux-gnu.
> 
> 	* sysdeps/i686/nptl/tls.h (THREAD_ATOMIC_CMPXCHG_VAL,
> 	THREAD_ATOMIC_AND, THREAD_ATOMIC_BIT_SET): Remove macros.

OK with that nit.

Siddhesh

Patch

diff --git a/sysdeps/i386/nptl/tls.h b/sysdeps/i386/nptl/tls.h
index 12285d3217..22ebf3d741 100644
--- a/sysdeps/i386/nptl/tls.h
+++ b/sysdeps/i386/nptl/tls.h
@@ -362,43 +362,6 @@  tls_fill_user_desc (union user_desc_init *desc,
        }})
 
 
-/* Atomic compare and exchange on TLS, returning old value.  */
-#define THREAD_ATOMIC_CMPXCHG_VAL(descr, member, newval, oldval) \
-  ({ __typeof (descr->member) __ret;					      \
-     __typeof (oldval) __old = (oldval);				      \
-     if (sizeof (descr->member) == 4)					      \
-       asm volatile (LOCK_PREFIX "cmpxchgl %2, %%gs:%P3"		      \
-		     : "=a" (__ret)					      \
-		     : "0" (__old), "r" (newval),			      \
-		       "i" (offsetof (struct pthread, member)));	      \
-     else								      \
-       /* Not necessary for other sizes in the moment.  */		      \
-       abort ();							      \
-     __ret; })
-
-
-/* Atomic logical and.  */
-#define THREAD_ATOMIC_AND(descr, member, val) \
-  (void) ({ if (sizeof ((descr)->member) == 4)				      \
-	      asm volatile (LOCK_PREFIX "andl %1, %%gs:%P0"		      \
-			    :: "i" (offsetof (struct pthread, member)),	      \
-			       "ir" (val));				      \
-	    else							      \
-	      /* Not necessary for other sizes in the moment.  */	      \
-	      abort (); })
-
-
-/* Atomic set bit.  */
-#define THREAD_ATOMIC_BIT_SET(descr, member, bit) \
-  (void) ({ if (sizeof ((descr)->member) == 4)				      \
-	      asm volatile (LOCK_PREFIX "orl %1, %%gs:%P0"		      \
-			    :: "i" (offsetof (struct pthread, member)),	      \
-			       "ir" (1 << (bit)));			      \
-	    else							      \
-	      /* Not necessary for other sizes in the moment.  */	      \
-	      abort (); })
-
-
 /* Set the stack guard field in TCB head.  */
 #define THREAD_SET_STACK_GUARD(value) \
   THREAD_SETMEM (THREAD_SELF, header.stack_guard, value)