From patchwork Sun Jun 1 15:04:48 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alexander Fyodorov X-Patchwork-Id: 1225 Received: (qmail 4939 invoked by alias); 1 Jun 2014 15:04:57 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 4925 invoked by uid 89); 1 Jun 2014 15:04:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.8 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS, T_FILL_THIS_FORM_SHORT, T_FSL_HELO_BARE_IP_2 autolearn=ham version=3.3.2 X-HELO: forward14.mail.yandex.net From: Alexander Fyodorov To: libc-alpha@sourceware.org Subject: [PATCH] nptl: optimize cancelstate and canceltype changing MIME-Version: 1.0 Message-Id: <70941401635088@web6j.yandex.ru> Date: Sun, 01 Jun 2014 19:04:48 +0400 Hi all, I'm posting this for review and more testing, as I do not have big or middle endian processors. The barrier's name here could use some thinking, but this was the best I could come up with. -------------- This patch improves performance of functions changing thread cancellation state and type by removing atomic operations from them. The idea is simple: since state and type are changed only by the thread itself, they should not require such rigorous synchronization. The 'cancelhandling' word takes 4 bytes, so we can put type and state in different bytes within the word and access them directly using 1-byte stores. Specific position of a given flag will then depend on endiannes. Checking whether the thread was canceled must be done _after_ it enables cancellation or turns on asynchronous mode. To enforce this order, a barrier is put between the respective store and load. Since the read is data-dependent on the corresponding write and some architectures may not reorder such accesses, putting atomic_full_barrier() there would be an overkill. So I added a new type of barrier which defaults to a full barrier. Also it looks like there is a missing "THREAD_SETMEM (self, result, PTHREAD_CANCELED)" in __pthread_setcancelstate() (it is present in __pthread_setcanceltype()). 2014-06-01 Fyodorov V. Alexander * nptl/descr.h: Change bits position in the 'cancelhandling' field. * include/atomic.h: Add default atomic_read_after_write_dependent_barrier() definition. * nptl/cancellation.c: Replace atomic operation with a barrier. * nptl/cleanup_defer.c: Likewise. * nptl/cleanup_defer_compat.c: Likewise. * nptl/pthread_setcanceltype.c: Likewise. * nptl/pthread_setcancelstate.c: Likewise, and set self->result to PTHREAD_CANCELED. diff --git a/include/atomic.h b/include/atomic.h index 3e82b6a..f0dac38 100644 --- a/include/atomic.h +++ b/include/atomic.h @@ -547,4 +547,9 @@ # define atomic_delay() do { /* nothing */ } while (0) #endif + +#ifndef atomic_read_after_write_dependent_barrier() +# define atomic_read_after_write_dependent_barrier() atomic_full_barrier () +#endif + #endif /* atomic.h */ diff --git a/nptl/cancellation.c b/nptl/cancellation.c index aaf102d..c127231 100644 --- a/nptl/cancellation.c +++ b/nptl/cancellation.c @@ -31,28 +31,15 @@ __pthread_enable_asynccancel (void) struct pthread *self = THREAD_SELF; int oldval = THREAD_GETMEM (self, cancelhandling); - while (1) + if ((oldval & CANCELTYPE_BITMASK) == 0) { - int newval = oldval | CANCELTYPE_BITMASK; - - if (newval == oldval) - break; - - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval, - oldval); - if (__glibc_likely (curval == oldval)) - { - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval)) - { - THREAD_SETMEM (self, result, PTHREAD_CANCELED); - __do_cancel (); - } - - break; - } - - /* Prepare the next round. */ - oldval = curval; + /* Set the new value. */ + THREAD_SETMEM (self, cancel.type, (char) PTHREAD_CANCEL_ASYNCHRONOUS); + + /* Synchronize with pthread_cancel() */ + atomic_read_after_write_dependent_barrier(); + + CANCELLATION_P (self); } return oldval; @@ -69,22 +56,14 @@ __pthread_disable_asynccancel (int oldtype) return; struct pthread *self = THREAD_SELF; - int newval; - - int oldval = THREAD_GETMEM (self, cancelhandling); - while (1) - { - newval = oldval & ~CANCELTYPE_BITMASK; + /* Set the new value. */ + THREAD_SETMEM (self, cancel.type, (char) PTHREAD_CANCEL_DEFERRED); - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval, - oldval); - if (__glibc_likely (curval == oldval)) - break; + /* Synchronize with pthread_cancel() */ + atomic_read_after_write_dependent_barrier(); - /* Prepare the next round. */ - oldval = curval; - } + int newval = THREAD_GETMEM (self, cancelhandling); /* We cannot return when we are being canceled. Upon return the thread might be things which would have to be undone. The diff --git a/nptl/cleanup_defer.c b/nptl/cleanup_defer.c index a8fc403..86097d7 100644 --- a/nptl/cleanup_defer.c +++ b/nptl/cleanup_defer.c @@ -35,19 +35,12 @@ __pthread_register_cancel_defer (__pthread_unwind_buf_t *buf) /* Disable asynchronous cancellation for now. */ if (__glibc_unlikely (cancelhandling & CANCELTYPE_BITMASK)) - while (1) - { - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, - cancelhandling - & ~CANCELTYPE_BITMASK, - cancelhandling); - if (__glibc_likely (curval == cancelhandling)) - /* Successfully replaced the value. */ - break; - - /* Prepare for the next round. */ - cancelhandling = curval; - } + { + THREAD_SETMEM (self, cancel.type, (char) PTHREAD_CANCEL_DEFERRED); + + /* Synchronize with pthread_cancel() */ + atomic_read_after_write_dependent_barrier(); + } ibuf->priv.data.canceltype = (cancelhandling & CANCELTYPE_BITMASK ? PTHREAD_CANCEL_ASYNCHRONOUS @@ -72,19 +65,10 @@ __pthread_unregister_cancel_restore (__pthread_unwind_buf_t *buf) && ((cancelhandling = THREAD_GETMEM (self, cancelhandling)) & CANCELTYPE_BITMASK) == 0) { - while (1) - { - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, - cancelhandling - | CANCELTYPE_BITMASK, - cancelhandling); - if (__glibc_likely (curval == cancelhandling)) - /* Successfully replaced the value. */ - break; - - /* Prepare for the next round. */ - cancelhandling = curval; - } + THREAD_SETMEM (self, cancel.type, PTHREAD_CANCEL_ASYNCHRONOUS); + + /* Synchronize with pthread_cancel() */ + atomic_read_after_write_dependent_barrier(); CANCELLATION_P (self); } diff --git a/nptl/cleanup_defer_compat.c b/nptl/cleanup_defer_compat.c index 9c52f5f..abff38c 100644 --- a/nptl/cleanup_defer_compat.c +++ b/nptl/cleanup_defer_compat.c @@ -35,19 +35,12 @@ _pthread_cleanup_push_defer (buffer, routine, arg) /* Disable asynchronous cancellation for now. */ if (__glibc_unlikely (cancelhandling & CANCELTYPE_BITMASK)) - while (1) - { - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, - cancelhandling - & ~CANCELTYPE_BITMASK, - cancelhandling); - if (__glibc_likely (curval == cancelhandling)) - /* Successfully replaced the value. */ - break; - - /* Prepare for the next round. */ - cancelhandling = curval; - } + { + THREAD_SETMEM (self, cancel.type, (char) PTHREAD_CANCEL_DEFERRED); + + /* Synchronize with pthread_cancel() */ + atomic_read_after_write_dependent_barrier(); + } buffer->__canceltype = (cancelhandling & CANCELTYPE_BITMASK ? PTHREAD_CANCEL_ASYNCHRONOUS @@ -72,19 +65,10 @@ _pthread_cleanup_pop_restore (buffer, execute) && ((cancelhandling = THREAD_GETMEM (self, cancelhandling)) & CANCELTYPE_BITMASK) == 0) { - while (1) - { - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, - cancelhandling - | CANCELTYPE_BITMASK, - cancelhandling); - if (__glibc_likely (curval == cancelhandling)) - /* Successfully replaced the value. */ - break; - - /* Prepare for the next round. */ - cancelhandling = curval; - } + THREAD_SETMEM (self, cancel.type, PTHREAD_CANCEL_ASYNCHRONOUS); + + /* Synchronize with pthread_cancel() */ + atomic_read_after_write_dependent_barrier(); CANCELLATION_P (self); } diff --git a/nptl/descr.h b/nptl/descr.h index 61d57d5..9671793 100644 --- a/nptl/descr.h +++ b/nptl/descr.h @@ -19,6 +19,7 @@ #ifndef _DESCR_H #define _DESCR_H 1 +#include #include #include #include @@ -255,30 +256,63 @@ struct pthread #define HAVE_CLEANUP_JMP_BUF /* Flags determining processing of cancellation. */ - int cancelhandling; + union { + int cancelhandling; + struct { + char state; + char type; + } cancel; + }; +#if BYTE_ORDER == LITTLE_ENDIAN /* Bit set if cancellation is disabled. */ #define CANCELSTATE_BIT 0 -#define CANCELSTATE_BITMASK (0x01 << CANCELSTATE_BIT) /* Bit set if asynchronous cancellation mode is selected. */ -#define CANCELTYPE_BIT 1 -#define CANCELTYPE_BITMASK (0x01 << CANCELTYPE_BIT) +#define CANCELTYPE_BIT 8 /* Bit set if canceling has been initiated. */ -#define CANCELING_BIT 2 -#define CANCELING_BITMASK (0x01 << CANCELING_BIT) +#define CANCELING_BIT 16 /* Bit set if canceled. */ -#define CANCELED_BIT 3 -#define CANCELED_BITMASK (0x01 << CANCELED_BIT) +#define CANCELED_BIT 17 /* Bit set if thread is exiting. */ -#define EXITING_BIT 4 -#define EXITING_BITMASK (0x01 << EXITING_BIT) +#define EXITING_BIT 18 /* Bit set if thread terminated and TCB is freed. */ -#define TERMINATED_BIT 5 -#define TERMINATED_BITMASK (0x01 << TERMINATED_BIT) +#define TERMINATED_BIT 19 /* Bit set if thread is supposed to change XID. */ -#define SETXID_BIT 6 +#define SETXID_BIT 20 +#else +#if BYTE_ORDER == BIG_ENDIAN + /* Bit set if cancellation is disabled. */ +#define CANCELSTATE_BIT 24 + /* Bit set if asynchronous cancellation mode is selected. */ +#define CANCELTYPE_BIT 16 +#else /* BYTE_ORDER == PDP_ENDIAN */ + /* Bit set if cancellation is disabled. */ +#define CANCELSTATE_BIT 16 + /* Bit set if asynchronous cancellation mode is selected. */ +#define CANCELTYPE_BIT 24 +#endif + /* Bit set if canceling has been initiated. */ +#define CANCELING_BIT 0 + /* Bit set if canceled. */ +#define CANCELED_BIT 1 + /* Bit set if thread is exiting. */ +#define EXITING_BIT 2 + /* Bit set if thread terminated and TCB is freed. */ +#define TERMINATED_BIT 3 + /* Bit set if thread is supposed to change XID. */ +#define SETXID_BIT 4 +#endif +#define CANCELSTATE_BITMASK (0x01 << CANCELSTATE_BIT) +#define CANCELTYPE_BITMASK (0x01 << CANCELTYPE_BIT) +#define CANCELING_BITMASK (0x01 << CANCELING_BIT) +#define CANCELED_BITMASK (0x01 << CANCELED_BIT) +#define EXITING_BITMASK (0x01 << EXITING_BIT) +#define TERMINATED_BITMASK (0x01 << TERMINATED_BIT) #define SETXID_BITMASK (0x01 << SETXID_BIT) /* Mask for the rest. Helps the compiler to optimize. */ -#define CANCEL_RESTMASK 0xffffff80 +#define CANCEL_RESTMASK \ + ( ~((int) (CANCELSTATE_BITMASK | CANCELTYPE_BITMASK | CANCELING_BITMASK | \ + CANCELED_BITMASK | EXITING_BITMASK | TERMINATED_BITMASK | \ + SETXID_BITMASK)) ) #define CANCEL_ENABLED_AND_CANCELED(value) \ (((value) & (CANCELSTATE_BITMASK | CANCELED_BITMASK | EXITING_BITMASK \ diff --git a/nptl/pthread_setcancelstate.c b/nptl/pthread_setcancelstate.c index 5c3ca86..c187563 100644 --- a/nptl/pthread_setcancelstate.c +++ b/nptl/pthread_setcancelstate.c @@ -34,37 +34,30 @@ __pthread_setcancelstate (state, oldstate) self = THREAD_SELF; int oldval = THREAD_GETMEM (self, cancelhandling); - while (1) + + int prev_state = ((oldval & CANCELSTATE_BITMASK) ? PTHREAD_CANCEL_DISABLE : + PTHREAD_CANCEL_ENABLE); + + /* Store the old value. */ + if (oldstate != NULL) + *oldstate = prev_state; + + /* Avoid doing unnecessary work. */ + if (state != prev_state) { - int newval = (state == PTHREAD_CANCEL_DISABLE - ? oldval | CANCELSTATE_BITMASK - : oldval & ~CANCELSTATE_BITMASK); - - /* Store the old value. */ - if (oldstate != NULL) - *oldstate = ((oldval & CANCELSTATE_BITMASK) - ? PTHREAD_CANCEL_DISABLE : PTHREAD_CANCEL_ENABLE); - - /* Avoid doing unnecessary work. The atomic operation can - potentially be expensive if the memory has to be locked and - remote cache lines have to be invalidated. */ - if (oldval == newval) - break; - - /* Update the cancel handling word. This has to be done - atomically since other bits could be modified as well. */ - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval, - oldval); - if (__glibc_likely (curval == oldval)) - { - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval)) - __do_cancel (); - - break; - } - - /* Prepare for the next round. */ - oldval = curval; + /* Set the new value. */ + THREAD_SETMEM (self, cancel.state, (char) state); + + /* Synchronize with pthread_cancel() */ + atomic_read_after_write_dependent_barrier(); + + int newval = THREAD_GETMEM (self, cancelhandling); + + if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval)) + { + THREAD_SETMEM (self, result, PTHREAD_CANCELED); + __do_cancel (); + } } return 0; diff --git a/nptl/pthread_setcanceltype.c b/nptl/pthread_setcanceltype.c index fb1631f..17d3593 100644 --- a/nptl/pthread_setcanceltype.c +++ b/nptl/pthread_setcanceltype.c @@ -34,40 +34,30 @@ __pthread_setcanceltype (type, oldtype) self = THREAD_SELF; int oldval = THREAD_GETMEM (self, cancelhandling); - while (1) + + int prev_type = ((oldval & CANCELTYPE_BITMASK) ? PTHREAD_CANCEL_ASYNCHRONOUS : + PTHREAD_CANCEL_DEFERRED); + + /* Store the old value. */ + if (oldtype != NULL) + *oldtype = prev_type; + + /* Avoid doing unnecessary work. */ + if (type != prev_type) { - int newval = (type == PTHREAD_CANCEL_ASYNCHRONOUS - ? oldval | CANCELTYPE_BITMASK - : oldval & ~CANCELTYPE_BITMASK); - - /* Store the old value. */ - if (oldtype != NULL) - *oldtype = ((oldval & CANCELTYPE_BITMASK) - ? PTHREAD_CANCEL_ASYNCHRONOUS : PTHREAD_CANCEL_DEFERRED); - - /* Avoid doing unnecessary work. The atomic operation can - potentially be expensive if the memory has to be locked and - remote cache lines have to be invalidated. */ - if (oldval == newval) - break; - - /* Update the cancel handling word. This has to be done - atomically since other bits could be modified as well. */ - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval, - oldval); - if (__glibc_likely (curval == oldval)) - { - if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval)) - { - THREAD_SETMEM (self, result, PTHREAD_CANCELED); - __do_cancel (); - } - - break; - } - - /* Prepare for the next round. */ - oldval = curval; + /* Set the new value. */ + THREAD_SETMEM (self, cancel.type, (char) type); + + /* Synchronize with pthread_cancel() */ + atomic_read_after_write_dependent_barrier(); + + int newval = THREAD_GETMEM (self, cancelhandling); + + if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval)) + { + THREAD_SETMEM (self, result, PTHREAD_CANCELED); + __do_cancel (); + } } return 0;