diff mbox

nptl: optimize cancelstate and canceltype changing

Message ID 70941401635088@web6j.yandex.ru
State New
Headers show

Commit Message

Alexander Fyodorov June 1, 2014, 3:04 p.m. UTC 
Hi all,

I'm posting this for review and more testing, as I do not have big or middle endian processors. 

The barrier's name here could use some thinking, but this was the best I could come up with.


--------------

This patch improves performance of functions changing thread cancellation state and type by removing atomic operations from them.

The idea is simple: since state and type are changed only by the thread itself, they should not require such rigorous synchronization. The 'cancelhandling' word takes 4 bytes, so we can put type and state in different bytes within the word and access them directly using 1-byte stores. Specific position of a given flag will then depend on endiannes.

Checking whether the thread was canceled must be done _after_ it enables cancellation or turns on asynchronous mode. To enforce this order, a barrier is put between the respective store and load. Since the read is data-dependent on the corresponding write and some architectures may not reorder such accesses, putting atomic_full_barrier() there would be an overkill. So I added a new type of barrier which defaults to a full barrier.

Also it looks like there is a missing "THREAD_SETMEM (self, result, PTHREAD_CANCELED)" in __pthread_setcancelstate() (it is present in __pthread_setcanceltype()).


2014-06-01  Fyodorov V. Alexander  <alexvf@bk.ru>

	* nptl/descr.h: Change bits position in the 'cancelhandling' field.
	* include/atomic.h: Add default
	atomic_read_after_write_dependent_barrier() definition.
	* nptl/cancellation.c: Replace atomic operation with a barrier.
	* nptl/cleanup_defer.c: Likewise.
	* nptl/cleanup_defer_compat.c: Likewise.
	* nptl/pthread_setcanceltype.c: Likewise.
	* nptl/pthread_setcancelstate.c: Likewise, and set self->result to PTHREAD_CANCELED.
diff mbox

Patch

diff --git a/include/atomic.h b/include/atomic.h
index 3e82b6a..f0dac38 100644
--- a/include/atomic.h
+++ b/include/atomic.h
@@ -547,4 +547,9 @@ 
 # define atomic_delay() do { /* nothing */ } while (0)
 #endif
 
+
+#ifndef atomic_read_after_write_dependent_barrier()
+# define atomic_read_after_write_dependent_barrier() atomic_full_barrier ()
+#endif
+
 #endif	/* atomic.h */
diff --git a/nptl/cancellation.c b/nptl/cancellation.c
index aaf102d..c127231 100644
--- a/nptl/cancellation.c
+++ b/nptl/cancellation.c
@@ -31,28 +31,15 @@  __pthread_enable_asynccancel (void)
   struct pthread *self = THREAD_SELF;
   int oldval = THREAD_GETMEM (self, cancelhandling);
 
-  while (1)
+  if ((oldval & CANCELTYPE_BITMASK) == 0)
     {
-      int newval = oldval | CANCELTYPE_BITMASK;
-
-      if (newval == oldval)
-	break;
-
-      int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
-					      oldval);
-      if (__glibc_likely (curval == oldval))
-	{
-	  if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
-	    {
-	      THREAD_SETMEM (self, result, PTHREAD_CANCELED);
-	      __do_cancel ();
-	    }
-
-	  break;
-	}
-
-      /* Prepare the next round.  */
-      oldval = curval;
+      /* Set the new value. */
+      THREAD_SETMEM (self, cancel.type, (char) PTHREAD_CANCEL_ASYNCHRONOUS);
+
+      /* Synchronize with pthread_cancel() */
+      atomic_read_after_write_dependent_barrier();
+
+      CANCELLATION_P (self);
     }
 
   return oldval;
@@ -69,22 +56,14 @@  __pthread_disable_asynccancel (int oldtype)
     return;
 
   struct pthread *self = THREAD_SELF;
-  int newval;
-
-  int oldval = THREAD_GETMEM (self, cancelhandling);
 
-  while (1)
-    {
-      newval = oldval & ~CANCELTYPE_BITMASK;
+  /* Set the new value. */
+  THREAD_SETMEM (self, cancel.type, (char) PTHREAD_CANCEL_DEFERRED);
 
-      int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
-					      oldval);
-      if (__glibc_likely (curval == oldval))
-	break;
+  /* Synchronize with pthread_cancel() */
+  atomic_read_after_write_dependent_barrier();
 
