[v3] lowlevellock comments

Thanks. I fixed all the easy typos. A few more comments inline below:

On 9 December 2014 at 17:50, Torvald Riegel <triegel@redhat.com> wrote:
> On Thu, 2014-10-23 at 13:12 +0100, Bernard Ogden wrote:
>> 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.

Also changed 'Again' to 'Try', just to make the comment fit on one line.

>> +/* 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.

Interesting - thanks for sharing.

(I updated the comment here as I noticed I'd just written (acquired) rather
than (acquired with no waiters) like I have everywhere else - but that's not
related to your point.)

>> +/* 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.

Changed to:

/* If FUTEX is 0 (not acquired), set to 1 (acquired with no waiters) and
   return.  Otherwise, ensure that it is >1 (acquired, possibly with waiters)
   and then block until we acquire the lock, at which point FUTEX will still be
   >1.  The lock is always acquired on return.  */

Note that I switched '2' to '>1' for consistency - generally I've said '>1'
where I can't see the value being set to '2' within lowlevellock.h itself.

This also prompted me to change the comment for __lll_robust_lock to indicate
a similar point about the FUTEX_WAITERS bit:

/* If FUTEX is 0 (not acquired), set to ID (acquired with no waiters) and
   return 0.  Otherwise, ensure that it is set to FUTEX | FUTEX_WAITERS
   (acquired, possibly with waiters) and block until we acquire the lock.
   FUTEX will now be ID | FUTEX_WAITERS and we return 0.
   If the previous owner of the lock dies before we acquire the lock then FUTEX
   will be the value of id as set by the previous owner, with FUTEX_OWNER_DIED
   set (FUTEX_WAITERS may or may not be set).  We return this value to indicate
   that the lock is not acquired.  */

And then I changed the comment for __lll_robust_cond_lock to point at the
__lll_robust_lock comment:

/* As __lll_robust_lock, but set to ID | FUTEX_WAITERS (acquired, possibly with
   waiters) if FUTEX is 0.  */

>> +/* 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.

I considered replacing this with some text about how it's generally allowable
to use '2' rather than '1', but I'd already been much more terse for
lll_cond_trylock so just removed the offending sentence.

OK now?

2014-12-10  Bernard Ogden  <bernie.ogden@linaro.org>

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

---

[v3] lowlevellock comments

Commit Message

Comments

Patch