[PATCH 00/11] nptl: pthread cancellation refactor

[PATCH 00/11] nptl: pthread cancellation refactor

[Adhemerval Zanella]

This patchset refactor and simplifies the nptl cancellation code.
The first 4 patches just add NFC changes and a new test.

The 5th patch fixes an memory leak issue when pthread_create fails to
setup either the scheduling or affinity parameter.  The asynchronous
cancellation is removed in favor of using a member to indicate the
setup has failed and letting the created thread to deallocate the
memory (similar on how detached threads do).

The 6h patch move the signal handler setup to pthread_cancel, now that
asynchronous cancellation is not used anymore on thread creation.

The 7th remove the CANCELING_BITMASK bit, since it is mainly used to
signal that a syscall that has returned should be cancelled (which is
a wrong assumption, since syscall with side-effects should *not* be
cancelled as per BZ#12683).

The 8th and 9th patches move both the cancel state and cancel type out
of 'cancelhandling' variable, since with CANCELING_BITMASK and
pthread_cancel change in previous patch they are not accessed in a
concurrent manner.

The 10th patch consolidates the raise as 'pthread_kill(pthread_self())'.

Finally, the 11th patch uses pthread_kill on pthread_cancel.

Adhemerval Zanella (11):
  nptl: Move Linux createthread to nptl
  nptl: Move createthread to pthread_create
  support: Add xpthread_attr_setaffinity_np wrapper
  nptl: Add pthread_attr_setaffinity_np failure test
  nptl: Deallocate the thread stack on setup failure (BZ #19511)
  nptl: Install cancellation handler on pthread_cancel
  nptl: Remove CANCELING_BITMASK
  nptl: Move cancel state out of cancelhandling
  nptl: Move cancel type out of cancelhandling
  nptl: Implement raise in terms of pthread_kill
  nptl: Use pthread_kill on pthread_cancel

 include/pthread.h                      |   5 +
 manual/pattern.texi                    |   1 -
 manual/process.texi                    |   3 +-
 nptl/Makefile                          |   1 +
 nptl/Versions                          |   3 +-
 nptl/allocatestack.c                   |   3 +
 nptl/cancellation.c                    |  62 ++--------
 nptl/cleanup_defer.c                   |  46 +-------
 nptl/createthread.c                    |  45 --------
 nptl/descr.h                           |  32 ++----
 nptl/libc-cleanup.c                    |  46 +-------
 nptl/pthreadP.h                        |  24 +---
 nptl/pthread_cancel.c                  | 145 ++++++++---------------
 nptl/pthread_create.c                  | 151 ++++++++++++++++++------
 nptl/pthread_join_common.c             |   7 +-
 nptl/pthread_kill.c                    |  52 ++++++---
 nptl/pthread_self.c                    |   4 +-
 nptl/pthread_setcancelstate.c          |  36 +-----
 nptl/pthread_setcanceltype.c           |  41 +------
 nptl/pthread_testcancel.c              |  11 +-
 nptl/tst-pthread-attr-affinity-fail.c  |  54 +++++++++
 support/Makefile                       |   1 +
 support/xpthread_attr_setaffinity_np.c |  28 +++++
 support/xthread.h                      |   3 +
 sysdeps/htl/pthreadP.h                 |   2 -
 sysdeps/posix/raise.c                  |  11 +-
 sysdeps/unix/sysv/linux/createthread.c | 153 -------------------------
 sysdeps/unix/sysv/linux/raise.c        |  52 ---------
 28 files changed, 369 insertions(+), 653 deletions(-)
 delete mode 100644 nptl/createthread.c
 create mode 100644 nptl/tst-pthread-attr-affinity-fail.c
 create mode 100644 support/xpthread_attr_setaffinity_np.c
 delete mode 100644 sysdeps/unix/sysv/linux/createthread.c
 delete mode 100644 sysdeps/unix/sysv/linux/raise.c

-- 
2.30.2

[PATCH 01/11] nptl: Move Linux createthread to nptl

[Adhemerval Zanella]

git mv -f sysdeps/unix/sysv/linux/createthread.c nptl/createthread.c

No functional change.
---
 nptl/createthread.c                    | 124 ++++++++++++++++++--
 sysdeps/unix/sysv/linux/createthread.c | 153 -------------------------
 2 files changed, 116 insertions(+), 161 deletions(-)
 delete mode 100644 sysdeps/unix/sysv/linux/createthread.c

diff --git a/nptl/createthread.c b/nptl/createthread.c
index 46943b33fe..bc3409b326 100644
--- a/nptl/createthread.c
+++ b/nptl/createthread.c
@@ -1,6 +1,7 @@
-/* Low-level thread creation for NPTL.  Stub version.
-   Copyright (C) 2014-2021 Free Software Foundation, Inc.
+/* Low-level thread creation for NPTL.  Linux version.
+   Copyright (C) 2002-2021 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
+   Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
 
    The GNU C Library is free software; you can redistribute it and/or
    modify it under the terms of the GNU Lesser General Public
@@ -16,30 +17,137 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+#include <sched.h>
+#include <setjmp.h>
+#include <signal.h>
+#include <stdlib.h>
+#include <atomic.h>
+#include <ldsodefs.h>
+#include <tls.h>
+#include <stdint.h>
+
+#include <arch-fork.h>
+
+#ifdef __NR_clone2
+# define ARCH_CLONE __clone2
+#else
+# define ARCH_CLONE __clone
+#endif
+
 /* See the comments in pthread_create.c for the requirements for these
    two macros and the create_thread function.  */
 
 #define START_THREAD_DEFN \
-  static void __attribute__ ((noreturn)) start_thread (void)
-#define START_THREAD_SELF THREAD_SELF
+  static int __attribute__ ((noreturn)) start_thread (void *arg)
+#define START_THREAD_SELF arg
+
+/* pthread_create.c defines this using START_THREAD_DEFN
+   We need a forward declaration here so we can take its address.  */
+static int start_thread (void *arg) __attribute__ ((noreturn));
 
 static int
 create_thread (struct pthread *pd, const struct pthread_attr *attr,
 	       bool *stopped_start, STACK_VARIABLES_PARMS, bool *thread_ran)
 {
-  /* If the implementation needs to do some tweaks to the thread after
-     it has been created at the OS level, it can set STOPPED_START here.  */
+  /* Determine whether the newly created threads has to be started
+     stopped since we have to set the scheduling parameters or set the
+     affinity.  */
+  bool need_setaffinity = (attr != NULL && attr->extension != NULL
+			   && attr->extension->cpuset != 0);
+  if (attr != NULL
+      && (__glibc_unlikely (need_setaffinity)
+	  || __glibc_unlikely ((attr->flags & ATTR_FLAG_NOTINHERITSCHED) != 0)))
+    *stopped_start = true;
 
   pd->stopped_start = *stopped_start;
   if (__glibc_unlikely (*stopped_start))
-    /* See CONCURRENCY NOTES in nptl/pthread_create.c.  */
+    /* See CONCURRENCY NOTES in nptl/pthread_creat.c.  */
     lll_lock (pd->lock, LLL_PRIVATE);
 
-  return ENOSYS;
+  /* We rely heavily on various flags the CLONE function understands:
+
+     CLONE_VM, CLONE_FS, CLONE_FILES
+	These flags select semantics with shared address space and
+	file descriptors according to what POSIX requires.
+
+     CLONE_SIGHAND, CLONE_THREAD
+	This flag selects the POSIX signal semantics and various
+	other kinds of sharing (itimers, POSIX timers, etc.).
+
+     CLONE_SETTLS
+	The sixth parameter to CLONE determines the TLS area for the
+	new thread.
+
+     CLONE_PARENT_SETTID
+	The kernels writes the thread ID of the newly created thread
+	into the location pointed to by the fifth parameters to CLONE.
+
+	Note that it would be semantically equivalent to use
+	CLONE_CHILD_SETTID but it is be more expensive in the kernel.
+
+     CLONE_CHILD_CLEARTID
+	The kernels clears the thread ID of a thread that has called
+	sys_exit() in the location pointed to by the seventh parameter
+	to CLONE.
+
+     The termination signal is chosen to be zero which means no signal
+     is sent.  */
+  const int clone_flags = (CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SYSVSEM
+			   | CLONE_SIGHAND | CLONE_THREAD
+			   | CLONE_SETTLS | CLONE_PARENT_SETTID
+			   | CLONE_CHILD_CLEARTID
+			   | 0);
+
+  TLS_DEFINE_INIT_TP (tp, pd);
+
+  if (__glibc_unlikely (ARCH_CLONE (&start_thread, STACK_VARIABLES_ARGS,
+				    clone_flags, pd, &pd->tid, tp, &pd->tid)
+			== -1))
+    return errno;
 
   /* It's started now, so if we fail below, we'll have to cancel it
      and let it clean itself up.  */
   *thread_ran = true;
 
+  /* Now we have the possibility to set scheduling parameters etc.  */
+  if (attr != NULL)
+    {
+      int res;
+
+      /* Set the affinity mask if necessary.  */
+      if (need_setaffinity)
+	{
+	  assert (*stopped_start);
+
+	  res = INTERNAL_SYSCALL_CALL (sched_setaffinity, pd->tid,
+				       attr->extension->cpusetsize,
+				       attr->extension->cpuset);
+
+	  if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (res)))
+	  err_out:
+	    {
+	      /* The operation failed.  We have to kill the thread.
+		 We let the normal cancellation mechanism do the work.  */
+
+	      pid_t pid = __getpid ();
+	      INTERNAL_SYSCALL_CALL (tgkill, pid, pd->tid, SIGCANCEL);
+
+	      return INTERNAL_SYSCALL_ERRNO (res);
+	    }
+	}
+
+      /* Set the scheduling parameters.  */
+      if ((attr->flags & ATTR_FLAG_NOTINHERITSCHED) != 0)
+	{
+	  assert (*stopped_start);
+
+	  res = INTERNAL_SYSCALL_CALL (sched_setscheduler, pd->tid,
+				       pd->schedpolicy, &pd->schedparam);
+
+	  if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (res)))
+	    goto err_out;
+	}
+    }
+
   return 0;
 }
diff --git a/sysdeps/unix/sysv/linux/createthread.c b/sysdeps/unix/sysv/linux/createthread.c
deleted file mode 100644
index bc3409b326..0000000000
--- a/sysdeps/unix/sysv/linux/createthread.c
+++ /dev/null
@@ -1,153 +0,0 @@
-/* Low-level thread creation for NPTL.  Linux version.
-   Copyright (C) 2002-2021 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
-
-   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
-   <https://www.gnu.org/licenses/>.  */
-
-#include <sched.h>
-#include <setjmp.h>
-#include <signal.h>
-#include <stdlib.h>
-#include <atomic.h>
-#include <ldsodefs.h>
-#include <tls.h>
-#include <stdint.h>
-
-#include <arch-fork.h>
-
-#ifdef __NR_clone2
-# define ARCH_CLONE __clone2
-#else
-# define ARCH_CLONE __clone
-#endif
-
-/* See the comments in pthread_create.c for the requirements for these
-   two macros and the create_thread function.  */
-
-#define START_THREAD_DEFN \
-  static int __attribute__ ((noreturn)) start_thread (void *arg)
-#define START_THREAD_SELF arg
-
-/* pthread_create.c defines this using START_THREAD_DEFN
-   We need a forward declaration here so we can take its address.  */
-static int start_thread (void *arg) __attribute__ ((noreturn));
-
-static int
-create_thread (struct pthread *pd, const struct pthread_attr *attr,
-	       bool *stopped_start, STACK_VARIABLES_PARMS, bool *thread_ran)
-{
-  /* Determine whether the newly created threads has to be started
-     stopped since we have to set the scheduling parameters or set the
-     affinity.  */
-  bool need_setaffinity = (attr != NULL && attr->extension != NULL
-			   && attr->extension->cpuset != 0);
-  if (attr != NULL
-      && (__glibc_unlikely (need_setaffinity)
-	  || __glibc_unlikely ((attr->flags & ATTR_FLAG_NOTINHERITSCHED) != 0)))
-    *stopped_start = true;
-
-  pd->stopped_start = *stopped_start;
-  if (__glibc_unlikely (*stopped_start))
-    /* See CONCURRENCY NOTES in nptl/pthread_creat.c.  */
-    lll_lock (pd->lock, LLL_PRIVATE);
-
-  /* We rely heavily on various flags the CLONE function understands:
-
-     CLONE_VM, CLONE_FS, CLONE_FILES
-	These flags select semantics with shared address space and
-	file descriptors according to what POSIX requires.
-
-     CLONE_SIGHAND, CLONE_THREAD
-	This flag selects the POSIX signal semantics and various
-	other kinds of sharing (itimers, POSIX timers, etc.).
-
-     CLONE_SETTLS
-	The sixth parameter to CLONE determines the TLS area for the
-	new thread.
-
-     CLONE_PARENT_SETTID
-	The kernels writes the thread ID of the newly created thread
-	into the location pointed to by the fifth parameters to CLONE.
-
-	Note that it would be semantically equivalent to use
-	CLONE_CHILD_SETTID but it is be more expensive in the kernel.
-
-     CLONE_CHILD_CLEARTID
-	The kernels clears the thread ID of a thread that has called
-	sys_exit() in the location pointed to by the seventh parameter
-	to CLONE.
-
-     The termination signal is chosen to be zero which means no signal
-     is sent.  */
-  const int clone_flags = (CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SYSVSEM
-			   | CLONE_SIGHAND | CLONE_THREAD
-			   | CLONE_SETTLS | CLONE_PARENT_SETTID
-			   | CLONE_CHILD_CLEARTID
-			   | 0);
-
-  TLS_DEFINE_INIT_TP (tp, pd);
-
-  if (__glibc_unlikely (ARCH_CLONE (&start_thread, STACK_VARIABLES_ARGS,
-				    clone_flags, pd, &pd->tid, tp, &pd->tid)
-			== -1))
-    return errno;
-
-  /* It's started now, so if we fail below, we'll have to cancel it
-     and let it clean itself up.  */
-  *thread_ran = true;
-
-  /* Now we have the possibility to set scheduling parameters etc.  */
-  if (attr != NULL)
-    {
-      int res;
-
-      /* Set the affinity mask if necessary.  */
-      if (need_setaffinity)
-	{
-	  assert (*stopped_start);
-
-	  res = INTERNAL_SYSCALL_CALL (sched_setaffinity, pd->tid,
-				       attr->extension->cpusetsize,
-				       attr->extension->cpuset);
-
-	  if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (res)))
-	  err_out:
-	    {
-	      /* The operation failed.  We have to kill the thread.
-		 We let the normal cancellation mechanism do the work.  */
-
-	      pid_t pid = __getpid ();
-	      INTERNAL_SYSCALL_CALL (tgkill, pid, pd->tid, SIGCANCEL);
-
-	      return INTERNAL_SYSCALL_ERRNO (res);
-	    }
-	}
-
-      /* Set the scheduling parameters.  */
-      if ((attr->flags & ATTR_FLAG_NOTINHERITSCHED) != 0)
-	{
-	  assert (*stopped_start);
-
-	  res = INTERNAL_SYSCALL_CALL (sched_setscheduler, pd->tid,
-				       pd->schedpolicy, &pd->schedparam);
-
-	  if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (res)))
-	    goto err_out;
-	}
-    }
-
-  return 0;
-}
-- 
2.30.2

