lowlevellock comments

This patch adds many (too many/too remedial?) comments to
lowlevellock.h, and a few comments to a couple of other files.

Needs review for both correctness and usefulness.

Thanks in advance.

2014-09-26  Bernard Ogden  <bernie.ogden@linaro.org>

	* nptl/lowlevellock.c: Add comments.
	* nptl/lowlevelrobustlock.c: Likewise.
	* sysdeps/nptl/lowlevellock.h: Likewise.

---

On 09/26/2014 07:55 AM, Bernard Ogden wrote:
> This patch adds many (too many/too remedial?) comments to
> lowlevellock.h, and a few comments to a couple of other files.

The more comments the better IMO.

I trust that you have made sure none of the lines are longer
than 80 characters.

> Needs review for both correctness and usefulness.
> 
> Thanks in advance.
> 
> 2014-09-26  Bernard Ogden  <bernie.ogden@linaro.org>
> 
> 	* nptl/lowlevellock.c: Add comments.
> 	* nptl/lowlevelrobustlock.c: Likewise.
> 	* sysdeps/nptl/lowlevellock.h: Likewise.

Looking like a good start.

Your concepts and ideas are good.

The details of the comments are good, I saw one spot
where I would change the comments.

The comments themselves need a little more polish and
full sentences.

Otherwise looking ood.

Please do a version 2.

> ---
> 
> diff --git a/nptl/lowlevellock.c b/nptl/lowlevellock.c
> index e198af7..9177af3 100644
> --- a/nptl/lowlevellock.c
> +++ b/nptl/lowlevellock.c
> @@ -27,10 +27,10 @@ void
>  __lll_lock_wait_private (int *futex)
>  {
>    if (*futex == 2)
> -    lll_futex_wait (futex, 2, LLL_PRIVATE);
> +    lll_futex_wait (futex, 2, LLL_PRIVATE); /* Wait if *futex == 2.  */
>  
>    while (atomic_exchange_acq (futex, 2) != 0)
> -    lll_futex_wait (futex, 2, LLL_PRIVATE);
> +    lll_futex_wait (futex, 2, LLL_PRIVATE); /* Wait if *futex == 2.  */

OK.

>  }
>  
>  
> @@ -40,10 +40,10 @@ void
>  __lll_lock_wait (int *futex, int private)
>  {
>    if (*futex == 2)
> -    lll_futex_wait (futex, 2, private);
> +    lll_futex_wait (futex, 2, private); /* Wait if *futex == 2.  */
>  
>    while (atomic_exchange_acq (futex, 2) != 0)
> -    lll_futex_wait (futex, 2, private);
> +    lll_futex_wait (futex, 2, private); /* Wait if *futex == 2.  */

OK.

>  }
>  
>  
> @@ -75,7 +75,7 @@ __lll_timedlock_wait (int *futex, const struct timespec *abstime, int private)
>        if (rt.tv_sec < 0)
>  	return ETIMEDOUT;
>  
> -      /* Wait.  */
> +      /* If *futex == 2, wait until woken or timeout.  */
>        lll_futex_timed_wait (futex, 2, &rt, private);

OK.

>      }
>  
> @@ -113,8 +113,8 @@ __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.  The kernel so far

Shouldn't this be "until thread terminates or the wait times out."?

Please verify the kernel does not use the private futex operations for this,
and when complete come back and update the comment to say:

"As of kernel X.Y.Z, the private futex operation is not used."

