* [PATCH 1/2] locking/mutex: Don't hog RCU read lock while optimistically spinning
@ 2020-08-07 19:16 Sultan Alsawaf
2020-08-07 19:16 ` [PATCH 2/2] locking/rwsem: " Sultan Alsawaf
2020-09-07 16:20 ` [PATCH 1/2] locking/mutex: " Will Deacon
0 siblings, 2 replies; 4+ messages in thread
From: Sultan Alsawaf @ 2020-08-07 19:16 UTC (permalink / raw)
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, linux-kernel, Sultan Alsawaf
From: Sultan Alsawaf <sultan@kerneltoast.com>
There's no reason to hold an RCU read lock the entire time while
optimistically spinning for a mutex lock. This can needlessly lengthen
RCU grace periods and slow down synchronize_rcu() when it doesn't brute
force the RCU grace period via rcupdate.rcu_expedited=1.
Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
---
kernel/locking/mutex.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index 5352ce50a97e..cc5676712458 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -552,21 +552,31 @@ bool mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner,
{
bool ret = true;
- rcu_read_lock();
- while (__mutex_owner(lock) == owner) {
+ for (;;) {
+ unsigned int cpu;
+ bool same_owner;
+
/*
- * Ensure we emit the owner->on_cpu, dereference _after_
- * checking lock->owner still matches owner. If that fails,
+ * Ensure lock->owner still matches owner. If that fails,
* owner might point to freed memory. If it still matches,
* the rcu_read_lock() ensures the memory stays valid.
*/
- barrier();
+ rcu_read_lock();
+ same_owner = __mutex_owner(lock) == owner;
+ if (same_owner) {
+ ret = owner->on_cpu;
+ if (ret)
+ cpu = task_cpu(owner);
+ }
+ rcu_read_unlock();
+
+ if (!ret || !same_owner)
+ break;
/*
* Use vcpu_is_preempted to detect lock holder preemption issue.
*/
- if (!owner->on_cpu || need_resched() ||
- vcpu_is_preempted(task_cpu(owner))) {
+ if (need_resched() || vcpu_is_preempted(cpu)) {
ret = false;
break;
}
@@ -578,7 +588,6 @@ bool mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner,
cpu_relax();
}
- rcu_read_unlock();
return ret;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH 2/2] locking/rwsem: Don't hog RCU read lock while optimistically spinning
2020-08-07 19:16 [PATCH 1/2] locking/mutex: Don't hog RCU read lock while optimistically spinning Sultan Alsawaf
@ 2020-08-07 19:16 ` Sultan Alsawaf
2020-09-07 16:20 ` [PATCH 1/2] locking/mutex: " Will Deacon
1 sibling, 0 replies; 4+ messages in thread
From: Sultan Alsawaf @ 2020-08-07 19:16 UTC (permalink / raw)
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, linux-kernel, Sultan Alsawaf
From: Sultan Alsawaf <sultan@kerneltoast.com>
There's no reason to hold an RCU read lock the entire time while
optimistically spinning for a rwsem. This can needlessly lengthen RCU
grace periods and slow down synchronize_rcu() when it doesn't brute
force the RCU grace period via rcupdate.rcu_expedited=1.
Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
---
kernel/locking/rwsem.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index f11b9bd3431d..a1e3ceb254d1 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -723,8 +723,10 @@ rwsem_spin_on_owner(struct rw_semaphore *sem, unsigned long nonspinnable)
if (state != OWNER_WRITER)
return state;
- rcu_read_lock();
for (;;) {
+ bool same_owner;
+
+ rcu_read_lock();
/*
* When a waiting writer set the handoff flag, it may spin
* on the owner as well. Once that writer acquires the lock,
@@ -732,27 +734,32 @@ rwsem_spin_on_owner(struct rw_semaphore *sem, unsigned long nonspinnable)
* handoff bit is set.
*/
new = rwsem_owner_flags(sem, &new_flags);
- if ((new != owner) || (new_flags != flags)) {
- state = rwsem_owner_state(new, new_flags, nonspinnable);
- break;
- }
/*
- * Ensure we emit the owner->on_cpu, dereference _after_
- * checking sem->owner still matches owner, if that fails,
+ * Ensure sem->owner still matches owner. If that fails,
* owner might point to free()d memory, if it still matches,
* the rcu_read_lock() ensures the memory stays valid.
*/
- barrier();
+ same_owner = new == owner && new_flags == flags;
+ if (same_owner && !owner_on_cpu(owner))
+ state = OWNER_NONSPINNABLE;
+ rcu_read_unlock();
- if (need_resched() || !owner_on_cpu(owner)) {
+ if (!same_owner) {
+ state = rwsem_owner_state(new, new_flags, nonspinnable);
+ break;
+ }
+
+ if (state == OWNER_NONSPINNABLE)
+ break;
+
+ if (need_resched()) {
state = OWNER_NONSPINNABLE;
break;
}
cpu_relax();
}
- rcu_read_unlock();
return state;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] locking/mutex: Don't hog RCU read lock while optimistically spinning
2020-08-07 19:16 [PATCH 1/2] locking/mutex: Don't hog RCU read lock while optimistically spinning Sultan Alsawaf
2020-08-07 19:16 ` [PATCH 2/2] locking/rwsem: " Sultan Alsawaf
@ 2020-09-07 16:20 ` Will Deacon
2020-09-14 0:36 ` Sultan Alsawaf
1 sibling, 1 reply; 4+ messages in thread
From: Will Deacon @ 2020-09-07 16:20 UTC (permalink / raw)
To: Sultan Alsawaf; +Cc: Peter Zijlstra, Ingo Molnar, linux-kernel
On Fri, Aug 07, 2020 at 12:16:35PM -0700, Sultan Alsawaf wrote:
> From: Sultan Alsawaf <sultan@kerneltoast.com>
>
> There's no reason to hold an RCU read lock the entire time while
> optimistically spinning for a mutex lock. This can needlessly lengthen
> RCU grace periods and slow down synchronize_rcu() when it doesn't brute
> force the RCU grace period via rcupdate.rcu_expedited=1.
Would be good to demonstrate this with numbers if you can.
> Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
> ---
> kernel/locking/mutex.c | 25 +++++++++++++++++--------
> 1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
> index 5352ce50a97e..cc5676712458 100644
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -552,21 +552,31 @@ bool mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner,
> {
> bool ret = true;
>
> - rcu_read_lock();
> - while (__mutex_owner(lock) == owner) {
> + for (;;) {
> + unsigned int cpu;
> + bool same_owner;
> +
> /*
> - * Ensure we emit the owner->on_cpu, dereference _after_
> - * checking lock->owner still matches owner. If that fails,
> + * Ensure lock->owner still matches owner. If that fails,
> * owner might point to freed memory. If it still matches,
> * the rcu_read_lock() ensures the memory stays valid.
> */
> - barrier();
> + rcu_read_lock();
> + same_owner = __mutex_owner(lock) == owner;
> + if (same_owner) {
> + ret = owner->on_cpu;
> + if (ret)
> + cpu = task_cpu(owner);
> + }
> + rcu_read_unlock();
Are you sure this doesn't break the ww mutex spinning? That thing also goes
and looks at the owner, but now it's called outside of the read-side
critical section.
Will
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] locking/mutex: Don't hog RCU read lock while optimistically spinning
2020-09-07 16:20 ` [PATCH 1/2] locking/mutex: " Will Deacon
@ 2020-09-14 0:36 ` Sultan Alsawaf
0 siblings, 0 replies; 4+ messages in thread
From: Sultan Alsawaf @ 2020-09-14 0:36 UTC (permalink / raw)
To: Will Deacon; +Cc: Peter Zijlstra, Ingo Molnar, linux-kernel
On Mon, Sep 07, 2020 at 05:20:31PM +0100, Will Deacon wrote:
> On Fri, Aug 07, 2020 at 12:16:35PM -0700, Sultan Alsawaf wrote:
> > From: Sultan Alsawaf <sultan@kerneltoast.com>
> >
> > There's no reason to hold an RCU read lock the entire time while
> > optimistically spinning for a mutex lock. This can needlessly lengthen
> > RCU grace periods and slow down synchronize_rcu() when it doesn't brute
> > force the RCU grace period via rcupdate.rcu_expedited=1.
>
> Would be good to demonstrate this with numbers if you can.
I could simulate the worst possible case, which would stall synchronize_rcu() by
one jiffy, which could be 10ms with CONFIG_HZ=100. The way that would happen is
when the mutex owner does a lot of non-sleeping work while the lock is held, and
while another CPU tries to acquire the lock.
This is a dummy example of the scenario I have in mind:
CPU0 CPU1
----------------------------------------------
mutex_lock(locky)
mdelay(100) mutex_lock(locky)
mutex_unlock(locky)
In this case, CPU1 could spin in mutex_lock() for up to a jiffy (until CPU0
releases locky, which won't happen for 100ms, or until CPU1's task needs to
reschedule). While the spinning occurs, the RCU read lock will be held the whole
time, and then synchronize_rcu() will be stalled.
One could argue that most mutex-locked critical sections probably wouldn't spend
so long working on something without scheduling (at least, not intentionally),
but on slower SMP CPUs I suspect that this is common.
> > Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
> > ---
> > kernel/locking/mutex.c | 25 +++++++++++++++++--------
> > 1 file changed, 17 insertions(+), 8 deletions(-)
> >
> > diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
> > index 5352ce50a97e..cc5676712458 100644
> > --- a/kernel/locking/mutex.c
> > +++ b/kernel/locking/mutex.c
> > @@ -552,21 +552,31 @@ bool mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner,
> > {
> > bool ret = true;
> >
> > - rcu_read_lock();
> > - while (__mutex_owner(lock) == owner) {
> > + for (;;) {
> > + unsigned int cpu;
> > + bool same_owner;
> > +
> > /*
> > - * Ensure we emit the owner->on_cpu, dereference _after_
> > - * checking lock->owner still matches owner. If that fails,
> > + * Ensure lock->owner still matches owner. If that fails,
> > * owner might point to freed memory. If it still matches,
> > * the rcu_read_lock() ensures the memory stays valid.
> > */
> > - barrier();
> > + rcu_read_lock();
> > + same_owner = __mutex_owner(lock) == owner;
> > + if (same_owner) {
> > + ret = owner->on_cpu;
> > + if (ret)
> > + cpu = task_cpu(owner);
> > + }
> > + rcu_read_unlock();
>
> Are you sure this doesn't break the ww mutex spinning? That thing also goes
> and looks at the owner, but now it's called outside of the read-side
> critical section.
Yes, it's safe because it's not dereferencing the owner pointer.
Sultan
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-09-14 0:36 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-07 19:16 [PATCH 1/2] locking/mutex: Don't hog RCU read lock while optimistically spinning Sultan Alsawaf
2020-08-07 19:16 ` [PATCH 2/2] locking/rwsem: " Sultan Alsawaf
2020-09-07 16:20 ` [PATCH 1/2] locking/mutex: " Will Deacon
2020-09-14 0:36 ` Sultan Alsawaf
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).