Re: [PATCH 01/11] nptl: Move Linux createthread to nptl

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> git mv -f sysdeps/unix/sysv/linux/createthread.c nptl/createthread.c
>
> No functional change.

Looks good.  Please commit this separately, thanks.

Florian

[PATCH 02/11] nptl: Move createthread to pthread_create

[Adhemerval Zanella]

The 'create_thread' function is moved to pthread_create.c.  It removes
the START_THREAD_DEFN and START_THREAD_SELF macros and make the
lock usage more clear (no need to cross-reference multiple files).

No functional change.
---
 nptl/createthread.c   | 153 ------------------------------------------
 nptl/pthread_create.c | 120 +++++++++++++++++++++++++++++++--
 2 files changed, 113 insertions(+), 160 deletions(-)
 delete mode 100644 nptl/createthread.c

diff --git a/nptl/createthread.c b/nptl/createthread.c
deleted file mode 100644
index bc3409b326..0000000000
--- a/nptl/createthread.c
+++ /dev/null
@@ -1,153 +0,0 @@
-/* Low-level thread creation for NPTL.  Linux version.
-   Copyright (C) 2002-2021 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
-
-   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
-   <https://www.gnu.org/licenses/>.  */
-
-#include <sched.h>
-#include <setjmp.h>
-#include <signal.h>
-#include <stdlib.h>
-#include <atomic.h>
-#include <ldsodefs.h>
-#include <tls.h>
-#include <stdint.h>
-
-#include <arch-fork.h>
-
-#ifdef __NR_clone2
-# define ARCH_CLONE __clone2
-#else
-# define ARCH_CLONE __clone
-#endif
-
-/* See the comments in pthread_create.c for the requirements for these
-   two macros and the create_thread function.  */
-
-#define START_THREAD_DEFN \
-  static int __attribute__ ((noreturn)) start_thread (void *arg)
-#define START_THREAD_SELF arg
-
-/* pthread_create.c defines this using START_THREAD_DEFN
-   We need a forward declaration here so we can take its address.  */
-static int start_thread (void *arg) __attribute__ ((noreturn));
-
-static int
-create_thread (struct pthread *pd, const struct pthread_attr *attr,
-	       bool *stopped_start, STACK_VARIABLES_PARMS, bool *thread_ran)
-{
-  /* Determine whether the newly created threads has to be started
-     stopped since we have to set the scheduling parameters or set the
-     affinity.  */
-  bool need_setaffinity = (attr != NULL && attr->extension != NULL
-			   && attr->extension->cpuset != 0);
-  if (attr != NULL
-      && (__glibc_unlikely (need_setaffinity)
-	  || __glibc_unlikely ((attr->flags & ATTR_FLAG_NOTINHERITSCHED) != 0)))
-    *stopped_start = true;
-
-  pd->stopped_start = *stopped_start;
-  if (__glibc_unlikely (*stopped_start))
-    /* See CONCURRENCY NOTES in nptl/pthread_creat.c.  */
-    lll_lock (pd->lock, LLL_PRIVATE);
-
-  /* We rely heavily on various flags the CLONE function understands:
-
-     CLONE_VM, CLONE_FS, CLONE_FILES
-	These flags select semantics with shared address space and
-	file descriptors according to what POSIX requires.
-
-     CLONE_SIGHAND, CLONE_THREAD
-	This flag selects the POSIX signal semantics and various
-	other kinds of sharing (itimers, POSIX timers, etc.).
-
-     CLONE_SETTLS
-	The sixth parameter to CLONE determines the TLS area for the
-	new thread.
-
-     CLONE_PARENT_SETTID
-	The kernels writes the thread ID of the newly created thread
-	into the location pointed to by the fifth parameters to CLONE.
-
-	Note that it would be semantically equivalent to use
-	CLONE_CHILD_SETTID but it is be more expensive in the kernel.
-
-     CLONE_CHILD_CLEARTID
-	The kernels clears the thread ID of a thread that has called
-	sys_exit() in the location pointed to by the seventh parameter
-	to CLONE.
-
-     The termination signal is chosen to be zero which means no signal
-     is sent.  */
-  const int clone_flags = (CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SYSVSEM
-			   | CLONE_SIGHAND | CLONE_THREAD
-			   | CLONE_SETTLS | CLONE_PARENT_SETTID
-			   | CLONE_CHILD_CLEARTID
-			   | 0);
-
-  TLS_DEFINE_INIT_TP (tp, pd);
-
-  if (__glibc_unlikely (ARCH_CLONE (&start_thread, STACK_VARIABLES_ARGS,
-				    clone_flags, pd, &pd->tid, tp, &pd->tid)
-			== -1))
-    return errno;
-
-  /* It's started now, so if we fail below, we'll have to cancel it
-     and let it clean itself up.  */
-  *thread_ran = true;
-
-  /* Now we have the possibility to set scheduling parameters etc.  */
-  if (attr != NULL)
-    {
-      int res;
-
-      /* Set the affinity mask if necessary.  */
-      if (need_setaffinity)
-	{
-	  assert (*stopped_start);
-
-	  res = INTERNAL_SYSCALL_CALL (sched_setaffinity, pd->tid,
-				       attr->extension->cpusetsize,
-				       attr->extension->cpuset);
-
-	  if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (res)))
-	  err_out:
-	    {
-	      /* The operation failed.  We have to kill the thread.
-		 We let the normal cancellation mechanism do the work.  */
-
-	      pid_t pid = __getpid ();
-	      INTERNAL_SYSCALL_CALL (tgkill, pid, pd->tid, SIGCANCEL);
-
-	      return INTERNAL_SYSCALL_ERRNO (res);
-	    }
-	}
-
-      /* Set the scheduling parameters.  */
-      if ((attr->flags & ATTR_FLAG_NOTINHERITSCHED) != 0)
-	{
-	  assert (*stopped_start);
-
-	  res = INTERNAL_SYSCALL_CALL (sched_setscheduler, pd->tid,
-				       pd->schedpolicy, &pd->schedparam);
-
-	  if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (res)))
-	    goto err_out;
-	}
-    }
-
-  return 0;
-}
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 5680687efe..b53e3f30a0 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -215,9 +215,6 @@ late_init (void)
 
 /* CREATE THREAD NOTES:
 
-   createthread.c defines the create_thread function, and two macros:
-   START_THREAD_DEFN and START_THREAD_SELF (see below).
-
    create_thread must initialize PD->stopped_start.  It should be true
    if the STOPPED_START parameter is true, or if create_thread needs the
    new thread to synchronize at startup for some other implementation
@@ -242,20 +239,129 @@ late_init (void)
    so create_thread need not do that.  On failure, *THREAD_RAN should
    be set to true iff the thread actually started up and then got
    canceled before calling user code (*PD->start_routine).  */
+
+static int _Noreturn start_thread (void *arg);
+
 static int create_thread (struct pthread *pd, const struct pthread_attr *attr,
 			  bool *stopped_start, STACK_VARIABLES_PARMS,
-			  bool *thread_ran);
+			  bool *thread_ran)
+{
+  /* Determine whether the newly created threads has to be started
+     stopped since we have to set the scheduling parameters or set the
+     affinity.  */
+  bool need_setaffinity = (attr != NULL && attr->extension != NULL
+			   && attr->extension->cpuset != 0);
+  if (attr != NULL
+      && (__glibc_unlikely (need_setaffinity)
+	  || __glibc_unlikely ((attr->flags & ATTR_FLAG_NOTINHERITSCHED) != 0)))
+    *stopped_start = true;
+
+  pd->stopped_start = *stopped_start;
+  if (__glibc_unlikely (*stopped_start))
+    lll_lock (pd->lock, LLL_PRIVATE);
+
+  /* We rely heavily on various flags the CLONE function understands:
+
+     CLONE_VM, CLONE_FS, CLONE_FILES
+	These flags select semantics with shared address space and
+	file descriptors according to what POSIX requires.
+
+     CLONE_SIGHAND, CLONE_THREAD
+	This flag selects the POSIX signal semantics and various
+	other kinds of sharing (itimers, POSIX timers, etc.).
+
+     CLONE_SETTLS
+	The sixth parameter to CLONE determines the TLS area for the
+	new thread.
+
+     CLONE_PARENT_SETTID
+	The kernels writes the thread ID of the newly created thread
+	into the location pointed to by the fifth parameters to CLONE.
+
+	Note that it would be semantically equivalent to use
+	CLONE_CHILD_SETTID but it is be more expensive in the kernel.
+
+     CLONE_CHILD_CLEARTID
+	The kernels clears the thread ID of a thread that has called
+	sys_exit() in the location pointed to by the seventh parameter
+	to CLONE.
+
+     The termination signal is chosen to be zero which means no signal
+     is sent.  */
+  const int clone_flags = (CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SYSVSEM
+			   | CLONE_SIGHAND | CLONE_THREAD
+			   | CLONE_SETTLS | CLONE_PARENT_SETTID
+			   | CLONE_CHILD_CLEARTID
+			   | 0);
+
+  TLS_DEFINE_INIT_TP (tp, pd);
+
+#ifdef __NR_clone2
+# define ARCH_CLONE __clone2
+#else
+# define ARCH_CLONE __clone
+#endif
+  if (__glibc_unlikely (ARCH_CLONE (&start_thread, STACK_VARIABLES_ARGS,
+				    clone_flags, pd, &pd->tid, tp, &pd->tid)
+			== -1))
+    return errno;
+
+  /* It's started now, so if we fail below, we'll have to cancel it
+     and let it clean itself up.  */
+  *thread_ran = true;
 
-#include <createthread.c>
+  /* Now we have the possibility to set scheduling parameters etc.  */
+  if (attr != NULL)
+    {
+      int res;
+
+      /* Set the affinity mask if necessary.  */
+      if (need_setaffinity)
+	{
+	  assert (*stopped_start);
+
+	  res = INTERNAL_SYSCALL_CALL (sched_setaffinity, pd->tid,
+				       attr->extension->cpusetsize,
+				       attr->extension->cpuset);
+
+	  if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (res)))
+	  err_out:
+	    {
+	      /* The operation failed.  We have to kill the thread.
+		 We let the normal cancellation mechanism do the work.  */
+
+	      pid_t pid = __getpid ();
+	      INTERNAL_SYSCALL_CALL (tgkill, pid, pd->tid, SIGCANCEL);
+
+	      return INTERNAL_SYSCALL_ERRNO (res);
+	    }
+	}
+
+      /* Set the scheduling parameters.  */
+      if ((attr->flags & ATTR_FLAG_NOTINHERITSCHED) != 0)
+	{
+	  assert (*stopped_start);
+
+	  res = INTERNAL_SYSCALL_CALL (sched_setscheduler, pd->tid,
+				       pd->schedpolicy, &pd->schedparam);
+
+	  if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (res)))
+	    goto err_out;
+	}
+    }
+
+  return 0;
+}
 
 /* Local function to start thread and handle cleanup.
    createthread.c defines the macro START_THREAD_DEFN to the
    declaration that its create_thread function will refer to, and
    START_THREAD_SELF to the expression to optimally deliver the new
    thread's THREAD_SELF value.  */
