* [PATCH 1/3] nptl: Remove x86_64 cancellation assembly implementations
@ 2020-04-01 14:24 Adhemerval Zanella
2020-04-01 14:24 ` [PATCH 2/3] nptl: Move cancel state out of cancelhandling Adhemerval Zanella
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Adhemerval Zanella @ 2020-04-01 14:24 UTC (permalink / raw)
To: libc-alpha
All cancellable syscalls are done by C implementations, so there is no
no need to use a specialized implementation to optimize register usage.
Checked on x86_64-linux-gnu.
---
sysdeps/unix/sysv/linux/x86_64/cancellation.S | 104 ------------------
.../sysv/linux/x86_64/libc-cancellation.S | 21 ----
.../sysv/linux/x86_64/librt-cancellation.S | 21 ----
sysdeps/x86_64/nptl/tcb-offsets.sym | 7 --
4 files changed, 153 deletions(-)
delete mode 100644 sysdeps/unix/sysv/linux/x86_64/cancellation.S
delete mode 100644 sysdeps/unix/sysv/linux/x86_64/libc-cancellation.S
delete mode 100644 sysdeps/unix/sysv/linux/x86_64/librt-cancellation.S
diff --git a/sysdeps/unix/sysv/linux/x86_64/cancellation.S b/sysdeps/unix/sysv/linux/x86_64/cancellation.S
deleted file mode 100644
index 256529c9f6..0000000000
--- a/sysdeps/unix/sysv/linux/x86_64/cancellation.S
+++ /dev/null
@@ -1,104 +0,0 @@
-/* Copyright (C) 2009-2020 Free Software Foundation, Inc.
- This file is part of the GNU C Library.
- Contributed by Ulrich Drepper <drepper@redhat.com>, 2009.
-
- 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>
-#include <tcb-offsets.h>
-#include <kernel-features.h>
-#include <lowlevellock-futex.h>
-
-#define PTHREAD_UNWIND JUMPTARGET(__pthread_unwind)
-#if IS_IN (libpthread)
-# if defined SHARED && !defined NO_HIDDEN
-# undef PTHREAD_UNWIND
-# define PTHREAD_UNWIND __GI___pthread_unwind
-# endif
-#else
-# ifndef SHARED
- .weak __pthread_unwind
-# endif
-#endif
-
-
-#define LOAD_PRIVATE_FUTEX_WAIT(reg) \
- movl $(FUTEX_WAIT | FUTEX_PRIVATE_FLAG), reg
-
-/* It is crucial that the functions in this file don't modify registers
- other than %rax and %r11. The syscall wrapper code depends on this
- because it doesn't explicitly save the other registers which hold
- relevant values. */
- .text
-
- .hidden __pthread_enable_asynccancel
-ENTRY(__pthread_enable_asynccancel)
- movl %fs:CANCELHANDLING, %eax
-2: movl %eax, %r11d
- orl $TCB_CANCELTYPE_BITMASK, %r11d
- cmpl %eax, %r11d
- je 1f
-
- lock
- cmpxchgl %r11d, %fs:CANCELHANDLING
- jnz 2b
-
- andl $(TCB_CANCELSTATE_BITMASK|TCB_CANCELTYPE_BITMASK|TCB_CANCELED_BITMASK|TCB_EXITING_BITMASK|TCB_CANCEL_RESTMASK|TCB_TERMINATED_BITMASK), %r11d
- cmpl $(TCB_CANCELTYPE_BITMASK|TCB_CANCELED_BITMASK), %r11d
- je 3f
-
-1: ret
-
-3: subq $8, %rsp
- cfi_adjust_cfa_offset(8)
- LP_OP(mov) $TCB_PTHREAD_CANCELED, %fs:RESULT
- lock
- orl $TCB_EXITING_BITMASK, %fs:CANCELHANDLING
- mov %fs:CLEANUP_JMP_BUF, %RDI_LP
- call PTHREAD_UNWIND
- hlt
-END(__pthread_enable_asynccancel)
-
-
- .hidden __pthread_disable_asynccancel
-ENTRY(__pthread_disable_asynccancel)
- testl $TCB_CANCELTYPE_BITMASK, %edi
- jnz 1f
-
- movl %fs:CANCELHANDLING, %eax
-2: movl %eax, %r11d
- andl $~TCB_CANCELTYPE_BITMASK, %r11d
- lock
- cmpxchgl %r11d, %fs:CANCELHANDLING
- jnz 2b
-
- movl %r11d, %eax
-3: andl $(TCB_CANCELING_BITMASK|TCB_CANCELED_BITMASK), %eax
- cmpl $TCB_CANCELING_BITMASK, %eax
- je 4f
-1: ret
-
- /* Performance doesn't matter in this loop. We will
- delay until the thread is canceled. And we will unlikely
- enter the loop twice. */
-4: mov %fs:0, %RDI_LP
- movl $__NR_futex, %eax
- xorq %r10, %r10
- addq $CANCELHANDLING, %rdi
- LOAD_PRIVATE_FUTEX_WAIT (%esi)
- syscall
- movl %fs:CANCELHANDLING, %eax
- jmp 3b
-END(__pthread_disable_asynccancel)
diff --git a/sysdeps/unix/sysv/linux/x86_64/libc-cancellation.S b/sysdeps/unix/sysv/linux/x86_64/libc-cancellation.S
deleted file mode 100644
index 6c6b2a814d..0000000000
--- a/sysdeps/unix/sysv/linux/x86_64/libc-cancellation.S
+++ /dev/null
@@ -1,21 +0,0 @@
-/* Copyright (C) 2009-2020 Free Software Foundation, Inc.
- This file is part of the GNU C Library.
- Contributed by Ulrich Drepper <drepper@redhat.com>, 2009.
-
- 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/>. */
-
-#define __pthread_enable_asynccancel __libc_enable_asynccancel
-#define __pthread_disable_asynccancel __libc_disable_asynccancel
-#include "cancellation.S"
diff --git a/sysdeps/unix/sysv/linux/x86_64/librt-cancellation.S b/sysdeps/unix/sysv/linux/x86_64/librt-cancellation.S
deleted file mode 100644
index 71ff1ca8ea..0000000000
--- a/sysdeps/unix/sysv/linux/x86_64/librt-cancellation.S
+++ /dev/null
@@ -1,21 +0,0 @@
-/* Copyright (C) 2009-2020 Free Software Foundation, Inc.
- This file is part of the GNU C Library.
- Contributed by Ulrich Drepper <drepper@redhat.com>, 2009.
-
- 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/>. */
-
-#define __pthread_enable_asynccancel __librt_enable_asynccancel
-#define __pthread_disable_asynccancel __librt_disable_asynccancel
-#include "cancellation.S"
diff --git a/sysdeps/x86_64/nptl/tcb-offsets.sym b/sysdeps/x86_64/nptl/tcb-offsets.sym
index ae8034743b..037759eb4f 100644
--- a/sysdeps/x86_64/nptl/tcb-offsets.sym
+++ b/sysdeps/x86_64/nptl/tcb-offsets.sym
@@ -16,11 +16,4 @@ FEATURE_1_OFFSET offsetof (tcbhead_t, feature_1)
SSP_BASE_OFFSET offsetof (tcbhead_t, ssp_base)
-- Not strictly offsets, but these values are also used in the TCB.
-TCB_CANCELSTATE_BITMASK CANCELSTATE_BITMASK
-TCB_CANCELTYPE_BITMASK CANCELTYPE_BITMASK
-TCB_CANCELING_BITMASK CANCELING_BITMASK
TCB_CANCELED_BITMASK CANCELED_BITMASK
-TCB_EXITING_BITMASK EXITING_BITMASK
-TCB_CANCEL_RESTMASK CANCEL_RESTMASK
-TCB_TERMINATED_BITMASK TERMINATED_BITMASK
-TCB_PTHREAD_CANCELED PTHREAD_CANCELED
--
2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3] nptl: Move cancel state out of cancelhandling
2020-04-01 14:24 [PATCH 1/3] nptl: Remove x86_64 cancellation assembly implementations Adhemerval Zanella
@ 2020-04-01 14:24 ` Adhemerval Zanella
2020-04-16 14:18 ` Adhemerval Zanella
` (2 more replies)
2020-04-01 14:24 ` [PATCH 3/3] nptl: Move cancel type " Adhemerval Zanella
` (2 subsequent siblings)
3 siblings, 3 replies; 15+ messages in thread
From: Adhemerval Zanella @ 2020-04-01 14:24 UTC (permalink / raw)
To: libc-alpha
The thread cancellation state is not accessed concurrently internally
neither the pthread interface allows changing the state of a different
thread than its own.
The code is also simplified: the CANCELLATION_P is replaced with a
internal pthread_testcancel call and the CANCELSTATE_BIT{MASK} is
removed.
Checked on x86_64-linux-gnu.
---
nptl/allocatestack.c | 1 +
nptl/cancellation.c | 3 ++-
nptl/cleanup_defer.c | 2 +-
nptl/cleanup_defer_compat.c | 2 +-
nptl/descr.h | 13 +++++--------
nptl/pthreadP.h | 12 ------------
nptl/pthread_cancel.c | 3 ++-
nptl/pthread_join_common.c | 5 ++++-
nptl/pthread_setcancelstate.c | 36 +++--------------------------------
nptl/pthread_setcanceltype.c | 3 ++-
nptl/pthread_testcancel.c | 11 ++++++++++-
11 files changed, 31 insertions(+), 60 deletions(-)
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index c94980c21c..d9174274c2 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -232,6 +232,7 @@ get_cached_stack (size_t *sizep, void **memp)
/* Cancellation handling is back to the default. */
result->cancelhandling = 0;
+ result->cancelstate = PTHREAD_CANCEL_ENABLE;
result->cleanup = NULL;
/* No pending event. */
diff --git a/nptl/cancellation.c b/nptl/cancellation.c
index 826071321e..7e8cbe9fe1 100644
--- a/nptl/cancellation.c
+++ b/nptl/cancellation.c
@@ -45,7 +45,8 @@ __pthread_enable_asynccancel (void)
oldval);
if (__glibc_likely (curval == oldval))
{
- if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
+ if (self->cancelstate == PTHREAD_CANCEL_ENABLE
+ && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval))
{
THREAD_SETMEM (self, result, PTHREAD_CANCELED);
__do_cancel ();
diff --git a/nptl/cleanup_defer.c b/nptl/cleanup_defer.c
index 8ad9a90c50..33d4ea6eef 100644
--- a/nptl/cleanup_defer.c
+++ b/nptl/cleanup_defer.c
@@ -86,6 +86,6 @@ __pthread_unregister_cancel_restore (__pthread_unwind_buf_t *buf)
cancelhandling = curval;
}
- CANCELLATION_P (self);
+ __pthread_testcancel ();
}
}
diff --git a/nptl/cleanup_defer_compat.c b/nptl/cleanup_defer_compat.c
index 33e47888f2..a1ad291fcc 100644
--- a/nptl/cleanup_defer_compat.c
+++ b/nptl/cleanup_defer_compat.c
@@ -83,7 +83,7 @@ _pthread_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer,
cancelhandling = curval;
}
- CANCELLATION_P (self);
+ __pthread_testcancel ();
}
/* If necessary call the cleanup routine after we removed the
diff --git a/nptl/descr.h b/nptl/descr.h
index 9dcf480bdf..61665bf859 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -269,9 +269,6 @@ struct pthread
/* Flags determining processing of cancellation. */
int cancelhandling;
- /* Bit set if cancellation is disabled. */
-#define CANCELSTATE_BIT 0
-#define CANCELSTATE_BITMASK (0x01 << CANCELSTATE_BIT)
/* Bit set if asynchronous cancellation mode is selected. */
#define CANCELTYPE_BIT 1
#define CANCELTYPE_BITMASK (0x01 << CANCELTYPE_BIT)
@@ -293,11 +290,8 @@ struct pthread
/* Mask for the rest. Helps the compiler to optimize. */
#define CANCEL_RESTMASK 0xffffff80
-#define CANCEL_ENABLED_AND_CANCELED(value) \
- (((value) & (CANCELSTATE_BITMASK | CANCELED_BITMASK | EXITING_BITMASK \
- | CANCEL_RESTMASK | TERMINATED_BITMASK)) == CANCELED_BITMASK)
-#define CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS(value) \
- (((value) & (CANCELSTATE_BITMASK | CANCELTYPE_BITMASK | CANCELED_BITMASK \
+#define CANCEL_CANCELED_AND_ASYNCHRONOUS(value) \
+ (((value) & (CANCELTYPE_BITMASK | CANCELED_BITMASK \
| EXITING_BITMASK | CANCEL_RESTMASK | TERMINATED_BITMASK)) \
== (CANCELTYPE_BITMASK | CANCELED_BITMASK))
@@ -394,6 +388,9 @@ struct pthread
/* Indicates whether is a C11 thread created by thrd_creat. */
bool c11;
+ /* Thread cancel state (enable, disable). */
+ unsigned char cancelstate;
+
/* This member must be last. */
char end_padding[];
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 06fb0d74c5..d55c3b26a4 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -258,18 +258,6 @@ extern int __pthread_debug attribute_hidden;
#endif
-/* Cancellation test. */
-#define CANCELLATION_P(self) \
- do { \
- int cancelhandling = THREAD_GETMEM (self, cancelhandling); \
- if (CANCEL_ENABLED_AND_CANCELED (cancelhandling)) \
- { \
- THREAD_SETMEM (self, result, PTHREAD_CANCELED); \
- __do_cancel (); \
- } \
- } while (0)
-
-
extern void __pthread_unwind (__pthread_unwind_buf_t *__buf)
__cleanup_fct_attribute __attribute ((__noreturn__))
#if !defined SHARED && !IS_IN (libpthread)
diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index 88c1ab8f6a..5b2789d620 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -55,7 +55,8 @@ __pthread_cancel (pthread_t th)
/* If the cancellation is handled asynchronously just send a
signal. We avoid this if possible since it's more
expensive. */
- if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
+ if (pd->cancelstate == PTHREAD_CANCEL_ENABLE
+ && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval))
{
/* Mark the cancellation as "in progress". */
if (atomic_compare_and_exchange_bool_acq (&pd->cancelhandling,
diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
index a96ceafde4..03e202136f 100644
--- a/nptl/pthread_join_common.c
+++ b/nptl/pthread_join_common.c
@@ -105,7 +105,10 @@ __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
&& (pd->cancelhandling
& (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
| TERMINATED_BITMASK)) == 0))
- && !CANCEL_ENABLED_AND_CANCELED (self->cancelhandling))
+ && !(self->cancelstate == PTHREAD_CANCEL_ENABLE
+ && (pd->cancelhandling & (CANCELED_BITMASK | EXITING_BITMASK
+ | TERMINATED_BITMASK))
+ == CANCELED_BITMASK))
/* This is a deadlock situation. The threads are waiting for each
other to finish. Note that this is a "may" error. To be 100%
sure we catch this error we would have to lock the data
diff --git a/nptl/pthread_setcancelstate.c b/nptl/pthread_setcancelstate.c
index 4d7f413e19..aa1c8073a8 100644
--- a/nptl/pthread_setcancelstate.c
+++ b/nptl/pthread_setcancelstate.c
@@ -31,39 +31,9 @@ __pthread_setcancelstate (int state, int *oldstate)
self = THREAD_SELF;
- int oldval = THREAD_GETMEM (self, cancelhandling);
- while (1)
- {
- int newval = (state == PTHREAD_CANCEL_DISABLE
- ? oldval | CANCELSTATE_BITMASK
- : oldval & ~CANCELSTATE_BITMASK);
-
- /* Store the old value. */
- if (oldstate != NULL)
- *oldstate = ((oldval & CANCELSTATE_BITMASK)
- ? PTHREAD_CANCEL_DISABLE : PTHREAD_CANCEL_ENABLE);
-
- /* Avoid doing unnecessary work. The atomic operation can
- potentially be expensive if the memory has to be locked and
- remote cache lines have to be invalidated. */
- if (oldval == newval)
- break;
-
- /* Update the cancel handling word. This has to be done
- atomically since other bits could be modified as well. */
- int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
- oldval);
- if (__glibc_likely (curval == oldval))
- {
- if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
- __do_cancel ();
-
- break;
- }
-
- /* Prepare for the next round. */
- oldval = curval;
- }
+ if (oldstate != NULL)
+ *oldstate = self->cancelstate;
+ self->cancelstate = state;
return 0;
}
diff --git a/nptl/pthread_setcanceltype.c b/nptl/pthread_setcanceltype.c
index fcaae8abc7..cc0507ae04 100644
--- a/nptl/pthread_setcanceltype.c
+++ b/nptl/pthread_setcanceltype.c
@@ -53,7 +53,8 @@ __pthread_setcanceltype (int type, int *oldtype)
oldval);
if (__glibc_likely (curval == oldval))
{
- if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
+ if (self->cancelstate == PTHREAD_CANCEL_ENABLE
+ && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval))
{
THREAD_SETMEM (self, result, PTHREAD_CANCELED);
__do_cancel ();
diff --git a/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c
index 30408c2008..3ffff4ebef 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 | EXITING_BITMASK
+ | TERMINATED_BITMASK))
+ == CANCELED_BITMASK)
+ {
+ THREAD_SETMEM (self, result, PTHREAD_CANCELED);
+ __do_cancel ();
+ }
}
strong_alias (__pthread_testcancel, pthread_testcancel)
hidden_def (__pthread_testcancel)
--
2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] nptl: Move cancel type out of cancelhandling
2020-04-01 14:24 [PATCH 1/3] nptl: Remove x86_64 cancellation assembly implementations Adhemerval Zanella
2020-04-01 14:24 ` [PATCH 2/3] nptl: Move cancel state out of cancelhandling Adhemerval Zanella
@ 2020-04-01 14:24 ` Adhemerval Zanella
2020-04-16 14:18 ` Adhemerval Zanella
2020-04-02 12:23 ` [PATCH 1/3] nptl: Remove x86_64 cancellation assembly implementations Adhemerval Zanella
2020-04-02 12:27 ` H.J. Lu
3 siblings, 1 reply; 15+ messages in thread
From: Adhemerval Zanella @ 2020-04-01 14:24 UTC (permalink / raw)
To: libc-alpha
The thread cancellation type is not accessed concurrently internally
neither its pthread interface allows changing the state of a different
thread than its own.
It allows simplifing the cancellation wrappers and the
CANCEL_CANCELED_AND_ASYNCHRONOUS is removed.
Checked on x86_64-linux-gnu.
---
nptl/allocatestack.c | 1 +
nptl/cancellation.c | 59 ++++++++++--------------------------
nptl/cleanup_defer.c | 46 +++-------------------------
nptl/cleanup_defer_compat.c | 46 +++-------------------------
nptl/descr.h | 13 ++------
nptl/nptl-init.c | 2 +-
nptl/pthread_cancel.c | 5 ++-
nptl/pthread_setcanceltype.c | 42 +++----------------------
nptl/pthread_testcancel.c | 2 +-
9 files changed, 41 insertions(+), 175 deletions(-)
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index d9174274c2..481487bbb4 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -233,6 +233,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;
/* No pending event. */
diff --git a/nptl/cancellation.c b/nptl/cancellation.c
index 7e8cbe9fe1..7127f9ae91 100644
--- a/nptl/cancellation.c
+++ b/nptl/cancellation.c
@@ -32,31 +32,18 @@ attribute_hidden
__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 | EXITING_BITMASK | TERMINATED_BITMASK))
+ == CANCELED_BITMASK)
+ {
+ THREAD_SETMEM (self, result, PTHREAD_CANCELED);
+ __do_cancel ();
}
return oldval;
@@ -70,36 +57,22 @@ __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;
- }
+ THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_DEFERRED);
/* 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))
+ int ch = THREAD_GETMEM (self, cancelhandling);
+ while (__glibc_unlikely ((ch & (CANCELING_BITMASK | CANCELED_BITMASK))
+ == CANCELING_BITMASK))
{
- futex_wait_simple ((unsigned int *) &self->cancelhandling, newval,
+ futex_wait_simple ((unsigned int *) &self->cancelhandling, ch,
FUTEX_PRIVATE);
- newval = THREAD_GETMEM (self, cancelhandling);
+ ch = THREAD_GETMEM (self, cancelhandling);
}
}
diff --git a/nptl/cleanup_defer.c b/nptl/cleanup_defer.c
index 33d4ea6eef..3300b4df46 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);
@@ -67,25 +49,7 @@ __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 ();
}
diff --git a/nptl/cleanup_defer_compat.c b/nptl/cleanup_defer_compat.c
index a1ad291fcc..77655c1b0a 100644
--- a/nptl/cleanup_defer_compat.c
+++ b/nptl/cleanup_defer_compat.c
@@ -29,27 +29,9 @@ _pthread_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer,
buffer->__arg = arg;
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);
}
@@ -64,27 +46,9 @@ _pthread_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;
- }
-
- __pthread_testcancel ();
- }
+ THREAD_SETMEM (self, canceltype, buffer->__canceltype);
+ if (buffer->__canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
+ __pthread_testcancel ();
/* If necessary call the cleanup routine after we removed the
current cleanup block from the list. */
diff --git a/nptl/descr.h b/nptl/descr.h
index 61665bf859..bae9457e33 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -269,9 +269,6 @@ struct pthread
/* Flags determining processing of cancellation. */
int cancelhandling;
- /* Bit set if 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)
@@ -287,13 +284,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;
@@ -391,6 +381,9 @@ struct pthread
/* Thread cancel state (enable, disable). */
unsigned char cancelstate;
+ /* Thread cancel type (deferred, asynchronous). */
+ unsigned char canceltype;
+
/* This member must be last. */
char end_padding[];
diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
index 96b1444a01..89f58d2e5e 100644
--- a/nptl/nptl-init.c
+++ b/nptl/nptl-init.c
@@ -157,7 +157,7 @@ sigcancel_handler (int sig, siginfo_t *si, void *ctx)
THREAD_SETMEM (self, result, PTHREAD_CANCELED);
/* Make sure asynchronous cancellation is still enabled. */
- if ((newval & CANCELTYPE_BITMASK) != 0)
+ if (self->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
/* Run the registered destructors and terminate the thread. */
__do_cancel ();
diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index 5b2789d620..7518c0b8bf 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -56,7 +56,10 @@ __pthread_cancel (pthread_t th)
signal. We avoid this if possible since it's more
expensive. */
if (pd->cancelstate == PTHREAD_CANCEL_ENABLE
- && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval))
+ && pd->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS
+ && (newval & (CANCELED_BITMASK | EXITING_BITMASK
+ | TERMINATED_BITMASK))
+ == CANCELED_BITMASK)
{
/* Mark the cancellation as "in progress". */
if (atomic_compare_and_exchange_bool_acq (&pd->cancelhandling,
diff --git a/nptl/pthread_setcanceltype.c b/nptl/pthread_setcanceltype.c
index cc0507ae04..d8cb54736d 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/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c
index 3ffff4ebef..026c20f82e 100644
--- a/nptl/pthread_testcancel.c
+++ b/nptl/pthread_testcancel.c
@@ -34,5 +34,5 @@ __pthread_testcancel (void)
__do_cancel ();
}
}
-strong_alias (__pthread_testcancel, pthread_testcancel)
+weak_alias (__pthread_testcancel, pthread_testcancel)
hidden_def (__pthread_testcancel)
--
2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] nptl: Remove x86_64 cancellation assembly implementations
2020-04-01 14:24 [PATCH 1/3] nptl: Remove x86_64 cancellation assembly implementations Adhemerval Zanella
2020-04-01 14:24 ` [PATCH 2/3] nptl: Move cancel state out of cancelhandling Adhemerval Zanella
2020-04-01 14:24 ` [PATCH 3/3] nptl: Move cancel type " Adhemerval Zanella
@ 2020-04-02 12:23 ` Adhemerval Zanella
2020-04-02 12:27 ` H.J. Lu
3 siblings, 0 replies; 15+ messages in thread
From: Adhemerval Zanella @ 2020-04-02 12:23 UTC (permalink / raw)
To: libc-alpha
On 01/04/2020 11:24, Adhemerval Zanella wrote:
> All cancellable syscalls are done by C implementations, so there is no
> no need to use a specialized implementation to optimize register usage.
>
> Checked on x86_64-linux-gnu.
This also fixes BZ#25765.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] nptl: Remove x86_64 cancellation assembly implementations
2020-04-01 14:24 [PATCH 1/3] nptl: Remove x86_64 cancellation assembly implementations Adhemerval Zanella
` (2 preceding siblings ...)
2020-04-02 12:23 ` [PATCH 1/3] nptl: Remove x86_64 cancellation assembly implementations Adhemerval Zanella
@ 2020-04-02 12:27 ` H.J. Lu
3 siblings, 0 replies; 15+ messages in thread
From: H.J. Lu @ 2020-04-02 12:27 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: GNU C Library
On Wed, Apr 1, 2020 at 7:25 AM Adhemerval Zanella via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> All cancellable syscalls are done by C implementations, so there is no
> no need to use a specialized implementation to optimize register usage.
>
> Checked on x86_64-linux-gnu.
Please add [BZ #25765] to commit message subject. OK with that
change.
Thanks.
--
H.J.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] nptl: Move cancel state out of cancelhandling
2020-04-01 14:24 ` [PATCH 2/3] nptl: Move cancel state out of cancelhandling Adhemerval Zanella
@ 2020-04-16 14:18 ` Adhemerval Zanella
2020-04-22 14:11 ` Florian Weimer
2020-05-16 18:38 ` Florian Weimer
2 siblings, 0 replies; 15+ messages in thread
From: Adhemerval Zanella @ 2020-04-16 14:18 UTC (permalink / raw)
To: libc-alpha
Ping.
On 01/04/2020 11:24, Adhemerval Zanella wrote:
> The thread cancellation state is not accessed concurrently internally
> neither the pthread interface allows changing the state of a different
> thread than its own.
>
> The code is also simplified: the CANCELLATION_P is replaced with a
> internal pthread_testcancel call and the CANCELSTATE_BIT{MASK} is
> removed.
>
> Checked on x86_64-linux-gnu.
> ---
> nptl/allocatestack.c | 1 +
> nptl/cancellation.c | 3 ++-
> nptl/cleanup_defer.c | 2 +-
> nptl/cleanup_defer_compat.c | 2 +-
> nptl/descr.h | 13 +++++--------
> nptl/pthreadP.h | 12 ------------
> nptl/pthread_cancel.c | 3 ++-
> nptl/pthread_join_common.c | 5 ++++-
> nptl/pthread_setcancelstate.c | 36 +++--------------------------------
> nptl/pthread_setcanceltype.c | 3 ++-
> nptl/pthread_testcancel.c | 11 ++++++++++-
> 11 files changed, 31 insertions(+), 60 deletions(-)
>
> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index c94980c21c..d9174274c2 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -232,6 +232,7 @@ get_cached_stack (size_t *sizep, void **memp)
>
> /* Cancellation handling is back to the default. */
> result->cancelhandling = 0;
> + result->cancelstate = PTHREAD_CANCEL_ENABLE;
> result->cleanup = NULL;
>
> /* No pending event. */
> diff --git a/nptl/cancellation.c b/nptl/cancellation.c
> index 826071321e..7e8cbe9fe1 100644
> --- a/nptl/cancellation.c
> +++ b/nptl/cancellation.c
> @@ -45,7 +45,8 @@ __pthread_enable_asynccancel (void)
> oldval);
> if (__glibc_likely (curval == oldval))
> {
> - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
> + if (self->cancelstate == PTHREAD_CANCEL_ENABLE
> + && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval))
> {
> THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> __do_cancel ();
> diff --git a/nptl/cleanup_defer.c b/nptl/cleanup_defer.c
> index 8ad9a90c50..33d4ea6eef 100644
> --- a/nptl/cleanup_defer.c
> +++ b/nptl/cleanup_defer.c
> @@ -86,6 +86,6 @@ __pthread_unregister_cancel_restore (__pthread_unwind_buf_t *buf)
> cancelhandling = curval;
> }
>
> - CANCELLATION_P (self);
> + __pthread_testcancel ();
> }
> }
> diff --git a/nptl/cleanup_defer_compat.c b/nptl/cleanup_defer_compat.c
> index 33e47888f2..a1ad291fcc 100644
> --- a/nptl/cleanup_defer_compat.c
> +++ b/nptl/cleanup_defer_compat.c
> @@ -83,7 +83,7 @@ _pthread_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer,
> cancelhandling = curval;
> }
>
> - CANCELLATION_P (self);
> + __pthread_testcancel ();
> }
>
> /* If necessary call the cleanup routine after we removed the
> diff --git a/nptl/descr.h b/nptl/descr.h
> index 9dcf480bdf..61665bf859 100644
> --- a/nptl/descr.h
> +++ b/nptl/descr.h
> @@ -269,9 +269,6 @@ struct pthread
>
> /* Flags determining processing of cancellation. */
> int cancelhandling;
> - /* Bit set if cancellation is disabled. */
> -#define CANCELSTATE_BIT 0
> -#define CANCELSTATE_BITMASK (0x01 << CANCELSTATE_BIT)
> /* Bit set if asynchronous cancellation mode is selected. */
> #define CANCELTYPE_BIT 1
> #define CANCELTYPE_BITMASK (0x01 << CANCELTYPE_BIT)
> @@ -293,11 +290,8 @@ struct pthread
> /* Mask for the rest. Helps the compiler to optimize. */
> #define CANCEL_RESTMASK 0xffffff80
>
> -#define CANCEL_ENABLED_AND_CANCELED(value) \
> - (((value) & (CANCELSTATE_BITMASK | CANCELED_BITMASK | EXITING_BITMASK \
> - | CANCEL_RESTMASK | TERMINATED_BITMASK)) == CANCELED_BITMASK)
> -#define CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS(value) \
> - (((value) & (CANCELSTATE_BITMASK | CANCELTYPE_BITMASK | CANCELED_BITMASK \
> +#define CANCEL_CANCELED_AND_ASYNCHRONOUS(value) \
> + (((value) & (CANCELTYPE_BITMASK | CANCELED_BITMASK \
> | EXITING_BITMASK | CANCEL_RESTMASK | TERMINATED_BITMASK)) \
> == (CANCELTYPE_BITMASK | CANCELED_BITMASK))
>
> @@ -394,6 +388,9 @@ struct pthread
> /* Indicates whether is a C11 thread created by thrd_creat. */
> bool c11;
>
> + /* Thread cancel state (enable, disable). */
> + unsigned char cancelstate;
> +
> /* This member must be last. */
> char end_padding[];
>
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 06fb0d74c5..d55c3b26a4 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -258,18 +258,6 @@ extern int __pthread_debug attribute_hidden;
> #endif
>
>
> -/* Cancellation test. */
> -#define CANCELLATION_P(self) \
> - do { \
> - int cancelhandling = THREAD_GETMEM (self, cancelhandling); \
> - if (CANCEL_ENABLED_AND_CANCELED (cancelhandling)) \
> - { \
> - THREAD_SETMEM (self, result, PTHREAD_CANCELED); \
> - __do_cancel (); \
> - } \
> - } while (0)
> -
> -
> extern void __pthread_unwind (__pthread_unwind_buf_t *__buf)
> __cleanup_fct_attribute __attribute ((__noreturn__))
> #if !defined SHARED && !IS_IN (libpthread)
> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
> index 88c1ab8f6a..5b2789d620 100644
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c
> @@ -55,7 +55,8 @@ __pthread_cancel (pthread_t th)
> /* If the cancellation is handled asynchronously just send a
> signal. We avoid this if possible since it's more
> expensive. */
> - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
> + if (pd->cancelstate == PTHREAD_CANCEL_ENABLE
> + && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval))
> {
> /* Mark the cancellation as "in progress". */
> if (atomic_compare_and_exchange_bool_acq (&pd->cancelhandling,
> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index a96ceafde4..03e202136f 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -105,7 +105,10 @@ __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
> && (pd->cancelhandling
> & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
> | TERMINATED_BITMASK)) == 0))
> - && !CANCEL_ENABLED_AND_CANCELED (self->cancelhandling))
> + && !(self->cancelstate == PTHREAD_CANCEL_ENABLE
> + && (pd->cancelhandling & (CANCELED_BITMASK | EXITING_BITMASK
> + | TERMINATED_BITMASK))
> + == CANCELED_BITMASK))
> /* This is a deadlock situation. The threads are waiting for each
> other to finish. Note that this is a "may" error. To be 100%
> sure we catch this error we would have to lock the data
> diff --git a/nptl/pthread_setcancelstate.c b/nptl/pthread_setcancelstate.c
> index 4d7f413e19..aa1c8073a8 100644
> --- a/nptl/pthread_setcancelstate.c
> +++ b/nptl/pthread_setcancelstate.c
> @@ -31,39 +31,9 @@ __pthread_setcancelstate (int state, int *oldstate)
>
> self = THREAD_SELF;
>
> - int oldval = THREAD_GETMEM (self, cancelhandling);
> - while (1)
> - {
> - int newval = (state == PTHREAD_CANCEL_DISABLE
> - ? oldval | CANCELSTATE_BITMASK
> - : oldval & ~CANCELSTATE_BITMASK);
> -
> - /* Store the old value. */
> - if (oldstate != NULL)
> - *oldstate = ((oldval & CANCELSTATE_BITMASK)
> - ? PTHREAD_CANCEL_DISABLE : PTHREAD_CANCEL_ENABLE);
> -
> - /* Avoid doing unnecessary work. The atomic operation can
> - potentially be expensive if the memory has to be locked and
> - remote cache lines have to be invalidated. */
> - if (oldval == newval)
> - break;
> -
> - /* Update the cancel handling word. This has to be done
> - atomically since other bits could be modified as well. */
> - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
> - oldval);
> - if (__glibc_likely (curval == oldval))
> - {
> - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
> - __do_cancel ();
> -
> - break;
> - }
> -
> - /* Prepare for the next round. */
> - oldval = curval;
> - }
> + if (oldstate != NULL)
> + *oldstate = self->cancelstate;
> + self->cancelstate = state;
>
> return 0;
> }
> diff --git a/nptl/pthread_setcanceltype.c b/nptl/pthread_setcanceltype.c
> index fcaae8abc7..cc0507ae04 100644
> --- a/nptl/pthread_setcanceltype.c
> +++ b/nptl/pthread_setcanceltype.c
> @@ -53,7 +53,8 @@ __pthread_setcanceltype (int type, int *oldtype)
> oldval);
> if (__glibc_likely (curval == oldval))
> {
> - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
> + if (self->cancelstate == PTHREAD_CANCEL_ENABLE
> + && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval))
> {
> THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> __do_cancel ();
> diff --git a/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c
> index 30408c2008..3ffff4ebef 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 | EXITING_BITMASK
> + | TERMINATED_BITMASK))
> + == CANCELED_BITMASK)
> + {
> + THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> + __do_cancel ();
> + }
> }
> strong_alias (__pthread_testcancel, pthread_testcancel)
> hidden_def (__pthread_testcancel)
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] nptl: Move cancel type out of cancelhandling
2020-04-01 14:24 ` [PATCH 3/3] nptl: Move cancel type " Adhemerval Zanella
@ 2020-04-16 14:18 ` Adhemerval Zanella
0 siblings, 0 replies; 15+ messages in thread
From: Adhemerval Zanella @ 2020-04-16 14:18 UTC (permalink / raw)
To: libc-alpha
Ping.
On 01/04/2020 11:24, Adhemerval Zanella wrote:
> The thread cancellation type is not accessed concurrently internally
> neither its pthread interface allows changing the state of a different
> thread than its own.
>
> It allows simplifing the cancellation wrappers and the
> CANCEL_CANCELED_AND_ASYNCHRONOUS is removed.
>
> Checked on x86_64-linux-gnu.
> ---
> nptl/allocatestack.c | 1 +
> nptl/cancellation.c | 59 ++++++++++--------------------------
> nptl/cleanup_defer.c | 46 +++-------------------------
> nptl/cleanup_defer_compat.c | 46 +++-------------------------
> nptl/descr.h | 13 ++------
> nptl/nptl-init.c | 2 +-
> nptl/pthread_cancel.c | 5 ++-
> nptl/pthread_setcanceltype.c | 42 +++----------------------
> nptl/pthread_testcancel.c | 2 +-
> 9 files changed, 41 insertions(+), 175 deletions(-)
>
> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index d9174274c2..481487bbb4 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -233,6 +233,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;
>
> /* No pending event. */
> diff --git a/nptl/cancellation.c b/nptl/cancellation.c
> index 7e8cbe9fe1..7127f9ae91 100644
> --- a/nptl/cancellation.c
> +++ b/nptl/cancellation.c
> @@ -32,31 +32,18 @@ attribute_hidden
> __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 | EXITING_BITMASK | TERMINATED_BITMASK))
> + == CANCELED_BITMASK)
> + {
> + THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> + __do_cancel ();
> }
>
> return oldval;
> @@ -70,36 +57,22 @@ __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;
> - }
> + THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_DEFERRED);
>
> /* 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))
> + int ch = THREAD_GETMEM (self, cancelhandling);
> + while (__glibc_unlikely ((ch & (CANCELING_BITMASK | CANCELED_BITMASK))
> + == CANCELING_BITMASK))
> {
> - futex_wait_simple ((unsigned int *) &self->cancelhandling, newval,
> + futex_wait_simple ((unsigned int *) &self->cancelhandling, ch,
> FUTEX_PRIVATE);
> - newval = THREAD_GETMEM (self, cancelhandling);
> + ch = THREAD_GETMEM (self, cancelhandling);
> }
> }
> diff --git a/nptl/cleanup_defer.c b/nptl/cleanup_defer.c
> index 33d4ea6eef..3300b4df46 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);
> @@ -67,25 +49,7 @@ __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 ();
> }
> diff --git a/nptl/cleanup_defer_compat.c b/nptl/cleanup_defer_compat.c
> index a1ad291fcc..77655c1b0a 100644
> --- a/nptl/cleanup_defer_compat.c
> +++ b/nptl/cleanup_defer_compat.c
> @@ -29,27 +29,9 @@ _pthread_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer,
> buffer->__arg = arg;
> 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);
> }
> @@ -64,27 +46,9 @@ _pthread_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;
> - }
> -
> - __pthread_testcancel ();
> - }
> + THREAD_SETMEM (self, canceltype, buffer->__canceltype);
> + if (buffer->__canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
> + __pthread_testcancel ();
>
> /* If necessary call the cleanup routine after we removed the
> current cleanup block from the list. */
> diff --git a/nptl/descr.h b/nptl/descr.h
> index 61665bf859..bae9457e33 100644
> --- a/nptl/descr.h
> +++ b/nptl/descr.h
> @@ -269,9 +269,6 @@ struct pthread
>
> /* Flags determining processing of cancellation. */
> int cancelhandling;
> - /* Bit set if 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)
> @@ -287,13 +284,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;
> @@ -391,6 +381,9 @@ struct pthread
> /* Thread cancel state (enable, disable). */
> unsigned char cancelstate;
>
> + /* Thread cancel type (deferred, asynchronous). */
> + unsigned char canceltype;
> +
> /* This member must be last. */
> char end_padding[];
>
> diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
> index 96b1444a01..89f58d2e5e 100644
> --- a/nptl/nptl-init.c
> +++ b/nptl/nptl-init.c
> @@ -157,7 +157,7 @@ sigcancel_handler (int sig, siginfo_t *si, void *ctx)
> THREAD_SETMEM (self, result, PTHREAD_CANCELED);
>
> /* Make sure asynchronous cancellation is still enabled. */
> - if ((newval & CANCELTYPE_BITMASK) != 0)
> + if (self->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
> /* Run the registered destructors and terminate the thread. */
> __do_cancel ();
>
> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
> index 5b2789d620..7518c0b8bf 100644
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c
> @@ -56,7 +56,10 @@ __pthread_cancel (pthread_t th)
> signal. We avoid this if possible since it's more
> expensive. */
> if (pd->cancelstate == PTHREAD_CANCEL_ENABLE
> - && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval))
> + && pd->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS
> + && (newval & (CANCELED_BITMASK | EXITING_BITMASK
> + | TERMINATED_BITMASK))
> + == CANCELED_BITMASK)
> {
> /* Mark the cancellation as "in progress". */
> if (atomic_compare_and_exchange_bool_acq (&pd->cancelhandling,
> diff --git a/nptl/pthread_setcanceltype.c b/nptl/pthread_setcanceltype.c
> index cc0507ae04..d8cb54736d 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/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c
> index 3ffff4ebef..026c20f82e 100644
> --- a/nptl/pthread_testcancel.c
> +++ b/nptl/pthread_testcancel.c
> @@ -34,5 +34,5 @@ __pthread_testcancel (void)
> __do_cancel ();
> }
> }
> -strong_alias (__pthread_testcancel, pthread_testcancel)
> +weak_alias (__pthread_testcancel, pthread_testcancel)
> hidden_def (__pthread_testcancel)
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] nptl: Move cancel state out of cancelhandling
2020-04-01 14:24 ` [PATCH 2/3] nptl: Move cancel state out of cancelhandling Adhemerval Zanella
2020-04-16 14:18 ` Adhemerval Zanella
@ 2020-04-22 14:11 ` Florian Weimer
2020-04-23 19:30 ` Adhemerval Zanella
2020-05-16 18:38 ` Florian Weimer
2 siblings, 1 reply; 15+ messages in thread
From: Florian Weimer @ 2020-04-22 14:11 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella via Libc-alpha:
> diff --git a/nptl/pthread_setcancelstate.c b/nptl/pthread_setcancelstate.c
> index 4d7f413e19..aa1c8073a8 100644
> --- a/nptl/pthread_setcancelstate.c
> +++ b/nptl/pthread_setcancelstate.c
> @@ -31,39 +31,9 @@ __pthread_setcancelstate (int state, int *oldstate)
>
> self = THREAD_SELF;
>
> - int oldval = THREAD_GETMEM (self, cancelhandling);
> - while (1)
> - {
> - int newval = (state == PTHREAD_CANCEL_DISABLE
> - ? oldval | CANCELSTATE_BITMASK
> - : oldval & ~CANCELSTATE_BITMASK);
> -
> - /* Store the old value. */
> - if (oldstate != NULL)
> - *oldstate = ((oldval & CANCELSTATE_BITMASK)
> - ? PTHREAD_CANCEL_DISABLE : PTHREAD_CANCEL_ENABLE);
> -
> - /* Avoid doing unnecessary work. The atomic operation can
> - potentially be expensive if the memory has to be locked and
> - remote cache lines have to be invalidated. */
> - if (oldval == newval)
> - break;
> -
> - /* Update the cancel handling word. This has to be done
> - atomically since other bits could be modified as well. */
> - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
> - oldval);
> - if (__glibc_likely (curval == oldval))
> - {
> - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
> - __do_cancel ();
> -
> - break;
> - }
> -
> - /* Prepare for the next round. */
> - oldval = curval;
> - }
> + if (oldstate != NULL)
> + *oldstate = self->cancelstate;
> + self->cancelstate = state;
>
> return 0;
> }
Why isn't this needed anymore? I think this should be mentioned
explicitly in the commit message.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] nptl: Move cancel state out of cancelhandling
2020-04-22 14:11 ` Florian Weimer
@ 2020-04-23 19:30 ` Adhemerval Zanella
2020-04-24 13:21 ` Florian Weimer
0 siblings, 1 reply; 15+ messages in thread
From: Adhemerval Zanella @ 2020-04-23 19:30 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella via Libc-alpha
On 22/04/2020 11:11, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> diff --git a/nptl/pthread_setcancelstate.c b/nptl/pthread_setcancelstate.c
>> index 4d7f413e19..aa1c8073a8 100644
>> --- a/nptl/pthread_setcancelstate.c
>> +++ b/nptl/pthread_setcancelstate.c
>> @@ -31,39 +31,9 @@ __pthread_setcancelstate (int state, int *oldstate)
>>
>> self = THREAD_SELF;
>>
>> - int oldval = THREAD_GETMEM (self, cancelhandling);
>> - while (1)
>> - {
>> - int newval = (state == PTHREAD_CANCEL_DISABLE
>> - ? oldval | CANCELSTATE_BITMASK
>> - : oldval & ~CANCELSTATE_BITMASK);
>> -
>> - /* Store the old value. */
>> - if (oldstate != NULL)
>> - *oldstate = ((oldval & CANCELSTATE_BITMASK)
>> - ? PTHREAD_CANCEL_DISABLE : PTHREAD_CANCEL_ENABLE);
>> -
>> - /* Avoid doing unnecessary work. The atomic operation can
>> - potentially be expensive if the memory has to be locked and
>> - remote cache lines have to be invalidated. */
>> - if (oldval == newval)
>> - break;
>> -
>> - /* Update the cancel handling word. This has to be done
>> - atomically since other bits could be modified as well. */
>> - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
>> - oldval);
>> - if (__glibc_likely (curval == oldval))
>> - {
>> - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
>> - __do_cancel ();
>> -
>> - break;
>> - }
>> -
>> - /* Prepare for the next round. */
>> - oldval = curval;
>> - }
>> + if (oldstate != NULL)
>> + *oldstate = self->cancelstate;
>> + self->cancelstate = state;
>>
>> return 0;
>> }
>
> Why isn't this needed anymore? I think this should be mentioned
> explicitly in the commit message.
>
Do you mean the pthread_setcancelstate not being a cancellation entrypoint
anymore? If it were the question, because POSIX does not state neither
if it should be or may be a cancellation entrypoint [1].
Also, if new the cancel state is PTHREAD_CANCEL_ENABLE then either the
thread will be cancelled by the signal handler in asynchronous mode
or in the next cancellation entrypoint for deferred mode.
[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] nptl: Move cancel state out of cancelhandling
2020-04-23 19:30 ` Adhemerval Zanella
@ 2020-04-24 13:21 ` Florian Weimer
2020-04-30 11:11 ` Adhemerval Zanella
0 siblings, 1 reply; 15+ messages in thread
From: Florian Weimer @ 2020-04-24 13:21 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella:
> On 22/04/2020 11:11, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>>
>>> diff --git a/nptl/pthread_setcancelstate.c b/nptl/pthread_setcancelstate.c
>>> index 4d7f413e19..aa1c8073a8 100644
>>> --- a/nptl/pthread_setcancelstate.c
>>> +++ b/nptl/pthread_setcancelstate.c
>>> @@ -31,39 +31,9 @@ __pthread_setcancelstate (int state, int *oldstate)
>>>
>>> self = THREAD_SELF;
>>>
>>> - int oldval = THREAD_GETMEM (self, cancelhandling);
>>> - while (1)
>>> - {
>>> - int newval = (state == PTHREAD_CANCEL_DISABLE
>>> - ? oldval | CANCELSTATE_BITMASK
>>> - : oldval & ~CANCELSTATE_BITMASK);
>>> -
>>> - /* Store the old value. */
>>> - if (oldstate != NULL)
>>> - *oldstate = ((oldval & CANCELSTATE_BITMASK)
>>> - ? PTHREAD_CANCEL_DISABLE : PTHREAD_CANCEL_ENABLE);
>>> -
>>> - /* Avoid doing unnecessary work. The atomic operation can
>>> - potentially be expensive if the memory has to be locked and
>>> - remote cache lines have to be invalidated. */
>>> - if (oldval == newval)
>>> - break;
>>> -
>>> - /* Update the cancel handling word. This has to be done
>>> - atomically since other bits could be modified as well. */
>>> - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
>>> - oldval);
>>> - if (__glibc_likely (curval == oldval))
>>> - {
>>> - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
>>> - __do_cancel ();
>>> -
>>> - break;
>>> - }
>>> -
>>> - /* Prepare for the next round. */
>>> - oldval = curval;
>>> - }
>>> + if (oldstate != NULL)
>>> + *oldstate = self->cancelstate;
>>> + self->cancelstate = state;
>>>
>>> return 0;
>>> }
>>
>> Why isn't this needed anymore? I think this should be mentioned
>> explicitly in the commit message.
>>
>
> Do you mean the pthread_setcancelstate not being a cancellation entrypoint
> anymore? If it were the question, because POSIX does not state neither
> if it should be or may be a cancellation entrypoint [1].
>
> Also, if new the cancel state is PTHREAD_CANCEL_ENABLE then either the
> thread will be cancelled by the signal handler in asynchronous mode
> or in the next cancellation entrypoint for deferred mode.
>
> [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html
We can have a conversation about this change, but has it to be part of
this patchset? I would like to keep it separate if possible, so that
we can make some progress here.
(I have not reviewed this patch, I raised this question because this
particular change stood out on my first pass through the patch.)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] nptl: Move cancel state out of cancelhandling
2020-04-24 13:21 ` Florian Weimer
@ 2020-04-30 11:11 ` Adhemerval Zanella
2020-05-12 14:47 ` Adhemerval Zanella
0 siblings, 1 reply; 15+ messages in thread
From: Adhemerval Zanella @ 2020-04-30 11:11 UTC (permalink / raw)
To: Florian Weimer; +Cc: Adhemerval Zanella via Libc-alpha
On 24/04/2020 10:21, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> On 22/04/2020 11:11, Florian Weimer wrote:
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> diff --git a/nptl/pthread_setcancelstate.c b/nptl/pthread_setcancelstate.c
>>>> index 4d7f413e19..aa1c8073a8 100644
>>>> --- a/nptl/pthread_setcancelstate.c
>>>> +++ b/nptl/pthread_setcancelstate.c
>>>> @@ -31,39 +31,9 @@ __pthread_setcancelstate (int state, int *oldstate)
>>>>
>>>> self = THREAD_SELF;
>>>>
>>>> - int oldval = THREAD_GETMEM (self, cancelhandling);
>>>> - while (1)
>>>> - {
>>>> - int newval = (state == PTHREAD_CANCEL_DISABLE
>>>> - ? oldval | CANCELSTATE_BITMASK
>>>> - : oldval & ~CANCELSTATE_BITMASK);
>>>> -
>>>> - /* Store the old value. */
>>>> - if (oldstate != NULL)
>>>> - *oldstate = ((oldval & CANCELSTATE_BITMASK)
>>>> - ? PTHREAD_CANCEL_DISABLE : PTHREAD_CANCEL_ENABLE);
>>>> -
>>>> - /* Avoid doing unnecessary work. The atomic operation can
>>>> - potentially be expensive if the memory has to be locked and
>>>> - remote cache lines have to be invalidated. */
>>>> - if (oldval == newval)
>>>> - break;
>>>> -
>>>> - /* Update the cancel handling word. This has to be done
>>>> - atomically since other bits could be modified as well. */
>>>> - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
>>>> - oldval);
>>>> - if (__glibc_likely (curval == oldval))
>>>> - {
>>>> - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
>>>> - __do_cancel ();
>>>> -
>>>> - break;
>>>> - }
>>>> -
>>>> - /* Prepare for the next round. */
>>>> - oldval = curval;
>>>> - }
>>>> + if (oldstate != NULL)
>>>> + *oldstate = self->cancelstate;
>>>> + self->cancelstate = state;
>>>>
>>>> return 0;
>>>> }
>>>
>>> Why isn't this needed anymore? I think this should be mentioned
>>> explicitly in the commit message.
>>>
>>
>> Do you mean the pthread_setcancelstate not being a cancellation entrypoint
>> anymore? If it were the question, because POSIX does not state neither
>> if it should be or may be a cancellation entrypoint [1].
>>
>> Also, if new the cancel state is PTHREAD_CANCEL_ENABLE then either the
>> thread will be cancelled by the signal handler in asynchronous mode
>> or in the next cancellation entrypoint for deferred mode.
>>
>> [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html
>
> We can have a conversation about this change, but has it to be part of
> this patchset? I would like to keep it separate if possible, so that
> we can make some progress here.
>
> (I have not reviewed this patch, I raised this question because this
> particular change stood out on my first pass through the patch.)
Alright, what about this commit message then:
The thread cancellation state is not accessed concurrently internally
neither the pthread interface allows changing the state of a different
thread than its own.
By removing the cancel state out of the internal thread cancel handling
state there is no need to check if cancelled bit was set in CAS
operation.
The code is also simplified: the CANCELLATION_P is replaced with a
internal pthread_testcancel call and the CANCELSTATE_BIT{MASK} is
removed.
Checked on x86_64-linux-gnu.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] nptl: Move cancel state out of cancelhandling
2020-04-30 11:11 ` Adhemerval Zanella
@ 2020-05-12 14:47 ` Adhemerval Zanella
2020-05-12 14:57 ` Florian Weimer
0 siblings, 1 reply; 15+ messages in thread
From: Adhemerval Zanella @ 2020-05-12 14:47 UTC (permalink / raw)
To: Florian Weimer; +Cc: Adhemerval Zanella via Libc-alpha
On 30/04/2020 08:11, Adhemerval Zanella wrote:
>
>
> On 24/04/2020 10:21, Florian Weimer wrote:
>> * Adhemerval Zanella:
>>
>>> On 22/04/2020 11:11, Florian Weimer wrote:
>>>> * Adhemerval Zanella via Libc-alpha:
>>>>
>>>>> diff --git a/nptl/pthread_setcancelstate.c b/nptl/pthread_setcancelstate.c
>>>>> index 4d7f413e19..aa1c8073a8 100644
>>>>> --- a/nptl/pthread_setcancelstate.c
>>>>> +++ b/nptl/pthread_setcancelstate.c
>>>>> @@ -31,39 +31,9 @@ __pthread_setcancelstate (int state, int *oldstate)
>>>>>
>>>>> self = THREAD_SELF;
>>>>>
>>>>> - int oldval = THREAD_GETMEM (self, cancelhandling);
>>>>> - while (1)
>>>>> - {
>>>>> - int newval = (state == PTHREAD_CANCEL_DISABLE
>>>>> - ? oldval | CANCELSTATE_BITMASK
>>>>> - : oldval & ~CANCELSTATE_BITMASK);
>>>>> -
>>>>> - /* Store the old value. */
>>>>> - if (oldstate != NULL)
>>>>> - *oldstate = ((oldval & CANCELSTATE_BITMASK)
>>>>> - ? PTHREAD_CANCEL_DISABLE : PTHREAD_CANCEL_ENABLE);
>>>>> -
>>>>> - /* Avoid doing unnecessary work. The atomic operation can
>>>>> - potentially be expensive if the memory has to be locked and
>>>>> - remote cache lines have to be invalidated. */
>>>>> - if (oldval == newval)
>>>>> - break;
>>>>> -
>>>>> - /* Update the cancel handling word. This has to be done
>>>>> - atomically since other bits could be modified as well. */
>>>>> - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
>>>>> - oldval);
>>>>> - if (__glibc_likely (curval == oldval))
>>>>> - {
>>>>> - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
>>>>> - __do_cancel ();
>>>>> -
>>>>> - break;
>>>>> - }
>>>>> -
>>>>> - /* Prepare for the next round. */
>>>>> - oldval = curval;
>>>>> - }
>>>>> + if (oldstate != NULL)
>>>>> + *oldstate = self->cancelstate;
>>>>> + self->cancelstate = state;
>>>>>
>>>>> return 0;
>>>>> }
>>>>
>>>> Why isn't this needed anymore? I think this should be mentioned
>>>> explicitly in the commit message.
>>>>
>>>
>>> Do you mean the pthread_setcancelstate not being a cancellation entrypoint
>>> anymore? If it were the question, because POSIX does not state neither
>>> if it should be or may be a cancellation entrypoint [1].
>>>
>>> Also, if new the cancel state is PTHREAD_CANCEL_ENABLE then either the
>>> thread will be cancelled by the signal handler in asynchronous mode
>>> or in the next cancellation entrypoint for deferred mode.
>>>
>>> [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html
>>
>> We can have a conversation about this change, but has it to be part of
>> this patchset? I would like to keep it separate if possible, so that
>> we can make some progress here.
>>
>> (I have not reviewed this patch, I raised this question because this
>> particular change stood out on my first pass through the patch.)
>
> Alright, what about this commit message then:
>
> The thread cancellation state is not accessed concurrently internally
> neither the pthread interface allows changing the state of a different
> thread than its own.
>
> By removing the cancel state out of the internal thread cancel handling
> state there is no need to check if cancelled bit was set in CAS
> operation.
>
> The code is also simplified: the CANCELLATION_P is replaced with a
> internal pthread_testcancel call and the CANCELSTATE_BIT{MASK} is
> removed.
>
> Checked on x86_64-linux-gnu.
>
Florian, is the commit message change enough to move forward or do you
think this patch requires some work/change?
(this is a pre-requisite to BZ#12678 which I would like to progress
as well).
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] nptl: Move cancel state out of cancelhandling
2020-05-12 14:47 ` Adhemerval Zanella
@ 2020-05-12 14:57 ` Florian Weimer
0 siblings, 0 replies; 15+ messages in thread
From: Florian Weimer @ 2020-05-12 14:57 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella:
>>> (I have not reviewed this patch, I raised this question because this
>>> particular change stood out on my first pass through the patch.)
>>
>> Alright, what about this commit message then:
>>
>> The thread cancellation state is not accessed concurrently internally
>> neither the pthread interface allows changing the state of a different
>> thread than its own.
>>
>> By removing the cancel state out of the internal thread cancel handling
>> state there is no need to check if cancelled bit was set in CAS
>> operation.
>>
>> The code is also simplified: the CANCELLATION_P is replaced with a
>> internal pthread_testcancel call and the CANCELSTATE_BIT{MASK} is
>> removed.
>>
>> Checked on x86_64-linux-gnu.
>>
>
> Florian, is the commit message change enough to move forward or do you
> think this patch requires some work/change?
>
> (this is a pre-requisite to BZ#12678 which I would like to progress
> as well).
As I said, I have not yet reviewed the patch. It is on my to-do list,
though.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] nptl: Move cancel state out of cancelhandling
2020-04-01 14:24 ` [PATCH 2/3] nptl: Move cancel state out of cancelhandling Adhemerval Zanella
2020-04-16 14:18 ` Adhemerval Zanella
2020-04-22 14:11 ` Florian Weimer
@ 2020-05-16 18:38 ` Florian Weimer
2020-05-20 14:51 ` Adhemerval Zanella
2 siblings, 1 reply; 15+ messages in thread
From: Florian Weimer @ 2020-05-16 18:38 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella via Libc-alpha:
> The thread cancellation state is not accessed concurrently internally
> neither the pthread interface allows changing the state of a different
> thread than its own.
>
> The code is also simplified: the CANCELLATION_P is replaced with a
> internal pthread_testcancel call and the CANCELSTATE_BIT{MASK} is
> removed.
>
> Checked on x86_64-linux-gnu.
Please use the more elaborate commit message. It is helpful.
> diff --git a/nptl/descr.h b/nptl/descr.h
> index 9dcf480bdf..61665bf859 100644
> --- a/nptl/descr.h
> +++ b/nptl/descr.h
> @@ -394,6 +388,9 @@ struct pthread
> /* Indicates whether is a C11 thread created by thrd_creat. */
> bool c11;
>
> + /* Thread cancel state (enable, disable). */
> + unsigned char cancelstate;
> +
Please document the permitted values in the comment.
> diff --git a/nptl/pthread_setcancelstate.c b/nptl/pthread_setcancelstate.c
> index 4d7f413e19..aa1c8073a8 100644
> --- a/nptl/pthread_setcancelstate.c
> +++ b/nptl/pthread_setcancelstate.c
> @@ -31,39 +31,9 @@ __pthread_setcancelstate (int state, int *oldstate)
>
> self = THREAD_SELF;
>
> - int oldval = THREAD_GETMEM (self, cancelhandling);
> - while (1)
> - {
> - int newval = (state == PTHREAD_CANCEL_DISABLE
> - ? oldval | CANCELSTATE_BITMASK
> - : oldval & ~CANCELSTATE_BITMASK);
> -
> - /* Store the old value. */
> - if (oldstate != NULL)
> - *oldstate = ((oldval & CANCELSTATE_BITMASK)
> - ? PTHREAD_CANCEL_DISABLE : PTHREAD_CANCEL_ENABLE);
> -
> - /* Avoid doing unnecessary work. The atomic operation can
> - potentially be expensive if the memory has to be locked and
> - remote cache lines have to be invalidated. */
> - if (oldval == newval)
> - break;
> -
> - /* Update the cancel handling word. This has to be done
> - atomically since other bits could be modified as well. */
> - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
> - oldval);
> - if (__glibc_likely (curval == oldval))
> - {
> - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
> - __do_cancel ();
> -
> - break;
> - }
> -
> - /* Prepare for the next round. */
> - oldval = curval;
> - }
> + if (oldstate != NULL)
> + *oldstate = self->cancelstate;
> + self->cancelstate = state;
>
> return 0;
> }
I've re-read the old code and I think this checks for cancellation
even in the absence of a race (i.e., if the CAS succeeds). I still
think we should preserve this behavior, also for symmetry with this
code below:
> diff --git a/nptl/pthread_setcanceltype.c b/nptl/pthread_setcanceltype.c
> index fcaae8abc7..cc0507ae04 100644
> --- a/nptl/pthread_setcanceltype.c
> +++ b/nptl/pthread_setcanceltype.c
> @@ -53,7 +53,8 @@ __pthread_setcanceltype (int type, int *oldtype)
> oldval);
> if (__glibc_likely (curval == oldval))
> {
> - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
> + if (self->cancelstate == PTHREAD_CANCEL_ENABLE
> + && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval))
> {
> THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> __do_cancel ();
> diff --git a/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c
> index 30408c2008..3ffff4ebef 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 | EXITING_BITMASK
> + | TERMINATED_BITMASK))
> + == CANCELED_BITMASK)
> + {
> + THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> + __do_cancel ();
> + }
> }
> strong_alias (__pthread_testcancel, pthread_testcancel)
> hidden_def (__pthread_testcancel)
I think you can write this as
self->cancelstate == PTHREAD_CANCEL_ENABLE
&& (cancelhandling & CANCELED_BITMASK)
&& !(cancelhandling & EXITING_BITMASK)
&& !(cancelhandling & TERMINATED_BITMASK)
and GCC will do the right thing. I find this variant easier to read,
but I don't have a strong preference.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] nptl: Move cancel state out of cancelhandling
2020-05-16 18:38 ` Florian Weimer
@ 2020-05-20 14:51 ` Adhemerval Zanella
0 siblings, 0 replies; 15+ messages in thread
From: Adhemerval Zanella @ 2020-05-20 14:51 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella via Libc-alpha
On 16/05/2020 15:38, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> The thread cancellation state is not accessed concurrently internally
>> neither the pthread interface allows changing the state of a different
>> thread than its own.
>>
>> The code is also simplified: the CANCELLATION_P is replaced with a
>> internal pthread_testcancel call and the CANCELSTATE_BIT{MASK} is
>> removed.
>>
>> Checked on x86_64-linux-gnu.
>
> Please use the more elaborate commit message. It is helpful.
Ack.
>
>> diff --git a/nptl/descr.h b/nptl/descr.h
>> index 9dcf480bdf..61665bf859 100644
>> --- a/nptl/descr.h
>> +++ b/nptl/descr.h
>
>> @@ -394,6 +388,9 @@ struct pthread
>> /* Indicates whether is a C11 thread created by thrd_creat. */
>> bool c11;
>>
>> + /* Thread cancel state (enable, disable). */
>> + unsigned char cancelstate;
>> +
>
> Please document the permitted values in the comment.
Ack.
>
>> diff --git a/nptl/pthread_setcancelstate.c b/nptl/pthread_setcancelstate.c
>> index 4d7f413e19..aa1c8073a8 100644
>> --- a/nptl/pthread_setcancelstate.c
>> +++ b/nptl/pthread_setcancelstate.c
>> @@ -31,39 +31,9 @@ __pthread_setcancelstate (int state, int *oldstate)
>>
>> self = THREAD_SELF;
>>
>> - int oldval = THREAD_GETMEM (self, cancelhandling);
>> - while (1)
>> - {
>> - int newval = (state == PTHREAD_CANCEL_DISABLE
>> - ? oldval | CANCELSTATE_BITMASK
>> - : oldval & ~CANCELSTATE_BITMASK);
>> -
>> - /* Store the old value. */
>> - if (oldstate != NULL)
>> - *oldstate = ((oldval & CANCELSTATE_BITMASK)
>> - ? PTHREAD_CANCEL_DISABLE : PTHREAD_CANCEL_ENABLE);
>> -
>> - /* Avoid doing unnecessary work. The atomic operation can
>> - potentially be expensive if the memory has to be locked and
>> - remote cache lines have to be invalidated. */
>> - if (oldval == newval)
>> - break;
>> -
>> - /* Update the cancel handling word. This has to be done
>> - atomically since other bits could be modified as well. */
>> - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
>> - oldval);
>> - if (__glibc_likely (curval == oldval))
>> - {
>> - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
>> - __do_cancel ();
>> -
>> - break;
>> - }
>> -
>> - /* Prepare for the next round. */
>> - oldval = curval;
>> - }
>> + if (oldstate != NULL)
>> + *oldstate = self->cancelstate;
>> + self->cancelstate = state;
>>
>> return 0;
>> }
>
> I've re-read the old code and I think this checks for cancellation
> even in the absence of a race (i.e., if the CAS succeeds). I still
> think we should preserve this behavior, also for symmetry with this
> code below:
The second part of this change keeps the pthread_setcanceltype as
cancellation entrypoint by calling pthread_testcancel if the new type
is PTHREAD_CANCEL_ASYNCHRONOUS.
And with this behavior I don't see a clear rationale to keep
pthread_setcancelstate as a cancellation entrypoint since it should act
iff cancel type is PTHREAD_CANCEL_ASYNCHRONOUS (which is already handled
by pthread_setcanceltype).
>
>> diff --git a/nptl/pthread_setcanceltype.c b/nptl/pthread_setcanceltype.c
>> index fcaae8abc7..cc0507ae04 100644
>> --- a/nptl/pthread_setcanceltype.c
>> +++ b/nptl/pthread_setcanceltype.c
>> @@ -53,7 +53,8 @@ __pthread_setcanceltype (int type, int *oldtype)
>> oldval);
>> if (__glibc_likely (curval == oldval))
>> {
>> - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
>> + if (self->cancelstate == PTHREAD_CANCEL_ENABLE
>> + && CANCEL_CANCELED_AND_ASYNCHRONOUS (newval))
>> {
>> THREAD_SETMEM (self, result, PTHREAD_CANCELED);
>> __do_cancel ();
>
>> diff --git a/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c
>> index 30408c2008..3ffff4ebef 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 | EXITING_BITMASK
>> + | TERMINATED_BITMASK))
>> + == CANCELED_BITMASK)
>> + {
>> + THREAD_SETMEM (self, result, PTHREAD_CANCELED);
>> + __do_cancel ();
>> + }
>> }
>> strong_alias (__pthread_testcancel, pthread_testcancel)
>> hidden_def (__pthread_testcancel)
>
> I think you can write this as
>
> self->cancelstate == PTHREAD_CANCEL_ENABLE
> && (cancelhandling & CANCELED_BITMASK)
> && !(cancelhandling & EXITING_BITMASK)
> && !(cancelhandling & TERMINATED_BITMASK)
>
> and GCC will do the right thing. I find this variant easier to read,
> but I don't have a strong preference.
>
Ack, I think it is slight better as well.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-05-20 14:51 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-01 14:24 [PATCH 1/3] nptl: Remove x86_64 cancellation assembly implementations Adhemerval Zanella
2020-04-01 14:24 ` [PATCH 2/3] nptl: Move cancel state out of cancelhandling Adhemerval Zanella
2020-04-16 14:18 ` Adhemerval Zanella
2020-04-22 14:11 ` Florian Weimer
2020-04-23 19:30 ` Adhemerval Zanella
2020-04-24 13:21 ` Florian Weimer
2020-04-30 11:11 ` Adhemerval Zanella
2020-05-12 14:47 ` Adhemerval Zanella
2020-05-12 14:57 ` Florian Weimer
2020-05-16 18:38 ` Florian Weimer
2020-05-20 14:51 ` Adhemerval Zanella
2020-04-01 14:24 ` [PATCH 3/3] nptl: Move cancel type " Adhemerval Zanella
2020-04-16 14:18 ` Adhemerval Zanella
2020-04-02 12:23 ` [PATCH 1/3] nptl: Remove x86_64 cancellation assembly implementations Adhemerval Zanella
2020-04-02 12:27 ` H.J. Lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).