Re: [PATCH 3/5] locking/rwsem: Enable reader optimistic lock stealing
On Tue, 17 Nov 2020, Waiman Long wrote: If the optimistic spinning queue is empty and the rwsem does not have the handoff or write-lock bits set, it is actually not necessary to call rwsem_optimistic_spin() to spin on it. Instead, it can steal the lock directly as its reader bias is in the count already. If it is the first reader in this state, it will try to wake up other readers in the wait queue. Signed-off-by: Waiman Long Reviewed-by: Davidlohr Bueso
Re: [PATCH 3/5] locking/rwsem: Enable reader optimistic lock stealing
On 11/20/20 9:36 AM, Peter Zijlstra wrote:
On Tue, Nov 17, 2020 at 10:04:27PM -0500, Waiman Long wrote:
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index ee374ae061c3..930dd4af3639 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -957,6 +957,12 @@ static inline bool rwsem_reader_phase_trylock(struct
rw_semaphore *sem,
}
return false;
}
+
+static inline bool osq_is_empty(struct rw_semaphore *sem)
+{
+ return !osq_is_locked(>osq);
+}
+
#else
static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem,
unsigned long nonspinnable)
@@ -977,6 +983,10 @@ static inline bool rwsem_reader_phase_trylock(struct
rw_semaphore *sem,
return false;
}
+static inline bool osq_is_empty(sem)
+{
+ return false;
+}
Hurph, the naming seems to suggest this ought to be in osq_lock.h, but
it really is part of rwsem, it captures the lack of osq member for this
configuration.
How about: rwsem_no_spinners() instead ?
Yes, sure. Will make the name change.
static inline int
rwsem_spin_on_owner(struct rw_semaphore *sem, unsigned long nonspinnable)
{
@@ -1007,6 +1017,22 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, int
state, long count)
!(count & RWSEM_WRITER_LOCKED))
goto queue;
+ /*
+* Reader optimistic lock stealing
+*
+* We can take the read lock directly without doing
+* rwsem_optimistic_spin() if the conditions are right.
+* Also wake up other readers if it is the first reader.
+*/
+ if (!(count & (RWSEM_WRITER_LOCKED | RWSEM_FLAG_HANDOFF)) &&
+ osq_is_empty(sem)) {
+ rwsem_set_reader_owned(sem);
+ lockevent_inc(rwsem_rlock_steal);
+ if (rcnt == 1)
+ goto wake_readers;
+ return sem;
+ }
AFAICT this saves at least 3 atomic ops; how common is this case
(you did add a counter but forgot to mention this).
Right, I should have mentioned the counter results.
Below is the relevant counter stats for a test system that have been up
for more than 21 hours:
rwsem_opt_rlock=11792583 (optmistically acquired read lock)
rwsem_rlock=193357272 (slowpath acquired read lock)
rwsem_rlock_steal=44795149 (lock stealing)
So lock stealing represents about 17.9% of the total read lock acquired
in non-fast path. I ran some microbenchmark test on the system before,
so it may skew a bit to the high side. Anyway, this is not an
insignificant amount.
Cheers,
Longman
Re: [PATCH 3/5] locking/rwsem: Enable reader optimistic lock stealing
On Tue, Nov 17, 2020 at 10:04:27PM -0500, Waiman Long wrote:
> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
> index ee374ae061c3..930dd4af3639 100644
> --- a/kernel/locking/rwsem.c
> +++ b/kernel/locking/rwsem.c
> @@ -957,6 +957,12 @@ static inline bool rwsem_reader_phase_trylock(struct
> rw_semaphore *sem,
> }
> return false;
> }
> +
> +static inline bool osq_is_empty(struct rw_semaphore *sem)
> +{
> + return !osq_is_locked(>osq);
> +}
> +
> #else
> static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem,
> unsigned long nonspinnable)
> @@ -977,6 +983,10 @@ static inline bool rwsem_reader_phase_trylock(struct
> rw_semaphore *sem,
> return false;
> }
>
> +static inline bool osq_is_empty(sem)
> +{
> + return false;
> +}
Hurph, the naming seems to suggest this ought to be in osq_lock.h, but
it really is part of rwsem, it captures the lack of osq member for this
configuration.
How about: rwsem_no_spinners() instead ?
> static inline int
> rwsem_spin_on_owner(struct rw_semaphore *sem, unsigned long nonspinnable)
> {
> @@ -1007,6 +1017,22 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, int
> state, long count)
> !(count & RWSEM_WRITER_LOCKED))
> goto queue;
>
> + /*
> + * Reader optimistic lock stealing
> + *
> + * We can take the read lock directly without doing
> + * rwsem_optimistic_spin() if the conditions are right.
> + * Also wake up other readers if it is the first reader.
> + */
> + if (!(count & (RWSEM_WRITER_LOCKED | RWSEM_FLAG_HANDOFF)) &&
> + osq_is_empty(sem)) {
> + rwsem_set_reader_owned(sem);
> + lockevent_inc(rwsem_rlock_steal);
> + if (rcnt == 1)
> + goto wake_readers;
> + return sem;
> + }
AFAICT this saves at least 3 atomic ops; how common is this case
(you did add a counter but forgot to mention this).
[PATCH 3/5] locking/rwsem: Enable reader optimistic lock stealing
If the optimistic spinning queue is empty and the rwsem does not have
the handoff or write-lock bits set, it is actually not necessary to
call rwsem_optimistic_spin() to spin on it. Instead, it can steal the
lock directly as its reader bias is in the count already. If it is
the first reader in this state, it will try to wake up other readers
in the wait queue.
Signed-off-by: Waiman Long
---
kernel/locking/lock_events_list.h | 1 +
kernel/locking/rwsem.c| 27 +++
2 files changed, 28 insertions(+)
diff --git a/kernel/locking/lock_events_list.h
b/kernel/locking/lock_events_list.h
index 239039d0ce21..270a0d351932 100644
--- a/kernel/locking/lock_events_list.h
+++ b/kernel/locking/lock_events_list.h
@@ -63,6 +63,7 @@ LOCK_EVENT(rwsem_opt_nospin) /* # of disabled optspins
*/
LOCK_EVENT(rwsem_opt_norspin) /* # of disabled reader-only optspins */
LOCK_EVENT(rwsem_opt_rlock2) /* # of opt-acquired 2ndary read locks */
LOCK_EVENT(rwsem_rlock)/* # of read locks acquired
*/
+LOCK_EVENT(rwsem_rlock_steal) /* # of read locks by lock stealing */
LOCK_EVENT(rwsem_rlock_fast) /* # of fast read locks acquired*/
LOCK_EVENT(rwsem_rlock_fail) /* # of failed read lock acquisitions */
LOCK_EVENT(rwsem_rlock_handoff)/* # of read lock handoffs
*/
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index ee374ae061c3..930dd4af3639 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -957,6 +957,12 @@ static inline bool rwsem_reader_phase_trylock(struct
rw_semaphore *sem,
}
return false;
}
+
+static inline bool osq_is_empty(struct rw_semaphore *sem)
+{
+ return !osq_is_locked(>osq);
+}
+
#else
static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem,
unsigned long nonspinnable)
@@ -977,6 +983,10 @@ static inline bool rwsem_reader_phase_trylock(struct
rw_semaphore *sem,
return false;
}
+static inline bool osq_is_empty(sem)
+{
+ return false;
+}
static inline int
rwsem_spin_on_owner(struct rw_semaphore *sem, unsigned long nonspinnable)
{
@@ -1007,6 +1017,22 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, int
state, long count)
!(count & RWSEM_WRITER_LOCKED))
goto queue;
+ /*
+* Reader optimistic lock stealing
+*
+* We can take the read lock directly without doing
+* rwsem_optimistic_spin() if the conditions are right.
+* Also wake up other readers if it is the first reader.
+*/
+ if (!(count & (RWSEM_WRITER_LOCKED | RWSEM_FLAG_HANDOFF)) &&
+ osq_is_empty(sem)) {
+ rwsem_set_reader_owned(sem);
+ lockevent_inc(rwsem_rlock_steal);
+ if (rcnt == 1)
+ goto wake_readers;
+ return sem;
+ }
+
/*
* Save the current read-owner of rwsem, if available, and the
* reader nonspinnable bit.
@@ -1029,6 +1055,7 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, int
state, long count)
* Wake up other readers in the wait list if the front
* waiter is a reader.
*/
+wake_readers:
if ((atomic_long_read(>count) & RWSEM_FLAG_WAITERS)) {
raw_spin_lock_irq(>wait_lock);
if (!list_empty(>wait_list))
--
2.18.1
