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

From: Long, Wai Man
Date: Wed Jun 11 2014 - 21:25:18 EST



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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/