On Tue, Jan 17, 2017 at 03:56:54PM +0100, Peter Zijlstra wrote:
> On Tue, Jan 17, 2017 at 03:55:32PM +0100, Peter Zijlstra wrote:
> > On Sun, Jan 15, 2017 at 06:24:08AM +0100, Mike Galbraith wrote:
> > > git blames locking/ww_mutex: Optimize ww-mutexes by waking at most one waiter for backoff when acquiring the lock
> >
> > > [ 0.000000] ------------[ cut here ]------------
> > > [ 0.000000] WARNING: CPU: 0 PID: 0 at kernel/locking/mutex.c:305 __ww_mutex_wakeup_for_backoff+0x8d/0xb0
> >
> > > [ 0.000000] Hardware name: MEDION MS-7848/MS-7848, BIOS M7848W08.20C 09/23/2013
> > > [ 0.000000] Call Trace:
> > > [ 0.000000] warn_slowpath_null+0x1d/0x20
> > > [ 0.000000] __ww_mutex_wakeup_for_backoff+0x8d/0xb0
> > > [ 0.000000] __ww_mutex_lock.constprop.10+0x3ae/0x13d0
> > > [ 0.000000] ww_mutex_lock+0x42/0x70
> > > [ 0.000000] ww_test_edeadlk_normal+0x1e9/0x220
> >
> > OK, I can reproduce. This looks like the lockdep_assert_held() I added
> > to replace a comment... Just not quite sure how we can get here without
> > actually holding that lock.
> >
> > /me goes poke at it.
>
> Oh duh, MUTEX_DEBUG uses arch_spin_lock() for wait_lock... *groan*.
Ingo, how about something like so? Lockdep should be able to tell us
about the interrupt cruft real quick, I don't see why we should
hand-craft this anymore.
---
kernel/locking/mutex-debug.h | 17 -----------------
kernel/locking/mutex.c | 25 +++++++++++--------------
kernel/locking/mutex.h | 4 ----
3 files changed, 11 insertions(+), 35 deletions(-)
diff --git a/kernel/locking/mutex-debug.h b/kernel/locking/mutex-debug.h
index a459faa48987..4174417d5309 100644
--- a/kernel/locking/mutex-debug.h
+++ b/kernel/locking/mutex-debug.h
@@ -26,20 +26,3 @@ extern void mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
extern void debug_mutex_unlock(struct mutex *lock);
extern void debug_mutex_init(struct mutex *lock, const char *name,
struct lock_class_key *key);
-
-#define spin_lock_mutex(lock, flags) \
- do { \
- struct mutex *l = container_of(lock, struct mutex, wait_lock); \
- \
- DEBUG_LOCKS_WARN_ON(in_interrupt()); \
- local_irq_save(flags); \
- arch_spin_lock(&(lock)->rlock.raw_lock);\
- DEBUG_LOCKS_WARN_ON(l->magic != l); \
- } while (0)
-
-#define spin_unlock_mutex(lock, flags) \
- do { \
- arch_spin_unlock(&(lock)->rlock.raw_lock); \
- local_irq_restore(flags); \
- preempt_check_resched(); \
- } while (0)
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index 0ec2ddc0ca0d..ad2d9e22697b 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -325,8 +325,6 @@ void __sched mutex_lock(struct mutex *lock)
static __always_inline void
ww_mutex_set_context_fastpath(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
{
- unsigned long flags;
-
ww_mutex_lock_acquired(lock, ctx);
lock->ctx = ctx;
@@ -350,9 +348,9 @@ void __sched mutex_lock(struct mutex *lock)
* Uh oh, we raced in fastpath, wake up everyone in this case,
* so they can see the new lock->ctx.
*/
- spin_lock_mutex(&lock->base.wait_lock, flags);
+ spin_lock(&lock->base.wait_lock);
__ww_mutex_wakeup_for_backoff(&lock->base, ctx);
- spin_unlock_mutex(&lock->base.wait_lock, flags);
+ spin_unlock(&lock->base.wait_lock);
}
/*
@@ -740,7 +738,6 @@ void __sched ww_mutex_unlock(struct ww_mutex *lock)
struct ww_acquire_ctx *ww_ctx, const bool use_ww_ctx)
{
struct mutex_waiter waiter;
- unsigned long flags;
bool first = false;
struct ww_mutex *ww;
int ret;
@@ -766,7 +763,7 @@ void __sched ww_mutex_unlock(struct ww_mutex *lock)
return 0;
}
- spin_lock_mutex(&lock->wait_lock, flags);
+ spin_lock(&lock->wait_lock);
/*
* After waiting to acquire the wait_lock, try again.
*/
@@ -830,7 +827,7 @@ void __sched ww_mutex_unlock(struct ww_mutex *lock)
goto err;
}
- spin_unlock_mutex(&lock->wait_lock, flags);
+ spin_unlock(&lock->wait_lock);
schedule_preempt_disabled();
/*
@@ -853,9 +850,9 @@ void __sched ww_mutex_unlock(struct ww_mutex *lock)
(first && mutex_optimistic_spin(lock, ww_ctx, use_ww_ctx, &waiter)))
break;
- spin_lock_mutex(&lock->wait_lock, flags);
+ spin_lock(&lock->wait_lock);
}
- spin_lock_mutex(&lock->wait_lock, flags);
+ spin_lock(&lock->wait_lock);
acquired:
__set_current_state(TASK_RUNNING);
@@ -872,7 +869,7 @@ void __sched ww_mutex_unlock(struct ww_mutex *lock)
if (use_ww_ctx && ww_ctx)
ww_mutex_set_context_slowpath(ww, ww_ctx);
- spin_unlock_mutex(&lock->wait_lock, flags);
+ spin_unlock(&lock->wait_lock);
preempt_enable();
return 0;
@@ -880,7 +877,7 @@ void __sched ww_mutex_unlock(struct ww_mutex *lock)
__set_current_state(TASK_RUNNING);
mutex_remove_waiter(lock, &waiter, current);
err_early_backoff:
- spin_unlock_mutex(&lock->wait_lock, flags);
+ spin_unlock(&lock->wait_lock);
debug_mutex_free_waiter(&waiter);
mutex_release(&lock->dep_map, 1, ip);
preempt_enable();
@@ -1013,8 +1010,8 @@ void __sched ww_mutex_unlock(struct ww_mutex *lock)
static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock, unsigned long ip)
{
struct task_struct *next = NULL;
- unsigned long owner, flags;
DEFINE_WAKE_Q(wake_q);
+ unsigned long owner;
mutex_release(&lock->dep_map, 1, ip);
@@ -1049,7 +1046,7 @@ static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock, unsigne
owner = old;
}
- spin_lock_mutex(&lock->wait_lock, flags);
+ spin_lock(&lock->wait_lock);
debug_mutex_unlock(lock);
if (!list_empty(&lock->wait_list)) {
/* get the first entry from the wait-list: */
@@ -1066,7 +1063,7 @@ static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock, unsigne
if (owner & MUTEX_FLAG_HANDOFF)
__mutex_handoff(lock, next);
- spin_unlock_mutex(&lock->wait_lock, flags);
+ spin_unlock(&lock->wait_lock);
wake_up_q(&wake_q);
}
diff --git a/kernel/locking/mutex.h b/kernel/locking/mutex.h
index 4410a4af42a3..6ebc1902f779 100644
--- a/kernel/locking/mutex.h
+++ b/kernel/locking/mutex.h
@@ -9,10 +9,6 @@
* !CONFIG_DEBUG_MUTEXES case. Most of them are NOPs:
*/
-#define spin_lock_mutex(lock, flags) \
- do { spin_lock(lock); (void)(flags); } while (0)
-#define spin_unlock_mutex(lock, flags) \
- do { spin_unlock(lock); (void)(flags); } while (0)
#define mutex_remove_waiter(lock, waiter, task) \
__list_del((waiter)->list.prev, (waiter)->list.next)
--
To unsubscribe from this list: send the line "unsubscribe linux-tip-commits" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
|