diff mbox series

[v2,1/2] nptl: Move cancel state out of cancelhandling

Message ID 20200520174819.1138124-1-adhemerval.zanella@linaro.org
State New
Headers show
Series [v2,1/2] nptl: Move cancel state out of cancelhandling | expand

Commit Message

Adhemerval Zanella May 20, 2020, 5:48 p.m. UTC
Changes from previous version:

  - Extended commit message.
  - Extended some comments.

---

The thread cancellation state is not accessed concurrently internally
neither the pthread interface allows changing the state of a different
thread than its own.

By removing the cancel state out of the internal thread cancel handling
state there is no need to check if cancelled bit was set in CAS
operation.

The code is also simplified: the CANCELLATION_P is replaced with a
internal pthread_testcancel call and the CANCELSTATE_BIT{MASK} is
removed.

The second part of this patchset also keeps the pthread_setcanceltype as
cancellation entrypoint by calling pthread_testcancel if the new type
is PTHREAD_CANCEL_ASYNCHRONOUS.

With this behavior pthread_setcancelstate does not require to act on
cancellation if cancel type is asynchronous (is already handled either
by pthread_setcanceltype or by the signal handler).

Checked on x86_64-linux-gnu.
---
 nptl/allocatestack.c          |  1 +
 nptl/cancellation.c           |  3 ++-
 nptl/cleanup_defer.c          |  2 +-
 nptl/cleanup_defer_compat.c   |  2 +-
 nptl/descr.h                  | 14 ++++++--------
 nptl/pthreadP.h               | 12 ------------
 nptl/pthread_cancel.c         |  3 ++-
 nptl/pthread_join_common.c    |  5 ++++-
 nptl/pthread_setcancelstate.c | 36 +++--------------------------------
 nptl/pthread_setcanceltype.c  |  3 ++-
 nptl/pthread_testcancel.c     | 11 ++++++++++-
 11 files changed, 32 insertions(+), 60 deletions(-)
diff mbox series

Patch

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index c94980c21c..d9174274c2 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -232,6 +232,7 @@  get_cached_stack (size_t *sizep, void **memp)
 
   /* Cancellation handling is back to the default.  */
   result->cancelhandling = 0;
+  result->cancelstate = PTHREAD_CANCEL_ENABLE;
   result->cleanup = NULL;
 
   /* No pending event.  */
diff --git a/nptl/cancellation.c b/nptl/cancellation.c
index 826071321e..7e8cbe9fe1 100644
--- a/nptl/cancellation.c
+++ b/nptl/cancellation.c
@@ -45,7 +45,8 @@  __pthread_enable_asynccancel (void)
 					      oldval);
       if (__glibc_likely (curval == oldval))
 	{
-	  if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
+	  if (self->cancelstate == PTHREAD_CANCEL_ENABLE
+	      && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval))
 	    {
 	      THREAD_SETMEM (self, result, PTHREAD_CANCELED);
 	      __do_cancel ();
diff --git a/nptl/cleanup_defer.c b/nptl/cleanup_defer.c
index 8ad9a90c50..33d4ea6eef 100644
--- a/nptl/cleanup_defer.c
+++ b/nptl/cleanup_defer.c
@@ -86,6 +86,6 @@  __pthread_unregister_cancel_restore (__pthread_unwind_buf_t *buf)
 	  cancelhandling = curval;
 	}
 
-      CANCELLATION_P (self);
+      __pthread_testcancel ();
     }
 }
diff --git a/nptl/cleanup_defer_compat.c b/nptl/cleanup_defer_compat.c
index 33e47888f2..a1ad291fcc 100644
--- a/nptl/cleanup_defer_compat.c
+++ b/nptl/cleanup_defer_compat.c
@@ -83,7 +83,7 @@  _pthread_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer,
 	  cancelhandling = curval;
 	}
 
-      CANCELLATION_P (self);
+      __pthread_testcancel ();
     }
 
   /* If necessary call the cleanup routine after we removed the
diff --git a/nptl/descr.h b/nptl/descr.h
index e1c7db5473..279df680d6 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -269,9 +269,6 @@  struct pthread
 
   /* Flags determining processing of cancellation.  */
   int cancelhandling;
-  /* 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)
@@ -293,11 +290,8 @@  struct pthread
   /* Mask for the rest.  Helps the compiler to optimize.  */
 #define CANCEL_RESTMASK		0xffffff80
 