-START_THREAD_DEFN
+static int _Noreturn
+start_thread (void *arg)
 {
-  struct pthread *pd = START_THREAD_SELF;
+  struct pthread *pd = arg;
 
   /* Initialize resolver state pointer.  */
   __resp = &pd->res;
-- 
2.30.2

Re: [PATCH 02/11] nptl: Move createthread to pthread_create

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> The 'create_thread' function is moved to pthread_create.c.  It removes
> the START_THREAD_DEFN and START_THREAD_SELF macros and make the
> lock usage more clear (no need to cross-reference multiple files).
>
> No functional change.

Please remove the remaining references to the macros

nptl/pthread_create.c:   unlocks PD->lock which synchronizes-with START_THREAD_DEFN in the
nptl/pthread_create.c:   createthread.c defines the macro START_THREAD_DEFN to the
nptl/pthread_create.c:   START_THREAD_SELF to the expression to optimally deliver the new

and commit this separately.  Thanks.

Florian

Re: [PATCH 02/11] nptl: Move createthread to pthread_create

[Adhemerval Zanella]

On 26/05/2021 14:16, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> The 'create_thread' function is moved to pthread_create.c.  It removes
>> the START_THREAD_DEFN and START_THREAD_SELF macros and make the
>> lock usage more clear (no need to cross-reference multiple files).
>>
>> No functional change.
> 
> Please remove the remaining references to the macros
> 
> nptl/pthread_create.c:   unlocks PD->lock which synchronizes-with START_THREAD_DEFN in the
> nptl/pthread_create.c:   createthread.c defines the macro START_THREAD_DEFN to the
> nptl/pthread_create.c:   START_THREAD_SELF to the expression to optimally deliver the new
> 
> and commit this separately.  Thanks.

Ack, I will remove them.

[PATCH 03/11] support: Add xpthread_attr_setaffinity_np wrapper

[Adhemerval Zanella]

---
 support/Makefile                       |  1 +
 support/xpthread_attr_setaffinity_np.c | 28 ++++++++++++++++++++++++++
 support/xthread.h                      |  3 +++
 3 files changed, 32 insertions(+)
 create mode 100644 support/xpthread_attr_setaffinity_np.c

diff --git a/support/Makefile b/support/Makefile
index da6dc58d37..0a4b057db5 100644
--- a/support/Makefile
+++ b/support/Makefile
@@ -128,6 +128,7 @@ libsupport-routines = \
   xpthread_attr_init \
   xpthread_attr_setdetachstate \
   xpthread_attr_setguardsize \
+  xpthread_attr_setaffinity_np \
   xpthread_attr_setstack \
   xpthread_attr_setstacksize \
   xpthread_barrier_destroy \
diff --git a/support/xpthread_attr_setaffinity_np.c b/support/xpthread_attr_setaffinity_np.c
new file mode 100644
index 0000000000..cddcd5fdb4
--- /dev/null
+++ b/support/xpthread_attr_setaffinity_np.c
@@ -0,0 +1,28 @@
+/* pthread_attr_setaffinity_np with error checking.
+   Copyright (C) 2021 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include <support/xthread.h>
+
+void
+xpthread_attr_setaffinity_np (pthread_attr_t *attr,
+                              size_t cpusetsize, const cpu_set_t *cpuset)
+{
+  xpthread_check_return ("pthread_attr_setaffinity_np",
+			 pthread_attr_setaffinity_np (attr, cpusetsize,
+                                                      cpuset));
+}
diff --git a/support/xthread.h b/support/xthread.h
index 1ba3f133ad..cffd9c1694 100644
--- a/support/xthread.h
+++ b/support/xthread.h
@@ -66,6 +66,9 @@ void *xpthread_join (pthread_t thr);
 void xpthread_once (pthread_once_t *guard, void (*func) (void));
 void xpthread_attr_destroy (pthread_attr_t *attr);
 void xpthread_attr_init (pthread_attr_t *attr);
+void xpthread_attr_setaffinity_np (pthread_attr_t *attr,
+                                   size_t cpusetsize,
+                                   const cpu_set_t *cpuset);
 void xpthread_attr_setdetachstate (pthread_attr_t *attr,
 				   int detachstate);
 void xpthread_attr_setstack (pthread_attr_t *attr, void *stackaddr,
-- 
2.30.2

Re: [PATCH 03/11] support: Add xpthread_attr_setaffinity_np wrapper

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> ---
>  support/Makefile                       |  1 +
>  support/xpthread_attr_setaffinity_np.c | 28 ++++++++++++++++++++++++++
>  support/xthread.h                      |  3 +++
>  3 files changed, 32 insertions(+)
>  create mode 100644 support/xpthread_attr_setaffinity_np.c

Looks good, please push this.

Thanks,
Florian

Re: [PATCH 03/11] support: Add xpthread_attr_setaffinity_np wrapper

[Joseph Myers]

I think this has broken the build for Hurd.

xpthread_attr_setaffinity_np.c: In function 'xpthread_attr_setaffinity_np':
xpthread_attr_setaffinity_np.c:26:26: error: implicit declaration of function 'pthread_attr_setaffinity_np'; did you mean 'xpthread_attr_setaffinity_np'? [-Werror=implicit-function-declaration]
   26 |                          pthread_attr_setaffinity_np (attr, cpusetsize,
      |                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                          xpthread_attr_setaffinity_np

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH 03/11] support: Add xpthread_attr_setaffinity_np wrapper

[Adhemerval Zanella]

On 27/05/2021 19:35, Joseph Myers wrote:
> I think this has broken the build for Hurd.
> 
> xpthread_attr_setaffinity_np.c: In function 'xpthread_attr_setaffinity_np':
> xpthread_attr_setaffinity_np.c:26:26: error: implicit declaration of function 'pthread_attr_setaffinity_np'; did you mean 'xpthread_attr_setaffinity_np'? [-Werror=implicit-function-declaration]
>    26 |                          pthread_attr_setaffinity_np (attr, cpusetsize,
>       |                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>       |                          xpthread_attr_setaffinity_np
> 

It does, I will fix it.

[PATCH 04/11] nptl: Add pthread_attr_setaffinity_np failure test

[Adhemerval Zanella]

It checks whether an invalid affinity mask does return an error,
similar to what sysdeps/pthread/tst-bad-schedattr.c does for
pthread_attr_setschedparam.

Checked on x86_64-linux-gnu.
---
 nptl/Makefile                         |  1 +
 nptl/tst-pthread-attr-affinity-fail.c | 54 +++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)
 create mode 100644 nptl/tst-pthread-attr-affinity-fail.c

diff --git a/nptl/Makefile b/nptl/Makefile
index 16eaf58948..9a5628b751 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -286,6 +286,7 @@ tests = tst-attr2 tst-attr3 tst-default-attr \
 	tst-exec4 tst-exec5 \
 	tst-stack2 tst-stack3 tst-stack4 \
 	tst-pthread-attr-affinity \
+	tst-pthread-attr-affinity-fail \
 	tst-dlsym1 \
 	tst-context1 \
 	tst-sched1 \
diff --git a/nptl/tst-pthread-attr-affinity-fail.c b/nptl/tst-pthread-attr-affinity-fail.c
new file mode 100644
index 0000000000..41a87ea8cb
--- /dev/null
+++ b/nptl/tst-pthread-attr-affinity-fail.c
@@ -0,0 +1,54 @@
+/* Check if invalid pthread_attr_getaffinity_np does not run any code
+   in the thread function.
+   Copyright (C) 2021 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <support/check.h>
+#include <support/xunistd.h>
+#include <support/xthread.h>
+#include <stdlib.h>
+
+static void *
+thr_func (void *arg)
+{
+  abort ();
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  int max_cpu = xsysconf (_SC_NPROCESSORS_CONF) + 1;
+  /* Set a affinaty mask with an invalid CPU.  */
+  cpu_set_t *cpuset = CPU_ALLOC (max_cpu);
+  size_t cpusetsize = CPU_ALLOC_SIZE (max_cpu);
+  CPU_ZERO_S (cpusetsize, cpuset);
+  CPU_SET_S (max_cpu, cpusetsize, cpuset);
+
+  pthread_attr_t attr;
+  xpthread_attr_init (&attr);
+  xpthread_attr_setaffinity_np (&attr, cpusetsize, cpuset);
+
+  pthread_t thr;
+  TEST_COMPARE (pthread_create (&thr, &attr, thr_func, NULL), EINVAL);
+  xpthread_attr_destroy (&attr);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.30.2

Re: [PATCH 04/11] nptl: Add pthread_attr_setaffinity_np failure test

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> It checks whether an invalid affinity mask does return an error,
> similar to what sysdeps/pthread/tst-bad-schedattr.c does for
> pthread_attr_setschedparam.
>
> Checked on x86_64-linux-gnu.
> ---
>  nptl/Makefile                         |  1 +
>  nptl/tst-pthread-attr-affinity-fail.c | 54 +++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+)
>  create mode 100644 nptl/tst-pthread-attr-affinity-fail.c
>
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 16eaf58948..9a5628b751 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -286,6 +286,7 @@ tests = tst-attr2 tst-attr3 tst-default-attr \
>  	tst-exec4 tst-exec5 \
>  	tst-stack2 tst-stack3 tst-stack4 \
>  	tst-pthread-attr-affinity \
> +	tst-pthread-attr-affinity-fail \
>  	tst-dlsym1 \
>  	tst-context1 \
>  	tst-sched1 \
> diff --git a/nptl/tst-pthread-attr-affinity-fail.c b/nptl/tst-pthread-attr-affinity-fail.c
> new file mode 100644
> index 0000000000..41a87ea8cb
> --- /dev/null
> +++ b/nptl/tst-pthread-attr-affinity-fail.c
> @@ -0,0 +1,54 @@
> +/* Check if invalid pthread_attr_getaffinity_np does not run any code
> +   in the thread function.
> +   Copyright (C) 2021 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
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <errno.h>
> +#include <support/check.h>
> +#include <support/xunistd.h>
> +#include <support/xthread.h>
> +#include <stdlib.h>
> +
> +static void *
> +thr_func (void *arg)
> +{
> +  abort ();
> +  return NULL;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  int max_cpu = xsysconf (_SC_NPROCESSORS_CONF) + 1;
> +  /* Set a affinaty mask with an invalid CPU.  */
> +  cpu_set_t *cpuset = CPU_ALLOC (max_cpu);

Perhaps:

TEST_VERIFY_EXIT (cpuset != NULL);

> +  size_t cpusetsize = CPU_ALLOC_SIZE (max_cpu);
> +  CPU_ZERO_S (cpusetsize, cpuset);
> +  CPU_SET_S (max_cpu, cpusetsize, cpuset);

You could check that sched_setaffinity with that mask fails.  Like this:

  TEST_COMPARE (sched_setaffinity (0, cpusetsize, cpuset), -1);
  TEST_COMPARE (errno, EINVAL);

But feel free to push this without this change.

Thanks,
Florian

Re: [PATCH 04/11] nptl: Add pthread_attr_setaffinity_np failure test

[Adhemerval Zanella]

On 26/05/2021 14:21, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> It checks whether an invalid affinity mask does return an error,
>> similar to what sysdeps/pthread/tst-bad-schedattr.c does for
>> pthread_attr_setschedparam.
>>
>> Checked on x86_64-linux-gnu.
>> ---
>>  nptl/Makefile                         |  1 +
>>  nptl/tst-pthread-attr-affinity-fail.c | 54 +++++++++++++++++++++++++++
>>  2 files changed, 55 insertions(+)
>>  create mode 100644 nptl/tst-pthread-attr-affinity-fail.c
>>
>> diff --git a/nptl/Makefile b/nptl/Makefile
>> index 16eaf58948..9a5628b751 100644
>> --- a/nptl/Makefile
>> +++ b/nptl/Makefile
>> @@ -286,6 +286,7 @@ tests = tst-attr2 tst-attr3 tst-default-attr \
>>  	tst-exec4 tst-exec5 \
>>  	tst-stack2 tst-stack3 tst-stack4 \
>>  	tst-pthread-attr-affinity \
>> +	tst-pthread-attr-affinity-fail \
>>  	tst-dlsym1 \
>>  	tst-context1 \
>>  	tst-sched1 \
>> diff --git a/nptl/tst-pthread-attr-affinity-fail.c b/nptl/tst-pthread-attr-affinity-fail.c
>> new file mode 100644
>> index 0000000000..41a87ea8cb
>> --- /dev/null
>> +++ b/nptl/tst-pthread-attr-affinity-fail.c
>> @@ -0,0 +1,54 @@
>> +/* Check if invalid pthread_attr_getaffinity_np does not run any code
>> +   in the thread function.
>> +   Copyright (C) 2021 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
>> +   <https://www.gnu.org/licenses/>.  */
>> +
>> +#include <errno.h>
>> +#include <support/check.h>
>> +#include <support/xunistd.h>
>> +#include <support/xthread.h>
>> +#include <stdlib.h>
>> +
>> +static void *
>> +thr_func (void *arg)
>> +{
>> +  abort ();
>> +  return NULL;
>> +}
>> +
>> +static int
>> +do_test (void)
>> +{
>> +  int max_cpu = xsysconf (_SC_NPROCESSORS_CONF) + 1;
>> +  /* Set a affinaty mask with an invalid CPU.  */
>> +  cpu_set_t *cpuset = CPU_ALLOC (max_cpu);
> 
> Perhaps:
> 
> TEST_VERIFY_EXIT (cpuset != NULL);

Ack.

> 
>> +  size_t cpusetsize = CPU_ALLOC_SIZE (max_cpu);
>> +  CPU_ZERO_S (cpusetsize, cpuset);
>> +  CPU_SET_S (max_cpu, cpusetsize, cpuset);
> 
> You could check that sched_setaffinity with that mask fails.  Like this:
> 
>   TEST_COMPARE (sched_setaffinity (0, cpusetsize, cpuset), -1);
>   TEST_COMPARE (errno, EINVAL);
> 
> But feel free to push this without this change.

I will integrate your suggestions.

[PATCH 05/11] nptl: Deallocate the thread stack on setup failure (BZ #19511)

[Adhemerval Zanella]

To setup either the thread scheduling parameters or affinity,
pthread_create enforce synchronization on created thread wait until
its parent either unlock the 'struct pthread' or send a cancellation
signal if a failure occurs.

However, cancelling the thread does not deallocate the newly created
stack since cancellation expects that a pthread_join to deallocate any
allocated thread resouces (threads stack or TLS).

I used a different strategy than the one proposed on BZ#19511
comment #4.  Making the parent responsible for the cleanup requires
additional synchronization similar to what pthread_join does.  Instead,
this patch reassigns an unused 'struct pthread' member,
parent_cancelhandling_unsed, to indicate whether the setup has failed
and set the thread itself to deallocate the allocated resouces (similar
to what detached mode does).

This strategy also simplifies by not using thread cancellation and
thus not running libgcc_so load in the signal handler (which is
avoided in thread cancellation since 'pthread_cancel' is the one
responsible to dlopen libgcc_s).

Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
 nptl/allocatestack.c  |  1 +
 nptl/descr.h          |  5 ++--
 nptl/pthread_create.c | 56 +++++++++++++++++++------------------------
 3 files changed, 29 insertions(+), 33 deletions(-)

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index dc81a2ca73..2114bd2e27 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -161,6 +161,7 @@ get_cached_stack (size_t *sizep, void **memp)
   /* Cancellation handling is back to the default.  */
   result->cancelhandling = 0;
   result->cleanup = NULL;
+  result->setup_failed = 0;
 
   /* No pending event.  */
   result->nextevent = NULL;
diff --git a/nptl/descr.h b/nptl/descr.h
index 3de9535449..9d8297b45f 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -340,8 +340,9 @@ struct pthread
   /* True if thread must stop at startup time.  */
   bool stopped_start;
 
-  /* Formerly used for dealing with cancellation.  */
-  int parent_cancelhandling_unsed;
+  /* Indicate that a thread creation setup has failed (for instance the
+     scheduler or affinity).  */
+  int setup_failed;
 
   /* Lock to synchronize access to the descriptor.  */
   int lock;
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index b53e3f30a0..d6b907827a 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -176,8 +176,6 @@ late_init (void)
        state.  Ownership cannot be released to the process via the
        return of pthread_create since a non-zero result entails PD is
        undefined and therefore cannot be joined to free the resources.
-       We privately call pthread_join on the thread to finish handling
-       the resource shutdown (Or at least we should, see bug 19511).
 
    (e) If the thread creation failed and THREAD_RAN is false, then the
        creating thread retains ownership of PD and must cleanup state.
@@ -313,28 +311,19 @@ static int create_thread (struct pthread *pd, const struct pthread_attr *attr,
   /* Now we have the possibility to set scheduling parameters etc.  */
   if (attr != NULL)
     {
-      int res;
-
       /* Set the affinity mask if necessary.  */
       if (need_setaffinity)
 	{
 	  assert (*stopped_start);
 
-	  res = INTERNAL_SYSCALL_CALL (sched_setaffinity, pd->tid,
-				       attr->extension->cpusetsize,
-				       attr->extension->cpuset);
-
+	  int res = INTERNAL_SYSCALL_CALL (sched_setaffinity, pd->tid,
+				           attr->extension->cpusetsize,
+				           attr->extension->cpuset);
 	  if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (res)))
-	  err_out:
-	    {
-	      /* The operation failed.  We have to kill the thread.
-		 We let the normal cancellation mechanism do the work.  */
-
-	      pid_t pid = __getpid ();
-	      INTERNAL_SYSCALL_CALL (tgkill, pid, pd->tid, SIGCANCEL);
-
-	      return INTERNAL_SYSCALL_ERRNO (res);
-	    }
+            {
+              pd->setup_failed = 1;
+              return INTERNAL_SYSCALL_ERRNO (res);
+            }
 	}
 
       /* Set the scheduling parameters.  */
@@ -342,11 +331,13 @@ static int create_thread (struct pthread *pd, const struct pthread_attr *attr,
 	{
 	  assert (*stopped_start);
 
-	  res = INTERNAL_SYSCALL_CALL (sched_setscheduler, pd->tid,
-				       pd->schedpolicy, &pd->schedparam);
-
+	  int res = INTERNAL_SYSCALL_CALL (sched_setscheduler, pd->tid,
+				           pd->schedpolicy, &pd->schedparam);
 	  if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (res)))
-	    goto err_out;
+            {
+              pd->setup_failed = 1;
+              return INTERNAL_SYSCALL_ERRNO (res);
+            }
 	}
     }
 
