| Message ID | 1480601214-26583-3-git-send-email-nhaehnle@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 01, 2016 at 03:06:45PM +0100, Nicolai Hähnle wrote: > From: Nicolai Hähnle <Nicolai.Haehnle@amd.com> > > In the following scenario, thread #1 should back off its attempt to lock > ww1 and unlock ww2 (assuming the acquire context stamps are ordered > accordingly). > > Thread #0 Thread #1 > --------- --------- > successfully lock ww2 > set ww1->base.owner > attempt to lock ww1 > confirm ww1->ctx == NULL > enter mutex_spin_on_owner > set ww1->ctx > > What was likely to happen previously is: > > attempt to lock ww2 > refuse to spin because > ww2->ctx != NULL > schedule() > detect thread #0 is off CPU > stop optimistic spin > return -EDEADLK > unlock ww2 > wakeup thread #0 > lock ww2 > > Now, we are more likely to see: > > detect ww1->ctx != NULL > stop optimistic spin > return -EDEADLK > unlock ww2 > successfully lock ww2 > > ... because thread #1 will stop its optimistic spin as soon as possible. > > The whole scenario is quite unlikely, since it requires thread #1 to get > between thread #0 setting the owner and setting the ctx. But since we're > idling here anyway, the additional check is basically free. > > Found by inspection. Similar question can be raised for can_spin_on_owner() as well. Is it worth for a contending ww_mutex to enter the osq queue if we expect a EDEADLK? It seems to boil down to how likely is the EDEADLK going to evaporate if we wait for the owner to finish and unlock. The patch looks reasonable, just a question of desirability. -Chris
On Thu, Dec 01, 2016 at 03:06:45PM +0100, Nicolai Hähnle wrote: > +++ b/kernel/locking/mutex.c > @@ -350,7 +350,8 @@ ww_mutex_set_context_slowpath(struct ww_mutex *lock, > * access and not reliable. > */ > static noinline > -bool mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner) > +bool mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner, > + bool use_ww_ctx, struct ww_acquire_ctx *ww_ctx) > { > bool ret = true; > > @@ -373,6 +374,28 @@ bool mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner) > break; > } > > + if (use_ww_ctx && ww_ctx->acquired > 0) { > + struct ww_mutex *ww; > + > + ww = container_of(lock, struct ww_mutex, base); > + > + /* > + * If ww->ctx is set the contents are undefined, only > + * by acquiring wait_lock there is a guarantee that > + * they are not invalid when reading. > + * > + * As such, when deadlock detection needs to be > + * performed the optimistic spinning cannot be done. > + * > + * Check this in every inner iteration because we may > + * be racing against another thread's ww_mutex_lock. > + */ > + if (READ_ONCE(ww->ctx)) { > + ret = false; > + break; > + } > + } > + > cpu_relax(); > } > rcu_read_unlock(); Aside from the valid question about mutex_can_spin_on_owner() there's another 'problem' here, mutex_spin_on_owner() is marked noinline, so all the use_ww_ctx stuff doesn't 'work' here. As is, I think we're missing an __always_inline on mutex_optimistic_spin, I'll have to go see what that does for code generation, but given both @use_ww_ctx and @waiter there that makes sense.
On Tue, Dec 06, 2016 at 11:03:28AM -0500, Waiman Long wrote: > > The mutex_spin_on_owner() function was originally marked noinline > because it could be a major consumer of CPU cycles in a contended lock. > Having it shown separately in the perf output will help the users have a > better understanding of what is consuming all the CPU cycles. So I would > still like to keep it this way. ah!, I tried to dig through history but couldn't find a reason for it. > > If you have concern about additional latency for non-ww_mutex calls, one > alternative can be: That's pretty horrific :/
On 12/06/2016 01:29 PM, Peter Zijlstra wrote: > On Tue, Dec 06, 2016 at 11:03:28AM -0500, Waiman Long wrote: >> The mutex_spin_on_owner() function was originally marked noinline >> because it could be a major consumer of CPU cycles in a contended lock. >> Having it shown separately in the perf output will help the users have a >> better understanding of what is consuming all the CPU cycles. So I would >> still like to keep it this way. > ah!, I tried to dig through history but couldn't find a reason for it. > >> If you have concern about additional latency for non-ww_mutex calls, one >> alternative can be: > That's pretty horrific :/ I am not totally against making mutex_spin_on_owner() an inline function. If you think it is the right way to go, I am OK with that. -Longman
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c index 9b34961..0afa998 100644 --- a/kernel/locking/mutex.c +++ b/kernel/locking/mutex.c @@ -350,7 +350,8 @@ ww_mutex_set_context_slowpath(struct ww_mutex *lock, * access and not reliable. */ static noinline -bool mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner) +bool mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner, + bool use_ww_ctx, struct ww_acquire_ctx *ww_ctx) { bool ret = true; @@ -373,6 +374,28 @@ bool mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner) break; } + if (use_ww_ctx && ww_ctx->acquired > 0) { + struct ww_mutex *ww; + + ww = container_of(lock, struct ww_mutex, base); + + /* + * If ww->ctx is set the contents are undefined, only + * by acquiring wait_lock there is a guarantee that + * they are not invalid when reading. + * + * As such, when deadlock detection needs to be + * performed the optimistic spinning cannot be done. + * + * Check this in every inner iteration because we may + * be racing against another thread's ww_mutex_lock. + */ + if (READ_ONCE(ww->ctx)) { + ret = false; + break; + } + } + cpu_relax(); } rcu_read_unlock(); @@ -460,22 +483,6 @@ static bool mutex_optimistic_spin(struct mutex *lock, for (;;) { struct task_struct *owner; - if (use_ww_ctx && ww_ctx->acquired > 0) { - struct ww_mutex *ww; - - ww = container_of(lock, struct ww_mutex, base); - /* - * If ww->ctx is set the contents are undefined, only - * by acquiring wait_lock there is a guarantee that - * they are not invalid when reading. - * - * As such, when deadlock detection needs to be - * performed the optimistic spinning cannot be done. - */ - if (READ_ONCE(ww->ctx)) - goto fail_unlock; - } - /* * If there's an owner, wait for it to either * release the lock or go to sleep. @@ -487,7 +494,8 @@ static bool mutex_optimistic_spin(struct mutex *lock, break; } - if (!mutex_spin_on_owner(lock, owner)) + if (!mutex_spin_on_owner(lock, owner, use_ww_ctx, + ww_ctx)) goto fail_unlock; }