-      /* Prepare the next round.  */
-      oldval = curval;
-    }
+  int newval = THREAD_GETMEM (self, cancelhandling);
 
   /* We cannot return when we are being canceled.  Upon return the
      thread might be things which would have to be undone.  The
diff --git a/nptl/cleanup_defer.c b/nptl/cleanup_defer.c
index a8fc403..86097d7 100644
--- a/nptl/cleanup_defer.c
+++ b/nptl/cleanup_defer.c
@@ -35,19 +35,12 @@  __pthread_register_cancel_defer (__pthread_unwind_buf_t *buf)
 
   /* Disable asynchronous cancellation for now.  */
   if (__glibc_unlikely (cancelhandling & CANCELTYPE_BITMASK))
-    while (1)
-      {
-	int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling,
-						cancelhandling
-						& ~CANCELTYPE_BITMASK,
-						cancelhandling);
-	if (__glibc_likely (curval == cancelhandling))
-	  /* Successfully replaced the value.  */
-	  break;
-
-	/* Prepare for the next round.  */
-	cancelhandling = curval;
-      }
+    {
+      THREAD_SETMEM (self, cancel.type, (char) PTHREAD_CANCEL_DEFERRED);
+
+      /* Synchronize with pthread_cancel() */
+      atomic_read_after_write_dependent_barrier();
+    }
 
   ibuf->priv.data.canceltype = (cancelhandling & CANCELTYPE_BITMASK
 				? PTHREAD_CANCEL_ASYNCHRONOUS
@@ -72,19 +65,10 @@  __pthread_unregister_cancel_restore (__pthread_unwind_buf_t *buf)
       && ((cancelhandling = THREAD_GETMEM (self, cancelhandling))
 	  & CANCELTYPE_BITMASK) == 0)
     {
-      while (1)
-	{
-	  int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling,
-						  cancelhandling
-						  | CANCELTYPE_BITMASK,
-						  cancelhandling);
-	  if (__glibc_likely (curval == cancelhandling))
-	    /* Successfully replaced the value.  */
-	    break;
-
-	  /* Prepare for the next round.  */
-	  cancelhandling = curval;
-	}
+      THREAD_SETMEM (self, cancel.type, PTHREAD_CANCEL_ASYNCHRONOUS);
+
+      /* Synchronize with pthread_cancel() */
+      atomic_read_after_write_dependent_barrier();
 
       CANCELLATION_P (self);
     }
diff --git a/nptl/cleanup_defer_compat.c b/nptl/cleanup_defer_compat.c
index 9c52f5f..abff38c 100644
--- a/nptl/cleanup_defer_compat.c
+++ b/nptl/cleanup_defer_compat.c
@@ -35,19 +35,12 @@  _pthread_cleanup_push_defer (buffer, routine, arg)
 
   /* Disable asynchronous cancellation for now.  */
   if (__glibc_unlikely (cancelhandling & CANCELTYPE_BITMASK))
-    while (1)
-      {
-	int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling,
-						cancelhandling
-						& ~CANCELTYPE_BITMASK,
-						cancelhandling);
-	if (__glibc_likely (curval == cancelhandling))
-	  /* Successfully replaced the value.  */
-	  break;
-
-	/* Prepare for the next round.  */
-	cancelhandling = curval;
-      }
+    {
+      THREAD_SETMEM (self, cancel.type, (char) PTHREAD_CANCEL_DEFERRED);
+
+      /* Synchronize with pthread_cancel() */
+      atomic_read_after_write_dependent_barrier();
+    }
 
   buffer->__canceltype = (cancelhandling & CANCELTYPE_BITMASK
 			  ? PTHREAD_CANCEL_ASYNCHRONOUS
@@ -72,19 +65,10 @@  _pthread_cleanup_pop_restore (buffer, execute)
       && ((cancelhandling = THREAD_GETMEM (self, cancelhandling))
 	  & CANCELTYPE_BITMASK) == 0)
     {
-      while (1)
-	{
-	  int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling,
-						  cancelhandling
-						  | CANCELTYPE_BITMASK,
-						  cancelhandling);
-	  if (__glibc_likely (curval == cancelhandling))
-	    /* Successfully replaced the value.  */
-	    break;
-
-	  /* Prepare for the next round.  */
-	  cancelhandling = curval;
-	}
+      THREAD_SETMEM (self, cancel.type, PTHREAD_CANCEL_ASYNCHRONOUS);
+
+      /* Synchronize with pthread_cancel() */
+      atomic_read_after_write_dependent_barrier();
 
       CANCELLATION_P (self);
     }
