This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v3] lowlevellock comments


Sorry for the delayed response.  I only have minor comments, mostly
typos and such; see below.  With these addressed, I think the patch is
fine.

On Thu, 2014-10-23 at 13:12 +0100, Bernard Ogden wrote:
> @@ -113,8 +118,10 @@ __lll_timedwait_tid (int *tidp, const struct timespec *abstime)
>        if (rt.tv_sec < 0)
>  	return ETIMEDOUT;
>  
> -      /* Wait until thread terminates.  The kernel so far does not use
> -	 the private futex operations for this.  */
> +      /* If *tidp == tid, wait until thread terminates or the wait times out.
> +         The kernel to version 3.16.3 does not use the private futex

"kernel up to", I guess.

> diff --git a/nptl/lowlevelrobustlock.c b/nptl/lowlevelrobustlock.c
> index 3525807..2f4f314 100644
> --- a/nptl/lowlevelrobustlock.c
> +++ b/nptl/lowlevelrobustlock.c
> @@ -36,14 +36,17 @@ __lll_robust_lock_wait (int *futex, int private)
>  
>    do
>      {
> +      /* If the owner died, return the present value of the futex.  */
>        if (__glibc_unlikely (oldval & FUTEX_OWNER_DIED))
>  	return oldval;
>  
> +      /* Attempt to put lock into state 'acquired, possibly with waiters'.  */

"the lock", I guess.

>        int newval = oldval | FUTEX_WAITERS;
>        if (oldval != newval
>  	  && atomic_compare_and_exchange_bool_acq (futex, newval, oldval))
>  	continue;
>  
> +      /* If *futex == 2, wait until woken.  */
>        lll_futex_wait (futex, newval, private);
>  
>      try:
> @@ -100,15 +103,17 @@ __lll_robust_timedlock_wait (int *futex, const struct timespec *abstime,
>  	return ETIMEDOUT;
>  #endif
>  
> -      /* Wait.  */
> +      /* If the owner died, return the present value of the futex.  */
>        if (__glibc_unlikely (oldval & FUTEX_OWNER_DIED))
>  	return oldval;
>  
> +      /* Attempt to put lock into state 'acquired, possibly with waiters'.  */

Likewise.

>        int newval = oldval | FUTEX_WAITERS;
>        if (oldval != newval
>  	  && atomic_compare_and_exchange_bool_acq (futex, newval, oldval))
>  	continue;
>  
> +      /* If *futex == 2, wait until woken or timeout.  */
>  #if (!defined __ASSUME_FUTEX_CLOCK_REALTIME \
>       || !defined lll_futex_timed_wait_bitset)
>        lll_futex_timed_wait (futex, newval, &rt, private);
> diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h
> index 28f4ba3..8f6e270 100644
> --- a/sysdeps/nptl/lowlevellock.h
> +++ b/sysdeps/nptl/lowlevellock.h
> @@ -22,9 +22,53 @@
>  #include <atomic.h>
>  #include <lowlevellock-futex.h>
>  
> +/* Low-level locks use a combination of atomic operations (to acquire and
> +   release lock ownership) and futex operations (to block until the state
> +   of a lock changes).  A lock can be in one of three states:
> +   0:  not acquired,
> +   1:  acquired with no waiters; no other threads are blocked or about to block
> +       for changes to the lock state,
> +   >1: acquired, possibly with waiters; there may be other threads blocked or
> +       about to block for changes to the lock state.
> +
> +   We expect that the common case is an uncontended lock, so we just need
> +   to transition the lock between states 0 and 1; releasing the lock does
> +   not need to wake any other blocked threads.  If the lock is contended
> +   and a thread decides to block using a futex operation, then this thread
> +   needs to first change the state to >1; if this state is observed during
> +   lock release, the releasing thread will wake one of the potentially
> +   blocked threads.
> +
> +   Much of this code takes a 'private' parameter.  This may be:
> +   LLL_PRIVATE: lock only shared within a process
> +   LLL_SHARED:  lock may be shared across processes.
> +
> +   Condition variables contain an optimization for broadcasts that requeues
> +   waiting threads on a lock's futex.  Therefore, there is a special
> +   variant of the locks (whose name contains "cond") that makes sure to
> +   always set the lock state to >1 and not just 1.
> +
> +   Robust locks set the lock to the id of the owner.  This allows detection
> +   of the case where the owner exits without releasing the lock.  Flags are
> +   OR'd with the owner id to record additional information about lock state.
> +   Therefore the states of robust locks are:
> +    0: not acquired
> +   id: acquired (by user identified by id & FUTEX_TID_MASK)
> +
> +   The following flags may be set in the robust lock value:
> +   FUTEX_WAITERS     - possibly has waiters
> +   FUTEX_OWNER_DIED  - owning user has exited without releasing the futex.  */
> +
> +
> +/* If LOCK is 0 (not acquired), set to 1 (acquired) and return 0.  Otherwise
> +   leave lock unchanged and return non-zero to indicate that the lock was not
> +   acquired.  */

