From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13506 invoked by alias); 6 Dec 2013 21:52:12 -0000 Mailing-List: contact libc-ports-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: libc-ports-owner@sourceware.org Received: (qmail 13454 invoked by uid 89); 6 Dec 2013 21:52:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.4 required=5.0 tests=AWL,BAYES_20,FREEMAIL_FROM,SPF_NEUTRAL autolearn=no version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: popelka.ms.mff.cuni.cz Received: from Unknown (HELO popelka.ms.mff.cuni.cz) (195.113.20.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 06 Dec 2013 21:52:09 +0000 Received: from domone.kolej.mff.cuni.cz (popelka.ms.mff.cuni.cz [195.113.20.131]) by popelka.ms.mff.cuni.cz (Postfix) with ESMTPS id 62D3969F50; Fri, 6 Dec 2013 22:51:58 +0100 (CET) Received: by domone.kolej.mff.cuni.cz (Postfix, from userid 1000) id 3F9CD5F767; Fri, 6 Dec 2013 22:51:58 +0100 (CET) Date: Fri, 06 Dec 2013 21:52:00 -0000 From: =?utf-8?B?T25kxZllaiBCw61sa2E=?= To: Mike Frysinger Cc: libc-ports@sourceware.org, Atsushi Nemoto , libc-alpha@sourceware.org Subject: Re: [RFC][BZ #13690] Always read private before lll_unlock. Message-ID: <20131206215158.GA27587@domone.podge> References: <20131206190159.GA25502@domone.podge> <20131206194305.GA26401@domone.podge> <201312061611.59957.vapier@gentoo.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <201312061611.59957.vapier@gentoo.org> User-Agent: Mutt/1.5.20 (2009-06-14) X-SW-Source: 2013-12/txt/msg00005.txt.bz2 On Fri, Dec 06, 2013 at 04:11:58PM -0500, Mike Frysinger wrote: > On Friday 06 December 2013 14:43:05 Ondřej Bílka wrote: > > --- /dev/null > > +++ b/include/futex_unlock.h > > probably should live at nptl/lowlevellock_unlock > or better name, see below. > > @@ -0,0 +1,15 @@ > > +#define lll_unlock(lock, private) \ > > all new files need a proper comment header block > > > + ((void)) ({ \ > > + int __private = private; \ > > + __lll_unlock (lock, __private); \ > > + }) > > + > > +#define lll_unlock(lock, private) \ > > did i misread, or are both of these macros named "lll_unlock" ? should one > have a __ prefix ? > yes, i missed that, thanks. Now when I looked to implementations we need more radical refactoring, These implementatation were created by copying so headers are mostly identical except of cosmetic changes like macro/inline function, added expect and similar. Some architectures need different primitives than atomic_compare_and_exchange_val_acq/rel but it looks most do not need that. As example a diff -uw between arm and tile is following: --- ports/sysdeps/unix/sysv/linux/tile/nptl/lowlevellock.h 2013-12-06 20:17:57.092514754 +0100 +++ ports/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h 2013-12-06 20:19:28.855381949 +0100 @@ -1,6 +1,5 @@ -/* Copyright (C) 2011-2013 Free Software Foundation, Inc. +/* Copyright (C) 2005-2013 Free Software Foundation, Inc. This file is part of the GNU C Library. - Contributed by Chris Metcalf , 2011. The GNU C Library is free software; you can redistribute it and/or modify it under the terms of the GNU Lesser General Public @@ -26,7 +25,6 @@ #include #include - #define FUTEX_WAIT 0 #define FUTEX_WAKE 1 #define FUTEX_REQUEUE 3 @@ -83,9 +81,11 @@ #define lll_futex_timed_wait(futexp, val, timespec, private) \ ({ \ INTERNAL_SYSCALL_DECL (__err); \ - INTERNAL_SYSCALL (futex, __err, 4, (futexp), \ + long int __ret; \ + __ret = INTERNAL_SYSCALL (futex, __err, 4, (futexp), \ __lll_private_flag (FUTEX_WAIT, private), \ (val), (timespec)); \ + __ret; \ }) #define lll_futex_timed_wait_bitset(futexp, val, timespec, clockbit, private) \ @@ -93,7 +93,6 @@ INTERNAL_SYSCALL_DECL (__err); \ long int __ret; \ int __op = FUTEX_WAIT_BITSET | clockbit; \ - \ __ret = INTERNAL_SYSCALL (futex, __err, 6, (futexp), \ __lll_private_flag (__op, private), \ (val), (timespec), NULL /* Unused. */, \ @@ -104,9 +103,11 @@ #define lll_futex_wake(futexp, nr, private) \ ({ \ INTERNAL_SYSCALL_DECL (__err); \ - INTERNAL_SYSCALL (futex, __err, 4, (futexp), \ + long int __ret; \ + __ret = INTERNAL_SYSCALL (futex, __err, 4, (futexp), \ __lll_private_flag (FUTEX_WAKE, private), \ (nr), 0); \ + __ret; \ }) #define lll_robust_dead(futexv, private) \ @@ -129,6 +130,7 @@ INTERNAL_SYSCALL_ERROR_P (__ret, __err); \ }) + /* Returns non-zero if error happened, zero if success. */ #define lll_futex_wake_unlock(futexp, nr_wake, nr_wake2, futexp2, private) \ ({ \ @@ -149,13 +151,11 @@ mutex, private) \ ({ \ INTERNAL_SYSCALL_DECL (__err); \ - long int __ret; \ int __op = FUTEX_WAIT_REQUEUE_PI | clockbit; \ \ - __ret = INTERNAL_SYSCALL (futex, __err, 5, (futexp), \ + INTERNAL_SYSCALL (futex, __err, 5, (futexp), \ __lll_private_flag (__op, private), \ (val), (timespec), mutex); \ - INTERNAL_SYSCALL_ERROR_P (__ret, __err); \ }) #define lll_futex_cmp_requeue_pi(futexp, nr_wake, nr_move, mutex, val, priv) \ @@ -170,27 +170,14 @@ }) -static inline int __attribute__ ((always_inline)) -__lll_trylock (int *futex) -{ - return atomic_compare_and_exchange_val_acq (futex, 1, 0) != 0; -} -#define lll_trylock(lock) __lll_trylock (&(lock)) - - -static inline int __attribute__ ((always_inline)) -__lll_cond_trylock (int *futex) -{ - return atomic_compare_and_exchange_val_acq (futex, 2, 0) != 0; -} -#define lll_cond_trylock(lock) __lll_cond_trylock (&(lock)) - - -static inline int __attribute__ ((always_inline)) -__lll_robust_trylock (int *futex, int id) -{ - return atomic_compare_and_exchange_val_acq (futex, id, 0) != 0; -} +#define lll_trylock(lock) \ + atomic_compare_and_exchange_val_acq(&(lock), 1, 0) + +#define lll_cond_trylock(lock) \ + atomic_compare_and_exchange_val_acq(&(lock), 2, 0) + +#define __lll_robust_trylock(futex, id) \ + (atomic_compare_and_exchange_val_acq (futex, id, 0) != 0) #define lll_robust_trylock(lock, id) \ __lll_robust_trylock (&(lock), id) @@ -198,38 +185,41 @@ extern void __lll_lock_wait (int *futex, int private) attribute_hidden; extern int __lll_robust_lock_wait (int *futex, int private) attribute_hidden; -static inline void __attribute__ ((always_inline)) -__lll_lock (int *futex, int private) -{ - if (atomic_compare_and_exchange_bool_acq (futex, 1, 0) != 0) - { - if (__builtin_constant_p (private) && private == LLL_PRIVATE) - __lll_lock_wait_private (futex); - else - __lll_lock_wait (futex, private); - } -} +#define __lll_lock(futex, private) \ + ((void) ({ \ + int *__futex = (futex); \ + if (__builtin_expect (atomic_compare_and_exchange_val_acq (__futex, \ + 1, 0), 0)) \ + { \ + if (__builtin_constant_p (private) && (private) == LLL_PRIVATE) \ + __lll_lock_wait_private (__futex); \ + else \ + __lll_lock_wait (__futex, private); \ + } \ + })) #define lll_lock(futex, private) __lll_lock (&(futex), private) -static inline int __attribute__ ((always_inline)) -__lll_robust_lock (int *futex, int id, int private) -{ - int result = 0; - if (atomic_compare_and_exchange_bool_acq (futex, id, 0) != 0) - result = __lll_robust_lock_wait (futex, private); - return result; -} +#define __lll_robust_lock(futex, id, private) \ + ({ \ + int *__futex = (futex); \ + int __val = 0; \ + \ + if (__builtin_expect (atomic_compare_and_exchange_bool_acq (__futex, id, \ + 0), 0)) \ + __val = __lll_robust_lock_wait (__futex, private); \ + __val; \ + }) #define lll_robust_lock(futex, id, private) \ __lll_robust_lock (&(futex), id, private) -static inline void __attribute__ ((always_inline)) -__lll_cond_lock (int *futex, int private) -{ - if (atomic_compare_and_exchange_bool_acq (futex, 2, 0) != 0) - __lll_lock_wait (futex, private); -} +#define __lll_cond_lock(futex, private) \ + ((void) ({ \ + int *__futex = (futex); \ + if (__builtin_expect (atomic_exchange_acq (__futex, 2), 0)) \ + __lll_lock_wait (__futex, private); \ + })) #define lll_cond_lock(futex, private) __lll_cond_lock (&(futex), private) @@ -242,27 +232,29 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *, int private) attribute_hidden; -static inline int __attribute__ ((always_inline)) -__lll_timedlock (int *futex, const struct timespec *abstime, int private) -{ - int result = 0; - if (atomic_compare_and_exchange_bool_acq (futex, 1, 0) != 0) - result = __lll_timedlock_wait (futex, abstime, private); - return result; -} +#define __lll_timedlock(futex, abstime, private) \ + ({ \ + int *__futex = (futex); \ + int __val = 0; \ + \ + if (__builtin_expect (atomic_exchange_acq (__futex, 1), 0)) \ + __val = __lll_timedlock_wait (__futex, abstime, private); \ + __val; \ + }) #define lll_timedlock(futex, abstime, private) \ __lll_timedlock (&(futex), abstime, private) -static inline int __attribute__ ((always_inline)) -__lll_robust_timedlock (int *futex, const struct timespec *abstime, - int id, int private) -{ - int result = 0; - if (atomic_compare_and_exchange_bool_acq (futex, id, 0) != 0) - result = __lll_robust_timedlock_wait (futex, abstime, private); - return result; -} +#define __lll_robust_timedlock(futex, abstime, id, private) \ + ({ \ + int *__futex = (futex); \ + int __val = 0; \ + \ + if (__builtin_expect (atomic_compare_and_exchange_bool_acq (__futex, id, \ + 0), 0)) \ + __val = __lll_robust_timedlock_wait (__futex, abstime, private); \ + __val; \ + }) #define lll_robust_timedlock(futex, abstime, id, private) \ __lll_robust_timedlock (&(futex), abstime, id, private) @@ -282,10 +274,18 @@ #define lll_islocked(futex) \ (futex != 0) + +/* Our internal lock implementation is identical to the binary-compatible + mutex implementation. */ + /* Initializers for lock. */ #define LLL_LOCK_INITIALIZER (0) #define LLL_LOCK_INITIALIZER_LOCKED (1) +/* The states of a lock are: + 0 - untaken + 1 - taken by one user + >1 - taken by more users */ /* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex wakeup when the clone terminates. The memory location contains the