From patchwork Mon Aug 24 18:11:35 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tulio Magno Quites Machado Filho X-Patchwork-Id: 8413 Received: (qmail 36093 invoked by alias); 24 Aug 2015 18:12:18 -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 36071 invoked by uid 89); 24 Aug 2015 18:12:17 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL, BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: e24smtp05.br.ibm.com X-MailFrom: tuliom@linux.vnet.ibm.com X-RcptTo: libc-alpha@sourceware.org From: "Tulio Magno Quites Machado Filho" To: carlos@redhat.com, adhemerval.zanella@linaro.org Cc: triegel@redhat.com, libc-alpha@sourceware.org, munroesj@linux.vnet.ibm.com Subject: [PATCHv3] PowerPC: Fix a race condition when eliding a lock Date: Mon, 24 Aug 2015 15:11:35 -0300 Message-Id: <1440439895-11812-1-git-send-email-tuliom@linux.vnet.ibm.com> In-Reply-To: <55D742D3.9050600@redhat.com> References: <55D742D3.9050600@redhat.com> X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15082418-0033-0000-0000-0000034E2D31 Changes since v2: - Moved part of the source code comments to the commit message. - Added O'Donnel's suggestion to the source code comments. Changes since v1: - Improved commit message. - Added new comments in the source code alerting to the concurrency details intrinsic to this code. - Removed compiler barriers from this patch, which will be treated in another patch and will be synchronized with the GCC implementation. 8<---------- The previous code used to evaluate the preprocessor token is_lock_free to a variable before starting a transaction. This behavior can cause an error if another thread got the lock (without using a transaction) between the evaluation of the token and the beginning of the transaction. This bug can be triggered with the following order of events: 1. The lock accessed by is_lock_free is free. 2. Thread T1 evaluates is_lock_free and stores into register R1 that the lock is free. 3. Thread T2 acquires the same lock used in is_lock_free. 4. T1 begins the transaction, creating a memory barrier where is_lock_free is false, but R1 is true. 5. T1 reads R1 and doesn't abort the transaction. 6. T1 calls ELIDE_UNLOCK, which reads false from is_lock_free and decides to unlock a lock acquired by T2, leading to undefined behavior. This patch delays the evaluation of is_lock_free to inside a transaction by moving this part of the code to the macro ELIDE_LOCK. 2015-08-24 Tulio Magno Quites Machado Filho [BZ #18743] * sysdeps/powerpc/nptl/elide.h (__elide_lock): Move most of this code to... (ELIDE_LOCK): ...here. (__get_new_count): New function with part of the code from __elide_lock that updates the value of adapt_count after a transaction abort. (__elided_trylock): Moved this code to... (ELIDE_TRYLOCK): ...here. --- NEWS | 3 +- sysdeps/powerpc/nptl/elide.h | 113 +++++++++++++++++++++++-------------------- 2 files changed, 63 insertions(+), 53 deletions(-) diff --git a/NEWS b/NEWS index 6b36c08..76df3fd 100644 --- a/NEWS +++ b/NEWS @@ -11,7 +11,8 @@ Version 2.23 14341, 16517, 16519, 16520, 16734, 16973, 17787, 17905, 18084, 18086, 18265, 18370, 18421, 18480, 18525, 18618, 18647, 18661, 18674, 18681, - 18778, 18781, 18787, 18789, 18790, 18795, 18796, 18820, 18823, 18824. + 18778, 18781, 18787, 18789, 18790, 18795, 18796, 18820, 18823, 18824, + 18743. * The obsolete header has been removed. Programs that require this header must be updated to use instead. diff --git a/sysdeps/powerpc/nptl/elide.h b/sysdeps/powerpc/nptl/elide.h index 389f5a5..1cc9890 100644 --- a/sysdeps/powerpc/nptl/elide.h +++ b/sysdeps/powerpc/nptl/elide.h @@ -23,67 +23,76 @@ # include # include -/* Returns true if the lock defined by is_lock_free as elided. - ADAPT_COUNT is a pointer to per-lock state variable. */ - +/* Get the new value of adapt_count according to the elision + configurations. Returns true if the system should retry again or false + otherwise. */ static inline bool -__elide_lock (uint8_t *adapt_count, int is_lock_free) +__get_new_count (uint8_t *adapt_count) { - if (*adapt_count > 0) + /* A persistent failure indicates that a retry will probably + result in another failure. Use normal locking now and + for the next couple of calls. */ + if (_TEXASRU_FAILURE_PERSISTENT (__builtin_get_texasru ())) { - (*adapt_count)--; + if (__elision_aconf.skip_lock_internal_abort > 0) + *adapt_count = __elision_aconf.skip_lock_internal_abort; return false; } - - for (int i = __elision_aconf.try_tbegin; i > 0; i--) - { - if (__builtin_tbegin (0)) - { - if (is_lock_free) - return true; - /* Lock was busy. */ - __builtin_tabort (_ABORT_LOCK_BUSY); - } - else - { - /* A persistent failure indicates that a retry will probably - result in another failure. Use normal locking now and - for the next couple of calls. */ - if (_TEXASRU_FAILURE_PERSISTENT (__builtin_get_texasru ())) - { - if (__elision_aconf.skip_lock_internal_abort > 0) - *adapt_count = __elision_aconf.skip_lock_internal_abort; - break; - } - /* Same logic as above, but for a number of temporary failures in a - a row. */ - else if (__elision_aconf.skip_lock_out_of_tbegin_retries > 0 - && __elision_aconf.try_tbegin > 0) - *adapt_count = __elision_aconf.skip_lock_out_of_tbegin_retries; - } - } - - return false; + /* Same logic as above, but for a number of temporary failures in a + a row. */ + else if (__elision_aconf.skip_lock_out_of_tbegin_retries > 0 + && __elision_aconf.try_tbegin > 0) + *adapt_count = __elision_aconf.skip_lock_out_of_tbegin_retries; + return true; } -# define ELIDE_LOCK(adapt_count, is_lock_free) \ - __elide_lock (&(adapt_count), is_lock_free) - - -static inline bool -__elide_trylock (uint8_t *adapt_count, int is_lock_free, int write) -{ - if (__elision_aconf.try_tbegin > 0) - { - if (write) - __builtin_tabort (_ABORT_NESTED_TRYLOCK); - return __elide_lock (adapt_count, is_lock_free); - } - return false; -} +/* CONCURRENCY NOTES: + + The value of is_lock_free is read and possibly written to by multiple + threads, either during lock acquisition, or elision. In order to avoid + this evaluation from becoming a data race the access of is_lock_free + is placed *inside* the transaction. Within the transaction is_lock_free + can be accessed with relaxed ordering. We are assured by the transaction + that the value of is_lock_free is consistent with the state of the lock, + otherwise the hardware will abort the transaction. */ + +/* Returns 0 if the lock defined by is_lock_free was elided. + ADAPT_COUNT is a per-lock state variable. */ +# define ELIDE_LOCK(adapt_count, is_lock_free) \ + ({ \ + int ret = 0; \ + if (adapt_count > 0) \ + (adapt_count)--; \ + else \ + for (int i = __elision_aconf.try_tbegin; i > 0; i--) \ + { \ + if (__builtin_tbegin (0)) \ + { \ + if (is_lock_free) \ + { \ + ret = 1; \ + break; \ + } \ + __builtin_tabort (_ABORT_LOCK_BUSY); \ + } \ + else \ + if (!__get_new_count(&adapt_count)) \ + break; \ + } \ + ret; \ + }) # define ELIDE_TRYLOCK(adapt_count, is_lock_free, write) \ - __elide_trylock (&(adapt_count), is_lock_free, write) + ({ \ + int ret = 0; \ + if (__elision_aconf.try_tbegin > 0) \ + { \ + if (write) \ + __builtin_tabort (_ABORT_NESTED_TRYLOCK); \ + ret = ELIDE_LOCK (adapt_count, is_lock_free); \ + } \ + ret; \ + }) static inline bool