* [PATCH] kernel/rtmutex: Handle non enqueued waiters gracefully in remove_waiter()
@ 2018-03-12 14:28 Sebastian Andrzej Siewior
2018-03-16 12:20 ` Peter Zijlstra
0 siblings, 1 reply; 5+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-03-12 14:28 UTC (permalink / raw)
To: linux-kernel; +Cc: Peter Zijlstra, Ingo Molnar, Thomas Gleixner
In -RT task_blocks_on_rt_mutex() may return with -EAGAIN due to
(->pi_blocked_on == PI_WAKEUP_INPROGRESS) before it added itself as a
waiter. In such a case we must not call remove_waiter() because without
a waiter it will trigger the BUG_ON() statement.
This was initially reported by Yimin Deng. Thomas Gleixner fixed it then
with an explicit check for waiters before calling remove_waiter() with
the following note:
| Guard it with rt_mutex_has_waiters(). This is a quick fix which is
| easy to backport. The proper fix is to have a central check in
| remove_waiter() so we can call it unconditionally.
This is the suggested change.
Now that it is possible to call remove_waiter() unconditionally I also
remove that check from rt_mutex_slowlock().
Link: https://lkml.kernel.org/r/CAAh1qt=DCL9aUXNxanP5BKtiPp3m+qj4yB+gDohhXPVFCxWwzg@mail.gmail.com
Reported-and-debugged-by: Yimin Deng <yimin11.deng@gmail.com>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/locking/rtmutex.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 65cc0cb984e6..57d28d8f5280 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1068,12 +1068,15 @@ static void mark_wakeup_next_waiter(struct wake_q_head *wake_q,
static void remove_waiter(struct rt_mutex *lock,
struct rt_mutex_waiter *waiter)
{
- bool is_top_waiter = (waiter == rt_mutex_top_waiter(lock));
+ bool is_top_waiter = false;
struct task_struct *owner = rt_mutex_owner(lock);
struct rt_mutex *next_lock;
lockdep_assert_held(&lock->wait_lock);
+ if (rt_mutex_has_waiters(lock))
+ is_top_waiter = waiter == rt_mutex_top_waiter(lock);
+
raw_spin_lock(¤t->pi_lock);
rt_mutex_dequeue(lock, waiter);
current->pi_blocked_on = NULL;
@@ -1268,8 +1271,7 @@ rt_mutex_slowlock(struct rt_mutex *lock, int state,
if (unlikely(ret)) {
__set_current_state(TASK_RUNNING);
- if (rt_mutex_has_waiters(lock))
- remove_waiter(lock, &waiter);
+ remove_waiter(lock, &waiter);
rt_mutex_handle_deadlock(ret, chwalk, &waiter);
}
--
2.16.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] kernel/rtmutex: Handle non enqueued waiters gracefully in remove_waiter()
2018-03-12 14:28 [PATCH] kernel/rtmutex: Handle non enqueued waiters gracefully in remove_waiter() Sebastian Andrzej Siewior
@ 2018-03-16 12:20 ` Peter Zijlstra
2018-03-19 15:11 ` Thomas Gleixner
0 siblings, 1 reply; 5+ messages in thread
From: Peter Zijlstra @ 2018-03-16 12:20 UTC (permalink / raw)
To: Sebastian Andrzej Siewior; +Cc: linux-kernel, Ingo Molnar, Thomas Gleixner
On Mon, Mar 12, 2018 at 03:28:45PM +0100, Sebastian Andrzej Siewior wrote:
> In -RT task_blocks_on_rt_mutex() may return with -EAGAIN due to
> (->pi_blocked_on == PI_WAKEUP_INPROGRESS) before it added itself as a
> waiter. In such a case we must not call remove_waiter() because without
> a waiter it will trigger the BUG_ON() statement.
>
> This was initially reported by Yimin Deng. Thomas Gleixner fixed it then
> with an explicit check for waiters before calling remove_waiter() with
> the following note:
>
> | Guard it with rt_mutex_has_waiters(). This is a quick fix which is
> | easy to backport. The proper fix is to have a central check in
> | remove_waiter() so we can call it unconditionally.
>
> This is the suggested change.
> Now that it is possible to call remove_waiter() unconditionally I also
> remove that check from rt_mutex_slowlock().
>
> Link: https://lkml.kernel.org/r/CAAh1qt=DCL9aUXNxanP5BKtiPp3m+qj4yB+gDohhXPVFCxWwzg@mail.gmail.com
> Reported-and-debugged-by: Yimin Deng <yimin11.deng@gmail.com>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> kernel/locking/rtmutex.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
> index 65cc0cb984e6..57d28d8f5280 100644
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -1068,12 +1068,15 @@ static void mark_wakeup_next_waiter(struct wake_q_head *wake_q,
> static void remove_waiter(struct rt_mutex *lock,
> struct rt_mutex_waiter *waiter)
> {
> - bool is_top_waiter = (waiter == rt_mutex_top_waiter(lock));
I'm a little confused, but isn't it easier to make rt_mutex_top_waiter()
return NULL if there aren't in fact any waiters?
diff --git a/kernel/locking/rtmutex_common.h b/kernel/locking/rtmutex_common.h
index 68686b3ec3c1..70bcafc385c4 100644
--- a/kernel/locking/rtmutex_common.h
+++ b/kernel/locking/rtmutex_common.h
@@ -52,11 +52,13 @@ static inline int rt_mutex_has_waiters(struct rt_mutex *lock)
static inline struct rt_mutex_waiter *
rt_mutex_top_waiter(struct rt_mutex *lock)
{
- struct rt_mutex_waiter *w;
+ struct rb_node *leftmost = rb_first_cached(&lock->waiters);
+ struct rt_mutex_waiter *w = NULL;
- w = rb_entry(lock->waiters.rb_leftmost,
- struct rt_mutex_waiter, tree_entry);
- BUG_ON(w->lock != lock);
+ if (leftmost) {
+ w = rb_entry(leftmost, struct rt_mutex_waiter, tree_entry);
+ BUG_ON(w->lock != lock);
+ }
return w;
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] kernel/rtmutex: Handle non enqueued waiters gracefully in remove_waiter()
2018-03-16 12:20 ` Peter Zijlstra
@ 2018-03-19 15:11 ` Thomas Gleixner
2018-03-27 12:14 ` [PATCH v2] locking/rtmutex: " Sebastian Andrzej Siewior
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Gleixner @ 2018-03-19 15:11 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Sebastian Andrzej Siewior, linux-kernel, Ingo Molnar
On Fri, 16 Mar 2018, Peter Zijlstra wrote:
> On Mon, Mar 12, 2018 at 03:28:45PM +0100, Sebastian Andrzej Siewior wrote:
> > In -RT task_blocks_on_rt_mutex() may return with -EAGAIN due to
> > (->pi_blocked_on == PI_WAKEUP_INPROGRESS) before it added itself as a
> > waiter. In such a case we must not call remove_waiter() because without
> > a waiter it will trigger the BUG_ON() statement.
> >
> > This was initially reported by Yimin Deng. Thomas Gleixner fixed it then
> > with an explicit check for waiters before calling remove_waiter() with
> > the following note:
> >
> > | Guard it with rt_mutex_has_waiters(). This is a quick fix which is
> > | easy to backport. The proper fix is to have a central check in
> > | remove_waiter() so we can call it unconditionally.
> >
> > This is the suggested change.
> > Now that it is possible to call remove_waiter() unconditionally I also
> > remove that check from rt_mutex_slowlock().
> >
> > Link: https://lkml.kernel.org/r/CAAh1qt=DCL9aUXNxanP5BKtiPp3m+qj4yB+gDohhXPVFCxWwzg@mail.gmail.com
> > Reported-and-debugged-by: Yimin Deng <yimin11.deng@gmail.com>
> > Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> > kernel/locking/rtmutex.c | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
> > index 65cc0cb984e6..57d28d8f5280 100644
> > --- a/kernel/locking/rtmutex.c
> > +++ b/kernel/locking/rtmutex.c
> > @@ -1068,12 +1068,15 @@ static void mark_wakeup_next_waiter(struct wake_q_head *wake_q,
> > static void remove_waiter(struct rt_mutex *lock,
> > struct rt_mutex_waiter *waiter)
> > {
> > - bool is_top_waiter = (waiter == rt_mutex_top_waiter(lock));
>
> I'm a little confused, but isn't it easier to make rt_mutex_top_waiter()
> return NULL if there aren't in fact any waiters?
That works as well.
Thanks,
tglx
> diff --git a/kernel/locking/rtmutex_common.h b/kernel/locking/rtmutex_common.h
> index 68686b3ec3c1..70bcafc385c4 100644
> --- a/kernel/locking/rtmutex_common.h
> +++ b/kernel/locking/rtmutex_common.h
> @@ -52,11 +52,13 @@ static inline int rt_mutex_has_waiters(struct rt_mutex *lock)
> static inline struct rt_mutex_waiter *
> rt_mutex_top_waiter(struct rt_mutex *lock)
> {
> - struct rt_mutex_waiter *w;
> + struct rb_node *leftmost = rb_first_cached(&lock->waiters);
> + struct rt_mutex_waiter *w = NULL;
>
> - w = rb_entry(lock->waiters.rb_leftmost,
> - struct rt_mutex_waiter, tree_entry);
> - BUG_ON(w->lock != lock);
> + if (leftmost) {
> + w = rb_entry(leftmost, struct rt_mutex_waiter, tree_entry);
> + BUG_ON(w->lock != lock);
> + }
>
> return w;
> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] locking/rtmutex: Handle non enqueued waiters gracefully in remove_waiter()
2018-03-19 15:11 ` Thomas Gleixner
@ 2018-03-27 12:14 ` Sebastian Andrzej Siewior
2018-03-28 21:07 ` [tip:locking/core] " tip-bot for Peter Zijlstra
0 siblings, 1 reply; 5+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-03-27 12:14 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: Peter Zijlstra, linux-kernel, Ingo Molnar
From: Peter Zijlstra <peterz@infradead.org>
In -RT task_blocks_on_rt_mutex() may return with -EAGAIN due to
(->pi_blocked_on == PI_WAKEUP_INPROGRESS) before it added itself as a
waiter. In such a case we must not call remove_waiter() because without
a waiter it will trigger the BUG_ON() statement.
This was initially reported by Yimin Deng. Thomas Gleixner fixed it then
with an explicit check for waiters before calling remove_waiter().
Instead of an explicit check before calling rt_mutex_top_waiter() we
could make it return NULL if there are no waiters.
Now that it is possible to call remove_waiter() unconditionally I also
remove that check from rt_mutex_slowlock().
Link: https://lkml.kernel.org/CAAh1qt=DCL9aUXNxanP5BKtiPp3m+qj4yB+gDohhXPVFCxWwzg@mail.gmail.com
Reported-and-debugged-by: Yimin Deng <yimin11.deng@gmail.com>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/locking/rtmutex.c | 3 +--
kernel/locking/rtmutex_common.h | 11 ++++++-----
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 65cc0cb984e6..355716d03b1a 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1268,8 +1268,7 @@ rt_mutex_slowlock(struct rt_mutex *lock, int state,
if (unlikely(ret)) {
__set_current_state(TASK_RUNNING);
- if (rt_mutex_has_waiters(lock))
- remove_waiter(lock, &waiter);
+ remove_waiter(lock, &waiter);
rt_mutex_handle_deadlock(ret, chwalk, &waiter);
}
diff --git a/kernel/locking/rtmutex_common.h b/kernel/locking/rtmutex_common.h
index 68686b3ec3c1..d1d62f942be2 100644
--- a/kernel/locking/rtmutex_common.h
+++ b/kernel/locking/rtmutex_common.h
@@ -52,12 +52,13 @@ static inline int rt_mutex_has_waiters(struct rt_mutex *lock)
static inline struct rt_mutex_waiter *
rt_mutex_top_waiter(struct rt_mutex *lock)
{
- struct rt_mutex_waiter *w;
-
- w = rb_entry(lock->waiters.rb_leftmost,
- struct rt_mutex_waiter, tree_entry);
- BUG_ON(w->lock != lock);
+ struct rb_node *leftmost = rb_first_cached(&lock->waiters);
+ struct rt_mutex_waiter *w = NULL;
+ if (leftmost) {
+ w = rb_entry(leftmost, struct rt_mutex_waiter, tree_entry);
+ BUG_ON(w->lock != lock);
+ }
return w;
}
--
2.16.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [tip:locking/core] locking/rtmutex: Handle non enqueued waiters gracefully in remove_waiter()
2018-03-27 12:14 ` [PATCH v2] locking/rtmutex: " Sebastian Andrzej Siewior
@ 2018-03-28 21:07 ` tip-bot for Peter Zijlstra
0 siblings, 0 replies; 5+ messages in thread
From: tip-bot for Peter Zijlstra @ 2018-03-28 21:07 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, yimin11.deng, mingo, peterz, tglx, hpa, bigeasy
Commit-ID: c28d62cf52d791ba5f6db7ce525ed06b86291c82
Gitweb: https://git.kernel.org/tip/c28d62cf52d791ba5f6db7ce525ed06b86291c82
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Tue, 27 Mar 2018 14:14:38 +0200
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 28 Mar 2018 23:01:30 +0200
locking/rtmutex: Handle non enqueued waiters gracefully in remove_waiter()
In -RT task_blocks_on_rt_mutex() may return with -EAGAIN due to
(->pi_blocked_on == PI_WAKEUP_INPROGRESS) before it added itself as a
waiter. In such a case remove_waiter() must not be called because without a
waiter it will trigger the BUG_ON() statement.
This was initially reported by Yimin Deng. Thomas Gleixner fixed it then
with an explicit check for waiters before calling remove_waiter().
Instead of an explicit NULL check before calling rt_mutex_top_waiter() make
the function return NULL if there are no waiters. With that fixed the now
pointless NULL check is removed from rt_mutex_slowlock().
Reported-and-debugged-by: Yimin Deng <yimin11.deng@gmail.com>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lkml.kernel.org/r/CAAh1qt=DCL9aUXNxanP5BKtiPp3m+qj4yB+gDohhXPVFCxWwzg@mail.gmail.com
Link: https://lkml.kernel.org/r/20180327121438.sss7hxg3crqy4ecd@linutronix.de
---
kernel/locking/rtmutex.c | 3 +--
kernel/locking/rtmutex_common.h | 11 ++++++-----
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 940633c63254..4f014be7a4b8 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1268,8 +1268,7 @@ rt_mutex_slowlock(struct rt_mutex *lock, int state,
if (unlikely(ret)) {
__set_current_state(TASK_RUNNING);
- if (rt_mutex_has_waiters(lock))
- remove_waiter(lock, &waiter);
+ remove_waiter(lock, &waiter);
rt_mutex_handle_deadlock(ret, chwalk, &waiter);
}
diff --git a/kernel/locking/rtmutex_common.h b/kernel/locking/rtmutex_common.h
index 68686b3ec3c1..d1d62f942be2 100644
--- a/kernel/locking/rtmutex_common.h
+++ b/kernel/locking/rtmutex_common.h
@@ -52,12 +52,13 @@ static inline int rt_mutex_has_waiters(struct rt_mutex *lock)
static inline struct rt_mutex_waiter *
rt_mutex_top_waiter(struct rt_mutex *lock)
{
- struct rt_mutex_waiter *w;
-
- w = rb_entry(lock->waiters.rb_leftmost,
- struct rt_mutex_waiter, tree_entry);
- BUG_ON(w->lock != lock);
+ struct rb_node *leftmost = rb_first_cached(&lock->waiters);
+ struct rt_mutex_waiter *w = NULL;
+ if (leftmost) {
+ w = rb_entry(leftmost, struct rt_mutex_waiter, tree_entry);
+ BUG_ON(w->lock != lock);
+ }
return w;
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-03-28 21:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-12 14:28 [PATCH] kernel/rtmutex: Handle non enqueued waiters gracefully in remove_waiter() Sebastian Andrzej Siewior
2018-03-16 12:20 ` Peter Zijlstra
2018-03-19 15:11 ` Thomas Gleixner
2018-03-27 12:14 ` [PATCH v2] locking/rtmutex: " Sebastian Andrzej Siewior
2018-03-28 21:07 ` [tip:locking/core] " tip-bot for Peter Zijlstra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).