> +         does not use the private futex operations for this.  */
>        if (lll_futex_timed_wait (tidp, tid, &rt, LLL_SHARED) == -ETIMEDOUT)
>  	return ETIMEDOUT;
>      }
> diff --git a/nptl/lowlevelrobustlock.c b/nptl/lowlevelrobustlock.c
> index 3525807..60e80d0 100644
> --- a/nptl/lowlevelrobustlock.c
> +++ b/nptl/lowlevelrobustlock.c
> @@ -36,14 +36,17 @@ __lll_robust_lock_wait (int *futex, int private)
>  
>    do
>      {
> +      /* If owner died, return the present value of the futex.  */
>        if (__glibc_unlikely (oldval & FUTEX_OWNER_DIED))

OK.

>  	return oldval;
>  
> +      /* Put lock into contended state.  */

Suggest: "Attempt to put lock into contended state."

>        int newval = oldval | FUTEX_WAITERS;
>        if (oldval != newval
>  	  && atomic_compare_and_exchange_bool_acq (futex, newval, oldval))
>  	continue;
>  
> +      /* If *futex == 2, wait until woken.  */

OK.

>        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 owner died, return the present value of the futex.  */
>        if (__glibc_unlikely (oldval & FUTEX_OWNER_DIED))
>  	return oldval;

OK.

>  
> +      /* Put lock into contended state.  */

Suggest: "Attempt to put lock into contended state."

>        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);

OK.

> diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h
> index 28f4ba3..0731e5c 100644
> --- a/sysdeps/nptl/lowlevellock.h
> +++ b/sysdeps/nptl/lowlevellock.h
> @@ -22,9 +22,38 @@
>  #include <atomic.h>
>  #include <lowlevellock-futex.h>
>  
> +/* Uses futexes to avoid unneccessary calls into the kernel. Likely paths are

unneccessary => unnecessary, spell check please.

> +   the unc	ontended cases and hence do not involve syscalls. Unlikely
> +   (contended case) paths usually will involve syscalls to wait or to wake.
> +
> +   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.  */
> +
> +/* The states of locks are:
> +    0  -  untaken
> +    1  -  taken (by one user)
> +   >1  -  contended (taken by 1 user, there may be other users waiting)

I don't think this is true, I think 2 is explicitly "locked, one or more waiters"

Suggest:

0 - unlocked.
1 - locked, no waiters.
2 - locked, one or more waiters.

> +
> +   cond_locks are never in the taken state, they are either untaken or
> +   contended.

Please use locked, or unlocked terms everywhere.

> +
> +   The states of robust locks are:
> +    0  - untaken
> +   id  - taken (by user identified by id & FUTEX_TID_MASK)
> +
> +   The following flags may be set in the robust lock value:
> +   FUTEX_WAITERS     - contended (taken by one user, there may be waiters)
> +   FUTEX_OWNER_DIED  - owning user has exited without releasing the futex.  */
> +
> +

OK.

> +/* If lock is 0 (untaken), set to 1 (taken). Return 0. Lock acquired.
> +   Otherwise lock is unchanged. Return nonzero. Lock not acquired.  */

Use GNU style variable name and value usage i.e. `lock` is the variable name,
while `LOCK` is the value of `lock`. So you can say "If the value of lock is..."
or "If LOCK is...".

Suggest:

If LOCK is 0 (unlocked), set to 1 (locked) and return 0 to indicate the
lock was acquired, otherwise the lock is unchanged and return non-zero
to indicate the lock was not acquired.

>  #define lll_trylock(lock)	\
>    atomic_compare_and_exchange_bool_acq (&(lock), 1, 0)
>  
> +/* If lock is 0 (untaken), set to 2 (contended). Return 0. Lock acquired.
> +   Otherwise lock is unchanged. Return nonzero. Lock not acquired.  */

Please rewrite into full sentences.

>  #define lll_cond_trylock(lock)	\
>    atomic_compare_and_exchange_bool_acq (&(lock), 2, 0)
>  
> @@ -35,6 +64,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.  */
> +/* 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.  */

OK.

> +/* If futex is 0 (untaken), set to 1 (taken). Lock acquired.
> +   Otherwise, block until lock is acquired. After blocking, futex is 2
> +   (contended).  */

Please rewrite into full sentences.