diff --git a/nptl/descr.h b/nptl/descr.h
index 61d57d5..9671793 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -19,6 +19,7 @@ 
 #ifndef _DESCR_H
 #define _DESCR_H	1
 
+#include <endian.h>
 #include <limits.h>
 #include <sched.h>
 #include <setjmp.h>
@@ -255,30 +256,63 @@  struct pthread
 #define HAVE_CLEANUP_JMP_BUF
 
   /* Flags determining processing of cancellation.  */
-  int cancelhandling;
+  union {
+    int cancelhandling;
+    struct {
+      char state;
+      char type;
+    } cancel;
+  };
+#if BYTE_ORDER == LITTLE_ENDIAN
   /* Bit set if cancellation is disabled.  */
 #define CANCELSTATE_BIT		0
-#define CANCELSTATE_BITMASK	(0x01 << CANCELSTATE_BIT)
   /* Bit set if asynchronous cancellation mode is selected.  */
-#define CANCELTYPE_BIT		1
-#define CANCELTYPE_BITMASK	(0x01 << CANCELTYPE_BIT)
+#define CANCELTYPE_BIT		8
   /* Bit set if canceling has been initiated.  */
-#define CANCELING_BIT		2
-#define CANCELING_BITMASK	(0x01 << CANCELING_BIT)
+#define CANCELING_BIT		16
   /* Bit set if canceled.  */
-#define CANCELED_BIT		3
-#define CANCELED_BITMASK	(0x01 << CANCELED_BIT)
+#define CANCELED_BIT		17
   /* Bit set if thread is exiting.  */
-#define EXITING_BIT		4
-#define EXITING_BITMASK		(0x01 << EXITING_BIT)
+#define EXITING_BIT		18
   /* Bit set if thread terminated and TCB is freed.  */
-#define TERMINATED_BIT		5
-#define TERMINATED_BITMASK	(0x01 << TERMINATED_BIT)
+#define TERMINATED_BIT		19
   /* Bit set if thread is supposed to change XID.  */
-#define SETXID_BIT		6
+#define SETXID_BIT		20
+#else
+#if BYTE_ORDER == BIG_ENDIAN
+  /* Bit set if cancellation is disabled.  */
+#define CANCELSTATE_BIT		24
+  /* Bit set if asynchronous cancellation mode is selected.  */
+#define CANCELTYPE_BIT		16
+#else /* BYTE_ORDER == PDP_ENDIAN */
+  /* Bit set if cancellation is disabled.  */
+#define CANCELSTATE_BIT		16
+  /* Bit set if asynchronous cancellation mode is selected.  */
+#define CANCELTYPE_BIT		24
+#endif
+  /* Bit set if canceling has been initiated.  */
+#define CANCELING_BIT		0
+  /* Bit set if canceled.  */
+#define CANCELED_BIT		1
+  /* Bit set if thread is exiting.  */
+#define EXITING_BIT		2
+  /* Bit set if thread terminated and TCB is freed.  */
+#define TERMINATED_BIT		3
+  /* Bit set if thread is supposed to change XID.  */
+#define SETXID_BIT		4
+#endif
+#define CANCELSTATE_BITMASK	(0x01 << CANCELSTATE_BIT)
+#define CANCELTYPE_BITMASK	(0x01 << CANCELTYPE_BIT)
+#define CANCELING_BITMASK	(0x01 << CANCELING_BIT)
+#define CANCELED_BITMASK	(0x01 << CANCELED_BIT)
+#define EXITING_BITMASK		(0x01 << EXITING_BIT)
+#define TERMINATED_BITMASK	(0x01 << TERMINATED_BIT)
 #define SETXID_BITMASK		(0x01 << SETXID_BIT)
   /* Mask for the rest.  Helps the compiler to optimize.  */
