locking/rwsem: Optimize write lock slowpath
PATCH 1/1 locking/rwsem: Optimize write lock slowpath
When acquiring the rwsem write lock in the slowpath, we first try to set count to RWSEM_WAITING_BIAS. When that is successful, we then atomically add the RWSEM_WAITING_BIAS in cases where there are other tasks on the wait list. This causes write lock operations to often issue multiple atomic operations.
We can instead make the list_is_singular() check first, and then set the count accordingly, so that we issue at most 1 atomic operation when acquiring the write lock and reduce unnecessary cacheline contention.
--- kernel/locking/rwsem-xadd.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
kernel/locking/rwsem-xadd.c +13 -7
@@ -258,14 +258,20 @@ EXPORT_SYMBOL(rwsem_down_read_failed);static inline bool rwsem_try_write_lock (long count,struct rw_semaphore *sem) {/* -* Try acquiring the write lock. Check count first in order -* to reduce unnecessary expensive cmpxchg() operations. +* Avoid trying to acquire write lock if count isn't RWSEM_WAITING_BIAS. */ -if (count ==RWSEM_WAITING_BIAS && -cmpxchg_acquire (&sem->count,RWSEM_WAITING_BIAS , -RWSEM_ACTIVE_WRITE_BIAS ) ==RWSEM_WAITING_BIAS ) { -if (!list_is_singular (&sem->wait_list)) -rwsem_atomic_update (RWSEM_WAITING_BIAS , sem); +if (count !=RWSEM_WAITING_BIAS ) +return false; + +/* +* Acquire the lock by trying to set it to ACTIVE_WRITE_BIAS. If there +* are other tasks on the wait list, we need to add on WAITING_BIAS. +*/ + count =list_is_singular (&sem->wait_list) ? +RWSEM_ACTIVE_WRITE_BIAS : +RWSEM_ACTIVE_WRITE_BIAS +RWSEM_WAITING_BIAS ; + +if (cmpxchg_acquire (&sem->count,RWSEM_WAITING_BIAS , count) ==RWSEM_WAITING_BIAS ) {rwsem_set_owner (sem);return true; }
Signature · 1 line
2.1.4
Quoted 38 lines · Jason Low
When acquiring the rwsem write lock in the slowpath, we first try to set count to
RWSEM_WAITING_BIAS. When that is successful, we then atomically add theRWSEM_WAITING_BIASin cases where there are other tasks on the wait list. This causes write lock operations to often issue multiple atomic operations.We can instead make the
list_is_singular()check first, and then set the count accordingly, so that we issue at most 1 atomic operation when acquiring the write lock and reduce unnecessary cacheline contention.Signed-off-by Jason Low<[email protected]>--- kernel/locking/rwsem-xadd.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)kernel/locking/rwsem-xadd.c +6
-7@@ -258,14 +258,20 @@ EXPORT_SYMBOL(rwsem_down_read_failed);static inline bool rwsem_try_write_lock (long count,struct rw_semaphore *sem) {/* -* Try acquiring the write lock. Check count first in order -* to reduce unnecessary expensive cmpxchg() operations. +* Avoid trying to acquire write lock if count isn't RWSEM_WAITING_BIAS. */ -if (count ==RWSEM_WAITING_BIAS && -cmpxchg_acquire (&sem->count,RWSEM_WAITING_BIAS , -RWSEM_ACTIVE_WRITE_BIAS ) ==RWSEM_WAITING_BIAS ) { -if (!list_is_singular (&sem->wait_list)) -rwsem_atomic_update (RWSEM_WAITING_BIAS , sem); +if (count !=RWSEM_WAITING_BIAS ) +return false; + +/* +* Acquire the lock by trying to set it to ACTIVE_WRITE_BIAS. If there
kernel/locking/rwsem-xadd.c EXPORT_SYMBOL(rwsem_down_read_failed); · line 277
Quoted 10 lines
kernel/locking/rwsem-xadd.c +7
-0+* are other tasks on the wait list, we need to add on WAITING_BIAS. +*/ + count =list_is_singular (&sem->wait_list) ? +RWSEM_ACTIVE_WRITE_BIAS : +RWSEM_ACTIVE_WRITE_BIAS +RWSEM_WAITING_BIAS ; + +if (cmpxchg_acquire (&sem->count,RWSEM_WAITING_BIAS , count) ==RWSEM_WAITING_BIAS ) {rwsem_set_owner (sem);return true; }
Peter Zijlstra
maintainerQuoted 31 lines · Jason Low
When acquiring the rwsem write lock in the slowpath, we first try to set count to
RWSEM_WAITING_BIAS. When that is successful, we then atomically add theRWSEM_WAITING_BIASin cases where there are other tasks on the wait list. This causes write lock operations to often issue multiple atomic operations.We can instead make the
list_is_singular()check first, and then set the count accordingly, so that we issue at most 1 atomic operation when acquiring the write lock and reduce unnecessary cacheline contention.Signed-off-by Jason Low <[email protected]>--- kernel/locking/rwsem-xadd.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)kernel/locking/rwsem-xadd.c +6
-0@@ -258,14 +258,20 @@ EXPORT_SYMBOL(rwsem_down_read_failed);static inline bool rwsem_try_write_lock (long count,struct rw_semaphore *sem) {/* +* Avoid trying to acquire write lock if count isn't RWSEM_WAITING_BIAS. */ +if (count !=RWSEM_WAITING_BIAS ) +return false; + +/* +* Acquire the lock by trying to set it to ACTIVE_WRITE_BIAS. If there
kernel/locking/rwsem-xadd.c EXPORT_SYMBOL(rwsem_down_read_failed); · line 277
Quoted 10 lines
kernel/locking/rwsem-xadd.c +7
-0+* are other tasks on the wait list, we need to add on WAITING_BIAS. +*/ + count =list_is_singular (&sem->wait_list) ? +RWSEM_ACTIVE_WRITE_BIAS : +RWSEM_ACTIVE_WRITE_BIAS +RWSEM_WAITING_BIAS ; + +if (cmpxchg_acquire (&sem->count,RWSEM_WAITING_BIAS , count) ==RWSEM_WAITING_BIAS ) {rwsem_set_owner (sem);return true; }
Right; so that whole thing works because we're holding sem->wait_lock. Should we clarify that someplace?
Also; should we not make rw_semaphore::count an atomic_long_t and kill rwsem_atomic_{update,add}() ?
Quoted 37 lines · Peter Zijlstra
Quoted 36 lines · Jason Low
When acquiring the rwsem write lock in the slowpath, we first try to set count to
RWSEM_WAITING_BIAS. When that is successful, we then atomically add theRWSEM_WAITING_BIASin cases where there are other tasks on the wait list. This causes write lock operations to often issue multiple atomic operations.We can instead make the
list_is_singular()check first, and then set the count accordingly, so that we issue at most 1 atomic operation when acquiring the write lock and reduce unnecessary cacheline contention.Signed-off-by Jason Low <[email protected]>--- kernel/locking/rwsem-xadd.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)kernel/locking/rwsem-xadd.c +11
-0@@ -258,14 +258,20 @@ EXPORT_SYMBOL(rwsem_down_read_failed);static inline bool rwsem_try_write_lock (long count,struct rw_semaphore *sem) {/* +* Avoid trying to acquire write lock if count isn't RWSEM_WAITING_BIAS. */ +if (count !=RWSEM_WAITING_BIAS ) +return false; + +/* +* Acquire the lock by trying to set it to ACTIVE_WRITE_BIAS. If there +* are other tasks on the wait list, we need to add on WAITING_BIAS. +*/ + count =list_is_singular (&sem->wait_list) ? +RWSEM_ACTIVE_WRITE_BIAS : +RWSEM_ACTIVE_WRITE_BIAS +RWSEM_WAITING_BIAS ;
Quoted 8 lines
kernel/locking/rwsem-xadd.c EXPORT_SYMBOL(rwsem_down_read_failed); · line 277
Quoted 5 lines
kernel/locking/rwsem-xadd.c +2
-0+ +if (cmpxchg_acquire (&sem->count,RWSEM_WAITING_BIAS , count) ==RWSEM_WAITING_BIAS ) {rwsem_set_owner (sem);return true; }Right; so that whole thing works because we're holding
sem->wait_lock. Should we clarify that someplace?
Yup, we can mention that the rwsem_try_write_lock() function must be called with the wait_lock held.
Quoted 2 lines
Also; should we not make
rw_semaphore::countanatomic_long_tand kill rwsem_atomic_{update,add}() ?
Right, it's better to just make the variable an atomic and remove the unnecessary rwsem_atomic_update() "abstraction". I'll send out a separate patch for this.
Peter Zijlstra
maintainerQuoted 26 lines · Jason Low
Quoted 15 lines · Peter Zijlstra
Quoted 15 lines
static inline bool
rwsem_try_write_lock(long count, struct rw_semaphore *sem){kernel/locking/rwsem-xadd.c +11
-0/* +* Avoid trying to acquire write lock if count isn't RWSEM_WAITING_BIAS. */ +if (count !=RWSEM_WAITING_BIAS ) +return false; + +/* +* Acquire the lock by trying to set it to ACTIVE_WRITE_BIAS. If there +* are other tasks on the wait list, we need to add on WAITING_BIAS. +*/ + count =list_is_singular (&sem->wait_list) ? +RWSEM_ACTIVE_WRITE_BIAS : +RWSEM_ACTIVE_WRITE_BIAS +RWSEM_WAITING_BIAS ;Quoted 8 lines
kernel/locking/rwsem-xadd.c EXPORT_SYMBOL(rwsem_down_read_failed); · line 277
Quoted 5 lines
kernel/locking/rwsem-xadd.c +2
-0+ +if (cmpxchg_acquire (&sem->count,RWSEM_WAITING_BIAS , count) ==RWSEM_WAITING_BIAS ) {rwsem_set_owner (sem);return true; }Right; so that whole thing works because we're holding
sem->wait_lock. Should we clarify that someplace?
Quoted 2 lines
Yup, we can mention that the
rwsem_try_write_lock()function must be called with thewait_lockheld.
Also try to explain _why_ it must be held.
Thanks!
Davidlohr Bueso
Acked-byQuoted 7 lines · Peter Zijlstra
Quoted 6 lines · Jason Low
When acquiring the rwsem write lock in the slowpath, we first try to set count to
RWSEM_WAITING_BIAS. When that is successful, we then atomically add theRWSEM_WAITING_BIASin cases where there are other tasks on the wait list. This causes write lock operations to often issue multiple atomic operations.
Quoted 6 lines
Quoted 6 lines
We can instead make the
list_is_singular()check first, and then set the count accordingly, so that we issue at most 1 atomic operation when acquiring the write lock and reduce unnecessary cacheline contention.Signed-off-by Jason Low <[email protected]>
(one nit: the patch title could be more informative to what optimization we are talking about here... ie: reduce atomic ops in writer slowpath' or something.)
Quoted 24 lines
Quoted 24 lines
--- kernel/locking/rwsem-xadd.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)kernel/locking/rwsem-xadd.c +11
-0@@ -258,14 +258,20 @@ EXPORT_SYMBOL(rwsem_down_read_failed);static inline bool rwsem_try_write_lock (long count,struct rw_semaphore *sem) {/* +* Avoid trying to acquire write lock if count isn't RWSEM_WAITING_BIAS. */ +if (count !=RWSEM_WAITING_BIAS ) +return false; + +/* +* Acquire the lock by trying to set it to ACTIVE_WRITE_BIAS. If there +* are other tasks on the wait list, we need to add on WAITING_BIAS. +*/ + count =list_is_singular (&sem->wait_list) ? +RWSEM_ACTIVE_WRITE_BIAS : +RWSEM_ACTIVE_WRITE_BIAS +RWSEM_WAITING_BIAS ;
Quoted 8 lines
kernel/locking/rwsem-xadd.c EXPORT_SYMBOL(rwsem_down_read_failed); · line 277
Quoted 5 lines
kernel/locking/rwsem-xadd.c +2
-0+ +if (cmpxchg_acquire (&sem->count,RWSEM_WAITING_BIAS , count) ==RWSEM_WAITING_BIAS ) {rwsem_set_owner (sem);return true; }Right; so that whole thing works because we're holding
sem->wait_lock. Should we clarify that someplace?
Yes exactly, rwsem_try_write_lock() is always called with the wait_lock held, unlike the unqueued cousin.
Thanks,
Davidlohr
Quoted 14 lines · Davidlohr Bueso
Quoted 12 lines · Peter Zijlstra
Quoted 11 lines · Jason Low
When acquiring the rwsem write lock in the slowpath, we first try to set count to
RWSEM_WAITING_BIAS. When that is successful, we then atomically add theRWSEM_WAITING_BIASin cases where there are other tasks on the wait list. This causes write lock operations to often issue multiple atomic operations.We can instead make the
list_is_singular()check first, and then set the count accordingly, so that we issue at most 1 atomic operation when acquiring the write lock and reduce unnecessary cacheline contention.
Quoted 7 lines
Quoted 1 line
Quoted 1 line
Signed-off-by Jason Low <[email protected]>Acked-by Davidlohr Bueso <[email protected]>(one nit: the patch title could be more informative to what optimization we are talking about here... ie: reduce atomic ops in writer slowpath' or something.)
Yeah, the "optimize write lock slowpath" subject is a bit generic. I'll make the title more specific in the next version.
Thanks,
Jason