>  #define __lll_lock(futex, private)                                      \
>    ((void)                                                               \
>     ({                                                                   \
> @@ -52,6 +87,11 @@ extern int __lll_robust_lock_wait (int *futex, int private) attribute_hidden;
>    __lll_lock (&(futex), private)
>  
>  
> +/* If futex is 0 (untaken), set to id (taken). Return 0. Lock acquired.
> +   Otherwise, block until either:
> +   1) Lock is acquired. Return 0. futex is id | FUTEX_WAITERS (contended).
> +   2) Previous owner of futex dies. Return value of futex, which is id of
> +      dead owner with FUTEX_OWNER_DIED set. Lock not acquired.  */

Please rewrite into full sentences.

>  #define __lll_robust_lock(futex, id, private)                           \
>    ({                                                                    \
>      int *__futex = (futex);                                             \
> @@ -69,6 +109,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 (contended). cond locks only have
> +   untaken and contended states, so there is no need to check the value of
> +   futex before setting.
> +   If previous value of futex was 0 (untaken), lock is acquired.
> +   Otherwise, block until lock is acquired. futex will still be 2 (contended)
> +   after acquisition.  */

Please rewrite into full sentences.

>  #define __lll_cond_lock(futex, private)                                 \
>    ((void)                                                               \
>     ({                                                                   \
> @@ -79,6 +125,12 @@ extern int __lll_robust_lock_wait (int *futex, int private) attribute_hidden;
>  #define lll_cond_lock(futex, private) __lll_cond_lock (&(futex), private)
>  
>  
> +/* If futex is 0 (untaken), set to id | FUTEX_WAITERS (contended). Lock
> +   acquired.
> +   Otherwise, block until either:
> +   1) Lock is acquired. Return 0. futex is id | FUTEX_WAITERS (contended).
> +   2) Previous owner of futex dies. Return the value of futex, which is id of
> +      dead owner with FUTEX_OWNER_DIED set. Lock not acquired.  */

Please rewrite into full sentences.

>  #define lll_robust_cond_lock(futex, id, private)	\
>    __lll_robust_lock (&(futex), (id) | FUTEX_WAITERS, private)
>  
> @@ -88,8 +140,7 @@ extern int __lll_timedlock_wait (int *futex, const struct timespec *,
>  extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
>  					int private) attribute_hidden;
>  
> -/* Take futex if it is untaken.
> -   Otherwise block until either we get the futex or abstime runs out.  */
> +/* As __lll_lock, but with a timeout.  */

Can you comment on the return value of the futex if it times out?

>  #define __lll_timedlock(futex, abstime, private)                \
>    ({                                                            \
>      int *__futex = (futex);                                     \
> @@ -104,6 +155,7 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
>    __lll_timedlock (&(futex), abstime, private)
>  
>  
> +/* As __lll_robust_lock, but with a timeout.  */

Similarly.

>  #define __lll_robust_timedlock(futex, abstime, id, private)             \
>    ({                                                                    \
>      int *__futex = (futex);                                             \
> @@ -121,6 +173,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 (untaken). Lock is released.
> +   If previous value of futex was > 1 (contended), wake any waiters. (Waiter
> +   that acquires the lock will set futex to >1 (contended).)  */

Please rewrite into full sentences.

>  #define __lll_unlock(futex, private)                    \
>    ((void)                                               \
>     ({                                                   \
> @@ -132,10 +187,12 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
>  #define lll_unlock(futex, private)	\
>    __lll_unlock (&(futex), private)
>  
> -
>  /* 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 (untaken). Lock is released.
> +   If previous value of futex had FUTEX_WAITERS set, wake any waiters. (Waiter
> +   that acquires the lock will set FUTEX_WAITERS.)  */

Please rewrite into full sentences.

>  #define __lll_robust_unlock(futex, private)             \
>    ((void)                                               \
>     ({                                                   \
> @@ -159,11 +216,6 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
>  #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
>     thread ID while the clone is running and is reset to zero
> @@ -178,6 +230,7 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
>  extern int __lll_timedwait_tid (int *, const struct timespec *)
>       attribute_hidden;
>  
> +/* As lll_wait_tid, but with a timeout.  */

OK.

>  #define lll_timedwait_tid(tid, abstime) \
>    ({							\
>      int __res = 0;					\
> 

Cheers,
Carlos.