-#define CANCEL_RESTMASK		0xffffff80
+#define CANCEL_RESTMASK	\
+  ( ~((int) (CANCELSTATE_BITMASK | CANCELTYPE_BITMASK | CANCELING_BITMASK | \
+	     CANCELED_BITMASK | EXITING_BITMASK | TERMINATED_BITMASK | \
+	     SETXID_BITMASK)) )
 
 #define CANCEL_ENABLED_AND_CANCELED(value) \
   (((value) & (CANCELSTATE_BITMASK | CANCELED_BITMASK | EXITING_BITMASK	      \
diff --git a/nptl/pthread_setcancelstate.c b/nptl/pthread_setcancelstate.c
index 5c3ca86..c187563 100644
--- a/nptl/pthread_setcancelstate.c
+++ b/nptl/pthread_setcancelstate.c
@@ -34,37 +34,30 @@  __pthread_setcancelstate (state, oldstate)
   self = THREAD_SELF;
 
   int oldval = THREAD_GETMEM (self, cancelhandling);
-  while (1)
+
+  int prev_state = ((oldval & CANCELSTATE_BITMASK) ? PTHREAD_CANCEL_DISABLE :
+						     PTHREAD_CANCEL_ENABLE);
+
+  /* Store the old value.  */
+  if (oldstate != NULL)
+    *oldstate = prev_state;
+
+  /* Avoid doing unnecessary work. */
+  if (state != prev_state)
     {
-      int newval = (state == PTHREAD_CANCEL_DISABLE
-		    ? oldval | CANCELSTATE_BITMASK
-		    : oldval & ~CANCELSTATE_BITMASK);
-
-      /* Store the old value.  */
-      if (oldstate != NULL)
-	*oldstate = ((oldval & CANCELSTATE_BITMASK)
-		     ? PTHREAD_CANCEL_DISABLE : PTHREAD_CANCEL_ENABLE);
-
-      /* Avoid doing unnecessary work.  The atomic operation can
-	 potentially be expensive if the memory has to be locked and
-	 remote cache lines have to be invalidated.  */
-      if (oldval == newval)
-	break;
-
-      /* Update the cancel handling word.  This has to be done
-	 atomically since other bits could be modified as well.  */
-      int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
-					      oldval);
-      if (__glibc_likely (curval == oldval))
-	{
-	  if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
-	    __do_cancel ();
-
-	  break;
-	}
-
-      /* Prepare for the next round.  */
-      oldval = curval;
+      /* Set the new value. */
+      THREAD_SETMEM (self, cancel.state, (char) state);
+
+      /* Synchronize with pthread_cancel() */
+      atomic_read_after_write_dependent_barrier();
+
+      int newval = THREAD_GETMEM (self, cancelhandling);
+
+      if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
+        {
+          THREAD_SETMEM (self, result, PTHREAD_CANCELED);
+          __do_cancel ();
+        }
     }
 
   return 0;
diff --git a/nptl/pthread_setcanceltype.c b/nptl/pthread_setcanceltype.c
index fb1631f..17d3593 100644
--- a/nptl/pthread_setcanceltype.c
+++ b/nptl/pthread_setcanceltype.c
@@ -34,40 +34,30 @@  __pthread_setcanceltype (type, oldtype)
   self = THREAD_SELF;
 
   int oldval = THREAD_GETMEM (self, cancelhandling);
-  while (1)
+
+  int prev_type = ((oldval & CANCELTYPE_BITMASK) ? PTHREAD_CANCEL_ASYNCHRONOUS :
+						  PTHREAD_CANCEL_DEFERRED);
+
+  /* Store the old value.  */
+  if (oldtype != NULL)
+    *oldtype = prev_type;
+
+  /* Avoid doing unnecessary work. */
+  if (type != prev_type)
     {
-      int newval = (type == PTHREAD_CANCEL_ASYNCHRONOUS
-		    ? oldval | CANCELTYPE_BITMASK
-		    : oldval & ~CANCELTYPE_BITMASK);
-
-      /* Store the old value.  */
-      if (oldtype != NULL)
-	*oldtype = ((oldval & CANCELTYPE_BITMASK)
-		    ? PTHREAD_CANCEL_ASYNCHRONOUS : PTHREAD_CANCEL_DEFERRED);
-
-      /* Avoid doing unnecessary work.  The atomic operation can
-	 potentially be expensive if the memory has to be locked and
-	 remote cache lines have to be invalidated.  */
-      if (oldval == newval)
-	break;
-
-      /* Update the cancel handling word.  This has to be done
-	 atomically since other bits could be modified as well.  */
-      int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
-					      oldval);
-      if (__glibc_likely (curval == oldval))
-	{
-	  if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
-	    {
-	      THREAD_SETMEM (self, result, PTHREAD_CANCELED);
-	      __do_cancel ();
-	    }
-
-	  break;
-	}
-
-      /* Prepare for the next round.  */
-      oldval = curval;
+      /* Set the new value. */
+      THREAD_SETMEM (self, cancel.type, (char) type);
+
+      /* Synchronize with pthread_cancel() */
+      atomic_read_after_write_dependent_barrier();
+
+      int newval = THREAD_GETMEM (self, cancelhandling);
+
+      if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
+        {
+          THREAD_SETMEM (self, result, PTHREAD_CANCELED);
+          __do_cancel ();
+        }
     }
 
   return 0;