linux-kernel - Re: [RFC PATCH 1/3] locking/mutex: Try to acquire mutex only if it is unlocked

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 11 Jun 2014 21:25:04 -0400
From:	"Long, Wai Man" <waiman.long@...com>
To:	Jason Low <jason.low2@...com>
CC:	Davidlohr Bueso <davidlohr@...com>,
	Peter Zijlstra <peterz@...radead.org>, mingo@...nel.org,
	tglx@...utronix.de, akpm@...ux-foundation.org,
	linux-kernel@...r.kernel.org, paulmck@...ux.vnet.ibm.com,
	tim.c.chen@...ux.intel.com, hpa@...or.com, aswin@...com,
	scott.norton@...com, chegu_vinod@...com
Subject: Re: [RFC PATCH 1/3] locking/mutex: Try to acquire mutex only if it
 is unlocked


On 6/11/2014 5:48 PM, Jason Low wrote:
> On Wed, 2014-06-11 at 17:00 -0400, Long, Wai Man wrote:
>> On 6/9/2014 1:38 PM, Jason Low wrote:
>>> On Wed, 2014-06-04 at 13:58 -0700, Davidlohr Bueso wrote:
>>>> On Wed, 2014-06-04 at 13:57 -0700, Davidlohr Bueso wrote:
>>>>> In addition, how about the following helpers instead:
>>>>> - mutex_is_unlocked() : count > 0
>>>>> - mutex_has_waiters() : count < 0, or list_empty(->wait_list)
>>>>                                         ^ err, that's !list_empty()
>>> Between checking for (count < 0) or checking for !list_empty(wait_list)
>>> for waiters:
>>>
>>> Now that I think about it, I would expect a mutex_has_waiters() function
>>> to return !list_empty(wait_list) as that really tells whether or not
>>> there are waiters. For example, in highly contended cases, there can
>>> still be waiters on the mutex if count is 1.
>>>
>>> Likewise, in places where we currently use "MUTEX_SHOW_NO_WAITER", we
>>> need to check for (count < 0) to ensure lock->count is a negative value
>>> before the thread sleeps on the mutex.
>>>
>>> One option would be to still remove MUTEX_SHOW_NO_WAITER(), directly use
>>> atomic_read() in place of the macro, and just comment on why we have an
>>> extra atomic_read() that may "appear redundant". Another option could be
>>> to provide a function that checks for "potential waiters" on the mutex.
>>>
>>> Any thoughts?
>>>
>> For the first MUTEX_SHOW_NO_WAITER() call site, you can replace it with
>> a check for (count > 0).
> Yup, in my v2 patch, the first call site becomes !mutex_is_locked(lock)
> which is really a check for (count == 1).

Yes, your v2 patch looks fine to me.

>> The second call site within the for loop,
>> however, is a bit more tricky. It has to serve 2 purposes:
>>
>> 1. Opportunistically get the lock
>> 2. Set the count value to -1 to indicate someone is waiting on the lock,
>> that is why an xchg() operation has to be done even if its value is 0.
>>
>> I do agree that the naming isn't that good. Maybe it can be changed to
>> something like
>>
>> static inline int mutex_value_has_waiters(mutex *lock)    { return
>> lock->count < 0; }
> So I can imagine that a mutex_value_has_waiters() function might still
> not be a great name, since the mutex can have waiters in the case that
> the value lock->count >= 0.
>
> In the second call site, do you think we should just do a direct
> atomic_read(lock->count) >= 0 and comment that we only do the xchg if
> the count is non-negative to avoid unnecessary xchg? That what I did in
> my v2 patch.

I think that is a good idea to avoid any controversy in naming.

-Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/