@@ -426,8 +417,6 @@ start_thread (void *arg)
 	 have ownership (see CONCURRENCY NOTES above).  */
       if (__glibc_unlikely (pd->stopped_start))
 	{
-	  int oldtype = LIBC_CANCEL_ASYNC ();
-
 	  /* Get the lock the parent locked to force synchronization.  */
 	  lll_lock (pd->lock, LLL_PRIVATE);
 
@@ -435,10 +424,11 @@ start_thread (void *arg)
 
 	  /* And give it up right away.  */
 	  lll_unlock (pd->lock, LLL_PRIVATE);
-
-	  LIBC_CANCEL_RESET (oldtype);
 	}
 
+      if (pd->setup_failed == 1)
+        __do_cancel ();
+
       LIBC_PROBE (pthread_start, 3, (pthread_t) pd, pd->start_routine, pd->arg);
 
       /* Run the code the user provided.  */
@@ -563,8 +553,8 @@ start_thread (void *arg)
       pd->setxid_futex = 0;
     }
 
-  /* If the thread is detached free the TCB.  */
-  if (IS_DETACHED (pd))
+  /* If the thread is detached or an setup error occurred, free the TCB.  */
+  if (IS_DETACHED (pd) || pd->setup_failed == 1)
     /* Free the TCB.  */
     __nptl_free_tcb (pd);
 
@@ -819,9 +809,13 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
 	   CONCURRENCY NOTES above).  We can assert that STOPPED_START
 	   must have been true because thread creation didn't fail, but
 	   thread attribute setting did.  */