-#define CANCEL_ENABLED_AND_CANCELED(value) \
-  (((value) & (CANCELSTATE_BITMASK | CANCELED_BITMASK | EXITING_BITMASK	      \
-	       | CANCEL_RESTMASK | TERMINATED_BITMASK)) == CANCELED_BITMASK)
-#define CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS(value) \
-  (((value) & (CANCELSTATE_BITMASK | CANCELTYPE_BITMASK | CANCELED_BITMASK    \
+#define CANCEL_CANCELED_AND_ASYNCHRONOUS(value) \
+  (((value) & (CANCELTYPE_BITMASK | CANCELED_BITMASK    \
 	       | EXITING_BITMASK | CANCEL_RESTMASK | TERMINATED_BITMASK))     \
    == (CANCELTYPE_BITMASK | CANCELED_BITMASK))
 
@@ -398,6 +392,10 @@  struct pthread
   /* Indicates whether is a C11 thread created by thrd_creat.  */
   bool c11;
 
+  /* Thread cancel state (PTHREAD_CANCEL_ENABLE or
+     PTHREAD_CANCEL_DISABLE).  */
+  unsigned char cancelstate;
+
   /* This member must be last.  */
   char end_padding[];
 
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index c4e72f57a9..7fe7d7f064 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -258,18 +258,6 @@  extern int __pthread_debug attribute_hidden;
 #endif
 
 
-/* Cancellation test.  */
-#define CANCELLATION_P(self) \
-  do {									      \
-    int cancelhandling = THREAD_GETMEM (self, cancelhandling);		      \
-    if (CANCEL_ENABLED_AND_CANCELED (cancelhandling))			      \
-      {									      \
-	THREAD_SETMEM (self, result, PTHREAD_CANCELED);			      \
-	__do_cancel ();							      \
-      }									      \
-  } while (0)
-
-
 extern void __pthread_unwind (__pthread_unwind_buf_t *__buf)
      __cleanup_fct_attribute __attribute ((__noreturn__))
 #if !defined SHARED && !IS_IN (libpthread)
diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index 88c1ab8f6a..5b2789d620 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -55,7 +55,8 @@  __pthread_cancel (pthread_t th)
       /* If the cancellation is handled asynchronously just send a
 	 signal.  We avoid this if possible since it's more
 	 expensive.  */
-      if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
+      if (pd->cancelstate == PTHREAD_CANCEL_ENABLE
+	  && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval))
 	{
 	  /* Mark the cancellation as "in progress".  */
 	  if (atomic_compare_and_exchange_bool_acq (&pd->cancelhandling,
diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
index a96ceafde4..03e202136f 100644
--- a/nptl/pthread_join_common.c
+++ b/nptl/pthread_join_common.c
@@ -105,7 +105,10 @@  __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
 	   && (pd->cancelhandling
 	       & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
 		  | TERMINATED_BITMASK)) == 0))
-      && !CANCEL_ENABLED_AND_CANCELED (self->cancelhandling))
+      && !(self->cancelstate == PTHREAD_CANCEL_ENABLE
+	   && (pd->cancelhandling & (CANCELED_BITMASK | EXITING_BITMASK
+				     | TERMINATED_BITMASK))
+	       == CANCELED_BITMASK))
     /* This is a deadlock situation.  The threads are waiting for each
        other to finish.  Note that this is a "may" error.  To be 100%
        sure we catch this error we would have to lock the data
diff --git a/nptl/pthread_setcancelstate.c b/nptl/pthread_setcancelstate.c
index 4d7f413e19..aa1c8073a8 100644
--- a/nptl/pthread_setcancelstate.c
+++ b/nptl/pthread_setcancelstate.c
@@ -31,39 +31,9 @@  __pthread_setcancelstate (int state, int *oldstate)
 
   self = THREAD_SELF;
 
-  int oldval = THREAD_GETMEM (self, cancelhandling);
-  while (1)
-    {
-      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;
-    }
+  if (oldstate != NULL)
+    *oldstate = self->cancelstate;
+  self->cancelstate = state;
 
   return 0;
 }
diff --git a/nptl/pthread_setcanceltype.c b/nptl/pthread_setcanceltype.c
index fcaae8abc7..cc0507ae04 100644
--- a/nptl/pthread_setcanceltype.c
+++ b/nptl/pthread_setcanceltype.c
@@ -53,7 +53,8 @@  __pthread_setcanceltype (int type, int *oldtype)
 					      oldval);
       if (__glibc_likely (curval == oldval))
 	{
-	  if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
+	  if (self->cancelstate == PTHREAD_CANCEL_ENABLE
+	      && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval))
 	    {
 	      THREAD_SETMEM (self, result, PTHREAD_CANCELED);
 	      __do_cancel ();
diff --git a/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c
index 30408c2008..cd937bb3a3 100644
--- a/nptl/pthread_testcancel.c
+++ b/nptl/pthread_testcancel.c
@@ -23,7 +23,16 @@ 
 void
 __pthread_testcancel (void)
 {
-  CANCELLATION_P (THREAD_SELF);
+  struct pthread *self = THREAD_SELF;
+  int cancelhandling = THREAD_GETMEM (self, cancelhandling);
+  if (self->cancelstate == PTHREAD_CANCEL_ENABLE
+      && (cancelhandling & CANCELED_BITMASK)
+      && !(cancelhandling & EXITING_BITMASK)
+      && !(cancelhandling & TERMINATED_BITMASK))
+    {
+      THREAD_SETMEM (self, result, PTHREAD_CANCELED);
+      __do_cancel ();
+    }
 }
 strong_alias (__pthread_testcancel, pthread_testcancel)
 hidden_def (__pthread_testcancel)