At a later time, we should be more precise regarding the semantics of
trylock.  See www.open-std.org/jtc1/sc22/WG14/www/docs/n1882.htm for a
related C11 issue.

>  #define lll_trylock(lock)	\
>    atomic_compare_and_exchange_bool_acq (&(lock), 1, 0)
>  
> +/* If LOCK is 0 (not acquired), set to 2 (acquired, possibly with waiters) and
> +   return 0.  Otherwise leave lock unchanged and return non-zero to indicate
> +   that the lock was not acquired.  */
>  #define lll_cond_trylock(lock)	\
>    atomic_compare_and_exchange_bool_acq (&(lock), 2, 0)
>  
> @@ -35,6 +79,13 @@ extern int __lll_robust_lock_wait (int *futex, int private) attribute_hidden;
>  /* This is an expression rather than a statement even though its value is
>     void, so that it can be used in a comma expression or as an expression
>     that's cast to void.  */
> +/* The inner conditional compiles to a call to __lll_lock_wait_private if
> +   private is known at compile time to be LLL_PRIVATE, and to a call to
> +   __lll_lock_wait otherwise.  */
> +/* If FUTEX is 0 (not acquired), set to 1 (acquired with no waiters) and
> +   return.  Otherwise block until we acquire the lock, at which point FUTEX is
> +   2 (acquired, possibly with waiters), then return.  The lock is aways
> +   acquired on return.  */

I'd rearrange the second sentence a little, making sure that we change
to 2 before blocking.

s/aways/always/

>  #define __lll_lock(futex, private)                                      \
>    ((void)                                                               \
>     ({                                                                   \
> @@ -52,6 +103,14 @@ extern int __lll_robust_lock_wait (int *futex, int private) attribute_hidden;
>    __lll_lock (&(futex), private)
>  
> 
> +/* If FUTEX is 0 (not acquired), set to ID (acquired with no waiters) and
> +   return 0.  Otherwise, block until either:
> +   1) We acquire the lock, at which point FUTEX will be ID | FUTEX_WAITERS
> +      (acquired, possibly with waiters), then return 0.
> +   2) The previous owner of the lock dies.  FUTEX will be the value of id as
> +      set by the previous owner, with FUTEX_OWNER_DIED set (FUTEX_WAITERS may
> +      also be set).  Return this value to indicate that the lock is not
> +      acquired.  */
>  #define __lll_robust_lock(futex, id, private)                           \
>    ({                                                                    \
>      int *__futex = (futex);                                             \
> @@ -69,6 +128,12 @@ extern int __lll_robust_lock_wait (int *futex, int private) attribute_hidden;
>  /* This is an expression rather than a statement even though its value is
>     void, so that it can be used in a comma expression or as an expression
>     that's cast to void.  */
> +/* Unconditionally set FUTEX to 2 (acquired, possibly with waiters).  condvar
> +   locks only have 'not acquired' and 'acquired, possibly with waiters' states,
> +   so there is no need to check FUTEX before setting.

I'm not sure whether there are just two states, necessarily.  Rather, we
need 'acquired, possibly with waiters'.  Generally, setting 'acquired,
possibly with waiters' instead of just 'acquired' is always allowed.

> @@ -121,6 +197,9 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
>  /* This is an expression rather than a statement even though its value is
>     void, so that it can be used in a comma expression or as an expression
>     that's cast to void.  */
> +/* Unconditionally set FUTEX to 0 (not acquired), releasing the lock.  If FUTEX
> +   was >1 (acquired, possibly with waiters), then wake any waiters.  The waiter
> +   who acquires the lock will set FUTEX to >1.  */

s/who/that/



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]