-	/* See bug 19511 which explains why doing nothing here is a
-	   resource leak for a joinable thread.  */
-	assert (stopped_start);
+        {
+	  assert (stopped_start);
+          if (stopped_start)
+  	    /* State (a), we own PD. The thread blocked on this lock will
+               finish due setup_failed being set.  */
+	    lll_unlock (pd->lock, LLL_PRIVATE);
+        }
       else
 	{
 	  /* State (e) and we have ownership of PD (see CONCURRENCY
-- 
2.30.2

Re: [PATCH 05/11] nptl: Deallocate the thread stack on setup failure (BZ #19511)

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> @@ -819,9 +809,13 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>  	   CONCURRENCY NOTES above).  We can assert that STOPPED_START
>  	   must have been true because thread creation didn't fail, but
>  	   thread attribute setting did.  */
> -	/* See bug 19511 which explains why doing nothing here is a
> -	   resource leak for a joinable thread.  */
> -	assert (stopped_start);
> +        {
> +	  assert (stopped_start);
> +          if (stopped_start)
> +  	    /* State (a), we own PD. The thread blocked on this lock will
> +               finish due setup_failed being set.  */
> +	    lll_unlock (pd->lock, LLL_PRIVATE);
> +        }
>        else
>  	{
>  	  /* State (e) and we have ownership of PD (see CONCURRENCY

The assert followed by the if doesn't look right.  One should check
setup_failed, I assume.  Is there any way to trigger the broken assert
or the hang from the missing wakeup?

I happend to have looked at this today for the sigaltstack stuff.

I felt that it would be simpler to perform the kernel-side setup
(affinity and scheduling policy) on the new thread.  Essentially use
stopped_start mode unconditionally.  And use a separate lock word for
the barrier/latch, instead of reusing the pd->lock.

The advantage is that we can pass an arbitrary temporary object into the
internal start function, such as the signal mask.  The creating thread
blocks until the initialization has happened and the immediate fate of
the thread has been decided.  After that point the creating thread
deallocates the temporary object, and if the thread couldn't start, any
thread-related data.  Otherwise the new thread takes ownership of that
data.

I think it will be much easier to explain what is going on in the
concurrency notes.  We have an additional synchronization step on all
created threads, but I doubt that this matters in practice.

What do you think?  I'm not sure if the change should happen as part of
this series.

Thanks,
Florian

Re: [PATCH 05/11] nptl: Deallocate the thread stack on setup failure (BZ #19511)

[Adhemerval Zanella]

On 26/05/2021 14:33, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> @@ -819,9 +809,13 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>>  	   CONCURRENCY NOTES above).  We can assert that STOPPED_START
>>  	   must have been true because thread creation didn't fail, but
>>  	   thread attribute setting did.  */
>> -	/* See bug 19511 which explains why doing nothing here is a
>> -	   resource leak for a joinable thread.  */
>> -	assert (stopped_start);
>> +        {
>> +	  assert (stopped_start);
>> +          if (stopped_start)
>> +  	    /* State (a), we own PD. The thread blocked on this lock will
>> +               finish due setup_failed being set.  */
>> +	    lll_unlock (pd->lock, LLL_PRIVATE);
>> +        }
>>        else
>>  	{
>>  	  /* State (e) and we have ownership of PD (see CONCURRENCY
> 
> The assert followed by the if doesn't look right.  One should check
> setup_failed, I assume.  Is there any way to trigger the broken assert
> or the hang from the missing wakeup?

This just check what is described in the CONCURRENCY NOTES:

204    Axioms:                                                                                          
205 
206    * The create_thread function can never set stopped_start to false.                               
207    * The created thread can read stopped_start but *never write to it*.    

> 
> I happend to have looked at this today for the sigaltstack stuff.
> 
> I felt that it would be simpler to perform the kernel-side setup
> (affinity and scheduling policy) on the new thread.  Essentially use
> stopped_start mode unconditionally.  And use a separate lock word for
> the barrier/latch, instead of reusing the pd->lock.
> 
> The advantage is that we can pass an arbitrary temporary object into the
> internal start function, such as the signal mask.  The creating thread
> blocks until the initialization has happened and the immediate fate of
> the thread has been decided.  After that point the creating thread
> deallocates the temporary object, and if the thread couldn't start, any
> thread-related data.  Otherwise the new thread takes ownership of that
> data.
> 
> I think it will be much easier to explain what is going on in the
> concurrency notes.  We have an additional synchronization step on all
> created threads, but I doubt that this matters in practice.
> 
> What do you think?  I'm not sure if the change should happen as part of
> this series.

I see two potential issues on this approach:

  1. The cpuset information is tied to the pthread_attr_t and can be
     potentially large, it means that if we want to make the created
     thread to consume we will need to either duplicate, add something
     like a reference count, or add more synchronization for the 
     consumer.  None is really simpler to implement and I don't think 
     it is really an improvement.

  2. For both schedule parameters and affinity ideally we want to inform
     right away to user if the operation has failed.  If we move it to
     thread itself, it still would require the synchronization to inform
     pthread_create the operation has failed.  I don't see much improvement,
     the pthread_create latency will be essentially the same.

After reading the CONCURRENCY NOTES (great work on this btw), I think the
main issue on the resource deallocation here is mixing pthread cancellation
and thread creation.

Re: [PATCH 05/11] nptl: Deallocate the thread stack on setup failure (BZ #19511)

[Florian Weimer]

* Adhemerval Zanella:

> On 26/05/2021 14:33, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>> @@ -819,9 +809,13 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>>>  	   CONCURRENCY NOTES above).  We can assert that STOPPED_START
>>>  	   must have been true because thread creation didn't fail, but
>>>  	   thread attribute setting did.  */
>>> -	/* See bug 19511 which explains why doing nothing here is a
>>> -	   resource leak for a joinable thread.  */
>>> -	assert (stopped_start);
>>> +        {
>>> +	  assert (stopped_start);
>>> +          if (stopped_start)
>>> +  	    /* State (a), we own PD. The thread blocked on this lock will
>>> +               finish due setup_failed being set.  */
>>> +	    lll_unlock (pd->lock, LLL_PRIVATE);
>>> +        }
>>>        else
>>>  	{
>>>  	  /* State (e) and we have ownership of PD (see CONCURRENCY
>> 
>> The assert followed by the if doesn't look right.  One should check
>> setup_failed, I assume.  Is there any way to trigger the broken assert
>> or the hang from the missing wakeup?
>
> This just check what is described in the CONCURRENCY NOTES:
>
> 204    Axioms:                                                                                          
> 205 
> 206    * The create_thread function can never set stopped_start to false.                               
> 207    * The created thread can read stopped_start but *never write to it*.   

I meant this:

+	  assert (stopped_start);
+         if (stopped_start)

It doesn't look right to me.

>> I happend to have looked at this today for the sigaltstack stuff.
>> 
>> I felt that it would be simpler to perform the kernel-side setup
>> (affinity and scheduling policy) on the new thread.  Essentially use
>> stopped_start mode unconditionally.  And use a separate lock word for
>> the barrier/latch, instead of reusing the pd->lock.
>> 
>> The advantage is that we can pass an arbitrary temporary object into the
>> internal start function, such as the signal mask.  The creating thread
>> blocks until the initialization has happened and the immediate fate of
>> the thread has been decided.  After that point the creating thread
>> deallocates the temporary object, and if the thread couldn't start, any
>> thread-related data.  Otherwise the new thread takes ownership of that
>> data.
>> 
>> I think it will be much easier to explain what is going on in the
>> concurrency notes.  We have an additional synchronization step on all
>> created threads, but I doubt that this matters in practice.
>> 
>> What do you think?  I'm not sure if the change should happen as part of
>> this series.
>
> I see two potential issues on this approach:
>
>   1. The cpuset information is tied to the pthread_attr_t and can be
>      potentially large, it means that if we want to make the created
>      thread to consume we will need to either duplicate, add something
>      like a reference count, or add more synchronization for the 
>      consumer.  None is really simpler to implement and I don't think 
>      it is really an improvement.

No, I think that's covered by the temporary object thing.  The creating
thread will block until it's guaranteed that the new thread won't need
this data anymore.

>   2. For both schedule parameters and affinity ideally we want to inform
>      right away to user if the operation has failed.  If we move it to
>      thread itself, it still would require the synchronization to inform
>      pthread_create the operation has failed.  I don't see much improvement,
>      the pthread_create latency will be essentially the same.

Correct.  But it will be much simpler because we can treat all
kernel-side initialization the same.  For example, sigaltstack might
fail with invalid flags, too.  And we'd definitely have to run that on
the new thread.

Thanks,
Florian

Re: [PATCH 05/11] nptl: Deallocate the thread stack on setup failure (BZ #19511)

[Adhemerval Zanella]

On 26/05/2021 14:58, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 26/05/2021 14:33, Florian Weimer wrote:
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> @@ -819,9 +809,13 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>>>>  	   CONCURRENCY NOTES above).  We can assert that STOPPED_START
>>>>  	   must have been true because thread creation didn't fail, but
>>>>  	   thread attribute setting did.  */
>>>> -	/* See bug 19511 which explains why doing nothing here is a
>>>> -	   resource leak for a joinable thread.  */
>>>> -	assert (stopped_start);
>>>> +        {
>>>> +	  assert (stopped_start);
>>>> +          if (stopped_start)
>>>> +  	    /* State (a), we own PD. The thread blocked on this lock will
>>>> +               finish due setup_failed being set.  */
>>>> +	    lll_unlock (pd->lock, LLL_PRIVATE);
>>>> +        }
>>>>        else
>>>>  	{
>>>>  	  /* State (e) and we have ownership of PD (see CONCURRENCY
>>>
>>> The assert followed by the if doesn't look right.  One should check
>>> setup_failed, I assume.  Is there any way to trigger the broken assert
>>> or the hang from the missing wakeup?
>>
>> This just check what is described in the CONCURRENCY NOTES:
>>
>> 204    Axioms:                                                                                          
>> 205 
>> 206    * The create_thread function can never set stopped_start to false.                               
>> 207    * The created thread can read stopped_start but *never write to it*.   
> 
> I meant this:
> 
> +	  assert (stopped_start);
> +         if (stopped_start)
> 
> It doesn't look right to me.

Yeah, the 'if' check is not really required.  It will hit this code path
only if clone has succeeded ('thread_ran' is true) but either 
sched_setaffinity or sched_setscheduler has failed (in this case
'stopped_start' will be always true).

I changed to:

  if (thread_ran)
    {
      /* State (c) or (d) and we may not have PD ownership (see
         CONCURRENCY NOTES above).  We can assert that STOPPED_START
         must have been true because thread creation didn't fail, but
         thread attribute setting did.  */
      assert (stopped_start);
      /* State (a), we own PD. The thread blocked on this lock will
         finish due setup_failed being set.
         The thread will be responsible to cleanup any allocated
         resource.  */
      lll_unlock (pd->lock, LLL_PRIVATE).
    }

> 
>>> I happend to have looked at this today for the sigaltstack stuff.
>>>
>>> I felt that it would be simpler to perform the kernel-side setup
>>> (affinity and scheduling policy) on the new thread.  Essentially use
>>> stopped_start mode unconditionally.  And use a separate lock word for
>>> the barrier/latch, instead of reusing the pd->lock.
>>>
>>> The advantage is that we can pass an arbitrary temporary object into the
>>> internal start function, such as the signal mask.  The creating thread
>>> blocks until the initialization has happened and the immediate fate of
>>> the thread has been decided.  After that point the creating thread
>>> deallocates the temporary object, and if the thread couldn't start, any
>>> thread-related data.  Otherwise the new thread takes ownership of that
>>> data.
>>>
>>> I think it will be much easier to explain what is going on in the
>>> concurrency notes.  We have an additional synchronization step on all
>>> created threads, but I doubt that this matters in practice.
>>>
>>> What do you think?  I'm not sure if the change should happen as part of
>>> this series.
>>
>> I see two potential issues on this approach:
>>
>>   1. The cpuset information is tied to the pthread_attr_t and can be
>>      potentially large, it means that if we want to make the created
>>      thread to consume we will need to either duplicate, add something
>>      like a reference count, or add more synchronization for the 
>>      consumer.  None is really simpler to implement and I don't think 
>>      it is really an improvement.
> 
> No, I think that's covered by the temporary object thing.  The creating
> thread will block until it's guaranteed that the new thread won't need
> this data anymore.
> 
>>   2. For both schedule parameters and affinity ideally we want to inform
>>      right away to user if the operation has failed.  If we move it to
>>      thread itself, it still would require the synchronization to inform
>>      pthread_create the operation has failed.  I don't see much improvement,
>>      the pthread_create latency will be essentially the same.
> 
> Correct.  But it will be much simpler because we can treat all
> kernel-side initialization the same.  For example, sigaltstack might
> fail with invalid flags, too.  And we'd definitely have to run that on
> the new thread.

The small downside is we will need to consume some of the thread own stack
to pass such information, but it can be only a pointer in any case.

But I still see setting the scheduling/affinity synchronous to have some
advantage: we can fast exit in the case of failure and join the thread
on pthread_create (instead of making it act a detached one).  This make
the failure case not 'leak' the thread context and resource out of
pthread_create.

It might be somewhat complex to code it right now, since we need to take
care of destructor for thread_local variables and thread-local data,
__libc_thread_freeres, etc.

That is also why I used the current unwind mechanism to exit in case of
setup failure (maybe instead of adding the setup_failed on the IS_DETACHED
check the patch could just setup the thread as detached, the pd->joinid
could be set non-atomically since it owns the pd).

In any case, since we do not implement this I don't have a preference.
Your arbitrary temporary object seems ok and should work on the
scheduler/affinity setup.

Re: [PATCH 05/11] nptl: Deallocate the thread stack on setup failure (BZ #19511)

[Andreas Schwab]

Please use consistent indentation.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 05/11] nptl: Deallocate the thread stack on setup failure (BZ #19511)

[Adhemerval Zanella]

On 26/05/2021 15:21, Andreas Schwab wrote:
> Please use consistent indentation.
> 
> Andreas.
> 

I think I used whitespaces only, I will fix it.

Re: [PATCH 05/11] nptl: Deallocate the thread stack on setup failure (BZ #19511)

[Adhemerval Zanella]

On 26/05/2021 13:57, Adhemerval Zanella wrote:
> @@ -435,10 +424,11 @@ start_thread (void *arg)
>  
>  	  /* And give it up right away.  */
>  	  lll_unlock (pd->lock, LLL_PRIVATE);
> -
> -	  LIBC_CANCEL_RESET (oldtype);
>  	}
>  
> +      if (pd->setup_failed == 1)
> +        __do_cancel ();
> +
>        LIBC_PROBE (pthread_start, 3, (pthread_t) pd, pd->start_routine, pd->arg);
>  
>        /* Run the code the user provided.  */

And this is wrong, the created thread does not own the PD anymore
since for setup_failed == 1, pd->stopped_start will be true.

We need to call __do_cancel in the if above.

Re: [PATCH 05/11] nptl: Deallocate the thread stack on setup failure (BZ #19511)

[Florian Weimer]

* Adhemerval Zanella:

> On 26/05/2021 13:57, Adhemerval Zanella wrote:
>> @@ -435,10 +424,11 @@ start_thread (void *arg)
>>  
>>  	  /* And give it up right away.  */
>>  	  lll_unlock (pd->lock, LLL_PRIVATE);
>> -
>> -	  LIBC_CANCEL_RESET (oldtype);
>>  	}
>>  
>> +      if (pd->setup_failed == 1)
>> +        __do_cancel ();
>> +
>>        LIBC_PROBE (pthread_start, 3, (pthread_t) pd, pd->start_routine, pd->arg);
>>  
>>        /* Run the code the user provided.  */
>
> And this is wrong, the created thread does not own the PD anymore
> since for setup_failed == 1, pd->stopped_start will be true.
>
> We need to call __do_cancel in the if above.

Okay, I will wait for the new version before I start a real review.

Thanks,
Florian

[PATCH 06/11] nptl: Install cancellation handler on pthread_cancel

[Adhemerval Zanella]

Now that cancellation is not used anymore to handle thread setup
creation failure, the sighandle can be installed only when
pthread_cancel is actually used.

Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
 nptl/Versions         |  3 +--
 nptl/pthreadP.h       |  6 ------
 nptl/pthread_cancel.c | 49 ++++++++++++++++++++++++-------------------
 nptl/pthread_create.c | 15 -------------
 4 files changed, 28 insertions(+), 45 deletions(-)

diff --git a/nptl/Versions b/nptl/Versions
index af62a47cca..590761e730 100644
--- a/nptl/Versions
+++ b/nptl/Versions
@@ -395,7 +395,6 @@ libc {
     __nptl_free_tcb;
     __nptl_nthreads;
     __nptl_setxid_sighandler;
-    __nptl_sigcancel_handler;
     __nptl_stack_list_add;
     __nptl_stack_list_del;
     __pthread_attr_copy;
@@ -514,4 +513,4 @@ ld {
      __nptl_initial_report_events;
      __nptl_set_robust_list_avail;
   }
-}
\ No newline at end of file
+}
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 05f2bae521..48d48e7008 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -569,12 +569,6 @@ libc_hidden_proto (__pthread_attr_setsigmask_internal)
 extern __typeof (pthread_attr_getsigmask_np) __pthread_attr_getsigmask_np;
 libc_hidden_proto (__pthread_attr_getsigmask_np)
 
-/* The cancellation signal handler defined alongside with
-   pthread_cancel.  This is included in statically linked binaries
-   only if pthread_cancel is linked in.  */
-void __nptl_sigcancel_handler (int sig, siginfo_t *si, void *ctx);
-libc_hidden_proto (__nptl_sigcancel_handler)
-
 /* Special versions which use non-exported functions.  */
 extern void __pthread_cleanup_push (struct _pthread_cleanup_buffer *buffer,
 				    void (*routine) (void *), void *arg);
diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index 802c691874..f264a4096a 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -28,11 +28,19 @@
 #include <gnu/lib-names.h>
 #include <sys/single_threaded.h>
 
-/* For asynchronous cancellation we use a signal.  This is the core
-   logic of the signal handler.  */
+/* For asynchronous cancellation we use a signal.  */
 static void
-sigcancel_handler (void)
+sigcancel_handler (int sig, siginfo_t *si, void *ctx)
 {
+  /* Safety check.  It would be possible to call this function for
+     other signals and send a signal from another process.  This is not
+     correct and might even be a security problem.  Try to catch as
+     many incorrect invocations as possible.  */
+  if (sig != SIGCANCEL
+      || si->si_pid != __getpid()
+      || si->si_code != SI_TKILL)
+    return;
+
   struct pthread *self = THREAD_SELF;
 
   int oldval = THREAD_GETMEM (self, cancelhandling);
@@ -66,24 +74,6 @@ sigcancel_handler (void)
     }
 }
 
-/* This is the actually installed SIGCANCEL handler.  It adds some
-   safety checks before performing the cancellation.  */
-void
-__nptl_sigcancel_handler (int sig, siginfo_t *si, void *ctx)
-{
-  /* Safety check.  It would be possible to call this function for
-     other signals and send a signal from another process.  This is not
-     correct and might even be a security problem.  Try to catch as
-     many incorrect invocations as possible.  */
-  if (sig != SIGCANCEL
-      || si->si_pid != __getpid()
-      || si->si_code != SI_TKILL)
-    return;
-
-  sigcancel_handler ();
-}
-libc_hidden_def (__nptl_sigcancel_handler)
-
 int
 __pthread_cancel (pthread_t th)
 {
@@ -94,6 +84,17 @@ __pthread_cancel (pthread_t th)
     /* Not a valid thread handle.  */
     return ESRCH;
 
+  static int init_sigcancel = 0;
+  if (atomic_load_relaxed (&init_sigcancel) == 0)
+    {
+      struct sigaction sa;
+      sa.sa_sigaction = sigcancel_handler;
+      sa.sa_flags = SA_SIGINFO;
+      __sigemptyset (&sa.sa_mask);
+      __libc_sigaction (SIGCANCEL, &sa, NULL);
+      atomic_store_relaxed (&init_sigcancel, 1);
+    }
+
 #ifdef SHARED
   /* Trigger an error if libgcc_s cannot be loaded.  */
   {
@@ -134,7 +135,11 @@ __pthread_cancel (pthread_t th)
 	       call pthread_cancel (pthread_self ()) without calling
 	       pthread_create, so the signal handler may not have been
 	       set up for a self-cancel.  */
-	    sigcancel_handler ();
+            {
+              THREAD_SETMEM (pd, result, PTHREAD_CANCELED);
+              if ((newval & CANCELTYPE_BITMASK) != 0)
+	        __do_cancel ();
+            }
 	  else
 	    {
 	      /* The cancellation handler will take care of marking the
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index d6b907827a..44d135212d 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -68,21 +68,6 @@ late_init (void)
   struct sigaction sa;
   __sigemptyset (&sa.sa_mask);
 
-  /* Install the cancellation signal handler (in static builds only if
-     pthread_cancel has been linked in).  If for some reason we cannot
-     install the handler we do not abort.  Maybe we should, but it is
-     only asynchronous cancellation which is affected.  */
-#ifndef SHARED
-  extern __typeof (__nptl_sigcancel_handler) __nptl_sigcancel_handler
-    __attribute__ ((weak));
-  if (__nptl_sigcancel_handler != NULL)
-#endif
-    {
-      sa.sa_sigaction = __nptl_sigcancel_handler;
-      sa.sa_flags = SA_SIGINFO;
-      (void) __libc_sigaction (SIGCANCEL, &sa, NULL);
-    }
-
   /* Install the handle to change the threads' uid/gid.  */
   sa.sa_sigaction = __nptl_setxid_sighandler;
   sa.sa_flags = SA_SIGINFO | SA_RESTART;
-- 
2.30.2

Re: [PATCH 06/11] nptl: Install cancellation handler on pthread_cancel

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> @@ -134,7 +135,11 @@ __pthread_cancel (pthread_t th)
>  	       call pthread_cancel (pthread_self ()) without calling
>  	       pthread_create, so the signal handler may not have been
>  	       set up for a self-cancel.  */
> -	    sigcancel_handler ();
> +            {
> +              THREAD_SETMEM (pd, result, PTHREAD_CANCELED);
> +              if ((newval & CANCELTYPE_BITMASK) != 0)
> +	        __do_cancel ();
> +            }

The comment is no longer up to date, I think.  Rest of the change looks
okay to me.

Thanks,
Florian

Re: [PATCH 06/11] nptl: Install cancellation handler on pthread_cancel

[Adhemerval Zanella]

On 26/05/2021 14:38, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> @@ -134,7 +135,11 @@ __pthread_cancel (pthread_t th)
>>  	       call pthread_cancel (pthread_self ()) without calling
>>  	       pthread_create, so the signal handler may not have been
>>  	       set up for a self-cancel.  */
>> -	    sigcancel_handler ();
>> +            {
>> +              THREAD_SETMEM (pd, result, PTHREAD_CANCELED);
>> +              if ((newval & CANCELTYPE_BITMASK) != 0)
>> +	        __do_cancel ();
>> +            }
> 
> The comment is no longer up to date, I think.  Rest of the change looks
> okay to me.

Indeed, I will remove it.

[PATCH 07/11] nptl: Remove CANCELING_BITMASK

[Adhemerval Zanella]

The CANCELING_BITMASK is used as an optimization to avoid sending
the signal when pthread_cancel is called in a concurrent manner.

This requires then to put both the cancellation state and type on
a shared state (cancelhandling), since 'pthread_cancel' checks
whether cancellation is enabled and asynchrnous to either cancel
itself of sending the signal.

It also requires handle the CANCELING_BITMASK on
__pthread_disable_asynccancel, however this is incurs in the same
issues described on BZ#12683: the cancellation is acting even *after*
the syscalls returns with user visible side-effects.

This patch removes this optimization and simplifies the pthread
cancellation implementation: pthread_cancel now first check if
cancellation is already pending and if not always send a signal
if the target is not itself.  The SIGCANCEL handler is also simpified
since there is not need to setup a CAS loop.

It also alows to move both the cancellation state and mode out of
'cancelhadling' (it is done in subsequent patches).

Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
 nptl/cancellation.c        |  12 ----
 nptl/descr.h               |   3 -
 nptl/pthread_cancel.c      | 124 ++++++++++---------------------------
 nptl/pthread_join_common.c |   2 +-
 4 files changed, 35 insertions(+), 106 deletions(-)

diff --git a/nptl/cancellation.c b/nptl/cancellation.c
index c20845adc0..b15f25d8f6 100644
--- a/nptl/cancellation.c
+++ b/nptl/cancellation.c
@@ -88,17 +88,5 @@ __pthread_disable_asynccancel (int oldtype)
       /* Prepare the next round.  */
       oldval = curval;
     }
-
-  /* We cannot return when we are being canceled.  Upon return the
-     thread might be things which would have to be undone.  The
-     following loop should loop until the cancellation signal is
-     delivered.  */
-  while (__builtin_expect ((newval & (CANCELING_BITMASK | CANCELED_BITMASK))
-			   == CANCELING_BITMASK, 0))
-    {
-      futex_wait_simple ((unsigned int *) &self->cancelhandling, newval,
-			 FUTEX_PRIVATE);
-      newval = THREAD_GETMEM (self, cancelhandling);
-    }
 }
 libc_hidden_def (__pthread_disable_asynccancel)
diff --git a/nptl/descr.h b/nptl/descr.h
index 9d8297b45f..a120365f88 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -283,9 +283,6 @@ struct pthread
   /* Bit set if asynchronous cancellation mode is selected.  */
 #define CANCELTYPE_BIT		1
 #define CANCELTYPE_BITMASK	(0x01 << CANCELTYPE_BIT)
-  /* Bit set if canceling has been initiated.  */
-#define CANCELING_BIT		2
-#define CANCELING_BITMASK	(0x01 << CANCELING_BIT)
   /* Bit set if canceled.  */
 #define CANCELED_BIT		3
 #define CANCELED_BITMASK	(0x01 << CANCELED_BIT)
diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index f264a4096a..c11a2b4551 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -43,35 +43,18 @@ sigcancel_handler (int sig, siginfo_t *si, void *ctx)
 
   struct pthread *self = THREAD_SELF;
 
-  int oldval = THREAD_GETMEM (self, cancelhandling);
-  while (1)
-    {
-      /* We are canceled now.  When canceled by another thread this flag
-	 is already set but if the signal is directly send (internally or
-	 from another process) is has to be done here.  */
-      int newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
-
-      if (oldval == newval || (oldval & EXITING_BITMASK) != 0)
-	/* Already canceled or exiting.  */
-	break;
-
-      int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
-					      oldval);
-      if (curval == oldval)
-	{
-	  /* Set the return value.  */
-	  THREAD_SETMEM (self, result, PTHREAD_CANCELED);
-
-	  /* Make sure asynchronous cancellation is still enabled.  */
-	  if ((newval & CANCELTYPE_BITMASK) != 0)
-	    /* Run the registered destructors and terminate the thread.  */
-	    __do_cancel ();
-
-	  break;
-	}
-
-      oldval = curval;
-    }
+  int ch = atomic_load_relaxed (&self->cancelhandling);
+  /* Cancelation not enabled, not cancelled, or already exitting.  */
+  if ((ch & CANCELSTATE_BITMASK) != 0
+      || (ch & CANCELED_BITMASK) == 0
+      || (ch & EXITING_BITMASK) != 0)
+    return;
+
+  /* Set the return value.  */
+  THREAD_SETMEM (self, result, PTHREAD_CANCELED);
+  /* Make sure asynchronous cancellation is still enabled.  */
+  if ((ch & CANCELTYPE_BITMASK) != 0)
+    __do_cancel ();
 }
 
 int
@@ -104,72 +87,33 @@ __pthread_cancel (pthread_t th)
 		    " must be installed for pthread_cancel to work\n");
   }
 #endif
