Re: [PATCH] mutex: do not unnecessarily deal with waiters

[Davidlohr Bueso]

On Thu, 2013-06-27 at 11:00 +0200, Ingo Molnar wrote:
[...]
> 
> So I tried this out yesterday, but it interacted with the Wait/Wound 
> patches in tip:core/mutexes.
> 
> Maarten Lankhorst pointed out that if this patch is applied on top of the 
> WW patches as-is, then we get this semantic merge conflict:
> 
> > > @@ -340,6 +339,14 @@ slowpath:
> > >  #endif
> > >     spin_lock_mutex(&lock->wait_lock, flags);
> > >  
> > > +   /* once more, can we acquire the lock? */
> > > +   if (MUTEX_SHOW_NO_WAITER(lock) && (atomic_xchg(&lock->count, 0) == 1)) {
> > > +           lock_acquired(&lock->dep_map, ip);
> > > +           mutex_set_owner(lock);
> > > +           spin_unlock_mutex(&lock->wait_lock, flags);
> > > +           goto done;
> > > +   }
> > >
> > ^^^^^^^^^^^^^^
> >
> > This part skips the whole if (!__builtin_constant_p(ww_ctx == NULL)) { 
> > section with the wait_lock held.

I see what you mean, I hadn't really looked at the W/W patches. BTW
those __builtin_constant_p() calls are pretty ugly/annoying to read,
plus why the negation of the NULL check? Couldn't we just do something
like:

#define is_ww_ctx(x) (__builtin_constant_p(x))
...
if (is_ww_ctxt(ww_ctx)) { ... }


Anyway, so going back to the actual patch, we need a few cleanups in
__mutex_lock_common() before we can rebase this patch - otherwise we're
going to end up duplicating a lot of code (and the function is already
big enough):

How about a new ww_mutex_set_context_slowpath() function that does the
w/w lock acquiring and wakes up any sleeping processes. We'd use this
function whenever we acquire the lock in the slowpath (with the
->wait_lock taken):

static __always_inline void
ww_mutex_set_context_slowpath(struct mutex *lock,
			      struct ww_acquire_ctx *ww_ctx, bool debug)
{
	if (!__builtin_constant_p(ww_ctx == NULL)) {
		struct mutex_waiter *cur;
		struct ww_mutex *ww = container_of(lock, struct ww_mutex, base);

		/*
		 * This branch gets optimized out for the common case,
		 * and is only important for ww_mutex_lock.
		 */
		ww_mutex_lock_acquired(ww, ww_ctx);
		ww->ctx = ww_ctx;

		/*
		 * Give any possible sleeping processes the chance to wake up,
		 * so they can recheck if they have to back off.
		 */
		list_for_each_entry(cur, &lock->wait_list, list) {
			if (debug)
				debug_mutex_wake_waiter(lock, cur);
			wake_up_process(cur->task);
		}
	}
}

In ww_mutex_set_context_fastpath() I'm a little confused with the
debug_mutex_wake_waiter() calls since we don't deal with debug in the
fast path (->wait_lock isn't held). So are these calls
correct/necessary?

For ww_mutex_set_context_slowpath(), the 'debug' parameter would be
necessary since with this patch we avoid doing the debug_mutex on a
quick attempt to grab the lock, otherwise we do the slowpath debug,
waiters, etc. For instance:

...
slowpath:
#endif
	spin_lock_mutex(&lock->wait_lock, flags);
	/* once more, can we acquire the lock? */
	if (MUTEX_SHOW_NO_WAITER(lock) && (atomic_xchg(&lock->count, 0) == 1)) {
        	lock_acquired(&lock->dep_map, ip);
	        mutex_set_owner(lock);
		ww_mutex_set_context_fastpath(lock, ww_ctx, false);
        	spin_unlock_mutex(&lock->wait_lock, flags);
	        goto done;
  	}
...

lock_acquired(&lock->dep_map, ip);
/* got the lock - rejoice! */
mutex_remove_waiter(lock, &waiter, current_thread_info());
mutex_set_owner(lock);
ww_mutex_set_context_slowpath(lock, ww_ctx, true);
...


Thanks,
Davidlohr

--
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/