* [PATCH v2 1/9] nptl: Remove exit-thread.h
2021-05-27 17:28 [PATCH v2 0/9] nptl: pthread cancellation refactor Adhemerval Zanella
@ 2021-05-27 17:28 ` Adhemerval Zanella
2021-06-01 7:51 ` Florian Weimer
2021-05-27 17:28 ` [PATCH v2 2/9] nptl: Deallocate the thread stack on setup failure (BZ #19511) Adhemerval Zanella
` (7 subsequent siblings)
8 siblings, 1 reply; 49+ messages in thread
From: Adhemerval Zanella @ 2021-05-27 17:28 UTC (permalink / raw)
To: libc-alpha
No function change. The code is used only for Linux, besides
being included in generic code.
---
csu/libc-start.c | 1 -
nptl/pthread_create.c | 6 +++--
sysdeps/generic/exit-thread.h | 28 --------------------
sysdeps/nptl/libc_start_call_main.h | 5 +++-
sysdeps/unix/sysv/linux/exit-thread.h | 37 ---------------------------
5 files changed, 8 insertions(+), 69 deletions(-)
delete mode 100644 sysdeps/generic/exit-thread.h
delete mode 100644 sysdeps/unix/sysv/linux/exit-thread.h
diff --git a/csu/libc-start.c b/csu/libc-start.c
index 8688cba76d..5b5913e7bf 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -30,7 +30,6 @@
#include <stdio.h>
#include <unistd.h>
#include <ldsodefs.h>
-#include <exit-thread.h>
#include <libc-diag.h>
#include <libc-internal.h>
#include <elf/libc-early-init.h>
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 2d2535b07d..aacbba1cac 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -30,7 +30,6 @@
#include <libc-internal.h>
#include <resolv.h>
#include <kernel-features.h>
-#include <exit-thread.h>
#include <default-sched.h>
#include <futex-internal.h>
#include <tls-setup.h>
@@ -575,7 +574,10 @@ start_thread (void *arg)
The exit code is zero since in case all threads exit by calling
'pthread_exit' the exit status must be 0 (zero). */
- __exit_thread ();
+ while (1)
+ {
+ INTERNAL_SYSCALL_CALL (exit, 0);
+ }
/* NOTREACHED */
}
diff --git a/sysdeps/generic/exit-thread.h b/sysdeps/generic/exit-thread.h
deleted file mode 100644
index 676ef553a5..0000000000
--- a/sysdeps/generic/exit-thread.h
+++ /dev/null
@@ -1,28 +0,0 @@
-/* Call to terminate the current thread. Stub version.
- Copyright (C) 2014-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/>. */
-
-/* This causes the current thread to exit, without affecting other
- threads in the process if there are any. If there are no other
- threads left, then this has the effect of _exit (0). */
-
-static inline void __attribute__ ((noreturn, always_inline, unused))
-__exit_thread (void)
-{
- while (1)
- asm ("write me!");
-}
diff --git a/sysdeps/nptl/libc_start_call_main.h b/sysdeps/nptl/libc_start_call_main.h
index c579c65f78..08b1800e83 100644
--- a/sysdeps/nptl/libc_start_call_main.h
+++ b/sysdeps/nptl/libc_start_call_main.h
@@ -67,7 +67,10 @@ __libc_start_call_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
result = 0;
if (! atomic_decrement_and_test (&__nptl_nthreads))
/* Not much left to do but to exit the thread, not the process. */
- __exit_thread ();
+ while (1)
+ {
+ INTERNAL_SYSCALL_CALL (exit, 0);
+ }
}
exit (result);
diff --git a/sysdeps/unix/sysv/linux/exit-thread.h b/sysdeps/unix/sysv/linux/exit-thread.h
deleted file mode 100644
index 9e1b7f3752..0000000000
--- a/sysdeps/unix/sysv/linux/exit-thread.h
+++ /dev/null
@@ -1,37 +0,0 @@
-/* Call to terminate the current thread. Linux version.
- Copyright (C) 2014-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 <sysdep.h>
-
-/* This causes the current thread to exit, without affecting other
- threads in the process if there are any. If there are no other
- threads left, then this has the effect of _exit (0). */
-
-static inline void __attribute__ ((noreturn, always_inline, unused))
-__exit_thread (void)
-{
- /* Doing this in a loop is mostly just to satisfy the compiler that the
- function really qualifies as noreturn. It also means that in some
- pathological situation where the system call does not get made or does
- not work, the thread will simply spin rather than running off the end
- of the caller and doing unexpectedly strange things. */
- while (1)
- {
- INTERNAL_SYSCALL_CALL (exit, 0);
- }
-}
--
2.30.2
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v2 1/9] nptl: Remove exit-thread.h
2021-05-27 17:28 ` [PATCH v2 1/9] nptl: Remove exit-thread.h Adhemerval Zanella
@ 2021-06-01 7:51 ` Florian Weimer
2021-06-01 12:55 ` Adhemerval Zanella
0 siblings, 1 reply; 49+ messages in thread
From: Florian Weimer @ 2021-06-01 7:51 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella via Libc-alpha:
> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index 2d2535b07d..aacbba1cac 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -30,7 +30,6 @@
> #include <libc-internal.h>
> #include <resolv.h>
> #include <kernel-features.h>
> -#include <exit-thread.h>
> #include <default-sched.h>
> #include <futex-internal.h>
> #include <tls-setup.h>
> @@ -575,7 +574,10 @@ start_thread (void *arg)
>
> The exit code is zero since in case all threads exit by calling
> 'pthread_exit' the exit status must be 0 (zero). */
> - __exit_thread ();
> + while (1)
> + {
> + INTERNAL_SYSCALL_CALL (exit, 0);
> + }
>
> /* NOTREACHED */
> }
You could leave off the braces. But still looks good.
Thanks,
Florian
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v2 1/9] nptl: Remove exit-thread.h
2021-06-01 7:51 ` Florian Weimer
@ 2021-06-01 12:55 ` Adhemerval Zanella
0 siblings, 0 replies; 49+ messages in thread
From: Adhemerval Zanella @ 2021-06-01 12:55 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella via Libc-alpha
On 01/06/2021 04:51, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
>> index 2d2535b07d..aacbba1cac 100644
>> --- a/nptl/pthread_create.c
>> +++ b/nptl/pthread_create.c
>> @@ -30,7 +30,6 @@
>> #include <libc-internal.h>
>> #include <resolv.h>
>> #include <kernel-features.h>
>> -#include <exit-thread.h>
>> #include <default-sched.h>
>> #include <futex-internal.h>
>> #include <tls-setup.h>
>> @@ -575,7 +574,10 @@ start_thread (void *arg)
>>
>> The exit code is zero since in case all threads exit by calling
>> 'pthread_exit' the exit status must be 0 (zero). */
>> - __exit_thread ();
>> + while (1)
>> + {
>> + INTERNAL_SYSCALL_CALL (exit, 0);
>> + }
>>
>> /* NOTREACHED */
>> }
>
> You could leave off the braces. But still looks good.
Ack.
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v2 2/9] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-05-27 17:28 [PATCH v2 0/9] nptl: pthread cancellation refactor Adhemerval Zanella
2021-05-27 17:28 ` [PATCH v2 1/9] nptl: Remove exit-thread.h Adhemerval Zanella
@ 2021-05-27 17:28 ` Adhemerval Zanella
2021-06-01 8:32 ` Florian Weimer
2021-06-02 12:56 ` [PATCH v3] " Adhemerval Zanella
2021-05-27 17:28 ` [PATCH v2 3/9] nptl: Install cancellation handler on pthread_cancel Adhemerval Zanella
` (6 subsequent siblings)
8 siblings, 2 replies; 49+ messages in thread
From: Adhemerval Zanella @ 2021-05-27 17:28 UTC (permalink / raw)
To: libc-alpha
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 | 70 ++++++++++++++++++++-----------------------
3 files changed, 36 insertions(+), 40 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 aacbba1cac..018af30c85 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -169,17 +169,15 @@ late_init (void)
(c) If the detached thread setup failed and THREAD_RAN is true, then
the creating thread releases ownership to the new thread by
- sending a cancellation signal. All threads set THREAD_RAN to
- true as quickly as possible after returning from the OS kernel's
- thread creation routine.
+ setting the PD->setup_failed 1 and releasing PD->lock. All threads
+ set THREAD_RAN to true as quickly as possible after returning from
+ the OS kernel's thread creation routine.
(d) If the joinable thread setup failed and THREAD_RAN is true, then
then the creating thread retains ownership of PD and must cleanup
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.
@@ -239,8 +237,8 @@ late_init (void)
The return value is zero for success or an errno code for failure.
If the return value is ENOMEM, that will be translated to EAGAIN,
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). */
+ be set to true iff the thread actually started up but before calling
+ the user code (*PD->start_routine). */
static int _Noreturn start_thread (void *arg);
@@ -308,35 +306,23 @@ static int create_thread (struct pthread *pd, const struct pthread_attr *attr,
== -1))
return errno;
- /* It's started now, so if we fail below, we'll have to cancel it
- and let it clean itself up. */
+ /* It's started now, so if we fail below, we'll have to 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);
-
+ 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);
- }
+ return INTERNAL_SYSCALL_ERRNO (res);
}
/* Set the scheduling parameters. */
@@ -344,11 +330,10 @@ 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;
+ return INTERNAL_SYSCALL_ERRNO (res);
}
}
@@ -424,17 +409,22 @@ 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);
/* We have ownership of PD now. */
+ if (pd->setup_failed == 1)
+ {
+ /* In the case of a failed setup, the created thread will be
+ responsible to free up the allocated resources. The
+ detached mode ensure it won't be joined and it will trigger
+ the required cleanup. */
+ pd->joinid = pd;
+ __do_cancel ();
+ }
/* And give it up right away. */
lll_unlock (pd->lock, LLL_PRIVATE);
-
- LIBC_CANCEL_RESET (oldtype);
}
LIBC_PROBE (pthread_start, 3, (pthread_t) pd, pd->start_routine, pd->arg);
@@ -561,7 +551,7 @@ start_thread (void *arg)
pd->setxid_futex = 0;
}
- /* If the thread is detached free the TCB. */
+ /* If the thread is detached or an setup error occurred, free the TCB. */
if (IS_DETACHED (pd))
/* Free the TCB. */
__nptl_free_tcb (pd);
@@ -761,7 +751,6 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
signal mask of this thread, so save it in the startup
information. */
pd->sigmask = original_sigmask;
-
/* Reset the cancellation signal mask in case this thread is
running cancellation. */
__sigdelset (&pd->sigmask, SIGCANCEL);
@@ -820,9 +809,14 @@ __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);
+ /* 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. */
+ pd->setup_failed = 1;
+ lll_unlock (pd->lock, LLL_PRIVATE);
+ }
else
{
/* State (e) and we have ownership of PD (see CONCURRENCY
--
2.30.2
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v2 2/9] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-05-27 17:28 ` [PATCH v2 2/9] nptl: Deallocate the thread stack on setup failure (BZ #19511) Adhemerval Zanella
@ 2021-06-01 8:32 ` Florian Weimer
2021-06-01 13:08 ` Adhemerval Zanella
2021-06-02 12:56 ` [PATCH v3] " Adhemerval Zanella
1 sibling, 1 reply; 49+ messages in thread
From: Florian Weimer @ 2021-06-01 8:32 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella via Libc-alpha:
> @@ -424,17 +409,22 @@ start_thread (void *arg)
> have ownership (see CONCURRENCY NOTES above). */
> if (__glibc_unlikely (pd->stopped_start))
> {
> /* Get the lock the parent locked to force synchronization. */
> lll_lock (pd->lock, LLL_PRIVATE);
>
> /* We have ownership of PD now. */
> + if (pd->setup_failed == 1)
> + {
> + /* In the case of a failed setup, the created thread will be
> + responsible to free up the allocated resources. The
> + detached mode ensure it won't be joined and it will trigger
> + the required cleanup. */
> + pd->joinid = pd;
> + __do_cancel ();
> + }
>
> /* And give it up right away. */
> lll_unlock (pd->lock, LLL_PRIVATE);
> }
I don't think this is quite right. The failed thread should not linger
around after pthread_create has reported failure to the caller. It's
better than what we had before, so maybe we should use this version and
clean it up further later.
At the point of the __do_cancel call, unwinding may not yet initialized,
so the implied dlopen happens on a potentially very small stack. It
should be possible to replace it with a goto to the cleanup code. Or
maybe put the cleanup code into a separate function and call it here and
on the regular cleanup path.
Apart from that, it looks fine to me.
Thanks,
Florian
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v2 2/9] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-06-01 8:32 ` Florian Weimer
@ 2021-06-01 13:08 ` Adhemerval Zanella
2021-06-01 13:55 ` Adhemerval Zanella
0 siblings, 1 reply; 49+ messages in thread
From: Adhemerval Zanella @ 2021-06-01 13:08 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella via Libc-alpha
On 01/06/2021 05:32, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> @@ -424,17 +409,22 @@ start_thread (void *arg)
>> have ownership (see CONCURRENCY NOTES above). */
>> if (__glibc_unlikely (pd->stopped_start))
>> {
>> /* Get the lock the parent locked to force synchronization. */
>> lll_lock (pd->lock, LLL_PRIVATE);
>>
>> /* We have ownership of PD now. */
>> + if (pd->setup_failed == 1)
>> + {
>> + /* In the case of a failed setup, the created thread will be
>> + responsible to free up the allocated resources. The
>> + detached mode ensure it won't be joined and it will trigger
>> + the required cleanup. */
>> + pd->joinid = pd;
>> + __do_cancel ();
>> + }
>>
>> /* And give it up right away. */
>> lll_unlock (pd->lock, LLL_PRIVATE);
>> }
>
> I don't think this is quite right. The failed thread should not linger
> around after pthread_create has reported failure to the caller. It's
> better than what we had before, so maybe we should use this version and
> clean it up further later.
Agreed, but I think this is really an improvement over current strategy
as I commented earlier [1]. I think to proper fix would move the
lock prior any initialization so we don't need to run any unwinding
operations, skip directly to INTERNAL_SYSCALL_CALL (exit, 0), and let the
parent join the thread and do the resource deallocation.
>
> At the point of the __do_cancel call, unwinding may not yet initialized,
> so the implied dlopen happens on a potentially very small stack. It
> should be possible to replace it with a goto to the cleanup code. Or
> maybe put the cleanup code into a separate function and call it here and
> on the regular cleanup path.
>
> Apart from that, it looks fine to me.
The __do_cancel is not done really in asynchronous mode, so it will use
the allocated thread stack for the dlopen. But I think with a better
strategy to move the 'struct thread' lock earlier than any initialization
will avoid such issue.
>
> Thanks,
> Florian
>
[1] https://sourceware.org/pipermail/libc-alpha/2021-May/126879.html
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v2 2/9] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-06-01 13:08 ` Adhemerval Zanella
@ 2021-06-01 13:55 ` Adhemerval Zanella
0 siblings, 0 replies; 49+ messages in thread
From: Adhemerval Zanella @ 2021-06-01 13:55 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella via Libc-alpha
On 01/06/2021 10:08, Adhemerval Zanella wrote:
>
>
> On 01/06/2021 05:32, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>>
>>> @@ -424,17 +409,22 @@ start_thread (void *arg)
>>> have ownership (see CONCURRENCY NOTES above). */
>>> if (__glibc_unlikely (pd->stopped_start))
>>> {
>>> /* Get the lock the parent locked to force synchronization. */
>>> lll_lock (pd->lock, LLL_PRIVATE);
>>>
>>> /* We have ownership of PD now. */
>>> + if (pd->setup_failed == 1)
>>> + {
>>> + /* In the case of a failed setup, the created thread will be
>>> + responsible to free up the allocated resources. The
>>> + detached mode ensure it won't be joined and it will trigger
>>> + the required cleanup. */
>>> + pd->joinid = pd;
>>> + __do_cancel ();
>>> + }
>>>
>>> /* And give it up right away. */
>>> lll_unlock (pd->lock, LLL_PRIVATE);
>>> }
>>
>> I don't think this is quite right. The failed thread should not linger
>> around after pthread_create has reported failure to the caller. It's
>> better than what we had before, so maybe we should use this version and
>> clean it up further later.
>
> Agreed, but I think this is really an improvement over current strategy
> as I commented earlier [1]. I think to proper fix would move the
> lock prior any initialization so we don't need to run any unwinding
> operations, skip directly to INTERNAL_SYSCALL_CALL (exit, 0), and let the
> parent join the thread and do the resource deallocation.
>
>>
>> At the point of the __do_cancel call, unwinding may not yet initialized,
>> so the implied dlopen happens on a potentially very small stack. It
>> should be possible to replace it with a goto to the cleanup code. Or
>> maybe put the cleanup code into a separate function and call it here and
>> on the regular cleanup path.
>>
>> Apart from that, it looks fine to me.
>
> The __do_cancel is not done really in asynchronous mode, so it will use
> the allocated thread stack for the dlopen. But I think with a better
> strategy to move the 'struct thread' lock earlier than any initialization
> will avoid such issue.
>
>>
>> Thanks,
>> Florian
>>
>
> [1] https://sourceware.org/pipermail/libc-alpha/2021-May/126879.html
>
The patch below on top of this patch implements the synchronous thread
join in case of setup failure. It shows no regressions on testcases,
and it is slight better because it avoid calling the __pthread_unwind for
such cases (and issuing the libgcc dlopen)
In fact since there is no registered user case that might required
calling either cancellation cleanup handler or thread destructors,
I think there is no need to actually issue a pthread_exit like
termination.
--
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 018af30c85..7a4c742416 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -346,6 +346,23 @@ start_thread (void *arg)
{
struct pthread *pd = arg;
+ /* We are either in (a) or (b), and in either case we either own PD already
+ (2) or are about to own PD (1), and so our only restriction would be that
+ we can't free PD until we know we have ownership (see CONCURRENCY NOTES
+ above). */
+ if (__glibc_unlikely (pd->stopped_start))
+ {
+ /* Get the lock the parent locked to force synchronization. */
+ lll_lock (pd->lock, LLL_PRIVATE);
+
+ /* We have ownership of PD now. */
+ if (pd->setup_failed == 1)
+ goto out;
+
+ /* And give it up right away. */
+ lll_unlock (pd->lock, LLL_PRIVATE);
+ }
+
/* Initialize resolver state pointer. */
__resp = &pd->res;
@@ -403,30 +420,6 @@ start_thread (void *arg)
/* Store the new cleanup handler info. */
THREAD_SETMEM (pd, cleanup_jmp_buf, &unwind_buf);
- /* We are either in (a) or (b), and in either case we either own
- PD already (2) or are about to own PD (1), and so our only
- restriction would be that we can't free PD until we know we
- have ownership (see CONCURRENCY NOTES above). */
- if (__glibc_unlikely (pd->stopped_start))
- {
- /* Get the lock the parent locked to force synchronization. */
- lll_lock (pd->lock, LLL_PRIVATE);
-
- /* We have ownership of PD now. */
- if (pd->setup_failed == 1)
- {
- /* In the case of a failed setup, the created thread will be
- responsible to free up the allocated resources. The
- detached mode ensure it won't be joined and it will trigger
- the required cleanup. */
- pd->joinid = pd;
- __do_cancel ();
- }
-
- /* And give it up right away. */
- lll_unlock (pd->lock, LLL_PRIVATE);
- }
-
LIBC_PROBE (pthread_start, 3, (pthread_t) pd, pd->start_routine, pd->arg);
/* Run the code the user provided. */
@@ -556,6 +549,7 @@ start_thread (void *arg)
/* Free the TCB. */
__nptl_free_tcb (pd);
+out:
/* We cannot call '_exit' here. '_exit' will terminate the process.
The 'exit' implementation in the kernel will signal when the
@@ -802,6 +796,8 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
thread. */
__libc_signal_restore_set (&original_sigmask);
+ int setup_failed = 0;
+
if (__glibc_unlikely (retval != 0))
{
if (thread_ran)
@@ -814,7 +810,7 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
/* 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. */
- pd->setup_failed = 1;
+ setup_failed = pd->setup_failed = 1;
lll_unlock (pd->lock, LLL_PRIVATE);
}
else
@@ -857,6 +853,9 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1);
}
+ if (setup_failed == 1)
+ __pthread_join (*newthread, NULL);
+
out:
if (destroy_default_attr)
__pthread_attr_destroy (&default_attr.external);
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v3] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-05-27 17:28 ` [PATCH v2 2/9] nptl: Deallocate the thread stack on setup failure (BZ #19511) Adhemerval Zanella
2021-06-01 8:32 ` Florian Weimer
@ 2021-06-02 12:56 ` Adhemerval Zanella
2021-06-02 13:08 ` Andreas Schwab
2021-06-08 10:56 ` Florian Weimer
1 sibling, 2 replies; 49+ messages in thread
From: Adhemerval Zanella @ 2021-06-02 12:56 UTC (permalink / raw)
To: libc-alpha
To setup either the thread scheduling parameters or affinity,
pthread_create enforce synchronization on created thread to wait until
its parent either release PD ownership 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).
This patch changes on how the thread resource is deallocate in case of
failure to be synchrnous, where the creating thread will signal the
created thread to early exit so it could be joined. The creating thread
will be reponsible for the resource cleanup before return to caller.
To signal the creating thread a failure has occured, an unused
'struct pthread' member, parent_cancelhandling_unsed, now indicates
whether the setup has failed so creating thread can proper exit.
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). Another advantage is since the
early exit is move to first step at thread creation, the signal
mask is not already set and thus it can not act on change ID setxid
handler.
Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
nptl/allocatestack.c | 1 +
nptl/descr.h | 5 +-
nptl/pthread_create.c | 140 ++++++++++++++++++++----------------------
3 files changed, 71 insertions(+), 75 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 52d6738f7f..4b143a5016 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -167,19 +167,19 @@ late_init (void)
without error, then the created thread owns PD; otherwise, see
(c) and (e) below.
- (c) If the detached thread setup failed and THREAD_RAN is true, then
- the creating thread releases ownership to the new thread by
- sending a cancellation signal. All threads set THREAD_RAN to
- true as quickly as possible after returning from the OS kernel's
- thread creation routine.
+ (c) If the detached thread setup failed and THREAD_RAN is true, then the
+ creating thread releases ownership to the new thread, the created
+ thread see the failed setup through a PD field and releases the
+ PD ownership and early exit. The creating thread will be one
+ responsible for cleanup state. All threads set THREAD_RAN to true as
+ quickly as possible after returning from the OS kernel's thread
+ creation routine.
(d) If the joinable thread setup failed and THREAD_RAN is true, then
- then the creating thread retains ownership of PD and must cleanup
+ the creating thread retains ownership of PD and must cleanup
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.
@@ -239,8 +239,8 @@ late_init (void)
The return value is zero for success or an errno code for failure.
If the return value is ENOMEM, that will be translated to EAGAIN,
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). */
+ be set to true iff the thread actually started up but before calling
+ the user code (*PD->start_routine). */
static int _Noreturn start_thread (void *arg);
@@ -308,35 +308,23 @@ static int create_thread (struct pthread *pd, const struct pthread_attr *attr,
== -1))
return errno;
- /* It's started now, so if we fail below, we'll have to cancel it
- and let it clean itself up. */
+ /* It's started now, so if we fail below, we'll have to 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);
-
+ 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);
- }
+ return INTERNAL_SYSCALL_ERRNO (res);
}
/* Set the scheduling parameters. */
@@ -344,11 +332,10 @@ 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;
+ return INTERNAL_SYSCALL_ERRNO (res);
}
}
@@ -361,6 +348,29 @@ start_thread (void *arg)
{
struct pthread *pd = arg;
+ /* We are either in (a) or (b), and in either case we either own PD already
+ (2) or are about to own PD (1), and so our only restriction would be that
+ we can't free PD until we know we have ownership (see CONCURRENCY NOTES
+ above). */
+ bool setup_failed = false;
+ if (__glibc_unlikely (pd->stopped_start))
+ {
+ /* Get the lock the parent locked to force synchronization. */
+ lll_lock (pd->lock, LLL_PRIVATE);
+
+ /* We have ownership of PD now, for detached threads with setup failure
+ we set it as joinable so the creating thread could synchronous join
+ and free any resource prior return to the pthread_create caller. */
+ setup_failed = pd->setup_failed == 1;
+ if (setup_failed)
+ pd->joinid = NULL;
+
+ /* And give it up right away. */
+ lll_unlock (pd->lock, LLL_PRIVATE);
+ }
+ if (setup_failed)
+ goto out;
+
/* Initialize resolver state pointer. */
__resp = &pd->res;
@@ -418,25 +428,6 @@ start_thread (void *arg)
/* Store the new cleanup handler info. */
THREAD_SETMEM (pd, cleanup_jmp_buf, &unwind_buf);
- /* We are either in (a) or (b), and in either case we either own
- PD already (2) or are about to own PD (1), and so our only
- restriction would be that we can't free PD until we know we
- 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);
-
- /* We have ownership of PD now. */
-
- /* And give it up right away. */
- lll_unlock (pd->lock, LLL_PRIVATE);
-
- LIBC_CANCEL_RESET (oldtype);
- }
-
LIBC_PROBE (pthread_start, 3, (pthread_t) pd, pd->start_routine, pd->arg);
/* Run the code the user provided. */
@@ -566,6 +557,7 @@ start_thread (void *arg)
/* Free the TCB. */
__nptl_free_tcb (pd);
+out:
/* We cannot call '_exit' here. '_exit' will terminate the process.
The 'exit' implementation in the kernel will signal when the
@@ -759,7 +751,6 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
signal mask of this thread, so save it in the startup
information. */
pd->sigmask = original_sigmask;
-
/* Reset the cancellation signal mask in case this thread is
running cancellation. */
__sigdelset (&pd->sigmask, SIGCANCEL);
@@ -814,30 +805,33 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
if (__glibc_unlikely (retval != 0))
{
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. */
- /* See bug 19511 which explains why doing nothing here is a
- resource leak for a joinable thread. */
- assert (stopped_start);
- else
- {
- /* State (e) and we have ownership of PD (see CONCURRENCY
- NOTES above). */
+ /* State (c) and we 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);
+ /* Signal the creating thread to release PD ownership and early
+ exit so it could be joined. */
+ pd->setup_failed = 1;
+ lll_unlock (pd->lock, LLL_PRIVATE);
- /* Oops, we lied for a second. */
- atomic_decrement (&__nptl_nthreads);
+ /* Similar to pthread_join, but since thread creation has failed at
+ startup there is no need to handle all the steps. */
+ pid_t tid;
+ while ((tid = atomic_load_acquire (&pd->tid)) != 0)
+ __futex_abstimed_wait_cancelable64 ((unsigned int *) &pd->tid,
+ tid, 0, NULL, LLL_SHARED);
+ }
- /* Perhaps a thread wants to change the IDs and is waiting for this
- stillborn thread. */
- if (__glibc_unlikely (atomic_exchange_acq (&pd->setxid_futex, 0)
- == -2))
- futex_wake (&pd->setxid_futex, 1, FUTEX_PRIVATE);
+ /* State (c), (d), or (e) and we have ownership of PD (see CONCURRENCY
+ NOTES above). */
- /* Free the resources. */
- __nptl_deallocate_stack (pd);
- }
+ /* Oops, we lied for a second. */
+ atomic_decrement (&__nptl_nthreads);
+
+ /* Free the resources. */
+ __nptl_deallocate_stack (pd);
/* We have to translate error codes. */
if (retval == ENOMEM)
--
2.30.2
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v3] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-06-02 12:56 ` [PATCH v3] " Adhemerval Zanella
@ 2021-06-02 13:08 ` Andreas Schwab
2021-06-02 13:39 ` Adhemerval Zanella
2021-06-08 10:56 ` Florian Weimer
1 sibling, 1 reply; 49+ messages in thread
From: Andreas Schwab @ 2021-06-02 13:08 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
On Jun 02 2021, Adhemerval Zanella via Libc-alpha wrote:
> 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;
Perhaps this should be turned into a bitfield?
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."
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v3] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-06-02 13:08 ` Andreas Schwab
@ 2021-06-02 13:39 ` Adhemerval Zanella
2021-06-02 13:41 ` Andreas Schwab
0 siblings, 1 reply; 49+ messages in thread
From: Adhemerval Zanella @ 2021-06-02 13:39 UTC (permalink / raw)
To: Andreas Schwab, Adhemerval Zanella via Libc-alpha
On 02/06/2021 10:08, Andreas Schwab wrote:
> On Jun 02 2021, Adhemerval Zanella via Libc-alpha wrote:
>
>> 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;
>
> Perhaps this should be turned into a bitfield?
I do not want to change the 'struct parent' layout or size with this
patch. I discussed briefly with Florian on another thread and maybe
we can cleanup the struct pthread, there are a couple of unused fields
and others that can use a better type.
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v3] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-06-02 13:39 ` Adhemerval Zanella
@ 2021-06-02 13:41 ` Andreas Schwab
2021-06-02 14:01 ` Adhemerval Zanella
0 siblings, 1 reply; 49+ messages in thread
From: Andreas Schwab @ 2021-06-02 13:41 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: Adhemerval Zanella via Libc-alpha
On Jun 02 2021, Adhemerval Zanella wrote:
> On 02/06/2021 10:08, Andreas Schwab wrote:
>> On Jun 02 2021, Adhemerval Zanella via Libc-alpha wrote:
>>
>>> 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;
>>
>> Perhaps this should be turned into a bitfield?
>
> I do not want to change the 'struct parent' layout or size with this
> patch.
I didn't say you should change the layout.
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."
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v3] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-06-02 13:41 ` Andreas Schwab
@ 2021-06-02 14:01 ` Adhemerval Zanella
2021-06-02 14:07 ` Andreas Schwab
0 siblings, 1 reply; 49+ messages in thread
From: Adhemerval Zanella @ 2021-06-02 14:01 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Adhemerval Zanella via Libc-alpha
On 02/06/2021 10:41, Andreas Schwab wrote:
> On Jun 02 2021, Adhemerval Zanella wrote:
>
>> On 02/06/2021 10:08, Andreas Schwab wrote:
>>> On Jun 02 2021, Adhemerval Zanella via Libc-alpha wrote:
>>>
>>>> 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;
>>>
>>> Perhaps this should be turned into a bitfield?
>>
>> I do not want to change the 'struct parent' layout or size with this
>> patch.
>
> I didn't say you should change the layout.
I don't have a strong preference here, maybe use a 'bool' then?
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v3] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-06-02 14:01 ` Adhemerval Zanella
@ 2021-06-02 14:07 ` Andreas Schwab
2021-06-02 14:15 ` Adhemerval Zanella
2021-06-02 18:20 ` Joseph Myers
0 siblings, 2 replies; 49+ messages in thread
From: Andreas Schwab @ 2021-06-02 14:07 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: Adhemerval Zanella via Libc-alpha
On Jun 02 2021, Adhemerval Zanella wrote:
> I don't have a strong preference here, maybe use a 'bool' then?
bool has architecture dependent size.
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."
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v3] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-06-02 14:07 ` Andreas Schwab
@ 2021-06-02 14:15 ` Adhemerval Zanella
2021-06-02 14:30 ` Andreas Schwab
2021-06-02 18:20 ` Joseph Myers
1 sibling, 1 reply; 49+ messages in thread
From: Adhemerval Zanella @ 2021-06-02 14:15 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Adhemerval Zanella via Libc-alpha
On 02/06/2021 11:07, Andreas Schwab wrote:
> On Jun 02 2021, Adhemerval Zanella wrote:
>
>> I don't have a strong preference here, maybe use a 'bool' then?
>
> bool has architecture dependent size.
Fair enough, I will change to:
int setup_failed:1;
int ununsed_0:31;
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v3] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-06-02 14:15 ` Adhemerval Zanella
@ 2021-06-02 14:30 ` Andreas Schwab
2021-06-02 14:42 ` Adhemerval Zanella
0 siblings, 1 reply; 49+ messages in thread
From: Andreas Schwab @ 2021-06-02 14:30 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: Adhemerval Zanella via Libc-alpha
On Jun 02 2021, Adhemerval Zanella wrote:
> On 02/06/2021 11:07, Andreas Schwab wrote:
>> On Jun 02 2021, Adhemerval Zanella wrote:
>>
>>> I don't have a strong preference here, maybe use a 'bool' then?
>>
>> bool has architecture dependent size.
>
> Fair enough, I will change to:
>
> int setup_failed:1;
That should be unsigned int.
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."
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v3] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-06-02 14:30 ` Andreas Schwab
@ 2021-06-02 14:42 ` Adhemerval Zanella
0 siblings, 0 replies; 49+ messages in thread
From: Adhemerval Zanella @ 2021-06-02 14:42 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Adhemerval Zanella via Libc-alpha
On 02/06/2021 11:30, Andreas Schwab wrote:
> On Jun 02 2021, Adhemerval Zanella wrote:
>
>> On 02/06/2021 11:07, Andreas Schwab wrote:
>>> On Jun 02 2021, Adhemerval Zanella wrote:
>>>
>>>> I don't have a strong preference here, maybe use a 'bool' then?
>>>
>>> bool has architecture dependent size.
>>
>> Fair enough, I will change to:
>>
>> int setup_failed:1;
>
> That should be unsigned int.
>
> Andreas.
>
Ack.
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v3] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-06-02 14:07 ` Andreas Schwab
2021-06-02 14:15 ` Adhemerval Zanella
@ 2021-06-02 18:20 ` Joseph Myers
2021-06-02 18:30 ` Florian Weimer
1 sibling, 1 reply; 49+ messages in thread
From: Joseph Myers @ 2021-06-02 18:20 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Adhemerval Zanella, Adhemerval Zanella via Libc-alpha
On Wed, 2 Jun 2021, Andreas Schwab wrote:
> On Jun 02 2021, Adhemerval Zanella wrote:
>
> > I don't have a strong preference here, maybe use a 'bool' then?
>
> bool has architecture dependent size.
bool is one byte for all configurations supported by glibc (the only
configuration supported by GCC for which it isn't is powerpc-darwin).
Although I don't see any likely reason for glibc code to depend on it
being one byte.
(GCC 2.95 had a different, four-byte bool in <stdbool.h>; if any future
glibc interfaces involve bool in ABIs, I expect we should simply not
support use of those interfaces with GCC 2.95.)
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v3] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-06-02 18:20 ` Joseph Myers
@ 2021-06-02 18:30 ` Florian Weimer
2021-06-02 19:02 ` Adhemerval Zanella
0 siblings, 1 reply; 49+ messages in thread
From: Florian Weimer @ 2021-06-02 18:30 UTC (permalink / raw)
To: Joseph Myers; +Cc: Andreas Schwab, Adhemerval Zanella via Libc-alpha
* Joseph Myers:
> On Wed, 2 Jun 2021, Andreas Schwab wrote:
>
>> On Jun 02 2021, Adhemerval Zanella wrote:
>>
>> > I don't have a strong preference here, maybe use a 'bool' then?
>>
>> bool has architecture dependent size.
>
> bool is one byte for all configurations supported by glibc (the only
> configuration supported by GCC for which it isn't is powerpc-darwin).
> Although I don't see any likely reason for glibc code to depend on it
> being one byte.
However, one reason not to use bool in internal code is that our own
atomics do not support one-byte types on all architectures.
For example this definition in sysdeps/powerpc/atomic-machine.h:
#define atomic_exchange_acq(mem, value) \
({ \
__typeof (*(mem)) __result; \
if (sizeof (*mem) == 4) \
__result = __arch_atomic_exchange_32_acq (mem, value); \
else if (sizeof (*mem) == 8) \
__result = __arch_atomic_exchange_64_acq (mem, value); \
else \
abort (); \
__result; \
})
Thanks,
Florian
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v3] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-06-02 18:30 ` Florian Weimer
@ 2021-06-02 19:02 ` Adhemerval Zanella
2021-06-02 19:11 ` Florian Weimer
0 siblings, 1 reply; 49+ messages in thread
From: Adhemerval Zanella @ 2021-06-02 19:02 UTC (permalink / raw)
To: libc-alpha
On 02/06/2021 15:30, Florian Weimer via Libc-alpha wrote:
> * Joseph Myers:
>
>> On Wed, 2 Jun 2021, Andreas Schwab wrote:
>>
>>> On Jun 02 2021, Adhemerval Zanella wrote:
>>>
>>>> I don't have a strong preference here, maybe use a 'bool' then?
>>>
>>> bool has architecture dependent size.
>>
>> bool is one byte for all configurations supported by glibc (the only
>> configuration supported by GCC for which it isn't is powerpc-darwin).
>> Although I don't see any likely reason for glibc code to depend on it
>> being one byte.
>
> However, one reason not to use bool in internal code is that our own
> atomics do not support one-byte types on all architectures.
>
> For example this definition in sysdeps/powerpc/atomic-machine.h:
>
> #define atomic_exchange_acq(mem, value) \
> ({ \
> __typeof (*(mem)) __result; \
> if (sizeof (*mem) == 4) \
> __result = __arch_atomic_exchange_32_acq (mem, value); \
> else if (sizeof (*mem) == 8) \
> __result = __arch_atomic_exchange_64_acq (mem, value); \
> else \
> abort (); \
> __result; \
> })
I think it should be safe to use on code that won't be used for atomic
operations. The this specific flag, for instance, is protected by
the PD lock.
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v3] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-06-02 19:02 ` Adhemerval Zanella
@ 2021-06-02 19:11 ` Florian Weimer
0 siblings, 0 replies; 49+ messages in thread
From: Florian Weimer @ 2021-06-02 19:11 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella via Libc-alpha:
> I think it should be safe to use on code that won't be used for atomic
> operations. The this specific flag, for instance, is protected by
> the PD lock.
Right, the atomics limitation does not seem to apply in this case.
Thanks,
Florian
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v3] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-06-02 12:56 ` [PATCH v3] " Adhemerval Zanella
2021-06-02 13:08 ` Andreas Schwab
@ 2021-06-08 10:56 ` Florian Weimer
2021-06-08 17:01 ` Adhemerval Zanella
1 sibling, 1 reply; 49+ messages in thread
From: Florian Weimer @ 2021-06-08 10:56 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella via Libc-alpha:
> To setup either the thread scheduling parameters or affinity,
> pthread_create enforce synchronization on created thread to wait until
> its parent either release PD ownership 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).
>
> This patch changes on how the thread resource is deallocate in case of
> failure to be synchrnous, where the creating thread will signal the
“synchronous”
> created thread to early exit so it could be joined. The creating thread
“to exit early”
> will be reponsible for the resource cleanup before return to caller.
“returning to the caller”
> To signal the creating thread a failure has occured, an unused
“that a failure has occured”
> 'struct pthread' member, parent_cancelhandling_unsed, now indicates
(okay, existing typo)
> whether the setup has failed so creating thread can proper exit.
>
> 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). Another advantage is since the
> early exit is move to first step at thread creation, the signal
> mask is not already set and thus it can not act on change ID setxid
> handler.
>
> Checked on x86_64-linux-gnu and aarch64-linux-gnu.
> ---
> nptl/allocatestack.c | 1 +
> nptl/descr.h | 5 +-
> nptl/pthread_create.c | 140 ++++++++++++++++++++----------------------
> 3 files changed, 71 insertions(+), 75 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 52d6738f7f..4b143a5016 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -167,19 +167,19 @@ late_init (void)
> without error, then the created thread owns PD; otherwise, see
> (c) and (e) below.
>
> - (c) If the detached thread setup failed and THREAD_RAN is true, then
> - the creating thread releases ownership to the new thread by
> - sending a cancellation signal. All threads set THREAD_RAN to
> - true as quickly as possible after returning from the OS kernel's
> - thread creation routine.
> + (c) If the detached thread setup failed and THREAD_RAN is true, then the
> + creating thread releases ownership to the new thread, the created
> + thread see the failed setup through a PD field and releases the
> + PD ownership and early exit. The creating thread will be one
> + responsible for cleanup state. All threads set THREAD_RAN to true as
> + quickly as possible after returning from the OS kernel's thread
> + creation routine.
“the created thread see[s]”, “and [exits early]”,
“will be [] responsible”.
I don't think (c) is no longer described correctly. THREAD_RAN is set
even before user code runs, and ownership is not released in this case.
(I think this shows the limits of describing what happens in terms of
ownership of PD.)
Perhaps add a note that the distinction between (c) and (d) only matters
briefly within create_thread?
> (d) If the joinable thread setup failed and THREAD_RAN is true, then
> - then the creating thread retains ownership of PD and must cleanup
> + the creating thread retains ownership of PD and must cleanup
> 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.
And I'm not sure if THREAD_RAN matters for the distinction between (d)
and (e), either.
This suggests to me that we should keep THREAD_RAN local to
create_thread, and handle all setup errors there.
> @@ -239,8 +239,8 @@ late_init (void)
> The return value is zero for success or an errno code for failure.
> If the return value is ENOMEM, that will be translated to EAGAIN,
> 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). */
> + be set to true iff the thread actually started up but before calling
> + the user code (*PD->start_routine). */
>
> static int _Noreturn start_thread (void *arg);
>
> @@ -308,35 +308,23 @@ static int create_thread (struct pthread *pd, const struct pthread_attr *attr,
> == -1))
> return errno;
>
> - /* It's started now, so if we fail below, we'll have to cancel it
> - and let it clean itself up. */
> + /* It's started now, so if we fail below, we'll have to 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);
> -
> + 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);
> - }
> + return INTERNAL_SYSCALL_ERRNO (res);
> }
>
> /* Set the scheduling parameters. */
> @@ -344,11 +332,10 @@ 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;
> + return INTERNAL_SYSCALL_ERRNO (res);
> }
> }
>
> @@ -361,6 +348,29 @@ start_thread (void *arg)
> {
> struct pthread *pd = arg;
>
> + /* We are either in (a) or (b), and in either case we either own PD already
> + (2) or are about to own PD (1), and so our only restriction would be that
> + we can't free PD until we know we have ownership (see CONCURRENCY NOTES
> + above). */
> + bool setup_failed = false;
> + if (__glibc_unlikely (pd->stopped_start))
I think we should remove __glibc_unlikely for cases which can always be
true due to particular application use cases.
> + {
> + /* Get the lock the parent locked to force synchronization. */
> + lll_lock (pd->lock, LLL_PRIVATE);
> +
> + /* We have ownership of PD now, for detached threads with setup failure
> + we set it as joinable so the creating thread could synchronous join
> + and free any resource prior return to the pthread_create caller. */
> + setup_failed = pd->setup_failed == 1;
> + if (setup_failed)
> + pd->joinid = NULL;
> +
> + /* And give it up right away. */
> + lll_unlock (pd->lock, LLL_PRIVATE);
> + }
> + if (setup_failed)
> + goto out;
I think it would be clearer if that goto were nested within the previous
if block.
> @@ -814,30 +805,33 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
> if (__glibc_unlikely (retval != 0))
> {
> 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. */
> - /* See bug 19511 which explains why doing nothing here is a
> - resource leak for a joinable thread. */
> - assert (stopped_start);
> - else
> - {
> - /* State (e) and we have ownership of PD (see CONCURRENCY
> - NOTES above). */
> + /* State (c) and we 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);
> + /* Signal the creating thread to release PD ownership and early
> + exit so it could be joined. */
> + pd->setup_failed = 1;
> + lll_unlock (pd->lock, LLL_PRIVATE);
“Signal the creat[ed] thread” (this code runs on the creating thread).
> + /* Similar to pthread_join, but since thread creation has failed at
> + startup there is no need to handle all the steps. */
> + pid_t tid;
> + while ((tid = atomic_load_acquire (&pd->tid)) != 0)
> + __futex_abstimed_wait_cancelable64 ((unsigned int *) &pd->tid,
> + tid, 0, NULL, LLL_SHARED);
> + }
>
> + /* State (c), (d), or (e) and we have ownership of PD (see CONCURRENCY
> + NOTES above). */
>
> + /* Oops, we lied for a second. */
> + atomic_decrement (&__nptl_nthreads);
> +
> + /* Free the resources. */
> + __nptl_deallocate_stack (pd);
>
> /* We have to translate error codes. */
> if (retval == ENOMEM)
What's missing further below is pd->setup_failed handling if there is an
early failure on the created thread (I have my sigaltstack changes in
this mind). No such code exits in your patch, so this is consistent in
this regard.
The actual code changes are fine, I think.
Thanks,
Florian
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v3] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-06-08 10:56 ` Florian Weimer
@ 2021-06-08 17:01 ` Adhemerval Zanella
2021-06-09 13:49 ` Florian Weimer
0 siblings, 1 reply; 49+ messages in thread
From: Adhemerval Zanella @ 2021-06-08 17:01 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella via Libc-alpha
On 08/06/2021 07:56, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> To setup either the thread scheduling parameters or affinity,
>> pthread_create enforce synchronization on created thread to wait until
>> its parent either release PD ownership 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).
>>
>> This patch changes on how the thread resource is deallocate in case of
>> failure to be synchrnous, where the creating thread will signal the
>
> “synchronous”
Ack.
>
>> created thread to early exit so it could be joined. The creating thread
>
> “to exit early”
Ack.
>
>> will be reponsible for the resource cleanup before return to caller.
>
> “returning to the caller”
Ack.
>
>> To signal the creating thread a failure has occured, an unused
>
> “that a failure has occured”
Ack.
>
>> 'struct pthread' member, parent_cancelhandling_unsed, now indicates
>
> (okay, existing typo)
>
>> whether the setup has failed so creating thread can proper exit.
>>
>> 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). Another advantage is since the
>> early exit is move to first step at thread creation, the signal
>> mask is not already set and thus it can not act on change ID setxid
>> handler.
>>
>> Checked on x86_64-linux-gnu and aarch64-linux-gnu.
>> ---
>> nptl/allocatestack.c | 1 +
>> nptl/descr.h | 5 +-
>> nptl/pthread_create.c | 140 ++++++++++++++++++++----------------------
>> 3 files changed, 71 insertions(+), 75 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 52d6738f7f..4b143a5016 100644
>> --- a/nptl/pthread_create.c
>> +++ b/nptl/pthread_create.c
>> @@ -167,19 +167,19 @@ late_init (void)
>> without error, then the created thread owns PD; otherwise, see
>> (c) and (e) below.
>>
>> - (c) If the detached thread setup failed and THREAD_RAN is true, then
>> - the creating thread releases ownership to the new thread by
>> - sending a cancellation signal. All threads set THREAD_RAN to
>> - true as quickly as possible after returning from the OS kernel's
>> - thread creation routine.
>> + (c) If the detached thread setup failed and THREAD_RAN is true, then the
>> + creating thread releases ownership to the new thread, the created
>> + thread see the failed setup through a PD field and releases the
>> + PD ownership and early exit. The creating thread will be one
>> + responsible for cleanup state. All threads set THREAD_RAN to true as
>> + quickly as possible after returning from the OS kernel's thread
>> + creation routine.
>
> “the created thread see[s]”, “and [exits early]”,
> “will be [] responsible”.
Ack.
>
> I don't think (c) is no longer described correctly. THREAD_RAN is set
> even before user code runs, and ownership is not released in this case.
> (I think this shows the limits of describing what happens in terms of
> ownership of PD.)
>
> Perhaps add a note that the distinction between (c) and (d) only matters
> briefly within create_thread?
>
>> (d) If the joinable thread setup failed and THREAD_RAN is true, then
>> - then the creating thread retains ownership of PD and must cleanup
>> + the creating thread retains ownership of PD and must cleanup
>> 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.
>
>
> And I'm not sure if THREAD_RAN matters for the distinction between (d)
> and (e), either.
>
> This suggests to me that we should keep THREAD_RAN local to
> create_thread, and handle all setup errors there.
THREAD_RAN only signals if whether clone() or thread setup has failed,
there is no synchronization involved and it is already local to
creating thread. I changed the concurrency comment to the following,
to indicate that (c) and (d) are essentially the same as you noted:
(c) If either a joinable or detached thread setup failed and THREAD_RAN
is true, then the creating thread releases ownership to the new thread,
the created thread sees the failed setup through PD SETUP_FAILED
member, releases the PD ownership, and exits. The creating thread will
be responsible for cleanup the allocated resources. The THREAD_RAN is
local to creating thread and indicate whether thread creation or setup
has failed.
(d) If the thread creation failed and THREAD_RAN is false (meaning
ARCH_CLONE has failed), then the creating thread retains ownership
of PD and must cleanup he allocated resource. No waiting for the new
thread is required because it never started.
(with the adjusted references to (c), (d), and (e) adjusted on other
comments).
>
>> @@ -239,8 +239,8 @@ late_init (void)
>> The return value is zero for success or an errno code for failure.
>> If the return value is ENOMEM, that will be translated to EAGAIN,
>> 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). */
>> + be set to true iff the thread actually started up but before calling
>> + the user code (*PD->start_routine). */
>>
>> static int _Noreturn start_thread (void *arg);
>>
>> @@ -308,35 +308,23 @@ static int create_thread (struct pthread *pd, const struct pthread_attr *attr,
>> == -1))
>> return errno;
>>
>> - /* It's started now, so if we fail below, we'll have to cancel it
>> - and let it clean itself up. */
>> + /* It's started now, so if we fail below, we'll have to 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);
>> -
>> + 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);
>> - }
>> + return INTERNAL_SYSCALL_ERRNO (res);
>> }
>>
>> /* Set the scheduling parameters. */
>> @@ -344,11 +332,10 @@ 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;
>> + return INTERNAL_SYSCALL_ERRNO (res);
>> }
>> }
>>
>> @@ -361,6 +348,29 @@ start_thread (void *arg)
>> {
>> struct pthread *pd = arg;
>>
>> + /* We are either in (a) or (b), and in either case we either own PD already
>> + (2) or are about to own PD (1), and so our only restriction would be that
>> + we can't free PD until we know we have ownership (see CONCURRENCY NOTES
>> + above). */
>> + bool setup_failed = false;
>> + if (__glibc_unlikely (pd->stopped_start))
>
> I think we should remove __glibc_unlikely for cases which can always be
> true due to particular application use cases.
Ack.
>
>> + {
>> + /* Get the lock the parent locked to force synchronization. */
>> + lll_lock (pd->lock, LLL_PRIVATE);
>> +
>> + /* We have ownership of PD now, for detached threads with setup failure
>> + we set it as joinable so the creating thread could synchronous join
>> + and free any resource prior return to the pthread_create caller. */
>> + setup_failed = pd->setup_failed == 1;
>> + if (setup_failed)
>> + pd->joinid = NULL;
>> +
>> + /* And give it up right away. */
>> + lll_unlock (pd->lock, LLL_PRIVATE);
>> + }
>> + if (setup_failed)
>> + goto out;
>
> I think it would be clearer if that goto were nested within the previous
> if block.
Ack, I moved both the 'setup_failed' and the goto check inside the if block.
>
>> @@ -814,30 +805,33 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>> if (__glibc_unlikely (retval != 0))
>> {
>> 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. */
>> - /* See bug 19511 which explains why doing nothing here is a
>> - resource leak for a joinable thread. */
>> - assert (stopped_start);
>> - else
>> - {
>> - /* State (e) and we have ownership of PD (see CONCURRENCY
>> - NOTES above). */
>> + /* State (c) and we 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);
>> + /* Signal the creating thread to release PD ownership and early
>> + exit so it could be joined. */
>> + pd->setup_failed = 1;
>> + lll_unlock (pd->lock, LLL_PRIVATE);
>
> “Signal the creat[ed] thread” (this code runs on the creating thread).
Ack.
>
>> + /* Similar to pthread_join, but since thread creation has failed at
>> + startup there is no need to handle all the steps. */
>> + pid_t tid;
>> + while ((tid = atomic_load_acquire (&pd->tid)) != 0)
>> + __futex_abstimed_wait_cancelable64 ((unsigned int *) &pd->tid,
>> + tid, 0, NULL, LLL_SHARED);
>> + }
>>
>> + /* State (c), (d), or (e) and we have ownership of PD (see CONCURRENCY
>> + NOTES above). */
>>
>> + /* Oops, we lied for a second. */
>> + atomic_decrement (&__nptl_nthreads);
>> +
>> + /* Free the resources. */
>> + __nptl_deallocate_stack (pd);
>>
>> /* We have to translate error codes. */
>> if (retval == ENOMEM)
>
> What's missing further below is pd->setup_failed handling if there is an
> early failure on the created thread (I have my sigaltstack changes in
> this mind). No such code exits in your patch, so this is consistent in
> this regard.
I think for such case you will need to set PD->stopped_start to true
before thread creation (similar to what affinity and schedule set do),
add the required initialization after the setup failure check, and
handle potential errors.
Something like (I removed the comments to simplify):
static int _Noreturn
start_thread (void *arg)
{
struct pthread *pd = arg;
if (pd->stopped_start)
{
bool setup_failed = false;
/* Get the lock the parent locked to force synchronization. */
lll_lock (pd->lock, LLL_PRIVATE);
setup_failed = pd->setup_failed == 1;
if (setup_failed)
pd->joinid = NULL;
/* Handle any thread setup requirement. If a failure occurs, set
PD->setup_failed and 'setup_failed' to true to exit early. */
lll_unlock (pd->lock, LLL_PRIVATE);
if (setup_failed)
goto out;
}
[...]
}
Then on the 'create_thread' error handling check for errors similar to
what my patch proposes:
if (__glibc_unlikely (retval != 0))
{
[...]
}
else
{
if (stopped_start)
{
lll_unlock (pd->lock, LLL_PRIVATE);
/* Thread starts the its own setup code. */
lll_lock (pd->lock, LLL_PRIVATE);
/* Handle any thread setup failure similar to early setup
failure. */
if (pd->setup_failed)
{
pid_t tid;
while ((tid = atomic_load_acquire (&pd->tid)) != 0)
__futex_abstimed_wait_cancelable64 ((unsigned int *) &pd->tid,
tid, 0, NULL, LLL_SHARED);
}
else
lll_unlock (pd->lock)
}
It would require a slight more synchronization so the created thread
signal that its own setup has failed, but I don't see a better
strategy here.
>
> The actual code changes are fine, I think.
Below it is the updated patch based on your suggestion, including the concurrency
state descriptor regarding THREAD_RAN. Are this version ok to commit?
---
[PATCH] nptl: Deallocate the thread stack on setup failure (BZ#19511)
To setup either the thread scheduling parameters or affinity,
pthread_create enforce synchronization on created thread to wait until
its parent either release PD ownership 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).
This patch changes on how the thread resource is deallocate in case of
failure to be synchronous, where the creating thread will signal the
created thread to exit early so it could be joined. The creating thread
will be reponsible for the resource cleanup before returning to the
caller.
To signal the creating thread that a failure has occured, an unused
'struct pthread' member, parent_cancelhandling_unsed, now indicates
whether the setup has failed so creating thread can proper exit.
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). Another advantage is since the
early exit is move to first step at thread creation, the signal
mask is not already set and thus it can not act on change ID setxid
handler.
Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
nptl/allocatestack.c | 1 +
nptl/descr.h | 5 +-
nptl/pthread_create.c | 162 ++++++++++++++++++++----------------------
3 files changed, 80 insertions(+), 88 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 52d6738f7f..9cca28419d 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -158,33 +158,27 @@ late_init (void)
or joinable (default PTHREAD_CREATE_JOINABLE) state and
STOPPED_START is true, then the creating thread has ownership of
PD until the PD->lock is released by pthread_create. If any
- errors occur we are in states (c), (d), or (e) below.
+ errors occur we are in states (c) or (d) below.
(b) If the created thread is in a detached state
(PTHREAD_CREATED_DETACHED), and STOPPED_START is false, then the
creating thread has ownership of PD until it invokes the OS
kernel's thread creation routine. If this routine returns
without error, then the created thread owns PD; otherwise, see
- (c) and (e) below.
-
- (c) If the detached thread setup failed and THREAD_RAN is true, then
- the creating thread releases ownership to the new thread by
- sending a cancellation signal. All threads set THREAD_RAN to
- true as quickly as possible after returning from the OS kernel's
- thread creation routine.
-
- (d) If the joinable thread setup failed and THREAD_RAN is true, then
- then the creating thread retains ownership of PD and must cleanup
- 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.
- No waiting for the new thread is required because it never
- started.
+ (c) or (d) below.
+
+ (c) If either a joinable or detached thread setup failed and THREAD_RAN
+ is true, then the creating thread releases ownership to the new thread,
+ the created thread sees the failed setup through PD SETUP_FAILED
+ member, releases the PD ownership, and exits. The creating thread will
+ be responsible for cleanup the allocated resources. The THREAD_RAN is
+ local to creating thread and indicate whether thread creation or setup
+ has failed.
+
+ (d) If the thread creation failed and THREAD_RAN is false (meaning
+ ARCH_CLONE has failed), then the creating thread retains ownership
+ of PD and must cleanup he allocated resource. No waiting for the new
+ thread is required because it never started.
The nptl_db interface:
@@ -239,8 +233,8 @@ late_init (void)
The return value is zero for success or an errno code for failure.
If the return value is ENOMEM, that will be translated to EAGAIN,
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). */
+ be set to true iff the thread actually started up but before calling
+ the user code (*PD->start_routine). */
static int _Noreturn start_thread (void *arg);
@@ -308,35 +302,23 @@ static int create_thread (struct pthread *pd, const struct pthread_attr *attr,
== -1))
return errno;
- /* It's started now, so if we fail below, we'll have to cancel it
- and let it clean itself up. */
+ /* It's started now, so if we fail below, we'll have to 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);
-
+ 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);
- }
+ return INTERNAL_SYSCALL_ERRNO (res);
}
/* Set the scheduling parameters. */
@@ -344,11 +326,10 @@ 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;
+ return INTERNAL_SYSCALL_ERRNO (res);
}
}
@@ -361,6 +342,31 @@ start_thread (void *arg)
{
struct pthread *pd = arg;
+ /* We are either in (a) or (b), and in either case we either own PD already
+ (2) or are about to own PD (1), and so our only restriction would be that
+ we can't free PD until we know we have ownership (see CONCURRENCY NOTES
+ above). */
+ if (pd->stopped_start)
+ {
+ bool setup_failed = false;
+
+ /* Get the lock the parent locked to force synchronization. */
+ lll_lock (pd->lock, LLL_PRIVATE);
+
+ /* We have ownership of PD now, for detached threads with setup failure
+ we set it as joinable so the creating thread could synchronous join
+ and free any resource prior return to the pthread_create caller. */
+ setup_failed = pd->setup_failed == 1;
+ if (setup_failed)
+ pd->joinid = NULL;
+
+ /* And give it up right away. */
+ lll_unlock (pd->lock, LLL_PRIVATE);
+
+ if (setup_failed)
+ goto out;
+ }
+
/* Initialize resolver state pointer. */
__resp = &pd->res;
@@ -418,25 +424,6 @@ start_thread (void *arg)
/* Store the new cleanup handler info. */
THREAD_SETMEM (pd, cleanup_jmp_buf, &unwind_buf);
- /* We are either in (a) or (b), and in either case we either own
- PD already (2) or are about to own PD (1), and so our only
- restriction would be that we can't free PD until we know we
- 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);
-
- /* We have ownership of PD now. */
-
- /* And give it up right away. */
- lll_unlock (pd->lock, LLL_PRIVATE);
-
- LIBC_CANCEL_RESET (oldtype);
- }
-
LIBC_PROBE (pthread_start, 3, (pthread_t) pd, pd->start_routine, pd->arg);
/* Run the code the user provided. */
@@ -566,6 +553,7 @@ start_thread (void *arg)
/* Free the TCB. */
__nptl_free_tcb (pd);
+out:
/* We cannot call '_exit' here. '_exit' will terminate the process.
The 'exit' implementation in the kernel will signal when the
@@ -759,7 +747,6 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
signal mask of this thread, so save it in the startup
information. */
pd->sigmask = original_sigmask;
-
/* Reset the cancellation signal mask in case this thread is
running cancellation. */
__sigdelset (&pd->sigmask, SIGCANCEL);
@@ -814,30 +801,33 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
if (__glibc_unlikely (retval != 0))
{
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. */
- /* See bug 19511 which explains why doing nothing here is a
- resource leak for a joinable thread. */
- assert (stopped_start);
- else
- {
- /* State (e) and we have ownership of PD (see CONCURRENCY
- NOTES above). */
+ /* State (c) and we 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);
+ /* Signal the created thread to release PD ownership and early
+ exit so it could be joined. */
+ pd->setup_failed = 1;
+ lll_unlock (pd->lock, LLL_PRIVATE);
+
+ /* Similar to pthread_join, but since thread creation has failed at
+ startup there is no need to handle all the steps. */
+ pid_t tid;
+ while ((tid = atomic_load_acquire (&pd->tid)) != 0)
+ __futex_abstimed_wait_cancelable64 ((unsigned int *) &pd->tid,
+ tid, 0, NULL, LLL_SHARED);
+ }
- /* Oops, we lied for a second. */
- atomic_decrement (&__nptl_nthreads);
+ /* State (c) or (d) and we have ownership of PD (see CONCURRENCY
+ NOTES above). */
- /* Perhaps a thread wants to change the IDs and is waiting for this
- stillborn thread. */
- if (__glibc_unlikely (atomic_exchange_acq (&pd->setxid_futex, 0)
- == -2))
- futex_wake (&pd->setxid_futex, 1, FUTEX_PRIVATE);
+ /* Oops, we lied for a second. */
+ atomic_decrement (&__nptl_nthreads);
- /* Free the resources. */
- __nptl_deallocate_stack (pd);
- }
+ /* Free the resources. */
+ __nptl_deallocate_stack (pd);
/* We have to translate error codes. */
if (retval == ENOMEM)
--
2.30.2
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v3] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-06-08 17:01 ` Adhemerval Zanella
@ 2021-06-09 13:49 ` Florian Weimer
2021-06-09 17:07 ` Adhemerval Zanella
0 siblings, 1 reply; 49+ messages in thread
From: Florian Weimer @ 2021-06-09 13:49 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella:
> THREAD_RAN only signals if whether clone() or thread setup has failed,
> there is no synchronization involved and it is already local to
> creating thread. I changed the concurrency comment to the following,
> to indicate that (c) and (d) are essentially the same as you noted:
>
> (c) If either a joinable or detached thread setup failed and THREAD_RAN
> is true, then the creating thread releases ownership to the new thread,
> the created thread sees the failed setup through PD SETUP_FAILED
PD->setup_failed (I think this is the GNU convention).
> member, releases the PD ownership, and exits. The creating thread will
> be responsible for cleanup the allocated resources. The THREAD_RAN is
> local to creating thread and indicate whether thread creation or setup
> has failed.
>
> (d) If the thread creation failed and THREAD_RAN is false (meaning
> ARCH_CLONE has failed), then the creating thread retains ownership
> of PD and must cleanup he allocated resource. No waiting for the new
> thread is required because it never started.
Yes, that's much better.
> I think for such case you will need to set PD->stopped_start to true
> before thread creation (similar to what affinity and schedule set do),
> add the required initialization after the setup failure check, and
> handle potential errors.
>
> Something like (I removed the comments to simplify):
Yeah, that is what I expect as well.
> Below it is the updated patch based on your suggestion, including the
> concurrency state descriptor regarding THREAD_RAN. Are this version
> ok to commit?
This version looks okay to me, with the PD->setup_failed change above.
Thanks,
Florian
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v3] nptl: Deallocate the thread stack on setup failure (BZ #19511)
2021-06-09 13:49 ` Florian Weimer
@ 2021-06-09 17:07 ` Adhemerval Zanella
0 siblings, 0 replies; 49+ messages in thread
From: Adhemerval Zanella @ 2021-06-09 17:07 UTC (permalink / raw)
To: Florian Weimer; +Cc: Adhemerval Zanella via Libc-alpha
On 09/06/2021 10:49, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> THREAD_RAN only signals if whether clone() or thread setup has failed,
>> there is no synchronization involved and it is already local to
>> creating thread. I changed the concurrency comment to the following,
>> to indicate that (c) and (d) are essentially the same as you noted:
>>
>> (c) If either a joinable or detached thread setup failed and THREAD_RAN
>> is true, then the creating thread releases ownership to the new thread,
>> the created thread sees the failed setup through PD SETUP_FAILED
>
> PD->setup_failed (I think this is the GNU convention).
Ack (I was in doubt which would the rule here).
>
>> member, releases the PD ownership, and exits. The creating thread will
>> be responsible for cleanup the allocated resources. The THREAD_RAN is
>> local to creating thread and indicate whether thread creation or setup
>> has failed.
>>
>> (d) If the thread creation failed and THREAD_RAN is false (meaning
>> ARCH_CLONE has failed), then the creating thread retains ownership
>> of PD and must cleanup he allocated resource. No waiting for the new
>> thread is required because it never started.
>
> Yes, that's much better.
>
>> I think for such case you will need to set PD->stopped_start to true
>> before thread creation (similar to what affinity and schedule set do),
>> add the required initialization after the setup failure check, and
>> handle potential errors.
>>
>> Something like (I removed the comments to simplify):
>
> Yeah, that is what I expect as well.
>
>> Below it is the updated patch based on your suggestion, including the
>> concurrency state descriptor regarding THREAD_RAN. Are this version
>> ok to commit?
>
> This version looks okay to me, with the PD->setup_failed change above.
Thanks, I will run a check on some architecture and commit the patchset.
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v2 3/9] nptl: Install cancellation handler on pthread_cancel
2021-05-27 17:28 [PATCH v2 0/9] nptl: pthread cancellation refactor Adhemerval Zanella
2021-05-27 17:28 ` [PATCH v2 1/9] nptl: Remove exit-thread.h Adhemerval Zanella
2021-05-27 17:28 ` [PATCH v2 2/9] nptl: Deallocate the thread stack on setup failure (BZ #19511) Adhemerval Zanella
@ 2021-05-27 17:28 ` Adhemerval Zanella
2021-05-31 18:18 ` Adhemerval Zanella
2021-06-01 8:39 ` Florian Weimer
2021-05-27 17:28 ` [PATCH v2 4/9] nptl: Remove CANCELING_BITMASK Adhemerval Zanella
` (5 subsequent siblings)
8 siblings, 2 replies; 49+ messages in thread
From: Adhemerval Zanella @ 2021-05-27 17:28 UTC (permalink / raw)
To: libc-alpha
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..deb404600c 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 018af30c85..15ce5ad4a1 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -67,21 +67,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. Use
SA_ONSTACK because the signal may be sent to threads that are
running with custom stacks. (This is less likely for
--
2.30.2
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v2 3/9] nptl: Install cancellation handler on pthread_cancel
2021-05-27 17:28 ` [PATCH v2 3/9] nptl: Install cancellation handler on pthread_cancel Adhemerval Zanella
@ 2021-05-31 18:18 ` Adhemerval Zanella
2021-06-01 8:38 ` Florian Weimer
2021-06-01 8:39 ` Florian Weimer
1 sibling, 1 reply; 49+ messages in thread
From: Adhemerval Zanella @ 2021-05-31 18:18 UTC (permalink / raw)
To: libc-alpha, Carlos O'Donell
On 27/05/2021 14:28, Adhemerval Zanella wrote:
> 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.
I also think it fixes BZ#14744, the cancellation handler is now used
solely when pthread_cancel is actually issue. The sigcancel_handler
already prevents signals from other processes and cancellation from
other processes are an extension that I think it does not really
make sense (as Carlos has pointed ack on the bug report).
> ---
> 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..deb404600c 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 018af30c85..15ce5ad4a1 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -67,21 +67,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. Use
> SA_ONSTACK because the signal may be sent to threads that are
> running with custom stacks. (This is less likely for
>
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v2 3/9] nptl: Install cancellation handler on pthread_cancel
2021-05-31 18:18 ` Adhemerval Zanella
@ 2021-06-01 8:38 ` Florian Weimer
2021-06-01 13:10 ` Adhemerval Zanella
0 siblings, 1 reply; 49+ messages in thread
From: Florian Weimer @ 2021-06-01 8:38 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella via Libc-alpha:
> On 27/05/2021 14:28, Adhemerval Zanella wrote:
>> 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.
>
> I also think it fixes BZ#14744, the cancellation handler is now used
> solely when pthread_cancel is actually issue. The sigcancel_handler
> already prevents signals from other processes and cancellation from
> other processes are an extension that I think it does not really
> make sense (as Carlos has pointed ack on the bug report).
I'm not sure if bug 14744 actually exists, given the checks we have in
the cancellation signal handler. Particularly the si_pid check should
block acting on signals sent from another process. And locally, we
filter out the NPTL signals in the system call wrapppers (and we
consider direct system calls undefined in this context).
Thanks,
Florian
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v2 3/9] nptl: Install cancellation handler on pthread_cancel
2021-06-01 8:38 ` Florian Weimer
@ 2021-06-01 13:10 ` Adhemerval Zanella
0 siblings, 0 replies; 49+ messages in thread
From: Adhemerval Zanella @ 2021-06-01 13:10 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella via Libc-alpha
On 01/06/2021 05:38, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> On 27/05/2021 14:28, Adhemerval Zanella wrote:
>>> 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.
>>
>> I also think it fixes BZ#14744, the cancellation handler is now used
>> solely when pthread_cancel is actually issue. The sigcancel_handler
>> already prevents signals from other processes and cancellation from
>> other processes are an extension that I think it does not really
>> make sense (as Carlos has pointed ack on the bug report).
>
> I'm not sure if bug 14744 actually exists, given the checks we have in
> the cancellation signal handler. Particularly the si_pid check should
> block acting on signals sent from another process. And locally, we
> filter out the NPTL signals in the system call wrapppers (and we
> consider direct system calls undefined in this context).
Indeed there is not the issue of external processes cancelling random
threads by throwing SIGCANCEL to random tids, I was referring to the
robustness suggestion to just install the signal handler when
pthread_cancel is actually used as per comment #1 (although it would
only help by invoking the default signal handler).
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v2 3/9] nptl: Install cancellation handler on pthread_cancel
2021-05-27 17:28 ` [PATCH v2 3/9] nptl: Install cancellation handler on pthread_cancel Adhemerval Zanella
2021-05-31 18:18 ` Adhemerval Zanella
@ 2021-06-01 8:39 ` Florian Weimer
1 sibling, 0 replies; 49+ messages in thread
From: Florian Weimer @ 2021-06-01 8:39 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella via Libc-alpha:
> 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.
This patch looks okay to me.
Thanks,
Florian
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v2 4/9] nptl: Remove CANCELING_BITMASK
2021-05-27 17:28 [PATCH v2 0/9] nptl: pthread cancellation refactor Adhemerval Zanella
` (2 preceding siblings ...)
2021-05-27 17:28 ` [PATCH v2 3/9] nptl: Install cancellation handler on pthread_cancel Adhemerval Zanella
@ 2021-05-27 17:28 ` Adhemerval Zanella
2021-06-01 9:03 ` Florian Weimer
2021-05-27 17:28 ` [PATCH v2 5/9] nptl: Move cancel state out of cancelhandling Adhemerval Zanella
` (4 subsequent siblings)
8 siblings, 1 reply; 49+ messages in thread
From: Adhemerval Zanella @ 2021-05-27 17:28 UTC (permalink / raw)
To: 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).
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 deb404600c..8dfbcff8c3 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
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v2 4/9] nptl: Remove CANCELING_BITMASK
2021-05-27 17:28 ` [PATCH v2 4/9] nptl: Remove CANCELING_BITMASK Adhemerval Zanella
@ 2021-06-01 9:03 ` Florian Weimer
2021-06-01 13:50 ` Adhemerval Zanella
0 siblings, 1 reply; 49+ messages in thread
From: Florian Weimer @ 2021-06-01 9:03 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* 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
typo: “this is incurs”
> issues described on BZ#12683: the cancellation is acting even *after*
“is acted upon 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
typo: check → checks
> cancellation is already pending and if not always send a signal
typo: send → sends
Maybe add a comma before “always”.
> 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).
typo: alows
> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
> index deb404600c..8dfbcff8c3 100644
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c
> @@ -104,72 +87,33 @@ __pthread_cancel (pthread_t th)
> " must be installed for pthread_cancel to work\n");
> }
> +
> + int oldch = atomic_fetch_or_acquire (&pd->cancelhandling, CANCELED_BITMASK);
> + if ((oldch & CANCELED_BITMASK) != 0)
> + return 0;
> +
> + if (pd == THREAD_SELF)
> {
> + /* 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;
> }
The last part is for asynchronous self-cancel, right? Don't we have to
check that cancellation is enabled, too?
Thanks,
Florian
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v2 4/9] nptl: Remove CANCELING_BITMASK
2021-06-01 9:03 ` Florian Weimer
@ 2021-06-01 13:50 ` Adhemerval Zanella
2021-06-01 16:36 ` Adhemerval Zanella
0 siblings, 1 reply; 49+ messages in thread
From: Adhemerval Zanella @ 2021-06-01 13:50 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella via Libc-alpha
On 01/06/2021 06:03, 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
>
> typo: “this is incurs”
Ack.
>
>> issues described on BZ#12683: the cancellation is acting even *after*
>
> “is acted upon even *after*”?
Ack.
>
>> 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
>
> typo: check → checks
Ack.
>
>> cancellation is already pending and if not always send a signal
>
> typo: send → sends
Ack.
>
> Maybe add a comma before “always”.
Ack.
>
>> 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).
>
> typo: alows
Ack.
>
>> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
>> index deb404600c..8dfbcff8c3 100644
>> --- a/nptl/pthread_cancel.c
>> +++ b/nptl/pthread_cancel.c
>
>> @@ -104,72 +87,33 @@ __pthread_cancel (pthread_t th)
>> " must be installed for pthread_cancel to work\n");
>> }
>> +
>> + int oldch = atomic_fetch_or_acquire (&pd->cancelhandling, CANCELED_BITMASK);
>> + if ((oldch & CANCELED_BITMASK) != 0)
>> + return 0;
>> +
>> + if (pd == THREAD_SELF)
>> {
>> + /* 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;
>> }
>
> The last part is for asynchronous self-cancel, right? Don't we have to
> check that cancellation is enabled, too?
Indeed it should be:
if ((oldch & CANCELSTATE_BIT)
&& (oldch & CANCELTYPE_BITMASK) != 0)
I ended up fixing it later in the serie with the cancel type and state
refactor. I have fixed it locally.
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v2 4/9] nptl: Remove CANCELING_BITMASK
2021-06-01 13:50 ` Adhemerval Zanella
@ 2021-06-01 16:36 ` Adhemerval Zanella
0 siblings, 0 replies; 49+ messages in thread
From: Adhemerval Zanella @ 2021-06-01 16:36 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella via Libc-alpha
On 01/06/2021 10:50, Adhemerval Zanella wrote:
>>> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
>>> index deb404600c..8dfbcff8c3 100644
>>> --- a/nptl/pthread_cancel.c
>>> +++ b/nptl/pthread_cancel.c
>>
>>> @@ -104,72 +87,33 @@ __pthread_cancel (pthread_t th)
>>> " must be installed for pthread_cancel to work\n");
>>> }
>>> +
>>> + int oldch = atomic_fetch_or_acquire (&pd->cancelhandling, CANCELED_BITMASK);
>>> + if ((oldch & CANCELED_BITMASK) != 0)
>>> + return 0;
>>> +
>>> + if (pd == THREAD_SELF)
>>> {
>>> + /* 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;
>>> }
>>
>> The last part is for asynchronous self-cancel, right? Don't we have to
>> check that cancellation is enabled, too?
Ok, so I recall why I did this originally and checking if cancel is enable
is not strictly necessary since SIGCANCEL signal handler will bail early
if cancellation is not enabled. But it does seems the correct way to do
it.
>
> Indeed it should be:
>
> if ((oldch & CANCELSTATE_BIT)
This should be '(oldch & CANCELSTATE_BIT) == 0'.
> && (oldch & CANCELTYPE_BITMASK) != 0)
>
> I ended up fixing it later in the serie with the cancel type and state
> refactor. I have fixed it locally.
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v2 5/9] nptl: Move cancel state out of cancelhandling
2021-05-27 17:28 [PATCH v2 0/9] nptl: pthread cancellation refactor Adhemerval Zanella
` (3 preceding siblings ...)
2021-05-27 17:28 ` [PATCH v2 4/9] nptl: Remove CANCELING_BITMASK Adhemerval Zanella
@ 2021-05-27 17:28 ` Adhemerval Zanella
2021-06-01 9:58 ` Florian Weimer
2021-05-27 17:28 ` [PATCH v2 6/9] nptl: Move cancel type " Adhemerval Zanella
` (3 subsequent siblings)
8 siblings, 1 reply; 49+ messages in thread
From: Adhemerval Zanella @ 2021-05-27 17:28 UTC (permalink / raw)
To: libc-alpha
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 ++++++++++-
sysdeps/nptl/dl-tls_init_tp.c | 2 ++
14 files changed, 34 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..35f5330e7f 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))
@@ -404,6 +398,10 @@ struct pthread
/* Indicates whether is a C11 thread created by thrd_creat. */
bool c11;
+ /* Thread cancel state (PTHREAD_CANCEL_ENABLE or
+ PTHREAD_CANCEL_DISABLE). */
+ unsigned char cancelstate;
+
/* Used on strsignal. */
struct tls_internal_t tls_state;
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 8dfbcff8c3..69b57c8808 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,
diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c
index 1f7790297f..378b26a2f5 100644
--- a/sysdeps/nptl/dl-tls_init_tp.c
+++ b/sysdeps/nptl/dl-tls_init_tp.c
@@ -94,4 +94,6 @@ __tls_init_tp (void)
It will be bigger than it actually is, but for unwind.c/pt-longjmp.c
purposes this is good enough. */
THREAD_SETMEM (pd, stackblock_size, (size_t) __libc_stack_end);
+
+ THREAD_SETMEM (pd, cancelstate, PTHREAD_CANCEL_ENABLE);
}
--
2.30.2
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v2 5/9] nptl: Move cancel state out of cancelhandling
2021-05-27 17:28 ` [PATCH v2 5/9] nptl: Move cancel state out of cancelhandling Adhemerval Zanella
@ 2021-06-01 9:58 ` Florian Weimer
2021-06-02 13:09 ` Adhemerval Zanella
0 siblings, 1 reply; 49+ messages in thread
From: Florian Weimer @ 2021-06-01 9:58 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella via Libc-alpha:
> 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
Drop “the”?
> 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.
What do you mean by “patchset” in this context? It's in the same
commit, right?
> 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).
Is this really true? What is supposed to happen with a pending
asynchronous cancellation request if cancellation is disabled for the
target thread? Isn't it supposed to be queued if it's a self-cancel?
Thanks,
Florian
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v2 5/9] nptl: Move cancel state out of cancelhandling
2021-06-01 9:58 ` Florian Weimer
@ 2021-06-02 13:09 ` Adhemerval Zanella
0 siblings, 0 replies; 49+ messages in thread
From: Adhemerval Zanella @ 2021-06-02 13:09 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella via Libc-alpha
On 01/06/2021 06:58, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> 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
>
> Drop “the”?
Ack.
>
>> 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.
>
> What do you mean by “patchset” in this context? It's in the same
> commit, right?
I did not update this comment with recent rebase. I will just remove
this paragraph.
>
>> 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).
>
> Is this really true? What is supposed to happen with a pending
> asynchronous cancellation request if cancellation is disabled for the
> target thread? Isn't it supposed to be queued if it's a self-cancel?
POSIX does not really define pthread_setcancelstate as a cancellation
entrypoint [1]. And the pthread_cancel already queue the cancellation
by setting the CANCELED_BITMASK, the cancellation will be acted upon
on next cancellation entrypoint when cancellation is enabled.
[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v2 6/9] nptl: Move cancel type out of cancelhandling
2021-05-27 17:28 [PATCH v2 0/9] nptl: pthread cancellation refactor Adhemerval Zanella
` (4 preceding siblings ...)
2021-05-27 17:28 ` [PATCH v2 5/9] nptl: Move cancel state out of cancelhandling Adhemerval Zanella
@ 2021-05-27 17:28 ` Adhemerval Zanella
2021-06-01 12:37 ` Florian Weimer
2021-05-27 17:28 ` [PATCH v2 7/9] nptl: Implement raise in terms of pthread_kill Adhemerval Zanella
` (2 subsequent siblings)
8 siblings, 1 reply; 49+ messages in thread
From: Adhemerval Zanella @ 2021-05-27 17:28 UTC (permalink / raw)
To: libc-alpha
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 ++++-------------------------
sysdeps/nptl/dl-tls_init_tp.c | 1 +
8 files changed, 34 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 35f5330e7f..c85778d449 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;
@@ -402,6 +392,10 @@ struct pthread
PTHREAD_CANCEL_DISABLE). */
unsigned char cancelstate;
+ /* Thread cancel type (PTHREAD_CANCEL_DEFERRED or
+ PTHREAD_CANCEL_ASYNCHRONOUS). */
+ unsigned char canceltype;
+
/* Used on strsignal. */
struct tls_internal_t tls_state;
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 69b57c8808..c6086b6c3c 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;
}
diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c
index 378b26a2f5..b7b3bb1cdb 100644
--- a/sysdeps/nptl/dl-tls_init_tp.c
+++ b/sysdeps/nptl/dl-tls_init_tp.c
@@ -96,4 +96,5 @@ __tls_init_tp (void)
THREAD_SETMEM (pd, stackblock_size, (size_t) __libc_stack_end);
THREAD_SETMEM (pd, cancelstate, PTHREAD_CANCEL_ENABLE);
+ THREAD_SETMEM (pd, canceltype, PTHREAD_CANCEL_DEFERRED);
}
--
2.30.2
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v2 6/9] nptl: Move cancel type out of cancelhandling
2021-05-27 17:28 ` [PATCH v2 6/9] nptl: Move cancel type " Adhemerval Zanella
@ 2021-06-01 12:37 ` Florian Weimer
2021-06-02 13:11 ` Adhemerval Zanella
0 siblings, 1 reply; 49+ messages in thread
From: Florian Weimer @ 2021-06-01 12:37 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella via Libc-alpha:
> 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.
Patch looks okay to me. Maybe this should be merged with the other
patch that changes the cancel state handling.
Thanks,
Florian
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v2 6/9] nptl: Move cancel type out of cancelhandling
2021-06-01 12:37 ` Florian Weimer
@ 2021-06-02 13:11 ` Adhemerval Zanella
2021-06-09 17:06 ` Adhemerval Zanella
0 siblings, 1 reply; 49+ messages in thread
From: Adhemerval Zanella @ 2021-06-02 13:11 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella via Libc-alpha
On 01/06/2021 09:37, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> 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.
>
> Patch looks okay to me. Maybe this should be merged with the other
> patch that changes the cancel state handling.
I don't have a strong preference, I split just to make easier to review
and see the logical changes. I will merge when commit then.
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v2 6/9] nptl: Move cancel type out of cancelhandling
2021-06-02 13:11 ` Adhemerval Zanella
@ 2021-06-09 17:06 ` Adhemerval Zanella
0 siblings, 0 replies; 49+ messages in thread
From: Adhemerval Zanella @ 2021-06-09 17:06 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella via Libc-alpha
On 02/06/2021 10:11, Adhemerval Zanella wrote:
>
>
> On 01/06/2021 09:37, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>>
>>> 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.
>>
>> Patch looks okay to me. Maybe this should be merged with the other
>> patch that changes the cancel state handling.
>
> I don't have a strong preference, I split just to make easier to review
> and see the logical changes. I will merge when commit then.
>
In fact merging will result in a larger, which might be harder for
a reader (and there is no direct gain in doing so). I will keep the
change in two patches.
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v2 7/9] nptl: Implement raise in terms of pthread_kill
2021-05-27 17:28 [PATCH v2 0/9] nptl: pthread cancellation refactor Adhemerval Zanella
` (5 preceding siblings ...)
2021-05-27 17:28 ` [PATCH v2 6/9] nptl: Move cancel type " Adhemerval Zanella
@ 2021-05-27 17:28 ` Adhemerval Zanella
2021-06-01 12:40 ` Florian Weimer
2021-05-27 17:28 ` [PATCH v2 8/9] nptl: Use pthread_kill on pthread_cancel Adhemerval Zanella
2021-05-27 17:28 ` [PATCH v2 9/9] nptl: Avoid async cancellation to wrongly update __nptl_nthreads (BZ #19366) Adhemerval Zanella
8 siblings, 1 reply; 49+ messages in thread
From: Adhemerval Zanella @ 2021-05-27 17:28 UTC (permalink / raw)
To: libc-alpha
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
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v2 7/9] nptl: Implement raise in terms of pthread_kill
2021-05-27 17:28 ` [PATCH v2 7/9] nptl: Implement raise in terms of pthread_kill Adhemerval Zanella
@ 2021-06-01 12:40 ` Florian Weimer
2021-06-02 13:14 ` Adhemerval Zanella
0 siblings, 1 reply; 49+ messages in thread
From: Florian Weimer @ 2021-06-01 12:40 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella via Libc-alpha:
> 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.
Please add the vfork information as a comment in pthread_kill.
Otherwise looks good.
Thanks,
Florian
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v2 7/9] nptl: Implement raise in terms of pthread_kill
2021-06-01 12:40 ` Florian Weimer
@ 2021-06-02 13:14 ` Adhemerval Zanella
0 siblings, 0 replies; 49+ messages in thread
From: Adhemerval Zanella @ 2021-06-02 13:14 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella via Libc-alpha
On 01/06/2021 09:40, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> 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.
>
> Please add the vfork information as a comment in pthread_kill.
> Otherwise looks good.
I added the following comment:
if (pd == THREAD_SELF)
/* It is a special case to handle raise() implementation after a vfork
call (which does not update the PD tid field). */
tid = INLINE_SYSCALL_CALL (gettid);
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v2 8/9] nptl: Use pthread_kill on pthread_cancel
2021-05-27 17:28 [PATCH v2 0/9] nptl: pthread cancellation refactor Adhemerval Zanella
` (6 preceding siblings ...)
2021-05-27 17:28 ` [PATCH v2 7/9] nptl: Implement raise in terms of pthread_kill Adhemerval Zanella
@ 2021-05-27 17:28 ` Adhemerval Zanella
2021-06-01 12:41 ` Florian Weimer
2021-05-27 17:28 ` [PATCH v2 9/9] nptl: Avoid async cancellation to wrongly update __nptl_nthreads (BZ #19366) Adhemerval Zanella
8 siblings, 1 reply; 49+ messages in thread
From: Adhemerval Zanella @ 2021-05-27 17:28 UTC (permalink / raw)
To: libc-alpha
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 c6086b6c3c..5fae763861 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
^ permalink raw reply [flat|nested] 49+ messages in thread* [PATCH v2 9/9] nptl: Avoid async cancellation to wrongly update __nptl_nthreads (BZ #19366)
2021-05-27 17:28 [PATCH v2 0/9] nptl: pthread cancellation refactor Adhemerval Zanella
` (7 preceding siblings ...)
2021-05-27 17:28 ` [PATCH v2 8/9] nptl: Use pthread_kill on pthread_cancel Adhemerval Zanella
@ 2021-05-27 17:28 ` Adhemerval Zanella
2021-06-01 14:29 ` Florian Weimer
8 siblings, 1 reply; 49+ messages in thread
From: Adhemerval Zanella @ 2021-05-27 17:28 UTC (permalink / raw)
To: libc-alpha
The testcase provided on BZ#19366 may update __nptl_nthreads in a wrong
order, triggering an early process exit because the thread decrement
the value twice.
The issue is once the thread exits without acting on cancellation,
it decreaments '__nptl_nthreads' and then atomically set
'cancelhandling' with EXITING_BIT (thus preventing further cancellation
handler to act). The issue happens if a SIGCANCEL is received between
checking '__ntpl_nthreads' and setting EXITING_BIT. To avoid it, the
'__nptl_nthreads' decrement is moved after EXITING_BIT.
It does not fully follow the POSIX XSH 2.9.5 Thread Cancellation under
the heading Thread Cancellation Cleanup Handlers that states that
when a cancellation request is acted upon, or when a thread calls
pthread_exit(), the thread first disables cancellation by setting its
cancelability state to PTHREAD_CANCEL_DISABLE and its cancelability type
to PTHREAD_CANCEL_DEFERRED. The issue is '__pthread_enable_asynccancel'
explicit enabled assynchrnous cancellation, so an interrupted syscall
within the cancellation cleanup handlers might see an invalid cancelling
type (a possible fix might be possible with my proposed solution to
BZ#12683).
Trying to come up with a test is quite hard since it requires to
mimic the timing issue described below, however I see that the
buz report reproducer does not early exit anymore.
Checked on x86_64-linux-gnu.
---
nptl/pthread_create.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 15ce5ad4a1..79af3412cf 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -442,13 +442,6 @@ start_thread (void *arg)
/* Clean up any state libc stored in thread-local variables. */
__libc_thread_freeres ();
- /* If this is the last thread we terminate the process now. We
- do not notify the debugger, it might just irritate it if there
- is no thread left. */
- if (__glibc_unlikely (atomic_decrement_and_test (&__nptl_nthreads)))
- /* This was the last thread. */
- exit (0);
-
/* Report the death of the thread if this is wanted. */
if (__glibc_unlikely (pd->report_events))
{
@@ -483,6 +476,10 @@ start_thread (void *arg)
the breakpoint reports TD_THR_RUN state rather than TD_THR_ZOMBIE. */
atomic_bit_set (&pd->cancelhandling, EXITING_BIT);
+ if (__glibc_unlikely (atomic_decrement_and_test (&__nptl_nthreads)))
+ /* This was the last thread. */
+ exit (0);
+
#ifndef __ASSUME_SET_ROBUST_LIST
/* If this thread has any robust mutexes locked, handle them now. */
# if __PTHREAD_MUTEX_HAVE_PREV
--
2.30.2
^ permalink raw reply [flat|nested] 49+ messages in thread