-  int result = 0;
-  int oldval;
-  int newval;
-  do
+
+  int oldch = atomic_fetch_or_acquire (&pd->cancelhandling, CANCELED_BITMASK);
+  if ((oldch & CANCELED_BITMASK) != 0)
+    return 0;
+
+  if (pd == THREAD_SELF)
     {
-    again:
-      oldval = pd->cancelhandling;
-      newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
-
-      /* Avoid doing unnecessary work.  The atomic operation can
-	 potentially be expensive if the bug has to be locked and
-	 remote cache lines have to be invalidated.  */
-      if (oldval == newval)
-	break;
-
-      /* 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))
-	{
-	  /* Mark the cancellation as "in progress".  */
-	  if (atomic_compare_and_exchange_bool_acq (&pd->cancelhandling,
-						    oldval | CANCELING_BITMASK,
-						    oldval))
-	    goto again;
-
-	  if (pd == THREAD_SELF)
-	    /* This is not merely an optimization: An application may
-	       call pthread_cancel (pthread_self ()) without calling
-	       pthread_create, so the signal handler may not have been
-	       set up for a self-cancel.  */
-            {
-              THREAD_SETMEM (pd, result, PTHREAD_CANCELED);
-              if ((newval & CANCELTYPE_BITMASK) != 0)
-	        __do_cancel ();
-            }
-	  else
-	    {
-	      /* The cancellation handler will take care of marking the
-		 thread as canceled.  */
-	      pid_t pid = __getpid ();
-
-	      int val = INTERNAL_SYSCALL_CALL (tgkill, pid, pd->tid,
-					       SIGCANCEL);
-	      if (INTERNAL_SYSCALL_ERROR_P (val))
-		result = INTERNAL_SYSCALL_ERRNO (val);
-	    }
-
-	  break;
-	}
-
-	/* A single-threaded process should be able to kill itself, since
-	   there is nothing in the POSIX specification that says that it
-	   cannot.  So we set multiple_threads to true so that cancellation
-	   points get executed.  */
-	THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1);
+      /* A single-threaded process should be able to kill itself, since there
+         is nothing in the POSIX specification that says that it cannot.  So
+         we set multiple_threads to true so that cancellation points get
+         executed.  */
+      THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1);
 #ifndef TLS_MULTIPLE_THREADS_IN_TCB
-	__libc_multiple_threads = 1;
+      __libc_multiple_threads = 1;
 #endif
+
+      THREAD_SETMEM (pd, result, PTHREAD_CANCELED);
+      if ((oldch & CANCELTYPE_BITMASK) != 0)
+        __do_cancel ();
+      return 0;
     }
-  /* Mark the thread as canceled.  This has to be done
-     atomically since other bits could be modified as well.  */
-  while (atomic_compare_and_exchange_bool_acq (&pd->cancelhandling, newval,
-					       oldval));
 
-  return result;
+  pid_t pid = __getpid ();
+  int val = INTERNAL_SYSCALL_CALL (tgkill, pid, pd->tid, SIGCANCEL);
+  return INTERNAL_SYSCALL_ERROR_P (val)
+         ? INTERNAL_SYSCALL_ERRNO (val)
+         : 0;
 }
 versioned_symbol (libc, __pthread_cancel, pthread_cancel, GLIBC_2_34);
 
diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
index e87801b5a3..f842c91a08 100644
--- a/nptl/pthread_join_common.c
+++ b/nptl/pthread_join_common.c
@@ -57,7 +57,7 @@ __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
   if ((pd == self
        || (self->joinid == pd
 	   && (pd->cancelhandling
-	       & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
+	       & (CANCELED_BITMASK | EXITING_BITMASK
 		  | TERMINATED_BITMASK)) == 0))
       && !CANCEL_ENABLED_AND_CANCELED (self->cancelhandling))
     /* This is a deadlock situation.  The threads are waiting for each
-- 
2.30.2

Re: [PATCH 07/11] nptl: Remove CANCELING_BITMASK

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> The CANCELING_BITMASK is used as an optimization to avoid sending
> the signal when pthread_cancel is called in a concurrent manner.
>
> This requires then to put both the cancellation state and type on
> a shared state (cancelhandling), since 'pthread_cancel' checks
> whether cancellation is enabled and asynchrnous to either cancel
> itself of sending the signal.
>
> It also requires handle the CANCELING_BITMASK on
> __pthread_disable_asynccancel, however this is incurs in the same
> issues described on BZ#12683: the cancellation is acting even *after*
> the syscalls returns with user visible side-effects.
>
> This patch removes this optimization and simplifies the pthread
> cancellation implementation: pthread_cancel now first check if
> cancellation is already pending and if not always send a signal
> if the target is not itself.  The SIGCANCEL handler is also simpified
> since there is not need to setup a CAS loop.

“and, if not, always sends a signal” ?

> +  int ch = atomic_load_relaxed (&self->cancelhandling);
> +  /* Cancelation not enabled, not cancelled, or already exitting.  */

Typo: exitting

The change itself looks good to me, and I agree with the direction.

Thanks,
Florian

Re: [PATCH 07/11] nptl: Remove CANCELING_BITMASK

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> The CANCELING_BITMASK is used as an optimization to avoid sending
> the signal when pthread_cancel is called in a concurrent manner.
>
> This requires then to put both the cancellation state and type on
> a shared state (cancelhandling), since 'pthread_cancel' checks
> whether cancellation is enabled and asynchrnous to either cancel
> itself of sending the signal.
>
> It also requires handle the CANCELING_BITMASK on
> __pthread_disable_asynccancel, however this is incurs in the same
> issues described on BZ#12683: the cancellation is acting even *after*
> the syscalls returns with user visible side-effects.
>
> This patch removes this optimization and simplifies the pthread
> cancellation implementation: pthread_cancel now first check if
> cancellation is already pending and if not always send a signal
> if the target is not itself.  The SIGCANCEL handler is also simpified
> since there is not need to setup a CAS loop.
>
> It also alows to move both the cancellation state and mode out of
> 'cancelhadling' (it is done in subsequent patches).

It looks like this causes sporadic failures in nptl/tst-sem16 because
sem_wait fails with EINTR, revealing that cancellation is implemented
using a signal (something that must not be visible according to POSIX).

This was not a problem before because pthread_cancel sent the signal
only when asynchronous cancellation was enabled.

We either have to revert this, or push forward with the transition so
that we can install the SIGCANCEL handler with SA_RESTART.

Thanks,
Florian

Re: [PATCH 07/11] nptl: Remove CANCELING_BITMASK

[Adhemerval Zanella]

On 15/06/2021 19:07, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> The CANCELING_BITMASK is used as an optimization to avoid sending
>> the signal when pthread_cancel is called in a concurrent manner.
>>
>> This requires then to put both the cancellation state and type on
>> a shared state (cancelhandling), since 'pthread_cancel' checks
>> whether cancellation is enabled and asynchrnous to either cancel
>> itself of sending the signal.
>>
>> It also requires handle the CANCELING_BITMASK on
>> __pthread_disable_asynccancel, however this is incurs in the same
>> issues described on BZ#12683: the cancellation is acting even *after*
>> the syscalls returns with user visible side-effects.
>>
>> This patch removes this optimization and simplifies the pthread
>> cancellation implementation: pthread_cancel now first check if
>> cancellation is already pending and if not always send a signal
>> if the target is not itself.  The SIGCANCEL handler is also simpified
>> since there is not need to setup a CAS loop.
>>
>> It also alows to move both the cancellation state and mode out of
>> 'cancelhadling' (it is done in subsequent patches).
> 
> It looks like this causes sporadic failures in nptl/tst-sem16 because
> sem_wait fails with EINTR, revealing that cancellation is implemented
> using a signal (something that must not be visible according to POSIX).
> 
> This was not a problem before because pthread_cancel sent the signal
> only when asynchronous cancellation was enabled.
> 
> We either have to revert this, or push forward with the transition so
> that we can install the SIGCANCEL handler with SA_RESTART.

Indeed now that SIGCANCEL is always sent by pthread_cancel we must
install the signal handle with SA_RESTART, the signal handle will
either act on cancellation or it should be not visible by the 
application.

I don't think there is a need to revert this change, running nptl/tst-sem16
with SA_RESTART I don't see the regression anymore with multiple thousands
runs.

Re: [PATCH 07/11] nptl: Remove CANCELING_BITMASK

[Adhemerval Zanella]

On 15/06/2021 20:33, Adhemerval Zanella wrote:
> 
> 
> On 15/06/2021 19:07, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>>
>>> The CANCELING_BITMASK is used as an optimization to avoid sending
>>> the signal when pthread_cancel is called in a concurrent manner.
>>>
>>> This requires then to put both the cancellation state and type on
>>> a shared state (cancelhandling), since 'pthread_cancel' checks
>>> whether cancellation is enabled and asynchrnous to either cancel
>>> itself of sending the signal.
>>>
>>> It also requires handle the CANCELING_BITMASK on
>>> __pthread_disable_asynccancel, however this is incurs in the same
>>> issues described on BZ#12683: the cancellation is acting even *after*
>>> the syscalls returns with user visible side-effects.
>>>
>>> This patch removes this optimization and simplifies the pthread
>>> cancellation implementation: pthread_cancel now first check if
>>> cancellation is already pending and if not always send a signal
>>> if the target is not itself.  The SIGCANCEL handler is also simpified
>>> since there is not need to setup a CAS loop.
>>>
>>> It also alows to move both the cancellation state and mode out of
>>> 'cancelhadling' (it is done in subsequent patches).
>>
>> It looks like this causes sporadic failures in nptl/tst-sem16 because
>> sem_wait fails with EINTR, revealing that cancellation is implemented
>> using a signal (something that must not be visible according to POSIX).
>>
>> This was not a problem before because pthread_cancel sent the signal
>> only when asynchronous cancellation was enabled.
>>
>> We either have to revert this, or push forward with the transition so
>> that we can install the SIGCANCEL handler with SA_RESTART.
> 
> Indeed now that SIGCANCEL is always sent by pthread_cancel we must
> install the signal handle with SA_RESTART, the signal handle will
> either act on cancellation or it should be not visible by the 
> application.
> 
> I don't think there is a need to revert this change, running nptl/tst-sem16
> with SA_RESTART I don't see the regression anymore with multiple thousands
> runs.
> 

I will send a patch to use SA_RESTART on cancellation.

[PATCH 08/11] nptl: Move cancel state out of cancelhandling

[Adhemerval Zanella]

Now that thread cancellation state is not accessed concurrently anymore,
it is possible to move it out the 'cancelhandling'.

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 and aarch64-linux-gnu.
---
 manual/pattern.texi           |  1 -
 manual/process.texi           |  3 +--
 nptl/allocatestack.c          |  1 +
 nptl/cancellation.c           |  3 ++-
 nptl/cleanup_defer.c          |  2 +-
 nptl/descr.h                  | 14 ++++++--------
 nptl/libc-cleanup.c           |  2 +-
 nptl/pthreadP.h               | 12 ------------
 nptl/pthread_cancel.c         |  2 +-
 nptl/pthread_join_common.c    |  5 ++++-
 nptl/pthread_setcancelstate.c | 36 +++--------------------------------
 nptl/pthread_setcanceltype.c  |  3 ++-
 nptl/pthread_testcancel.c     | 11 ++++++++++-
 13 files changed, 32 insertions(+), 63 deletions(-)

diff --git a/manual/pattern.texi b/manual/pattern.texi
index 39ae97a3c4..4fa4c25525 100644
--- a/manual/pattern.texi
+++ b/manual/pattern.texi
@@ -1820,7 +1820,6 @@ the beginning of the vector.
 @c      (disable cancellation around exec_comm; it may do_cancel the
 @c       second time, if async cancel is enabled)
 @c     THREAD_ATOMIC_CMPXCHG_VAL dup ok
-@c     CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS dup ok
 @c     do_cancel @ascuplugin @ascuheap @acsmem
 @c      THREAD_ATOMIC_BIT_SET dup ok
 @c      pthread_unwind @ascuplugin @ascuheap @acsmem
diff --git a/manual/process.texi b/manual/process.texi
index 54e65f76c6..134d5c6143 100644
--- a/manual/process.texi
+++ b/manual/process.texi
@@ -68,7 +68,7 @@ until the subprogram terminates before you can do anything else.
 @c   CLEANUP_HANDLER @ascuplugin @ascuheap @acsmem
 @c    libc_cleanup_region_start @ascuplugin @ascuheap @acsmem
 @c     pthread_cleanup_push_defer @ascuplugin @ascuheap @acsmem
-@c      CANCELLATION_P @ascuplugin @ascuheap @acsmem
+@c      __pthread_testcancel @ascuplugin @ascuheap @acsmem
 @c       CANCEL_ENABLED_AND_CANCELED ok
 @c       do_cancel @ascuplugin @ascuheap @acsmem
 @c    cancel_handler ok
@@ -86,7 +86,6 @@ until the subprogram terminates before you can do anything else.
 @c  SINGLE_THREAD_P ok
 @c  LIBC_CANCEL_ASYNC @ascuplugin @ascuheap @acsmem
 @c   libc_enable_asynccancel @ascuplugin @ascuheap @acsmem
-@c    CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS dup ok
 @c    do_cancel dup @ascuplugin @ascuheap @acsmem
 @c  LIBC_CANCEL_RESET ok
 @c   libc_disable_asynccancel ok
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 2114bd2e27..54e95baad7 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -160,6 +160,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;
   result->setup_failed = 0;
 
diff --git a/nptl/cancellation.c b/nptl/cancellation.c
index b15f25d8f6..ce00603109 100644
--- a/nptl/cancellation.c
+++ b/nptl/cancellation.c
@@ -44,7 +44,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 6d85359118..873ab007fd 100644
--- a/nptl/cleanup_defer.c
+++ b/nptl/cleanup_defer.c
@@ -92,7 +92,7 @@ ___pthread_unregister_cancel_restore (__pthread_unwind_buf_t *buf)
 	  cancelhandling = curval;
 	}
 
-      CANCELLATION_P (self);
+      __pthread_testcancel ();
     }
 }
 versioned_symbol (libc, ___pthread_unregister_cancel_restore,
diff --git a/nptl/descr.h b/nptl/descr.h
index a120365f88..a3084fdf60 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -277,9 +277,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)
@@ -298,11 +295,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))
 
@@ -407,6 +401,10 @@ struct pthread
   /* Used on strsignal.  */
   struct tls_internal_t tls_state;
 
+  /* 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/libc-cleanup.c b/nptl/libc-cleanup.c
index 14ccfe9285..6286b8b525 100644
--- a/nptl/libc-cleanup.c
+++ b/nptl/libc-cleanup.c
@@ -79,7 +79,7 @@ __libc_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer)
 	  cancelhandling = curval;
 	}
 
-      CANCELLATION_P (self);
+      __pthread_testcancel ();
     }
 }
 libc_hidden_def (__libc_cleanup_pop_restore)
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 48d48e7008..3e7a4f52ab 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -245,18 +245,6 @@ libc_hidden_proto (__pthread_current_priority)
 #define INVALID_TD_P(pd) __builtin_expect ((pd)->tid <= 0, 0)
 #define INVALID_NOT_TERMINATED_TD_P(pd) __builtin_expect ((pd)->tid < 0, 0)
 
-/* 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 c11a2b4551..0588f086a8 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -45,7 +45,7 @@ sigcancel_handler (int sig, siginfo_t *si, void *ctx)
 
   int ch = atomic_load_relaxed (&self->cancelhandling);
   /* Cancelation not enabled, not cancelled, or already exitting.  */
-  if ((ch & CANCELSTATE_BITMASK) != 0
+  if (self->cancelstate == PTHREAD_CANCEL_DISABLE
       || (ch & CANCELED_BITMASK) == 0
       || (ch & EXITING_BITMASK) != 0)
     return;
diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
index f842c91a08..7303069316 100644
--- a/nptl/pthread_join_common.c
+++ b/nptl/pthread_join_common.c
@@ -59,7 +59,10 @@ __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
 	   && (pd->cancelhandling
 	       & (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 e3696ca348..7e2b6e4974 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 5f061d512b..ae5df1d591 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 a9e941ddb7..920374643a 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 ();
+    }
 }
 versioned_symbol (libc, ___pthread_testcancel, pthread_testcancel, GLIBC_2_34);
 versioned_symbol (libc, ___pthread_testcancel, __pthread_testcancel,
-- 
2.30.2

Re: [PATCH 08/11] nptl: Move cancel state out of cancelhandling

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> diff --git a/nptl/descr.h b/nptl/descr.h
> index a120365f88..a3084fdf60 100644
> --- a/nptl/descr.h
> +++ b/nptl/descr.h

> @@ -407,6 +401,10 @@ struct pthread
>    /* Used on strsignal.  */
>    struct tls_internal_t tls_state;
>  
> +  /* Thread cancel state (PTHREAD_CANCEL_ENABLE or
> +     PTHREAD_CANCEL_DISABLE).  */
> +  unsigned char cancelstate;

You could move this into the padding after the c11 flag, I think.

I think there is an implied dependency on PTHREAD_CANCEL_ENABLE == 0 in
__tls_init_tp and somewhere in pthread_create.  Maybe add a static
assert for PTHREAD_CANCEL_ENABLE == 0?

Thanks,
Florian

Re: [PATCH 08/11] nptl: Move cancel state out of cancelhandling

[Adhemerval Zanella]

On 26/05/2021 15:20, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> diff --git a/nptl/descr.h b/nptl/descr.h
>> index a120365f88..a3084fdf60 100644
>> --- a/nptl/descr.h
>> +++ b/nptl/descr.h
> 
>> @@ -407,6 +401,10 @@ struct pthread
>>    /* Used on strsignal.  */
>>    struct tls_internal_t tls_state;
>>  
>> +  /* Thread cancel state (PTHREAD_CANCEL_ENABLE or
>> +     PTHREAD_CANCEL_DISABLE).  */
>> +  unsigned char cancelstate;
> 
> You could move this into the padding after the c11 flag, I think.

Right, I moved to below c11.  What kind of constraint we have for the
'struct pthread' regarding its internal member layout? We have a couple
of unused fields and might be good to clean them up.

> 
> I think there is an implied dependency on PTHREAD_CANCEL_ENABLE == 0 in
> __tls_init_tp and somewhere in pthread_create.  Maybe add a static
> assert for PTHREAD_CANCEL_ENABLE == 0?

I think it would be better to add an initialization on __tls_init_tp,
similar to get_cached_stack.  It would be better if we consolidate
the 'struct pthread' initialization in a common place.

Re: [PATCH 08/11] nptl: Move cancel state out of cancelhandling

[Florian Weimer]

* Adhemerval Zanella:

> On 26/05/2021 15:20, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>> diff --git a/nptl/descr.h b/nptl/descr.h
>>> index a120365f88..a3084fdf60 100644
>>> --- a/nptl/descr.h
>>> +++ b/nptl/descr.h
>> 
>>> @@ -407,6 +401,10 @@ struct pthread
>>>    /* Used on strsignal.  */
>>>    struct tls_internal_t tls_state;
>>>  
>>> +  /* Thread cancel state (PTHREAD_CANCEL_ENABLE or
>>> +     PTHREAD_CANCEL_DISABLE).  */
>>> +  unsigned char cancelstate;
>> 
>> You could move this into the padding after the c11 flag, I think.
>
> Right, I moved to below c11.  What kind of constraint we have for the
> 'struct pthread' regarding its internal member layout? We have a couple
> of unused fields and might be good to clean them up.

The sanitizers depend on the size to derive the TLS memory area from the
thread pointer.

>> I think there is an implied dependency on PTHREAD_CANCEL_ENABLE == 0 in
>> __tls_init_tp and somewhere in pthread_create.  Maybe add a static
>> assert for PTHREAD_CANCEL_ENABLE == 0?
>
> I think it would be better to add an initialization on __tls_init_tp,
> similar to get_cached_stack.  It would be better if we consolidate
> the 'struct pthread' initialization in a common place.

We depend a lot on zero initialization, though.  Maybe it would be
better to use memset in get_cached_stack.  Unrelated change, of course.

Thanks,
Florian

Re: [PATCH 08/11] nptl: Move cancel state out of cancelhandling

[Adhemerval Zanella]

On 27/05/2021 13:48, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 26/05/2021 15:20, Florian Weimer wrote:
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> diff --git a/nptl/descr.h b/nptl/descr.h
>>>> index a120365f88..a3084fdf60 100644
>>>> --- a/nptl/descr.h
>>>> +++ b/nptl/descr.h
>>>
>>>> @@ -407,6 +401,10 @@ struct pthread
>>>>    /* Used on strsignal.  */
>>>>    struct tls_internal_t tls_state;
>>>>  
>>>> +  /* Thread cancel state (PTHREAD_CANCEL_ENABLE or
>>>> +     PTHREAD_CANCEL_DISABLE).  */
>>>> +  unsigned char cancelstate;
>>>
>>> You could move this into the padding after the c11 flag, I think.
>>
>> Right, I moved to below c11.  What kind of constraint we have for the
>> 'struct pthread' regarding its internal member layout? We have a couple
>> of unused fields and might be good to clean them up.
> 
> The sanitizers depend on the size to derive the TLS memory area from the
> thread pointer.

Yeah, but they use a map to version and size to get the correct size.
If we change the size, sanitizer might fix with a new entry in the
map.

> 
>>> I think there is an implied dependency on PTHREAD_CANCEL_ENABLE == 0 in
>>> __tls_init_tp and somewhere in pthread_create.  Maybe add a static
>>> assert for PTHREAD_CANCEL_ENABLE == 0?
>>
>> I think it would be better to add an initialization on __tls_init_tp,
>> similar to get_cached_stack.  It would be better if we consolidate
>> the 'struct pthread' initialization in a common place.
> 
> We depend a lot on zero initialization, though.  Maybe it would be
> better to use memset in get_cached_stack.  Unrelated change, of course.

Indeed, but implicit initialization is usually harder to understand
specially in this case where it is done in multiple places.

[PATCH 09/11] nptl: Move cancel type out of cancelhandling

[Adhemerval Zanella]

Now that the thread cancellation type is not accessed concurrently
anymore, it is possible to move it out the cancelhandling.

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.

It allows simplifing the cancellation wrappers and the
CANCEL_CANCELED_AND_ASYNCHRONOUS is removed.

Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
 nptl/allocatestack.c         |  1 +
 nptl/cancellation.c          | 51 +++++++++---------------------------
 nptl/cleanup_defer.c         | 46 ++++----------------------------
 nptl/descr.h                 | 14 +++-------
 nptl/libc-cleanup.c          | 44 +++----------------------------
 nptl/pthread_cancel.c        |  4 +--
 nptl/pthread_setcanceltype.c | 42 ++++-------------------------
 7 files changed, 33 insertions(+), 169 deletions(-)

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 54e95baad7..9be6c42894 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -161,6 +161,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->canceltype = PTHREAD_CANCEL_DEFERRED;
   result->cleanup = NULL;
   result->setup_failed = 0;
 
diff --git a/nptl/cancellation.c b/nptl/cancellation.c
index ce00603109..05962784d5 100644
--- a/nptl/cancellation.c
+++ b/nptl/cancellation.c
@@ -31,31 +31,19 @@ int
 __pthread_enable_asynccancel (void)
 {
   struct pthread *self = THREAD_SELF;
-  int oldval = THREAD_GETMEM (self, cancelhandling);
 
-  while (1)
-    {
-      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 (self->cancelstate == PTHREAD_CANCEL_ENABLE
-	      && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval))
-	    {
-	      THREAD_SETMEM (self, result, PTHREAD_CANCELED);
-	      __do_cancel ();
-	    }
+  int oldval = THREAD_GETMEM (self, canceltype);
+  THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_ASYNCHRONOUS);
 
-	  break;
-	}
+  int ch = THREAD_GETMEM (self, cancelhandling);
 
-      /* Prepare the next round.  */
-      oldval = curval;
+  if (self->cancelstate == PTHREAD_CANCEL_ENABLE
+      && (ch & CANCELED_BITMASK)
+      && !(ch & EXITING_BITMASK)
+      && !(ch & TERMINATED_BITMASK))
+    {
+      THREAD_SETMEM (self, result, PTHREAD_CANCELED);
+      __do_cancel ();
     }
 
   return oldval;
@@ -69,25 +57,10 @@ __pthread_disable_asynccancel (int oldtype)
 {
   /* If asynchronous cancellation was enabled before we do not have
      anything to do.  */
-  if (oldtype & CANCELTYPE_BITMASK)
+  if (oldtype == PTHREAD_CANCEL_ASYNCHRONOUS)
     return;
 
   struct pthread *self = THREAD_SELF;
-  int newval;
-
-  int oldval = THREAD_GETMEM (self, cancelhandling);
-
-  while (1)
-    {
-      newval = oldval & ~CANCELTYPE_BITMASK;
-
-      int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
-					      oldval);
-      if (__glibc_likely (curval == oldval))
-	break;
-
-      /* Prepare the next round.  */
-      oldval = curval;
-    }
+  self->canceltype = PTHREAD_CANCEL_DEFERRED;
 }
 libc_hidden_def (__pthread_disable_asynccancel)
diff --git a/nptl/cleanup_defer.c b/nptl/cleanup_defer.c
index 873ab007fd..7e858d0df0 100644
--- a/nptl/cleanup_defer.c
+++ b/nptl/cleanup_defer.c
@@ -31,27 +31,9 @@ ___pthread_register_cancel_defer (__pthread_unwind_buf_t *buf)
   ibuf->priv.data.prev = THREAD_GETMEM (self, cleanup_jmp_buf);
   ibuf->priv.data.cleanup = THREAD_GETMEM (self, cleanup);
 
-  int cancelhandling = THREAD_GETMEM (self, cancelhandling);
-
   /* 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;
-      }
-
-  ibuf->priv.data.canceltype = (cancelhandling & CANCELTYPE_BITMASK
-				? PTHREAD_CANCEL_ASYNCHRONOUS
-				: PTHREAD_CANCEL_DEFERRED);
+  ibuf->priv.data.canceltype = THREAD_GETMEM (self, canceltype);
+  THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_DEFERRED);
 
   /* Store the new cleanup handler info.  */
   THREAD_SETMEM (self, cleanup_jmp_buf, (struct pthread_unwind_buf *) buf);
@@ -73,27 +55,9 @@ ___pthread_unregister_cancel_restore (__pthread_unwind_buf_t *buf)
 
   THREAD_SETMEM (self, cleanup_jmp_buf, ibuf->priv.data.prev);
 
-  int cancelhandling;
-  if (ibuf->priv.data.canceltype != PTHREAD_CANCEL_DEFERRED
-      && ((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;
-	}
-
-      __pthread_testcancel ();
-    }
+  THREAD_SETMEM (self, canceltype, ibuf->priv.data.canceltype);
+  if (ibuf->priv.data.canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
+    __pthread_testcancel ();
 }
 versioned_symbol (libc, ___pthread_unregister_cancel_restore,
 		  __pthread_unregister_cancel_restore, GLIBC_2_34);
diff --git a/nptl/descr.h b/nptl/descr.h
index a3084fdf60..04476b3f4e 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -277,9 +277,6 @@ struct pthread
 
   /* Flags determining processing of cancellation.  */
   int cancelhandling;
-  /* Bit set if asynchronous cancellation mode is selected.  */
-#define CANCELTYPE_BIT		1
-#define CANCELTYPE_BITMASK	(0x01 << CANCELTYPE_BIT)
   /* Bit set if canceled.  */
 #define CANCELED_BIT		3
 #define CANCELED_BITMASK	(0x01 << CANCELED_BIT)
@@ -292,13 +289,6 @@ struct pthread
   /* Bit set if thread is supposed to change XID.  */
 #define SETXID_BIT		6
 #define SETXID_BITMASK		(0x01 << SETXID_BIT)
-  /* Mask for the rest.  Helps the compiler to optimize.  */
-#define CANCEL_RESTMASK		0xffffff80
-
-#define CANCEL_CANCELED_AND_ASYNCHRONOUS(value) \
-  (((value) & (CANCELTYPE_BITMASK | CANCELED_BITMASK    \
-	       | EXITING_BITMASK | CANCEL_RESTMASK | TERMINATED_BITMASK))     \
-   == (CANCELTYPE_BITMASK | CANCELED_BITMASK))
 
   /* Flags.  Including those copied from the thread attribute.  */
   int flags;
@@ -405,6 +395,10 @@ struct pthread
      PTHREAD_CANCEL_DISABLE).  */
   unsigned char cancelstate;
 
+  /* Thread cancel type (PTHREAD_CANCEL_DEFERRED or
+     PTHREAD_CANCEL_ASYNCHRONOUS).  */
+  unsigned char canceltype;
+
   /* This member must be last.  */
   char end_padding[];
 
diff --git a/nptl/libc-cleanup.c b/nptl/libc-cleanup.c
index 6286b8b525..180d15bc9e 100644
--- a/nptl/libc-cleanup.c
+++ b/nptl/libc-cleanup.c
@@ -27,27 +27,9 @@ __libc_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer)
 
   buffer->__prev = THREAD_GETMEM (self, cleanup);
 
-  int cancelhandling = THREAD_GETMEM (self, cancelhandling);
-
   /* 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;
-      }
-
-  buffer->__canceltype = (cancelhandling & CANCELTYPE_BITMASK
-			  ? PTHREAD_CANCEL_ASYNCHRONOUS
-			  : PTHREAD_CANCEL_DEFERRED);
+  buffer->__canceltype = THREAD_GETMEM (self, canceltype);
+  THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_DEFERRED);
 
   THREAD_SETMEM (self, cleanup, buffer);
 }
@@ -60,26 +42,8 @@ __libc_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer)
 
   THREAD_SETMEM (self, cleanup, buffer->__prev);
 
-  int cancelhandling;
-  if (__builtin_expect (buffer->__canceltype != PTHREAD_CANCEL_DEFERRED, 0)
-      && ((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, canceltype, buffer->__canceltype);
+  if (buffer->__canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
       __pthread_testcancel ();
-    }
 }
 libc_hidden_def (__libc_cleanup_pop_restore)
diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index 0588f086a8..3be3d0fce7 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -53,7 +53,7 @@ sigcancel_handler (int sig, siginfo_t *si, void *ctx)
   /* Set the return value.  */
   THREAD_SETMEM (self, result, PTHREAD_CANCELED);
   /* Make sure asynchronous cancellation is still enabled.  */
-  if ((ch & CANCELTYPE_BITMASK) != 0)
+  if (self->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
     __do_cancel ();
 }
 
@@ -104,7 +104,7 @@ __pthread_cancel (pthread_t th)
 #endif
 
       THREAD_SETMEM (pd, result, PTHREAD_CANCELED);
-      if ((oldch & CANCELTYPE_BITMASK) != 0)
+      if (pd->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
         __do_cancel ();
       return 0;
     }
diff --git a/nptl/pthread_setcanceltype.c b/nptl/pthread_setcanceltype.c
index ae5df1d591..e7b24ae733 100644
--- a/nptl/pthread_setcanceltype.c
+++ b/nptl/pthread_setcanceltype.c
@@ -29,43 +29,11 @@ __pthread_setcanceltype (int type, int *oldtype)
 
   volatile struct pthread *self = THREAD_SELF;
 
-  int oldval = THREAD_GETMEM (self, cancelhandling);
-  while (1)
-    {
-      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 (self->cancelstate == PTHREAD_CANCEL_ENABLE
-	      && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval))
-	    {
-	      THREAD_SETMEM (self, result, PTHREAD_CANCELED);
-	      __do_cancel ();
-	    }
-
-	  break;
-	}
-
-      /* Prepare for the next round.  */
-      oldval = curval;
-    }
+  if (oldtype != NULL)
+    *oldtype = self->canceltype;
+  self->canceltype = type;
+  if (type == PTHREAD_CANCEL_ASYNCHRONOUS)
+    __pthread_testcancel ();
 
   return 0;
 }
-- 
2.30.2

[PATCH 10/11] nptl: Implement raise in terms of pthread_kill

[Adhemerval Zanella]

Now that pthread_kill is provided by libc.so it is possible to
implement the generic POSIX implementation as
'pthread_kill(pthread_self(), sig)'.

For Linux implementation, pthread_kill read the targeting TID from
the TCB.  For raise, this it not possible because it would make raise
fail when issue after vfork (where creates the resulting process
has a different TID from the parent, but its TCB is not updated as
for pthread_create).  To make raise use pthread_kill, it is make
usable from vfork by getting the target thread id through gettid
syscall.

Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
 include/pthread.h               |  5 ++++
 nptl/pthreadP.h                 |  4 ++-
 nptl/pthread_kill.c             | 40 ++++++++++++++++---------
 nptl/pthread_self.c             |  4 ++-
 sysdeps/htl/pthreadP.h          |  2 --
 sysdeps/posix/raise.c           | 11 +++++--
 sysdeps/unix/sysv/linux/raise.c | 52 ---------------------------------
 7 files changed, 47 insertions(+), 71 deletions(-)
 delete mode 100644 sysdeps/unix/sysv/linux/raise.c

diff --git a/include/pthread.h b/include/pthread.h
index a3e1cf51b0..1158919247 100644
--- a/include/pthread.h
+++ b/include/pthread.h
@@ -16,4 +16,9 @@ extern int __pthread_barrier_wait (pthread_barrier_t *__barrier)
 
 /* This function is called to initialize the pthread library.  */
 extern void __pthread_initialize (void) __attribute__ ((weak));
+
+extern int __pthread_kill (pthread_t threadid, int signo);
+
+extern pthread_t __pthread_self (void);
+
 #endif
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 3e7a4f52ab..618922f47a 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -508,11 +508,13 @@ extern int __pthread_once (pthread_once_t *once_control,
 libc_hidden_proto (__pthread_once)
 extern int __pthread_atfork (void (*prepare) (void), void (*parent) (void),
 			     void (*child) (void));
-extern pthread_t __pthread_self (void);
+libc_hidden_proto (__pthread_self)
 extern int __pthread_equal (pthread_t thread1, pthread_t thread2);
 extern int __pthread_detach (pthread_t th);
 libc_hidden_proto (__pthread_detach)
 extern int __pthread_kill (pthread_t threadid, int signo);
+libc_hidden_proto (__pthread_kill)
+extern int __pthread_cancel (pthread_t th);
 extern void __pthread_exit (void *value) __attribute__ ((__noreturn__));
 libc_hidden_proto (__pthread_exit)
 extern int __pthread_join (pthread_t threadid, void **thread_return);
diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c
index ad7e011779..0392c3f9e5 100644
--- a/nptl/pthread_kill.c
+++ b/nptl/pthread_kill.c
@@ -28,24 +28,38 @@ __pthread_kill (pthread_t threadid, int signo)
   if (__is_internal_signal (signo))
     return EINVAL;
 
-  /* Force load of pd->tid into local variable or register.  Otherwise
-     if a thread exits between ESRCH test and tgkill, we might return
-     EINVAL, because pd->tid would be cleared by the kernel.  */
+  pid_t tid;
   struct pthread *pd = (struct pthread *) threadid;
-  pid_t tid = atomic_forced_read (pd->tid);
-  if (__glibc_unlikely (tid <= 0))
-    /* Not a valid thread handle.  */
-    return ESRCH;
 
-  /* We have a special syscall to do the work.  */
-  pid_t pid = __getpid ();
+  if (pd == THREAD_SELF)
+    tid = INLINE_SYSCALL_CALL (gettid);
+  else
+    /* Force load of pd->tid into local variable or register.  Otherwise
+       if a thread exits between ESRCH test and tgkill, we might return
+       EINVAL, because pd->tid would be cleared by the kernel.  */
+    tid = atomic_forced_read (pd->tid);
 
-  int val = INTERNAL_SYSCALL_CALL (tgkill, pid, tid, signo);
-  return (INTERNAL_SYSCALL_ERROR_P (val)
-	  ? INTERNAL_SYSCALL_ERRNO (val) : 0);
+  int val;
+  if (__glibc_likely (tid > 0))
+    {
+      pid_t pid = __getpid ();
+
+      val = INTERNAL_SYSCALL_CALL (tgkill, pid, tid, signo);
+      val = (INTERNAL_SYSCALL_ERROR_P (val)
+	    ? INTERNAL_SYSCALL_ERRNO (val) : 0);
+    }
+  else
+    val = ESRCH;
+
+  return val;
 }
+/* Some architectures (for instance arm) might pull raise through libgcc, so
+   avoid the symbol version if it ends up being used on ld.so.  */
+#if !IS_IN(rtld)
+libc_hidden_def (__pthread_kill)
 versioned_symbol (libc, __pthread_kill, pthread_kill, GLIBC_2_34);
 
-#if OTHER_SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_34)
+# if OTHER_SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_34)
 compat_symbol (libc, __pthread_kill, pthread_kill, GLIBC_2_0);
+# endif
 #endif
diff --git a/nptl/pthread_self.c b/nptl/pthread_self.c
index f877a2e6bd..196d93fb8e 100644
--- a/nptl/pthread_self.c
+++ b/nptl/pthread_self.c
@@ -20,7 +20,9 @@
 #include <tls.h>
 
 pthread_t
-pthread_self (void)
+__pthread_self (void)
 {
   return (pthread_t) THREAD_SELF;
 }
+libc_hidden_def (__pthread_self)
+weak_alias (__pthread_self, pthread_self)
diff --git a/sysdeps/htl/pthreadP.h b/sysdeps/htl/pthreadP.h
index 8e2cf2ce65..3b357b7bdc 100644
--- a/sysdeps/htl/pthreadP.h
+++ b/sysdeps/htl/pthreadP.h
@@ -31,8 +31,6 @@ extern void __pthread_init_static_tls (struct link_map *) attribute_hidden;
 
 /* These represent the interface used by glibc itself.  */
 
-extern pthread_t __pthread_self (void);
-extern int __pthread_kill (pthread_t threadid, int signo);
 extern struct __pthread **__pthread_threads;
 
 extern int __pthread_mutex_init (pthread_mutex_t *__mutex, const pthread_mutexattr_t *__attr);
diff --git a/sysdeps/posix/raise.c b/sysdeps/posix/raise.c
index 9fdb4d538b..4806c0cc63 100644
--- a/sysdeps/posix/raise.c
+++ b/sysdeps/posix/raise.c
@@ -16,13 +16,20 @@
    <https://www.gnu.org/licenses/>.  */
 
 #include <signal.h>
-#include <unistd.h>
+#include <errno.h>
+#include <pthread.h>
 
 /* Raise the signal SIG.  */
 int
 raise (int sig)
 {
-  return __kill (__getpid (), sig);
+  int ret = __pthread_kill (__pthread_self (), sig);
+  if (ret != 0)
+    {
+      __set_errno (ret);
+      ret = -1;
+    }
+  return ret;
 }
 libc_hidden_def (raise)
 weak_alias (raise, gsignal)
diff --git a/sysdeps/unix/sysv/linux/raise.c b/sysdeps/unix/sysv/linux/raise.c
deleted file mode 100644
index 9be3b37f53..0000000000
--- a/sysdeps/unix/sysv/linux/raise.c
+++ /dev/null
@@ -1,52 +0,0 @@
-/* Copyright (C) 2002-2021 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
-
-   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
-   <https://www.gnu.org/licenses/>.  */
-
-#include <signal.h>
-#include <sysdep.h>
-#include <errno.h>
-#include <sys/types.h>
-#include <unistd.h>
-#include <internal-signals.h>
-
-int
-raise (int sig)
-{
-  /* rt_sigprocmask may fail if:
-
-     1. sigsetsize != sizeof (sigset_t) (EINVAL)
-     2. a failure in copy from/to user space (EFAULT)
-     3. an invalid 'how' operation (EINVAL)
-
-     The first case is already handle in glibc syscall call by using the arch
-     defined _NSIG.  Second case is handled by using a stack allocated mask.
-     The last one should be handled by the block/unblock functions.  */
-
-  sigset_t set;
-  __libc_signal_block_app (&set);
-
-  pid_t pid = INTERNAL_SYSCALL_CALL (getpid);
-  pid_t tid = INTERNAL_SYSCALL_CALL (gettid);
-
-  int ret = INLINE_SYSCALL_CALL (tgkill, pid, tid, sig);
-
-  __libc_signal_restore_set (&set);
-
-  return ret;
-}
-libc_hidden_def (raise)
-weak_alias (raise, gsignal)
-- 
2.30.2

[PATCH 11/11] nptl: Use pthread_kill on pthread_cancel

[Adhemerval Zanella]

It consolidates the tgkill call and it is the first step of making
pthread_cancel async-signal-safe.  It also fix a possible issue
where the 'struct pthread' tid is not read atomically, which might
send an invalid cancellation signal (similar to what
db988e50a87f613cb6b9e98a2fc66a4848bc3546 fixed for pthread_join).

Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
 nptl/pthreadP.h       |  2 ++
 nptl/pthread_cancel.c |  6 +-----
 nptl/pthread_kill.c   | 18 ++++++++++++------
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 618922f47a..675d1de753 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -515,6 +515,8 @@ libc_hidden_proto (__pthread_detach)
 extern int __pthread_kill (pthread_t threadid, int signo);
 libc_hidden_proto (__pthread_kill)
 extern int __pthread_cancel (pthread_t th);
+extern int __pthread_kill_internal (pthread_t threadid, int signo)
+  attribute_hidden;
 extern void __pthread_exit (void *value) __attribute__ ((__noreturn__));
 libc_hidden_proto (__pthread_exit)
 extern int __pthread_join (pthread_t threadid, void **thread_return);
diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index 3be3d0fce7..e6dc135e9b 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -109,11 +109,7 @@ __pthread_cancel (pthread_t th)
       return 0;
     }
 
-  pid_t pid = __getpid ();
-  int val = INTERNAL_SYSCALL_CALL (tgkill, pid, pd->tid, SIGCANCEL);
-  return INTERNAL_SYSCALL_ERROR_P (val)
-         ? INTERNAL_SYSCALL_ERRNO (val)
-         : 0;
+  return __pthread_kill_internal (th, SIGCANCEL);
 }
 versioned_symbol (libc, __pthread_cancel, pthread_cancel, GLIBC_2_34);
 
diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c
index 0392c3f9e5..93387dd60a 100644
--- a/nptl/pthread_kill.c
+++ b/nptl/pthread_kill.c
@@ -21,13 +21,8 @@
 #include <shlib-compat.h>
 
 int
-__pthread_kill (pthread_t threadid, int signo)
+__pthread_kill_internal (pthread_t threadid, int signo)
 {
-  /* Disallow sending the signal we use for cancellation, timers,
-     for the setxid implementation.  */
-  if (__is_internal_signal (signo))
-    return EINVAL;
-
   pid_t tid;
   struct pthread *pd = (struct pthread *) threadid;
 
@@ -53,6 +48,17 @@ __pthread_kill (pthread_t threadid, int signo)
 
   return val;
 }
+
+int
+__pthread_kill (pthread_t threadid, int signo)
+{
+  /* Disallow sending the signal we use for cancellation, timers,
+     for the setxid implementation.  */
+  if (__is_internal_signal (signo))
+    return EINVAL;
+
+  return __pthread_kill_internal (threadid, signo);
+}
 /* Some architectures (for instance arm) might pull raise through libgcc, so
    avoid the symbol version if it ends up being used on ld.so.  */
 #if !IS_IN(rtld)
-- 
2.30.2