rwsem: check the lock before cpmxchg in down_write_trylock and rwsem_do_wake
Subsystems (5)
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/2 rwsem: check the lock before cpmxchg in down_write_trylock and rwsem_do_wake
Doing cmpxchg will cause cache bouncing when checking sem->count. This could cause scalability issue in a large machine (e.g. a 80 cores box).
A pre-read of sem->count can mitigate this.
--- include/asm-generic/rwsem.h | 8 ++++---- lib/rwsem.c | 21 +++++++++++++-------- 2 files changed, 17 insertions(+), 12 deletions(-)
include/asm-generic/rwsem.h +4 -4
@@ -70,11 +70,11 @@ static inline void __down_write(struct rw_semaphore *sem)static inline int __down_write_trylock (struct rw_semaphore *sem) { -long tmp; +if (unlikely (&sem->count !=RWSEM_UNLOCKED_VALUE )) +return 0 ; - tmp =cmpxchg (&sem->count,RWSEM_UNLOCKED_VALUE , -RWSEM_ACTIVE_WRITE_BIAS ); -return tmp ==RWSEM_UNLOCKED_VALUE ; +return cmpxchg (&sem->count,RWSEM_UNLOCKED_VALUE , +RWSEM_ACTIVE_WRITE_BIAS ) ==RWSEM_UNLOCKED_VALUE ; }/*
lib/rwsem.c +13 -8
@@ -75,7 +75,7 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type)* will block as they will notice the queued writer. */ wake_up_process (waiter->task); - goto out; +return sem; }/* Writers might steal the lock before we grant it to the next reader.
@@ -85,15 +85,21 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type) adjustment =0 ;if (wake_type !=RWSEM_WAKE_READ_OWNED ) { adjustment =RWSEM_ACTIVE_READ_BIAS ; - try_reader_grant: - oldcount =rwsem_atomic_update (adjustment, sem) - adjustment; -if (unlikely (oldcount <RWSEM_WAITING_BIAS )) { -/* A writer stole the lock. Undo our reader grant. */ +while (1 ) { +/* A writer stole the lock. */ +if (sem->count <RWSEM_WAITING_BIAS ) +return sem; + + oldcount =rwsem_atomic_update (adjustment, sem) + - adjustment; +if (likely (oldcount >=RWSEM_WAITING_BIAS )) +break ; + +/* A writer stole the lock. Undo our reader grant. */ if (rwsem_atomic_update (-adjustment, sem) &RWSEM_ACTIVE_MASK ) - goto out; +return sem;/* Last active locker left. Retry waking readers. */ - goto try_reader_grant; } }
@@ -136,7 +142,6 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type) sem->wait_list.next = next; next->prev = &sem->wait_list; - out:return sem; }
Signature · 9 lines
1.7.4.4 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 8 lines · Tim Chen
Doing cmpxchg will cause cache bouncing when checking
sem->count. This could cause scalability issue in a large machine (e.g. a 80 cores box).A pre-read of
sem->countcan mitigate this.Signed-off-by Alex Shi <[email protected]>Signed-off-by Tim Chen <[email protected]>
Hi Tim,
there is a technical error in this patch.
the "From: " line should be 'Alex Shi', since he made the most input of this patch.
And I still think split this patch to 4 smaller will make it more simple to review, that I had sent you and Davidlohr.
could you like to re-send with my 4 patch version? :)
Quoted 75 lines
--- include/asm-generic/rwsem.h | 8 ++++---- lib/rwsem.c | 21 +++++++++++++-------- 2 files changed, 17 insertions(+), 12 deletions(-)include/asm-generic/rwsem.h +4
-4@@ -70,11 +70,11 @@ static inline void __down_write(struct rw_semaphore *sem)static inline int __down_write_trylock (struct rw_semaphore *sem) { -long tmp; +if (unlikely (&sem->count !=RWSEM_UNLOCKED_VALUE )) +return 0 ; - tmp =cmpxchg (&sem->count,RWSEM_UNLOCKED_VALUE , -RWSEM_ACTIVE_WRITE_BIAS ); -return tmp ==RWSEM_UNLOCKED_VALUE ; +return cmpxchg (&sem->count,RWSEM_UNLOCKED_VALUE , +RWSEM_ACTIVE_WRITE_BIAS ) ==RWSEM_UNLOCKED_VALUE ; }/* lib/rwsem.c +13
-8@@ -75,7 +75,7 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type)* will block as they will notice the queued writer. */ wake_up_process (waiter->task); - goto out; +return sem; }/* Writers might steal the lock before we grant it to the next reader. @@ -85,15 +85,21 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type) adjustment =0 ;if (wake_type !=RWSEM_WAKE_READ_OWNED ) { adjustment =RWSEM_ACTIVE_READ_BIAS ; - try_reader_grant: - oldcount =rwsem_atomic_update (adjustment, sem) - adjustment; -if (unlikely (oldcount <RWSEM_WAITING_BIAS )) { -/* A writer stole the lock. Undo our reader grant. */ +while (1 ) { +/* A writer stole the lock. */ +if (sem->count <RWSEM_WAITING_BIAS ) +return sem; + + oldcount =rwsem_atomic_update (adjustment, sem) + - adjustment; +if (likely (oldcount >=RWSEM_WAITING_BIAS )) +break ; + +/* A writer stole the lock. Undo our reader grant. */ if (rwsem_atomic_update (-adjustment, sem) &RWSEM_ACTIVE_MASK ) - goto out; +return sem;/* Last active locker left. Retry waking readers. */ - goto try_reader_grant; } }@@ -136,7 +142,6 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type) sem->wait_list.next = next; next->prev = &sem->wait_list; - out:return sem; }
Signature · 8 lines
Thanks
Alex
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>Davidlohr Bueso
Acked-byQuoted 10 lines · Alex Shi
Quoted 8 lines · Tim Chen
Doing cmpxchg will cause cache bouncing when checking
sem->count. This could cause scalability issue in a large machine (e.g. a 80 cores box).A pre-read of
sem->countcan mitigate this.Signed-off-by Alex Shi <[email protected]>Signed-off-by Tim Chen <[email protected]>
Quoted 7 lines
Hi Tim,
there is a technical error in this patch.
the "From: " line should be 'Alex Shi', since he made the most input of this patch.And I still think split this patch to 4 smaller will make it more simple to review, that I had sent you and Davidlohr.
Yep, and you had updated the changelog for 1/4: rwsem: check the lock before cpmxchg in down_write_trylock to:
"cmpxchg will cause cache bouncing when do the value checking, that cause scalability issue in a large machine (like a 80 cores box).
A lock status pre-read can relief this."
Quoted 2 lines
could you like to re-send with my 4 patch version? :)
For those 4 patches:
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 7 lines · Alex Shi
Quoted 6 lines · Tim Chen
Doing cmpxchg will cause cache bouncing when checking
sem->count. This could cause scalability issue in a large machine (e.g. a 80 cores box).A pre-read of
sem->countcan mitigate this.
Quoted 8 lines
Quoted 2 lines
Signed-off-by Alex Shi <[email protected]>Signed-off-by Tim Chen <[email protected]>Hi Tim,
there is a technical error in this patch.
the "From: " line should be 'Alex Shi', since he made the most input of this patch.
Actually in my local git repository, I have the From line as from you that's reflected in the change log of patch[0/2].
I'll put this as your signed-off only if you preferred for v2. The "From" field was you in my git but changes when I send them out.
Quoted 82 lines
And I still think split this patch to 4 smaller will make it more simple to review, that I had sent you and Davidlohr.
could you like to re-send with my 4 patch version? :)
Quoted 75 lines
--- include/asm-generic/rwsem.h | 8 ++++---- lib/rwsem.c | 21 +++++++++++++-------- 2 files changed, 17 insertions(+), 12 deletions(-)include/asm-generic/rwsem.h +4
-4@@ -70,11 +70,11 @@ static inline void __down_write(struct rw_semaphore *sem)static inline int __down_write_trylock (struct rw_semaphore *sem) { -long tmp; +if (unlikely (&sem->count !=RWSEM_UNLOCKED_VALUE )) +return 0 ; - tmp =cmpxchg (&sem->count,RWSEM_UNLOCKED_VALUE , -RWSEM_ACTIVE_WRITE_BIAS ); -return tmp ==RWSEM_UNLOCKED_VALUE ; +return cmpxchg (&sem->count,RWSEM_UNLOCKED_VALUE , +RWSEM_ACTIVE_WRITE_BIAS ) ==RWSEM_UNLOCKED_VALUE ; }/* lib/rwsem.c +13
-8@@ -75,7 +75,7 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type)* will block as they will notice the queued writer. */ wake_up_process (waiter->task); - goto out; +return sem; }/* Writers might steal the lock before we grant it to the next reader. @@ -85,15 +85,21 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type) adjustment =0 ;if (wake_type !=RWSEM_WAKE_READ_OWNED ) { adjustment =RWSEM_ACTIVE_READ_BIAS ; - try_reader_grant: - oldcount =rwsem_atomic_update (adjustment, sem) - adjustment; -if (unlikely (oldcount <RWSEM_WAITING_BIAS )) { -/* A writer stole the lock. Undo our reader grant. */ +while (1 ) { +/* A writer stole the lock. */ +if (sem->count <RWSEM_WAITING_BIAS ) +return sem; + + oldcount =rwsem_atomic_update (adjustment, sem) + - adjustment; +if (likely (oldcount >=RWSEM_WAITING_BIAS )) +break ; + +/* A writer stole the lock. Undo our reader grant. */ if (rwsem_atomic_update (-adjustment, sem) &RWSEM_ACTIVE_MASK ) - goto out; +return sem;/* Last active locker left. Retry waking readers. */ - goto try_reader_grant; } }@@ -136,7 +142,6 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type) sem->wait_list.next = next; next->prev = &sem->wait_list; - out:return sem; }
Quoted 18 lines · Tim Chen
Doing cmpxchg will cause cache bouncing when checking
sem->count. This could cause scalability issue in a large machine (e.g. a 80 cores box).A pre-read of
sem->countcan mitigate this.Signed-off-by Alex Shi <[email protected]>Signed-off-by Tim Chen <[email protected]>--- include/asm-generic/rwsem.h | 8 ++++---- lib/rwsem.c | 21 +++++++++++++-------- 2 files changed, 17 insertions(+), 12 deletions(-)include/asm-generic/rwsem.h +0
-0
include/asm-generic/rwsem.h static inline void __down_write(struct rw_semaphore *sem) · line 73
Quoted 5 lines
include/asm-generic/rwsem.h +1
-1@@ -70,11 +70,11 @@ static inline void __down_write(struct rw_semaphore *sem)static inline int __down_write_trylock (struct rw_semaphore *sem) { -long tmp; +if (unlikely (&sem->count !=RWSEM_UNLOCKED_VALUE ))
^^^^^^^^^^^
This is probably not what you want.
Quoted 15 lines
include/asm-generic/rwsem.h +3
-3+return 0 ; - tmp =cmpxchg (&sem->count,RWSEM_UNLOCKED_VALUE , -RWSEM_ACTIVE_WRITE_BIAS ); -return tmp ==RWSEM_UNLOCKED_VALUE ; +return cmpxchg (&sem->count,RWSEM_UNLOCKED_VALUE , +RWSEM_ACTIVE_WRITE_BIAS ) ==RWSEM_UNLOCKED_VALUE ; }/* lib/rwsem.c +0
-0
lib/rwsem.c __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type) · line 78
Quoted 5 lines
lib/rwsem.c +1
-1@@ -75,7 +75,7 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type)* will block as they will notice the queued writer. */ wake_up_process (waiter->task); - goto out; +return sem;
Please put these flow control changes in a separate patch.
Quoted 5 lines
lib/rwsem.c +0
-0}/* Writers might steal the lock before we grant it to the next reader. @@ -85,15 +85,21 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type) adjustment =0 ;
lib/rwsem.c __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type) · line 91
Quoted 10 lines
lib/rwsem.c +4
-4if (wake_type !=RWSEM_WAKE_READ_OWNED ) { adjustment =RWSEM_ACTIVE_READ_BIAS ; - try_reader_grant: - oldcount =rwsem_atomic_update (adjustment, sem) - adjustment; -if (unlikely (oldcount <RWSEM_WAITING_BIAS )) { -/* A writer stole the lock. Undo our reader grant. */ +while (1 ) { +/* A writer stole the lock. */ +if (sem->count <RWSEM_WAITING_BIAS ) +return sem;
I'm all for structured looping instead of goto labels but this optimization is only useful on the 1st iteration. IOW, on the second iteration you already know that you need to try for reclaiming the lock.
Quoted 17 lines
lib/rwsem.c +8
-2+ + oldcount =rwsem_atomic_update (adjustment, sem) + - adjustment; +if (likely (oldcount >=RWSEM_WAITING_BIAS )) +break ; + +/* A writer stole the lock. Undo our reader grant. */ if (rwsem_atomic_update (-adjustment, sem) &RWSEM_ACTIVE_MASK ) - goto out; +return sem;/* Last active locker left. Retry waking readers. */ - goto try_reader_grant; } }
lib/rwsem.c __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type) · line 146
Quoted 6 lines
lib/rwsem.c +0
-1@@ -136,7 +142,6 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type) sem->wait_list.next = next; next->prev = &sem->wait_list; - out:return sem; }
Alex and Tim,
Was there a v1 of this series; ie., is this v2 (or higher)?
How are you validating lock correctness/behavior with this series?
Regards,
Peter Hurley
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 21 lines · Peter Hurley
Quoted 20 lines · Tim Chen
Doing cmpxchg will cause cache bouncing when checking
sem->count. This could cause scalability issue in a large machine (e.g. a 80 cores box).A pre-read of
sem->countcan mitigate this.Signed-off-by Alex Shi <[email protected]>Signed-off-by Tim Chen <[email protected]>--- include/asm-generic/rwsem.h | 8 ++++---- lib/rwsem.c | 21 +++++++++++++-------- 2 files changed, 17 insertions(+), 12 deletions(-)include/asm-generic/rwsem.h +0
-0@@ -70,11 +70,11 @@ static inline void __down_write(struct rw_semaphore *sem)
Quoted 8 lines
include/asm-generic/rwsem.h static inline void __down_write(struct rw_semaphore *sem) · line 73
Quoted 4 lines
include/asm-generic/rwsem.h +1
-1static inline int __down_write_trylock (struct rw_semaphore *sem) { -long tmp; +if (unlikely (&sem->count !=RWSEM_UNLOCKED_VALUE ))^^^^^^^^^^^This is probably not what you want.
this function logical is quite simple. check the sem->count before cmpxchg is no harm this logical.
So could you like to tell us what should we want?
Quoted 10 lines
Quoted 10 lines
include/asm-generic/rwsem.h +3
-3+return 0 ; - tmp =cmpxchg (&sem->count,RWSEM_UNLOCKED_VALUE , -RWSEM_ACTIVE_WRITE_BIAS ); -return tmp ==RWSEM_UNLOCKED_VALUE ; +return cmpxchg (&sem->count,RWSEM_UNLOCKED_VALUE , +RWSEM_ACTIVE_WRITE_BIAS ) ==RWSEM_UNLOCKED_VALUE ; }
Quoted 14 lines
lib/rwsem.c __rwsem_do_wake(struct rw_semaphore *sem, enum · line 79
Quoted 12 lines
/*
lib/rwsem.c +1
-1@@ -75,7 +75,7 @@ __rwsem_do_wake(struct rw_semaphore *sem, enumrwsem_wake_type wake_type) * will block as they will notice the queued writer. */ wake_up_process (waiter->task); - goto out; +return sem;Please put these flow control changes in a separate patch.
I had sent the split patches to Tim&Davidlohr. They will send them out as a single patchset.
Quoted 16 lines
Quoted 16 lines
lib/rwsem.c +0
-0}/* Writers might steal the lock before we grant it to the nextreader.
lib/rwsem.c +1
-4@@ -85,15 +85,21 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type) adjustment =0 ;if (wake_type !=RWSEM_WAKE_READ_OWNED ) { adjustment =RWSEM_ACTIVE_READ_BIAS ; - try_reader_grant: - oldcount =rwsem_atomic_update (adjustment, sem) - adjustment; -if (unlikely (oldcount <RWSEM_WAITING_BIAS )) { -/* A writer stole the lock. Undo our reader grant. */ +while (1 ) {
Quoted 9 lines
lib/rwsem.c __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type) · line 91
Quoted 3 lines
lib/rwsem.c +3
-0+/* A writer stole the lock. */ +if (sem->count <RWSEM_WAITING_BIAS ) +return sem;I'm all for structured looping instead of goto labels but this optimization is only useful on the 1st iteration. IOW, on the second iteration you already
know that you need to try for reclaiming the lock.
sorry. could you like to say more clear, what's the 1st or 2nd iteration or others?
Quoted 29 lines
Quoted 18 lines
lib/rwsem.c +8
-2+ + oldcount =rwsem_atomic_update (adjustment, sem) + - adjustment; +if (likely (oldcount >=RWSEM_WAITING_BIAS )) +break ; + +/* A writer stole the lock. Undo our reader grant. */ if (rwsem_atomic_update (-adjustment, sem) &RWSEM_ACTIVE_MASK ) - goto out; +return sem;/* Last active locker left. Retry waking readers. */ - goto try_reader_grant; } }lib/rwsem.c __rwsem_do_wake(struct rw_semaphore *sem, enum · line 147
Quoted 7 lines
lib/rwsem.c +0
-1@@ -136,7 +142,6 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type) sem->wait_list.next = next; next->prev = &sem->wait_list; - out:return sem; }Alex and Tim,
Quoted 3 lines
Was there a v1 of this series; ie., is this v2 (or higher)?
How are you validating lock correctness/behavior with this series?
some benchmark tested against this patch, mainly aim7. plus by eyes, we didn't change the logical except check the lock value before do locking
Quoted 4 lines
Regards,
Peter Hurley
Signature · 8 lines
Thanks
Alex
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>Quoted 13 lines
Quoted 13 lines
include/asm-generic/rwsem.h static inline void __down_write(struct · line 74
Quoted 11 lines
include/asm-generic/rwsem.h +1
-1@@ -70,11 +70,11 @@ static inline void __down_write(struct rw_semaphore *sem)static inline int __down_write_trylock (struct rw_semaphore *sem) { -long tmp; +if (unlikely (&sem->count !=RWSEM_UNLOCKED_VALUE ))^^^^^^^^^^^
Quoted 7 lines
Quoted 2 lines
This is probably not what you want.
this function logical is quite simple. check the
sem->countbefore cmpxchg is no harm this logical.So could you like to tell us what should we want?
You are comparing the address, not the value. Remove the & This was a nop too.
-Andi
Signature · 7 lines
[email protected] -- Speaking for myself only. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 16 lines · Andi Kleen
Quoted 16 lines
Quoted 16 lines
Quoted 15 lines
include/asm-generic/rwsem.h +0
-0Quoted 14 lines
Quoted 10 lines
include/asm-generic/rwsem.h static inline void __down_write(struct · line 74
Quoted 10 lines
index bb1e2cd..052d973 100644
include/asm-generic/rwsem.h +1
-1@@ -70,11 +70,11 @@ static inline void __down_write(struct rw_semaphore *sem)static inline int __down_write_trylock (struct rw_semaphore *sem) { -long tmp; +if (unlikely (&sem->count !=RWSEM_UNLOCKED_VALUE ))^^^^^^^^^^^This is probably not what you want.
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
this function logical is quite simple. check the
sem->countbefore cmpxchg is no harm this logical.So could you like to tell us what should we want?
You are comparing the address, not the value. Remove the & This was a nop too.
ops, So stupid I am! :(
Signature · 8 lines
Thanks
Alex
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>PATCH 2/2 rwsem: do optimistic spinning for writer lock acquisition
In reply to a message not in the archiveIntroduce in this patch optimistic spinning for writer lock acquisition in read write semaphore. The logic is similar to the optimistic spinning in mutex but without the MCS lock queueing of the spinner. This provides a better chance for a writer to acquire the lock before being we block it and put it to sleep.
Disabling of pre-emption during optimistic spinning was suggested by Davidlohr Bueso. It improved performance of aim7 for his test suite.
Combined with the patch to avoid unnecesary cmpxchg,
in testing by Davidlohr Bueso on aim7 workloads
on 8 socket 80 cores system, he saw improvements of alltests (+14.5%), custom (+17%), disk (+11%), high_systime (+5%), shared (+15%) and short (+4%), most of them after around 500 users when he implemented i_mmap as rwsem.
--- Makefile | 2 +- include/linux/rwsem.h | 3 + init/Kconfig | 9 +++ kernel/rwsem.c | 29 +++++++++- lib/rwsem.c | 148 +++++++++++++++++++++++++++++++++++++++++++++---- 5 files changed, 178 insertions(+), 13 deletions(-)
Makefile +1 -1
@@ -1,7 +1,7 @@ VERSION = 3 PATCHLEVEL = 10 SUBLEVEL = 0 -EXTRAVERSION = -rc4 +EXTRAVERSION = -rc4-optspin4 NAME = Unicycling Gorilla # *DOCUMENTATION*
include/linux/rwsem.h +3 -0
@@ -29,6 +29,9 @@ struct rw_semaphore {#ifdef CONFIG_DEBUG_LOCK_ALLOC struct lockdep_map dep_map;#endif +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +struct task_struct *owner; +#endif };extern struct rw_semaphore *rwsem_down_read_failed (struct rw_semaphore *sem);
init/Kconfig +9 -0
@@ -1595,6 +1595,15 @@ config TRACEPOINTS source "arch/Kconfig" +config RWSEM_SPIN_ON_WRITE_OWNER + bool "Optimistic spin write acquisition for writer owned rw-sem" + default n + depends on SMP + help + Allows a writer to perform optimistic spinning if another writer own + the read write semaphore. This gives a greater chance for writer to + acquire a semaphore before blocking it and putting it to sleep. + endmenu # General setup config HAVE_GENERIC_DMA_COHERENT
kernel/rwsem.c +28 -1
@@ -12,6 +12,26 @@#include <linux/atomic.h> +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +static inline void rwsem_set_owner (struct rw_semaphore *sem) +{ + sem->owner = current; +} + +static inline void rwsem_clear_owner (struct rw_semaphore *sem) +{ + sem->owner =NULL ; +} +#else +static inline void rwsem_set_owner (struct rw_semaphore *sem) +{ +} + +static inline void rwsem_clear_owner (struct rw_semaphore *sem) +{ +} +#endif +/* * lock for reading */
@@ -48,6 +68,7 @@ void __sched down_write(struct rw_semaphore *sem)rwsem_acquire (&sem->dep_map,0 ,0 , _RET_IP_);LOCK_CONTENDED (sem, __down_write_trylock, __down_write); +rwsem_set_owner (sem); }EXPORT_SYMBOL (down_write);
@@ -59,8 +80,10 @@ int down_write_trylock(struct rw_semaphore *sem) {int ret =__down_write_trylock (sem); -if (ret ==1 ) +if (ret ==1 ) {rwsem_acquire (&sem->dep_map,0 ,1 , _RET_IP_); +rwsem_set_owner (sem); + }return ret; }
@@ -86,6 +109,7 @@ void up_write(struct rw_semaphore *sem)rwsem_release (&sem->dep_map,1 , _RET_IP_);__up_write (sem); +rwsem_clear_owner (sem); }EXPORT_SYMBOL (up_write);
@@ -100,6 +124,7 @@ void downgrade_write(struct rw_semaphore *sem)* dependency. */ __downgrade_write (sem); +rwsem_clear_owner (sem); }EXPORT_SYMBOL (downgrade_write);
@@ -122,6 +147,7 @@ void _down_write_nest_lock(struct rw_semaphore *sem, struct lockdep_map *nest)rwsem_acquire_nest (&sem->dep_map,0 ,0 , nest, _RET_IP_);LOCK_CONTENDED (sem, __down_write_trylock, __down_write); +rwsem_set_owner (sem); }EXPORT_SYMBOL (_down_write_nest_lock);
@@ -141,6 +167,7 @@ void down_write_nested(struct rw_semaphore *sem, int subclass)rwsem_acquire (&sem->dep_map, subclass,0 , _RET_IP_);LOCK_CONTENDED (sem, __down_write_trylock, __down_write); +rwsem_set_owner (sem); }EXPORT_SYMBOL (down_write_nested);
lib/rwsem.c +137 -11
181 lines not on this page · raw message
Signature · 8 lines
1.7.4.4 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 2/2] rwsem: do optimistic spinning for writer lock acquisition
Quoted 11 lines · Tim Chen
Introduce in this patch optimistic spinning for writer lock acquisition in read write semaphore. The logic is similar to the optimistic spinning in mutex but without the MCS lock queueing of the spinner. This provides a better chance for a writer to acquire the lock before being we block it and put it to sleep.
Disabling of pre-emption during optimistic spinning was suggested by Davidlohr Bueso. It improved performance of aim7 for his test suite.
Quoted 8 lines
Combined with the patch to avoid unnecesary cmpxchg,
in testing by Davidlohr Bueso on aim7 workloads
on 8 socket 80 cores system, he saw improvements of alltests (+14.5%), custom (+17%), disk (+11%),high_systime(+5%), shared (+15%) and short (+4%), most of them after around 500 users when he implementedi_mmapas rwsem.Signed-off-by Tim Chen <[email protected]>
Quoted 318 lines
--- Makefile | 2 +- include/linux/rwsem.h | 3 + init/Kconfig | 9 +++ kernel/rwsem.c | 29 +++++++++- lib/rwsem.c | 148 +++++++++++++++++++++++++++++++++++++++++++++---- 5 files changed, 178 insertions(+), 13 deletions(-)Makefile +1
-1@@ -1,7 +1,7 @@ VERSION = 3 PATCHLEVEL = 10 SUBLEVEL = 0 -EXTRAVERSION = -rc4 +EXTRAVERSION = -rc4-optspin4 NAME = Unicycling Gorilla # *DOCUMENTATION*include/linux/rwsem.h +3
-0@@ -29,6 +29,9 @@ struct rw_semaphore {#ifdef CONFIG_DEBUG_LOCK_ALLOC struct lockdep_map dep_map;#endif +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +struct task_struct *owner; +#endif };extern struct rw_semaphore *rwsem_down_read_failed (struct rw_semaphore *sem);init/Kconfig +9
-0@@ -1595,6 +1595,15 @@ config TRACEPOINTS source "arch/Kconfig" +config RWSEM_SPIN_ON_WRITE_OWNER + bool "Optimistic spin write acquisition for writer owned rw-sem" + default n + depends on SMP + help + Allows a writer to perform optimistic spinning if another writer own + the read write semaphore. This gives a greater chance for writer to + acquire a semaphore before blocking it and putting it to sleep. + endmenu # General setup config HAVE_GENERIC_DMA_COHERENTkernel/rwsem.c +28
-1@@ -12,6 +12,26 @@#include <linux/atomic.h> +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +static inline void rwsem_set_owner (struct rw_semaphore *sem) +{ + sem->owner = current; +} + +static inline void rwsem_clear_owner (struct rw_semaphore *sem) +{ + sem->owner =NULL ; +} +#else +static inline void rwsem_set_owner (struct rw_semaphore *sem) +{ +} + +static inline void rwsem_clear_owner (struct rw_semaphore *sem) +{ +} +#endif +/* * lock for reading */ @@ -48,6 +68,7 @@ void __sched down_write(struct rw_semaphore *sem)rwsem_acquire (&sem->dep_map,0 ,0 , _RET_IP_);LOCK_CONTENDED (sem, __down_write_trylock, __down_write); +rwsem_set_owner (sem); }EXPORT_SYMBOL (down_write);@@ -59,8 +80,10 @@ int down_write_trylock(struct rw_semaphore *sem) {int ret =__down_write_trylock (sem); -if (ret ==1 ) +if (ret ==1 ) {rwsem_acquire (&sem->dep_map,0 ,1 , _RET_IP_); +rwsem_set_owner (sem); + }return ret; }@@ -86,6 +109,7 @@ void up_write(struct rw_semaphore *sem)rwsem_release (&sem->dep_map,1 , _RET_IP_);__up_write (sem); +rwsem_clear_owner (sem); }EXPORT_SYMBOL (up_write);@@ -100,6 +124,7 @@ void downgrade_write(struct rw_semaphore *sem)* dependency. */ __downgrade_write (sem); +rwsem_clear_owner (sem); }EXPORT_SYMBOL (downgrade_write);@@ -122,6 +147,7 @@ void _down_write_nest_lock(struct rw_semaphore *sem, struct lockdep_map *nest)rwsem_acquire_nest (&sem->dep_map,0 ,0 , nest, _RET_IP_);LOCK_CONTENDED (sem, __down_write_trylock, __down_write); +rwsem_set_owner (sem); }EXPORT_SYMBOL (_down_write_nest_lock);@@ -141,6 +167,7 @@ void down_write_nested(struct rw_semaphore *sem, int subclass)rwsem_acquire (&sem->dep_map, subclass,0 , _RET_IP_);LOCK_CONTENDED (sem, __down_write_trylock, __down_write); +rwsem_set_owner (sem); }EXPORT_SYMBOL (down_write_nested);lib/rwsem.c +131
-11@@ -8,6 +8,7 @@*/ #include <linux/rwsem.h> #include <linux/sched.h> +#include <linux/sched/rt.h> #include <linux/init.h> #include <linux/export.h> @@ -27,6 +28,9 @@ void __init_rwsem(struct rw_semaphore *sem, const char *name, sem->count =RWSEM_UNLOCKED_VALUE ;raw_spin_lock_init (&sem->wait_lock);INIT_LIST_HEAD (&sem->wait_list); +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER + sem->owner =NULL ; +#endif }EXPORT_SYMBOL (__init_rwsem);@@ -192,6 +196,128 @@ struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem)return sem; } +static inline int rwsem_try_write_lock (long count,bool need_lock, +struct rw_semaphore *sem) +{ +if (!(count &RWSEM_ACTIVE_MASK )) { +/* Try acquiring the write lock. */ +if (sem->count ==RWSEM_WAITING_BIAS && +cmpxchg (&sem->count,RWSEM_WAITING_BIAS , +RWSEM_ACTIVE_WRITE_BIAS ) ==RWSEM_WAITING_BIAS ) { +if (need_lock) +raw_spin_lock_irq (&sem->wait_lock); +if (!list_is_singular (&sem->wait_list)) +rwsem_atomic_update (RWSEM_WAITING_BIAS , sem); +return 1 ; + } + } +return 0 ; +} + +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +static inline bool rwsem_can_spin_on_owner (struct rw_semaphore *sem) +{ +int retval = true; + +/* Spin only if active writer running */ +if (!sem->owner) +return false; + +rcu_read_lock (); +if (sem->owner) + retval = sem->owner->on_cpu; +rcu_read_unlock (); +/* +* if lock->owner is not set, the sem owner may have just acquired +* it and not set the owner yet, or the sem has been released, or +* reader active. +*/ +return retval; +} + +static inline bool owner_running (struct rw_semaphore *lock, +struct task_struct *owner) +{ +if (lock->owner != owner) +return false; + +/* +* Ensure we emit the owner->on_cpu, dereference _after_ checking +* lock->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 (); + +return owner->on_cpu; +} + +static noinline +int rwsem_spin_on_owner(struct rw_semaphore *lock, struct task_struct *owner) +{ +rcu_read_lock (); +while (owner_running (lock, owner)) { +if (need_resched ()) +break ; + +arch_mutex_cpu_relax (); + } +rcu_read_unlock (); + +/* +* We break out the loop above on need_resched() and when the +* owner changed, which is a sign for heavy contention. Return +* success only when lock->owner is NULL. +*/ +return lock->owner ==NULL ; +} + +int rwsem_optimistic_spin (struct rw_semaphore *sem) +{ +struct task_struct *owner; +int ret =0 ; + +/* sem->wait_lock should not be held when doing optimistic spinning */ +if (!rwsem_can_spin_on_owner (sem)) +return ret; + +preempt_disable (); +for (;;) { + owner =ACCESS_ONCE (sem->owner); +if (owner && !rwsem_spin_on_owner (sem, owner)) +break ; + +/* wait_lock will be acquired if write_lock is obtained */ +if (rwsem_try_write_lock (sem->count, true, sem)) { + ret =1 ; + goto out; + } + +/* +* When there's no owner, we might have preempted between the +* owner acquiring the lock and setting the owner field. If +* we're an RT task that will live-lock because we won't let +* the owner complete. +*/ +if (!owner && (need_resched () ||rt_task (current))) +break ; + +/* +* The cpu_relax() call is a compiler barrier which forces +* everything in this loop to be re-loaded. We don't need +* memory barriers as we'll eventually observe the right +* values at the cost of a few extra spins. +*/ +arch_mutex_cpu_relax (); + + } + +out: +preempt_enable (); +return ret; +} +#endif +/* * wait until we successfully acquire the write lock */ @@ -200,6 +326,9 @@ struct rw_semaphore __sched *rwsem_down_write_failed(struct rw_semaphore *sem)long count, adjustment = -RWSEM_ACTIVE_WRITE_BIAS ;struct rwsem_waiter waiter;struct task_struct *tsk = current; +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +bool try_optimistic_spin = true; +#endif /* set up my own style of waitqueue */ waiter.task = tsk;@@ -223,20 +352,17 @@ struct rw_semaphore __sched *rwsem_down_write_failed(struct rw_semaphore *sem)/* wait until we successfully acquire the lock */ set_task_state (tsk,TASK_UNINTERRUPTIBLE );while (true) { -if (!(count &RWSEM_ACTIVE_MASK )) { -/* Try acquiring the write lock. */ - count =RWSEM_ACTIVE_WRITE_BIAS ; -if (!list_is_singular (&sem->wait_list)) - count +=RWSEM_WAITING_BIAS ; - -if (sem->count ==RWSEM_WAITING_BIAS && -cmpxchg (&sem->count,RWSEM_WAITING_BIAS , count) == -RWSEM_WAITING_BIAS ) -break ; - } +if (rwsem_try_write_lock (count, false, sem)) +break ;raw_spin_unlock_irq (&sem->wait_lock);
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_write_failed(struct rw_semaphore *sem) · line 368
Quoted 10 lines
lib/rwsem.c +6
-0+#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +/* do optimistic spinning */ +if (try_optimistic_spin &&rwsem_optimistic_spin (sem)) +break ; + try_optimistic_spin = false; +#endif /* Block until there are no active lockers. */ do { schedule();
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 2/2] rwsem: do optimistic spinning for writer lock acquisition
In reply to Tim ChenQuoted 6 lines · Tim Chen
Introduce in this patch optimistic spinning for writer lock acquisition in read write semaphore. The logic is similar to the optimistic spinning in mutex but without the MCS lock queueing of the spinner. This provides a better chance for a writer to acquire the lock before being we block it and put it to sleep.
This is just my opinion but I'd rather read the justification here instead of referencing mutex logic that may or may not exist in 2 years.
Quoted 196 lines
Disabling of pre-emption during optimistic spinning was suggested by Davidlohr Bueso. It improved performance of aim7 for his test suite.
Combined with the patch to avoid unnecesary cmpxchg,
in testing by Davidlohr Bueso on aim7 workloads
on 8 socket 80 cores system, he saw improvements of alltests (+14.5%), custom (+17%), disk (+11%),high_systime(+5%), shared (+15%) and short (+4%), most of them after around 500 users when he implementedi_mmapas rwsem.Signed-off-by Tim Chen <[email protected]>--- Makefile | 2 +- include/linux/rwsem.h | 3 + init/Kconfig | 9 +++ kernel/rwsem.c | 29 +++++++++- lib/rwsem.c | 148 +++++++++++++++++++++++++++++++++++++++++++++---- 5 files changed, 178 insertions(+), 13 deletions(-)Makefile +1
-1@@ -1,7 +1,7 @@ VERSION = 3 PATCHLEVEL = 10 SUBLEVEL = 0 -EXTRAVERSION = -rc4 +EXTRAVERSION = -rc4-optspin4 NAME = Unicycling Gorilla # *DOCUMENTATION*include/linux/rwsem.h +3
-0@@ -29,6 +29,9 @@ struct rw_semaphore {#ifdef CONFIG_DEBUG_LOCK_ALLOC struct lockdep_map dep_map;#endif +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +struct task_struct *owner; +#endif };extern struct rw_semaphore *rwsem_down_read_failed (struct rw_semaphore *sem);init/Kconfig +9
-0@@ -1595,6 +1595,15 @@ config TRACEPOINTS source "arch/Kconfig" +config RWSEM_SPIN_ON_WRITE_OWNER + bool "Optimistic spin write acquisition for writer owned rw-sem" + default n + depends on SMP + help + Allows a writer to perform optimistic spinning if another writer own + the read write semaphore. This gives a greater chance for writer to + acquire a semaphore before blocking it and putting it to sleep. + endmenu # General setup config HAVE_GENERIC_DMA_COHERENTkernel/rwsem.c +28
-1@@ -12,6 +12,26 @@#include <linux/atomic.h> +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +static inline void rwsem_set_owner (struct rw_semaphore *sem) +{ + sem->owner = current; +} + +static inline void rwsem_clear_owner (struct rw_semaphore *sem) +{ + sem->owner =NULL ; +} +#else +static inline void rwsem_set_owner (struct rw_semaphore *sem) +{ +} + +static inline void rwsem_clear_owner (struct rw_semaphore *sem) +{ +} +#endif +/* * lock for reading */ @@ -48,6 +68,7 @@ void __sched down_write(struct rw_semaphore *sem)rwsem_acquire (&sem->dep_map,0 ,0 , _RET_IP_);LOCK_CONTENDED (sem, __down_write_trylock, __down_write); +rwsem_set_owner (sem); }EXPORT_SYMBOL (down_write);@@ -59,8 +80,10 @@ int down_write_trylock(struct rw_semaphore *sem) {int ret =__down_write_trylock (sem); -if (ret ==1 ) +if (ret ==1 ) {rwsem_acquire (&sem->dep_map,0 ,1 , _RET_IP_); +rwsem_set_owner (sem); + }return ret; }@@ -86,6 +109,7 @@ void up_write(struct rw_semaphore *sem)rwsem_release (&sem->dep_map,1 , _RET_IP_);__up_write (sem); +rwsem_clear_owner (sem); }EXPORT_SYMBOL (up_write);@@ -100,6 +124,7 @@ void downgrade_write(struct rw_semaphore *sem)* dependency. */ __downgrade_write (sem); +rwsem_clear_owner (sem); }EXPORT_SYMBOL (downgrade_write);@@ -122,6 +147,7 @@ void _down_write_nest_lock(struct rw_semaphore *sem, struct lockdep_map *nest)rwsem_acquire_nest (&sem->dep_map,0 ,0 , nest, _RET_IP_);LOCK_CONTENDED (sem, __down_write_trylock, __down_write); +rwsem_set_owner (sem); }EXPORT_SYMBOL (_down_write_nest_lock);@@ -141,6 +167,7 @@ void down_write_nested(struct rw_semaphore *sem, int subclass)rwsem_acquire (&sem->dep_map, subclass,0 , _RET_IP_);LOCK_CONTENDED (sem, __down_write_trylock, __down_write); +rwsem_set_owner (sem); }EXPORT_SYMBOL (down_write_nested);lib/rwsem.c +24
-0@@ -8,6 +8,7 @@*/ #include <linux/rwsem.h> #include <linux/sched.h> +#include <linux/sched/rt.h> #include <linux/init.h> #include <linux/export.h> @@ -27,6 +28,9 @@ void __init_rwsem(struct rw_semaphore *sem, const char *name, sem->count =RWSEM_UNLOCKED_VALUE ;raw_spin_lock_init (&sem->wait_lock);INIT_LIST_HEAD (&sem->wait_list); +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER + sem->owner =NULL ; +#endif }EXPORT_SYMBOL (__init_rwsem);@@ -192,6 +196,128 @@ struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem)return sem; } +static inline int rwsem_try_write_lock (long count,bool need_lock, +struct rw_semaphore *sem) +{ +if (!(count &RWSEM_ACTIVE_MASK )) { +/* Try acquiring the write lock. */ +if (sem->count ==RWSEM_WAITING_BIAS && +cmpxchg (&sem->count,RWSEM_WAITING_BIAS , +RWSEM_ACTIVE_WRITE_BIAS ) ==RWSEM_WAITING_BIAS ) { +if (need_lock) +raw_spin_lock_irq (&sem->wait_lock); +if (!list_is_singular (&sem->wait_list)) +rwsem_atomic_update (RWSEM_WAITING_BIAS , sem); +return 1 ; + } + } +return 0 ; +} + +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +static inline bool rwsem_can_spin_on_owner (struct rw_semaphore *sem)
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 228
Quoted 10 lines
lib/rwsem.c +10
-0+{ +int retval = true; + +/* Spin only if active writer running */ +if (!sem->owner) +return false; + +rcu_read_lock (); +if (sem->owner) + retval = sem->owner->on_cpu;
^^^^^^^^^^^^^^^^^^
Why is this a safe dereference? Could not another cpu have just dropped the sem (and thus set sem->owner to NULL and oops)?
Quoted 11 lines
lib/rwsem.c +11
-0+rcu_read_unlock (); +/* +* if lock->owner is not set, the sem owner may have just acquired +* it and not set the owner yet, or the sem has been released, or +* reader active. +*/ +return retval; +} + +static inline bool owner_running (struct rw_semaphore *lock, +struct task_struct *owner)
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 249
Quoted 10 lines
lib/rwsem.c +10
-0+{ +if (lock->owner != owner) +return false; + +/* +* Ensure we emit the owner->on_cpu, dereference _after_ checking +* lock->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. +*/
Again just my opinion, but kernel style is to prefer multi-line comments in a function comment block.
Quoted 29 lines
lib/rwsem.c +29
-0+barrier (); + +return owner->on_cpu; +} + +static noinline +int rwsem_spin_on_owner(struct rw_semaphore *lock, struct task_struct *owner) +{ +rcu_read_lock (); +while (owner_running (lock, owner)) { +if (need_resched ()) +break ; + +arch_mutex_cpu_relax (); + } +rcu_read_unlock (); + +/* +* We break out the loop above on need_resched() and when the +* owner changed, which is a sign for heavy contention. Return +* success only when lock->owner is NULL. +*/ +return lock->owner ==NULL ; +} + +int rwsem_optimistic_spin (struct rw_semaphore *sem) +{ +struct task_struct *owner; +int ret =0 ;
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 288
Quoted 10 lines
lib/rwsem.c +10
-0+ +/* sem->wait_lock should not be held when doing optimistic spinning */ +if (!rwsem_can_spin_on_owner (sem)) +return ret; + +preempt_disable (); +for (;;) { + owner =ACCESS_ONCE (sem->owner); +if (owner && !rwsem_spin_on_owner (sem, owner)) +break ;
Will this spin for full scheduler value on a reader-owned lock?
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 297
Quoted 8 lines
lib/rwsem.c +8
-0+/* wait_lock will be acquired if write_lock is obtained */ +if (rwsem_try_write_lock (sem->count, true, sem)) { + ret =1 ; + goto out; + } + +/* +* When there's no owner, we might have preempted between the
^^^^^^^^
Isn't pre-emption disabled?
Quoted 55 lines
lib/rwsem.c +28
-11+* owner acquiring the lock and setting the owner field. If +* we're an RT task that will live-lock because we won't let +* the owner complete. +*/ +if (!owner && (need_resched () ||rt_task (current))) +break ; + +/* +* The cpu_relax() call is a compiler barrier which forces +* everything in this loop to be re-loaded. We don't need +* memory barriers as we'll eventually observe the right +* values at the cost of a few extra spins. +*/ + arch_mutex_cpu_relax(); + + } + +out: +preempt_enable (); +return ret; +} +#endif +/* * wait until we successfully acquire the write lock */ @@ -200,6 +326,9 @@ struct rw_semaphore __sched *rwsem_down_write_failed(struct rw_semaphore *sem)long count, adjustment = -RWSEM_ACTIVE_WRITE_BIAS ;struct rwsem_waiter waiter;struct task_struct *tsk = current; +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +bool try_optimistic_spin = true; +#endif /* set up my own style of waitqueue */ waiter.task = tsk;@@ -223,20 +352,17 @@ struct rw_semaphore __sched *rwsem_down_write_failed(struct rw_semaphore *sem)/* wait until we successfully acquire the lock */ set_task_state (tsk,TASK_UNINTERRUPTIBLE );while (true) { -if (!(count &RWSEM_ACTIVE_MASK )) { -/* Try acquiring the write lock. */ - count =RWSEM_ACTIVE_WRITE_BIAS ; -if (!list_is_singular (&sem->wait_list)) - count +=RWSEM_WAITING_BIAS ; - -if (sem->count ==RWSEM_WAITING_BIAS && -cmpxchg (&sem->count,RWSEM_WAITING_BIAS , count) == -RWSEM_WAITING_BIAS ) -break ; - } +if (rwsem_try_write_lock (count, false, sem)) +break ;raw_spin_unlock_irq (&sem->wait_lock);
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_write_failed(struct rw_semaphore *sem) · line 368
Quoted 10 lines
lib/rwsem.c +6
-0+#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +/* do optimistic spinning */ +if (try_optimistic_spin &&rwsem_optimistic_spin (sem)) +break ; + try_optimistic_spin = false; +#endif /* Block until there are no active lockers. */ do { schedule();
Regards,
Peter Hurley
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 2/2] rwsem: do optimistic spinning for writer lock acquisition
Quoted 11 lines · Peter Hurley
Quoted 6 lines · Tim Chen
Introduce in this patch optimistic spinning for writer lock acquisition in read write semaphore. The logic is similar to the optimistic spinning in mutex but without the MCS lock queueing of the spinner. This provides a better chance for a writer to acquire the lock before being we block it and put it to sleep.
This is just my opinion but I'd rather read the justification here instead of referencing mutex logic that may or may not exist in 2 years.
We want to add optimistic spinning to rwsems because we've noticed that the writer rwsem does not perform as well as mutexes. Tim noticed that for exim (mail server) workloads, when reverting commit 4fc3f1d6 and I noticed it when converting the i_mmap_mutex to a rwsem in some aim7 workloads. We've noticed that the biggest difference, in a nutshell, is when we fail to acquire a mutex in the fastpath, optimistic spinning comes in to play and we can avoid a large amount of unnecessary sleeping and wait queue overhead.
For rwsems on the other hand, upon entering the writer slowpath in rwsem_down_write_failed(), we just acquire the ->wait_lock, add ourselves to the wait_queue and blocking until we get the lock.
Makes sense?
More information from the original thread:
https://lkml.org/lkml/2013/6/21/482
Quoted 22 lines
Quoted 22 lines
Disabling of pre-emption during optimistic spinning was suggested by Davidlohr Bueso. It improved performance of aim7 for his test suite.
Combined with the patch to avoid unnecesary cmpxchg,
in testing by Davidlohr Bueso on aim7 workloads
on 8 socket 80 cores system, he saw improvements of alltests (+14.5%), custom (+17%), disk (+11%),high_systime(+5%), shared (+15%) and short (+4%), most of them after around 500 users when he implementedi_mmapas rwsem.Signed-off-by Tim Chen <[email protected]>--- Makefile | 2 +- include/linux/rwsem.h | 3 + init/Kconfig | 9 +++ kernel/rwsem.c | 29 +++++++++- lib/rwsem.c | 148 +++++++++++++++++++++++++++++++++++++++++++++---- 5 files changed, 178 insertions(+), 13 deletions(-)
Quoted 11 lines
Makefile line 5
Quoted 11 lines
Makefile +1
-1@@ -1,7 +1,7 @@ VERSION = 3 PATCHLEVEL = 10 SUBLEVEL = 0 -EXTRAVERSION = -rc4 +EXTRAVERSION = -rc4-optspin4 NAME = Unicycling Gorilla
This must obviously go.
Quoted 24 lines
Quoted 24 lines
Makefile +0
-0# *DOCUMENTATION*include/linux/rwsem.h +3
-0@@ -29,6 +29,9 @@ struct rw_semaphore {#ifdef CONFIG_DEBUG_LOCK_ALLOC struct lockdep_map dep_map;#endif +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +struct task_struct *owner; +#endif };extern struct rw_semaphore *rwsem_down_read_failed (struct rw_semaphore *sem);init/Kconfig +0
-0@@ -1595,6 +1595,15 @@ config TRACEPOINTS source "arch/Kconfig"
Quoted 10 lines
init/Kconfig config TRACEPOINTS · line 1607
Quoted 10 lines
init/Kconfig +9
-0+config RWSEM_SPIN_ON_WRITE_OWNER + bool "Optimistic spin write acquisition for writer owned rw-sem" + default n + depends on SMP + help + Allows a writer to perform optimistic spinning if another writer own + the read write semaphore. This gives a greater chance for writer to + acquire a semaphore before blocking it and putting it to sleep. + endmenu # General setup
Can't we just use CONFIG_SMP insted of adding a new Kconfig variable?
Quoted 172 lines
Quoted 131 lines
init/Kconfig +0
-0config HAVE_GENERIC_DMA_COHERENTkernel/rwsem.c +28
-1@@ -12,6 +12,26 @@#include <linux/atomic.h> +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +static inline void rwsem_set_owner (struct rw_semaphore *sem) +{ + sem->owner = current; +} + +static inline void rwsem_clear_owner (struct rw_semaphore *sem) +{ + sem->owner =NULL ; +} +#else +static inline void rwsem_set_owner (struct rw_semaphore *sem) +{ +} + +static inline void rwsem_clear_owner (struct rw_semaphore *sem) +{ +} +#endif +/* * lock for reading */ @@ -48,6 +68,7 @@ void __sched down_write(struct rw_semaphore *sem)rwsem_acquire (&sem->dep_map,0 ,0 , _RET_IP_);LOCK_CONTENDED (sem, __down_write_trylock, __down_write); +rwsem_set_owner (sem); }EXPORT_SYMBOL (down_write);@@ -59,8 +80,10 @@ int down_write_trylock(struct rw_semaphore *sem) {int ret =__down_write_trylock (sem); -if (ret ==1 ) +if (ret ==1 ) {rwsem_acquire (&sem->dep_map,0 ,1 , _RET_IP_); +rwsem_set_owner (sem); + }return ret; }@@ -86,6 +109,7 @@ void up_write(struct rw_semaphore *sem)rwsem_release (&sem->dep_map,1 , _RET_IP_);__up_write (sem); +rwsem_clear_owner (sem); }EXPORT_SYMBOL (up_write);@@ -100,6 +124,7 @@ void downgrade_write(struct rw_semaphore *sem)* dependency. */ __downgrade_write (sem); +rwsem_clear_owner (sem); }EXPORT_SYMBOL (downgrade_write);@@ -122,6 +147,7 @@ void _down_write_nest_lock(struct rw_semaphore *sem, struct lockdep_map *nest)rwsem_acquire_nest (&sem->dep_map,0 ,0 , nest, _RET_IP_);LOCK_CONTENDED (sem, __down_write_trylock, __down_write); +rwsem_set_owner (sem); }EXPORT_SYMBOL (_down_write_nest_lock);@@ -141,6 +167,7 @@ void down_write_nested(struct rw_semaphore *sem, int subclass)rwsem_acquire (&sem->dep_map, subclass,0 , _RET_IP_);LOCK_CONTENDED (sem, __down_write_trylock, __down_write); +rwsem_set_owner (sem); }EXPORT_SYMBOL (down_write_nested);lib/rwsem.c +24
-0@@ -8,6 +8,7 @@*/ #include <linux/rwsem.h> #include <linux/sched.h> +#include <linux/sched/rt.h> #include <linux/init.h> #include <linux/export.h> @@ -27,6 +28,9 @@ void __init_rwsem(struct rw_semaphore *sem, const char *name, sem->count =RWSEM_UNLOCKED_VALUE ;raw_spin_lock_init (&sem->wait_lock);INIT_LIST_HEAD (&sem->wait_list); +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER + sem->owner =NULL ; +#endif }EXPORT_SYMBOL (__init_rwsem);@@ -192,6 +196,128 @@ struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem)return sem; } +static inline int rwsem_try_write_lock (long count,bool need_lock, +struct rw_semaphore *sem) +{ +if (!(count &RWSEM_ACTIVE_MASK )) { +/* Try acquiring the write lock. */ +if (sem->count ==RWSEM_WAITING_BIAS && +cmpxchg (&sem->count,RWSEM_WAITING_BIAS , +RWSEM_ACTIVE_WRITE_BIAS ) ==RWSEM_WAITING_BIAS ) { +if (need_lock) +raw_spin_lock_irq (&sem->wait_lock); +if (!list_is_singular (&sem->wait_list)) +rwsem_atomic_update (RWSEM_WAITING_BIAS , sem); +return 1 ; + } + } +return 0 ; +} + +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +static inline bool rwsem_can_spin_on_owner (struct rw_semaphore *sem)lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 228
Quoted 10 lines
lib/rwsem.c +10
-0+{ +int retval = true; + +/* Spin only if active writer running */ +if (!sem->owner) +return false; + +rcu_read_lock (); +if (sem->owner) + retval = sem->owner->on_cpu;^^^^^^^^^^^^^^^^^^Why is this a safe dereference? Could not another cpu have just dropped the sem (and thus set
sem->ownerto NULL and oops)?Quoted 11 lines
lib/rwsem.c +11
-0+rcu_read_unlock (); +/* +* if lock->owner is not set, the sem owner may have just acquired +* it and not set the owner yet, or the sem has been released, or +* reader active. +*/ +return retval; +} + +static inline bool owner_running (struct rw_semaphore *lock, +struct task_struct *owner)lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 249
Quoted 10 lines
lib/rwsem.c +10
-0+{ +if (lock->owner != owner) +return false; + +/* +* Ensure we emit the owner->on_cpu, dereference _after_ checking +* lock->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. +*/ Again just my opinion, but kernel style is to prefer multi-line comments in a function comment block.
Quoted 4 lines
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 253
Quoted 4 lines
lib/rwsem.c +4
-0+barrier (); + +return owner->on_cpu; +}
A lot of these functions are exact duplicates of kernel/mutex.c - we should probably think of adding generic interfaces for mutexes and rwsems...
Quoted 12 lines
Quoted 12 lines
lib/rwsem.c +12
-0+ +static noinline +int rwsem_spin_on_owner(struct rw_semaphore *lock, struct task_struct *owner) +{ +rcu_read_lock (); +while (owner_running (lock, owner)) { +if (need_resched ()) +break ; + +arch_mutex_cpu_relax (); + } +rcu_read_unlock ();
Quoted 8 lines
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 273
Quoted 8 lines
lib/rwsem.c +8
-0+ +/* +* We break out the loop above on need_resched() and when the +* owner changed, which is a sign for heavy contention. Return +* success only when lock->owner is NULL. +*/ +return lock->owner ==NULL ; +}
ditto
Quoted 9 lines
Quoted 9 lines
lib/rwsem.c +9
-0+ +int rwsem_optimistic_spin (struct rw_semaphore *sem) +{ +struct task_struct *owner; +int ret =0 ; + +/* sem->wait_lock should not be held when doing optimistic spinning */ +if (!rwsem_can_spin_on_owner (sem)) +return ret;
Quoted 8 lines
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 288
Quoted 6 lines
lib/rwsem.c +6
-0+ +preempt_disable (); +for (;;) { + owner =ACCESS_ONCE (sem->owner); +if (owner && !rwsem_spin_on_owner (sem, owner)) +break ;Will this spin for full scheduler value on a reader-owned lock?
Yep, it should.
Quoted 11 lines
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 297
Quoted 9 lines
lib/rwsem.c +8
-0+/* wait_lock will be acquired if write_lock is obtained */ +if (rwsem_try_write_lock (sem->count, true, sem)) { + ret =1 ; + goto out; + } + +/* +* When there's no owner, we might have preempted between the ^^^^^^^^
Quoted 1 line
Isn't pre-emption disabled?
Hmm yeah, that might be a bogus comment.
Quoted 58 lines
Quoted 58 lines
lib/rwsem.c +28
-11+* owner acquiring the lock and setting the owner field. If +* we're an RT task that will live-lock because we won't let +* the owner complete. +*/ +if (!owner && (need_resched () ||rt_task (current))) +break ; + +/* +* The cpu_relax() call is a compiler barrier which forces +* everything in this loop to be re-loaded. We don't need +* memory barriers as we'll eventually observe the right +* values at the cost of a few extra spins. +*/ + arch_mutex_cpu_relax(); + + } + +out: +preempt_enable (); +return ret; +} +#endif +/* * wait until we successfully acquire the write lock */ @@ -200,6 +326,9 @@ struct rw_semaphore __sched *rwsem_down_write_failed(struct rw_semaphore *sem)long count, adjustment = -RWSEM_ACTIVE_WRITE_BIAS ;struct rwsem_waiter waiter;struct task_struct *tsk = current; +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +bool try_optimistic_spin = true; +#endif /* set up my own style of waitqueue */ waiter.task = tsk;@@ -223,20 +352,17 @@ struct rw_semaphore __sched *rwsem_down_write_failed(struct rw_semaphore *sem)/* wait until we successfully acquire the lock */ set_task_state (tsk,TASK_UNINTERRUPTIBLE );while (true) { -if (!(count &RWSEM_ACTIVE_MASK )) { -/* Try acquiring the write lock. */ - count =RWSEM_ACTIVE_WRITE_BIAS ; -if (!list_is_singular (&sem->wait_list)) - count +=RWSEM_WAITING_BIAS ; - -if (sem->count ==RWSEM_WAITING_BIAS && -cmpxchg (&sem->count,RWSEM_WAITING_BIAS , count) == -RWSEM_WAITING_BIAS ) -break ; - } +if (rwsem_try_write_lock (count, false, sem)) +break ;raw_spin_unlock_irq (&sem->wait_lock);
Quoted 13 lines
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_write_failed(struct rw_semaphore *sem) · line 368
Quoted 9 lines
lib/rwsem.c +6
-0+#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +/* do optimistic spinning */ +if (try_optimistic_spin &&rwsem_optimistic_spin (sem)) +break ; + try_optimistic_spin = false; +#endif /* Block until there are no active lockers. */ do { schedule();Regards,
Peter Hurley
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 2/2] rwsem: do optimistic spinning for writer lock acquisition
Quoted 22 lines · Davidlohr Bueso
Quoted 11 lines · Peter Hurley
Quoted 6 lines · Tim Chen
Introduce in this patch optimistic spinning for writer lock acquisition in read write semaphore. The logic is similar to the optimistic spinning in mutex but without the MCS lock queueing of the spinner. This provides a better chance for a writer to acquire the lock before being we block it and put it to sleep.
This is just my opinion but I'd rather read the justification here instead of referencing mutex logic that may or may not exist in 2 years.
We want to add optimistic spinning to rwsems because we've noticed that the writer rwsem does not perform as well as mutexes. Tim noticed that for exim (mail server) workloads, when reverting commit
4fc3f1d6and I noticed it when converting thei_mmap_mutexto a rwsem in some aim7 workloads. We've noticed that the biggest difference, in a nutshell, is when we fail to acquire a mutex in the fastpath, optimistic spinning comes in to play and we can avoid a large amount of unnecessary sleeping and wait queue overhead.
Quoted 8 lines
For rwsems on the other hand, upon entering the writer slowpath in
rwsem_down_write_failed(), we just acquire the ->wait_lock, add ourselves to thewait_queueand blocking until we get the lock.Makes sense?
More information from the original thread:
https://lkml.org/lkml/2013/6/21/482
Sounds good.
Quoted 23 lines
Quoted 23 lines
Quoted 23 lines
Disabling of pre-emption during optimistic spinning was suggested by Davidlohr Bueso. It improved performance of aim7 for his test suite.
Combined with the patch to avoid unnecesary cmpxchg,
in testing by Davidlohr Bueso on aim7 workloads
on 8 socket 80 cores system, he saw improvements of alltests (+14.5%), custom (+17%), disk (+11%),high_systime(+5%), shared (+15%) and short (+4%), most of them after around 500 users when he implementedi_mmapas rwsem.Signed-off-by Tim Chen <[email protected]>--- Makefile | 2 +- include/linux/rwsem.h | 3 + init/Kconfig | 9 +++ kernel/rwsem.c | 29 +++++++++- lib/rwsem.c | 148 +++++++++++++++++++++++++++++++++++++++++++++---- 5 files changed, 178 insertions(+), 13 deletions(-)
Quoted 13 lines
Quoted 11 lines
Makefile line 5
Quoted 11 lines
Makefile +1
-1@@ -1,7 +1,7 @@ VERSION = 3 PATCHLEVEL = 10 SUBLEVEL = 0 -EXTRAVERSION = -rc4 +EXTRAVERSION = -rc4-optspin4 NAME = Unicycling GorillaThis must obviously go.
Yes. Should not be there.
Quoted 25 lines
Quoted 25 lines
Quoted 25 lines
Makefile +0
-0# *DOCUMENTATION*include/linux/rwsem.h +3
-0@@ -29,6 +29,9 @@ struct rw_semaphore {#ifdef CONFIG_DEBUG_LOCK_ALLOC struct lockdep_map dep_map;#endif +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +struct task_struct *owner; +#endif };extern struct rw_semaphore *rwsem_down_read_failed (struct rw_semaphore *sem);init/Kconfig +0
-0@@ -1595,6 +1595,15 @@ config TRACEPOINTS source "arch/Kconfig"
Quoted 12 lines
Quoted 10 lines
init/Kconfig config TRACEPOINTS · line 1607
Quoted 10 lines
init/Kconfig +9
-0+config RWSEM_SPIN_ON_WRITE_OWNER + bool "Optimistic spin write acquisition for writer owned rw-sem" + default n + depends on SMP + help + Allows a writer to perform optimistic spinning if another writer own + the read write semaphore. This gives a greater chance for writer to + acquire a semaphore before blocking it and putting it to sleep. + endmenu # General setupCan't we just use
CONFIG_SMPinsted of adding a newKconfigvariable?
I am not comfortable to make the optimistic spinning of rw-sem a default SMP config option yet. I will like it to see more performance testing in the tree. I want the ability to turn it off easily.
Quoted 144 lines
Quoted 144 lines
Quoted 132 lines
init/Kconfig +0
-0config HAVE_GENERIC_DMA_COHERENTkernel/rwsem.c +28
-1@@ -12,6 +12,26 @@#include <linux/atomic.h> +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +static inline void rwsem_set_owner (struct rw_semaphore *sem) +{ + sem->owner = current; +} + +static inline void rwsem_clear_owner (struct rw_semaphore *sem) +{ + sem->owner =NULL ; +} +#else +static inline void rwsem_set_owner (struct rw_semaphore *sem) +{ +} + +static inline void rwsem_clear_owner (struct rw_semaphore *sem) +{ +} +#endif +/* * lock for reading */ @@ -48,6 +68,7 @@ void __sched down_write(struct rw_semaphore *sem)rwsem_acquire (&sem->dep_map,0 ,0 , _RET_IP_);LOCK_CONTENDED (sem, __down_write_trylock, __down_write); +rwsem_set_owner (sem); }EXPORT_SYMBOL (down_write);@@ -59,8 +80,10 @@ int down_write_trylock(struct rw_semaphore *sem) {int ret =__down_write_trylock (sem); -if (ret ==1 ) +if (ret ==1 ) {rwsem_acquire (&sem->dep_map,0 ,1 , _RET_IP_); +rwsem_set_owner (sem); + }return ret; }@@ -86,6 +109,7 @@ void up_write(struct rw_semaphore *sem)rwsem_release (&sem->dep_map,1 , _RET_IP_);__up_write (sem); +rwsem_clear_owner (sem); }EXPORT_SYMBOL (up_write);@@ -100,6 +124,7 @@ void downgrade_write(struct rw_semaphore *sem)* dependency. */ __downgrade_write (sem); +rwsem_clear_owner (sem); }EXPORT_SYMBOL (downgrade_write);@@ -122,6 +147,7 @@ void _down_write_nest_lock(struct rw_semaphore *sem, struct lockdep_map *nest)rwsem_acquire_nest (&sem->dep_map,0 ,0 , nest, _RET_IP_);LOCK_CONTENDED (sem, __down_write_trylock, __down_write); +rwsem_set_owner (sem); }EXPORT_SYMBOL (_down_write_nest_lock);@@ -141,6 +167,7 @@ void down_write_nested(struct rw_semaphore *sem, int subclass)rwsem_acquire (&sem->dep_map, subclass,0 , _RET_IP_);LOCK_CONTENDED (sem, __down_write_trylock, __down_write); +rwsem_set_owner (sem); }EXPORT_SYMBOL (down_write_nested);lib/rwsem.c +24
-0@@ -8,6 +8,7 @@*/ #include <linux/rwsem.h> #include <linux/sched.h> +#include <linux/sched/rt.h> #include <linux/init.h> #include <linux/export.h> @@ -27,6 +28,9 @@ void __init_rwsem(struct rw_semaphore *sem, const char *name, sem->count =RWSEM_UNLOCKED_VALUE ;raw_spin_lock_init (&sem->wait_lock);INIT_LIST_HEAD (&sem->wait_list); +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER + sem->owner =NULL ; +#endif }EXPORT_SYMBOL (__init_rwsem);@@ -192,6 +196,128 @@ struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem)return sem; } +static inline int rwsem_try_write_lock (long count,bool need_lock, +struct rw_semaphore *sem) +{ +if (!(count &RWSEM_ACTIVE_MASK )) { +/* Try acquiring the write lock. */ +if (sem->count ==RWSEM_WAITING_BIAS && +cmpxchg (&sem->count,RWSEM_WAITING_BIAS , +RWSEM_ACTIVE_WRITE_BIAS ) ==RWSEM_WAITING_BIAS ) { +if (need_lock) +raw_spin_lock_irq (&sem->wait_lock); +if (!list_is_singular (&sem->wait_list)) +rwsem_atomic_update (RWSEM_WAITING_BIAS , sem); +return 1 ; + } + } +return 0 ; +} + +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +static inline bool rwsem_can_spin_on_owner (struct rw_semaphore *sem)lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 228
Quoted 10 lines
lib/rwsem.c +10
-0+{ +int retval = true; + +/* Spin only if active writer running */ +if (!sem->owner) +return false; + +rcu_read_lock (); +if (sem->owner) + retval = sem->owner->on_cpu;^^^^^^^^^^^^^^^^^^
Quoted 3 lines
Quoted 3 lines
Why is this a safe dereference? Could not another cpu have just dropped the sem (and thus set
sem->ownerto NULL and oops)?
The rcu read lock should protect against sem->owner being NULL.
Quoted 26 lines
Quoted 26 lines
Quoted 12 lines
lib/rwsem.c +11
-0+rcu_read_unlock (); +/* +* if lock->owner is not set, the sem owner may have just acquired +* it and not set the owner yet, or the sem has been released, or +* reader active. +*/ +return retval; +} + +static inline bool owner_running (struct rw_semaphore *lock, +struct task_struct *owner)lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 249
Quoted 10 lines
lib/rwsem.c +10
-0+{ +if (lock->owner != owner) +return false; + +/* +* Ensure we emit the owner->on_cpu, dereference _after_ checking +* lock->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. +*/ Again just my opinion, but kernel style is to prefer multi-line comments in a function comment block.
Quoted 8 lines
Quoted 4 lines
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 253
Quoted 4 lines
lib/rwsem.c +4
-0+barrier (); + +return owner->on_cpu; +}A lot of these functions are exact duplicates of
kernel/mutex.c- we should probably think of adding generic interfaces for mutexes and rwsems...
Probably there are pros and cons. The cons is the logic are similar but may not be exact duplicate if later on we are adding rw-sem specific tweaks. May be cleaner to keep the two separated.
Quoted 42 lines
Quoted 13 lines
Quoted 13 lines
lib/rwsem.c +12
-0+ +static noinline +int rwsem_spin_on_owner(struct rw_semaphore *lock, struct task_struct *owner) +{ +rcu_read_lock (); +while (owner_running (lock, owner)) { +if (need_resched ()) +break ; + +arch_mutex_cpu_relax (); + } +rcu_read_unlock ();Quoted 8 lines
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 273
Quoted 8 lines
lib/rwsem.c +8
-0+ +/* +* We break out the loop above on need_resched() and when the +* owner changed, which is a sign for heavy contention. Return +* success only when lock->owner is NULL. +*/ +return lock->owner ==NULL ; +}ditto
Quoted 9 lines
Quoted 9 lines
lib/rwsem.c +9
-0+ +int rwsem_optimistic_spin (struct rw_semaphore *sem) +{ +struct task_struct *owner; +int ret =0 ; + +/* sem->wait_lock should not be held when doing optimistic spinning */ +if (!rwsem_can_spin_on_owner (sem)) +return ret;Quoted 8 lines
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 288
Quoted 6 lines
lib/rwsem.c +6
-0+ +preempt_disable (); +for (;;) { + owner =ACCESS_ONCE (sem->owner); +if (owner && !rwsem_spin_on_owner (sem, owner)) +break ;Will this spin for full scheduler value on a reader-owned lock?
Quoted 1 line
Yep, it should.
No. We will spin only on writer-owned lock in the current version. The owner field is only set when a writer own it. Spinning on reader is tricky as there could be multiple readers. Earlier Davidlohr and I have privately tested a different version where we allowed the owner field to be set by reader but only get cleared if sem->owner == current (suggested by Matthew Wilcox). However we didn't get a performance boost so I did not include this.
Quoted 12 lines
Quoted 12 lines
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 297
Quoted 10 lines
lib/rwsem.c +8
-0+/* wait_lock will be acquired if write_lock is obtained */ +if (rwsem_try_write_lock (sem->count, true, sem)) { + ret =1 ; + goto out; + } + +/* +* When there's no owner, we might have preempted between the ^^^^^^^^
Quoted 3 lines
Quoted 1 line
Isn't pre-emption disabled?
Hmm yeah, that might be a bogus comment.
That's true. My original change didn't have pre-emption disabled so this slipped through.
Quoted 74 lines
Quoted 72 lines
Quoted 58 lines
lib/rwsem.c +28
-11+* owner acquiring the lock and setting the owner field. If +* we're an RT task that will live-lock because we won't let +* the owner complete. +*/ +if (!owner && (need_resched () ||rt_task (current))) +break ; + +/* +* The cpu_relax() call is a compiler barrier which forces +* everything in this loop to be re-loaded. We don't need +* memory barriers as we'll eventually observe the right +* values at the cost of a few extra spins. +*/ + arch_mutex_cpu_relax(); + + } + +out: +preempt_enable (); +return ret; +} +#endif +/* * wait until we successfully acquire the write lock */ @@ -200,6 +326,9 @@ struct rw_semaphore __sched *rwsem_down_write_failed(struct rw_semaphore *sem)long count, adjustment = -RWSEM_ACTIVE_WRITE_BIAS ;struct rwsem_waiter waiter;struct task_struct *tsk = current; +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +bool try_optimistic_spin = true; +#endif /* set up my own style of waitqueue */ waiter.task = tsk;@@ -223,20 +352,17 @@ struct rw_semaphore __sched *rwsem_down_write_failed(struct rw_semaphore *sem)/* wait until we successfully acquire the lock */ set_task_state (tsk,TASK_UNINTERRUPTIBLE );while (true) { -if (!(count &RWSEM_ACTIVE_MASK )) { -/* Try acquiring the write lock. */ - count =RWSEM_ACTIVE_WRITE_BIAS ; -if (!list_is_singular (&sem->wait_list)) - count +=RWSEM_WAITING_BIAS ; - -if (sem->count ==RWSEM_WAITING_BIAS && -cmpxchg (&sem->count,RWSEM_WAITING_BIAS , count) == -RWSEM_WAITING_BIAS ) -break ; - } +if (rwsem_try_write_lock (count, false, sem)) +break ;raw_spin_unlock_irq (&sem->wait_lock);lib/rwsem.c struct rw_semaphore __sched *rwsem_down_write_failed(struct rw_semaphore *sem) · line 368
Quoted 10 lines
lib/rwsem.c +6
-0+#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +/* do optimistic spinning */ +if (try_optimistic_spin &&rwsem_optimistic_spin (sem)) +break ; + try_optimistic_spin = false; +#endif /* Block until there are no active lockers. */ do { schedule();Regards,
Peter Hurley
Re: [PATCH 2/2] rwsem: do optimistic spinning for writer lock acquisition
Quoted 17 lines · Tim Chen
Quoted 16 lines · Davidlohr Bueso
Quoted 15 lines · Peter Hurley
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 228
Quoted 12 lines · Tim Chen
lib/rwsem.c +11
-0+static inline bool rwsem_can_spin_on_owner (struct rw_semaphore *sem) +{ +int retval = true; + +/* Spin only if active writer running */ +if (!sem->owner) +return false; + +rcu_read_lock (); +if (sem->owner) + retval = sem->owner->on_cpu;^^^^^^^^^^^^^^^^^^
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Why is this a safe dereference? Could not another cpu have just dropped the sem (and thus set
sem->ownerto NULL and oops)?The rcu read lock should protect against
sem->ownerbeing NULL.
It doesn't.
Here's the comment from mutex_spin_on_owner():
/*
* Look out! "owner" is an entirely speculative pointer
* access and not reliable.
*/
Regards,
Peter Hurley
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 2/2] rwsem: do optimistic spinning for writer lock acquisition
Quoted 24 lines · Peter Hurley
Quoted 17 lines · Tim Chen
Quoted 16 lines · Davidlohr Bueso
Quoted 15 lines · Peter Hurley
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 228
Quoted 12 lines · Tim Chen
lib/rwsem.c +11
-0+static inline bool rwsem_can_spin_on_owner (struct rw_semaphore *sem) +{ +int retval = true; + +/* Spin only if active writer running */ +if (!sem->owner) +return false; + +rcu_read_lock (); +if (sem->owner) + retval = sem->owner->on_cpu;^^^^^^^^^^^^^^^^^^Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Why is this a safe dereference? Could not another cpu have just dropped the sem (and thus set
sem->ownerto NULL and oops)?The rcu read lock should protect against
sem->ownerbeing NULL.
Quoted 9 lines
It doesn't.
Here's the comment from
mutex_spin_on_owner():/*
* Look out! "owner" is an entirely speculative pointer
* access and not reliable.
*/
In mutex_spin_on_owner, after rcu_read_lock, the owner_running() function de-references the owner pointer. The rcu_read_lock prevents owner from getting freed. The comment's intention is to warn that owner->on_cpu may not be reliable.
I'm using similar logic in rw-sem.
Tim
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 2/2] rwsem: do optimistic spinning for writer lock acquisition
Quoted 29 lines · Tim Chen
Quoted 28 lines · Peter Hurley
Quoted 17 lines · Tim Chen
Quoted 16 lines · Davidlohr Bueso
Quoted 15 lines · Peter Hurley
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 228
Quoted 12 lines · Tim Chen
lib/rwsem.c +11
-0+static inline bool rwsem_can_spin_on_owner (struct rw_semaphore *sem) +{ +int retval = true; + +/* Spin only if active writer running */ +if (!sem->owner) +return false; + +rcu_read_lock (); +if (sem->owner) + retval = sem->owner->on_cpu;^^^^^^^^^^^^^^^^^^Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Why is this a safe dereference? Could not another cpu have just dropped the sem (and thus set
sem->ownerto NULL and oops)?The rcu read lock should protect against
sem->ownerbeing NULL.It doesn't.
Here's the comment from
mutex_spin_on_owner():
Quoted 8 lines
Quoted 5 lines
/*
* Look out! "owner" is an entirely speculative pointer
* access and not reliable.
*/In
mutex_spin_on_owner, afterrcu_read_lock, theowner_running()function de-references the owner pointer.
Only after establishing the following preconditions:
1. snapshot of owner is non-NULL
2. mutex->owner == snapshot owner
3. memory holding mutex has not been freed (that's what the rcu_read_lock() is for)
Only then is the owner dereferenced and only through the snapshot (not the now-possibly-rewritten sem->owner).
Quoted 1 line
I'm using similar logic in rw-sem.
With crucial details absent.
Regards,
Peter Hurley
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 2/2] rwsem: do optimistic spinning for writer lock acquisition
In reply to Peter HurleyQuoted 24 lines · Peter Hurley
Quoted 17 lines · Tim Chen
Quoted 16 lines · Davidlohr Bueso
Quoted 15 lines · Peter Hurley
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 228
Quoted 12 lines · Tim Chen
lib/rwsem.c +11
-0+static inline bool rwsem_can_spin_on_owner (struct rw_semaphore *sem) +{ +int retval = true; + +/* Spin only if active writer running */ +if (!sem->owner) +return false; + +rcu_read_lock (); +if (sem->owner) + retval = sem->owner->on_cpu;^^^^^^^^^^^^^^^^^^Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Why is this a safe dereference? Could not another cpu have just dropped the sem (and thus set
sem->ownerto NULL and oops)?The rcu read lock should protect against
sem->ownerbeing NULL.
Quoted 8 lines
It doesn't.
Here's the comment from
mutex_spin_on_owner():/*
* Look out! "owner" is an entirely speculative pointer
* access and not reliable.
*/
On second thought, I agree with you. I should change this to something like
int retval = true; task_struct *sem_owner; /* Spin only if active writer running */ if (!sem->owner) return false; rcu_read_lock(); sem_owner = sem->owner; if (sem_owner) retval = sem_owner->on_cpu;
Thanks.
Tim
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 2/2] rwsem: do optimistic spinning for writer lock acquisition
Quoted 37 lines · Tim Chen
Quoted 24 lines · Peter Hurley
Quoted 17 lines · Tim Chen
Quoted 16 lines · Davidlohr Bueso
Quoted 15 lines · Peter Hurley
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 228
Quoted 12 lines · Tim Chen
lib/rwsem.c +11
-0+static inline bool rwsem_can_spin_on_owner (struct rw_semaphore *sem) +{ +int retval = true; + +/* Spin only if active writer running */ +if (!sem->owner) +return false; + +rcu_read_lock (); +if (sem->owner) + retval = sem->owner->on_cpu;^^^^^^^^^^^^^^^^^^Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Why is this a safe dereference? Could not another cpu have just dropped the sem (and thus set
sem->ownerto NULL and oops)?The rcu read lock should protect against
sem->ownerbeing NULL.Quoted 8 lines
It doesn't.
Here's the comment from
mutex_spin_on_owner():/*
* Look out! "owner" is an entirely speculative pointer
* access and not reliable.
*/On second thought, I agree with you. I should change this to something like
Quoted 12 lines
int retval = true; task_struct *sem_owner; /* Spin only if active writer running */ if (!sem->owner) return false; rcu_read_lock(); sem_owner = sem->owner; if (sem_owner) retval = sem_owner->on_cpu;
Our emails passed each other.
Also, I haven't given a lot of thought to if preemption must be disabled before calling rwsem_can_spin_on_owner(). If so, wouldn't you just drop rwsem_can_spin_on_owner() (because the conditions tested in the loop are equivalent)?
Regards,
Peter Hurley
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 2/2] rwsem: do optimistic spinning for writer lock acquisition
Quoted 6 lines · Peter Hurley
Also, I haven't given a lot of thought to if preemption must be disabled before calling
rwsem_can_spin_on_owner(). If so, wouldn't you just droprwsem_can_spin_on_owner()(because the conditions tested in the loop are equivalent)?
Not totally equivalent. If we drop the call to rwsem_can_spin_on_owner, we will spin when readers are holding the lock (owner is null). Right now we only allow writers to spin when other writers are holding the lock by adding the rwsem_can_spin_on_owner check. Letting spinning on readers held lock is tricky as
we could have a reader that sleeps and if we don't detect the case. We could spin for too long.
Thanks.
Tim
Peter Zijlstra
maintainerRe: [PATCH 2/2] rwsem: do optimistic spinning for writer lock acquisition
In reply to Tim ChenQuoted 12 lines · Tim Chen
On second thought, I agree with you. I should change this to something like
int retval = true; task_struct *sem_owner; /* Spin only if active writer running */ if (!sem->owner) return false; rcu_read_lock(); sem_owner = sem->owner;
That should be: sem_owner = ACCESS_ONCE(sem->owner); to make sure the compiler doesn't try and be clever and rereads.
Quoted 3 lines
if (sem_owner) retval = sem_owner->on_cpu;
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 2/2] rwsem: do optimistic spinning for writer lock acquisition
Quoted 11 lines · Peter Zijlstra
Quoted 10 lines · Tim Chen
On second thought, I agree with you. I should change this to something like
int retval = true; task_struct *sem_owner; /* Spin only if active writer running */ if (!sem->owner) return false;
Quoted 5 lines
Quoted 2 lines
rcu_read_lock(); sem_owner = sem->owner;That should be:
sem_owner=ACCESS_ONCE(sem->owner); to make sure the compiler doesn't try and be clever and rereads.
Thanks. Will incorporate this in next version of the patch.
Tim
Re: [PATCH 2/2] rwsem: do optimistic spinning for writer lock acquisition
In reply to Peter HurleyQuoted 12 lines · Peter Hurley
Will this spin for full scheduler value on a reader-owned lock?
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 297
Quoted 8 lines
lib/rwsem.c +8
-0+/* wait_lock will be acquired if write_lock is obtained */ +if (rwsem_try_write_lock (sem->count, true, sem)) { + ret =1 ; + goto out; + } + +/* +* When there's no owner, we might have preempted between the ^^^^^^^^
Quoted 2 lines
Isn't pre-emption disabled?
Peter, on further review, this code is needed. This code guard against the case of this thread preempting another thread in the middle of setting the owner field. Disabling preemption does not prevent this thread from preempting others, even though others cannot preempt this thread.
Quoted 57 lines
Quoted 57 lines
lib/rwsem.c +28
-11+* owner acquiring the lock and setting the owner field. If +* we're an RT task that will live-lock because we won't let +* the owner complete. +*/ +if (!owner && (need_resched () ||rt_task (current))) +break ; + +/* +* The cpu_relax() call is a compiler barrier which forces +* everything in this loop to be re-loaded. We don't need +* memory barriers as we'll eventually observe the right +* values at the cost of a few extra spins. +*/ + arch_mutex_cpu_relax(); + + } + +out: +preempt_enable (); +return ret; +} +#endif +/* * wait until we successfully acquire the write lock */ @@ -200,6 +326,9 @@ struct rw_semaphore __sched *rwsem_down_write_failed(struct rw_semaphore *sem)long count, adjustment = -RWSEM_ACTIVE_WRITE_BIAS ;struct rwsem_waiter waiter;struct task_struct *tsk = current; +#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +bool try_optimistic_spin = true; +#endif /* set up my own style of waitqueue */ waiter.task = tsk;@@ -223,20 +352,17 @@ struct rw_semaphore __sched *rwsem_down_write_failed(struct rw_semaphore *sem)/* wait until we successfully acquire the lock */ set_task_state (tsk,TASK_UNINTERRUPTIBLE );while (true) { -if (!(count &RWSEM_ACTIVE_MASK )) { -/* Try acquiring the write lock. */ - count =RWSEM_ACTIVE_WRITE_BIAS ; -if (!list_is_singular (&sem->wait_list)) - count +=RWSEM_WAITING_BIAS ; - -if (sem->count ==RWSEM_WAITING_BIAS && -cmpxchg (&sem->count,RWSEM_WAITING_BIAS , count) == -RWSEM_WAITING_BIAS ) -break ; - } +if (rwsem_try_write_lock (count, false, sem)) +break ;raw_spin_unlock_irq (&sem->wait_lock);
Quoted 10 lines
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_write_failed(struct rw_semaphore *sem) · line 368
Quoted 9 lines
lib/rwsem.c +6
-0+#ifdef CONFIG_RWSEM_SPIN_ON_WRITE_OWNER +/* do optimistic spinning */ +if (try_optimistic_spin &&rwsem_optimistic_spin (sem)) +break ; + try_optimistic_spin = false; +#endif /* Block until there are no active lockers. */ do { schedule();
Thanks.
Tim
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 2/2] rwsem: do optimistic spinning for writer lock acquisition
Quoted 13 lines · Tim Chen
Quoted 12 lines · Peter Hurley
Will this spin for full scheduler value on a reader-owned lock?
lib/rwsem.c struct rw_semaphore __sched *rwsem_down_read_failed(struct rw_semaphore *sem) · line 297
Quoted 8 lines
lib/rwsem.c +8
-0+/* wait_lock will be acquired if write_lock is obtained */ +if (rwsem_try_write_lock (sem->count, true, sem)) { + ret =1 ; + goto out; + } + +/* +* When there's no owner, we might have preempted between the ^^^^^^^^
Quoted 8 lines
Quoted 2 lines
Isn't pre-emption disabled?
Peter, on further review, this code is needed. This code guard against the case of this thread preempting another thread in the middle of setting the owner field. Disabling preemption does not prevent this thread from preempting others, even though others cannot preempt this thread.
Yep; so the "we" in the quoted comment really refers to another thread executing down_write_xxxx().
Thanks for the clarification.
Regards,
Peter Hurley
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Peter Zijlstra
maintainerRe: [PATCH 2/2] rwsem: do optimistic spinning for writer lock acquisition
In reply to Tim ChenQuoted 32 lines · Tim Chen
Introduce in this patch optimistic spinning for writer lock acquisition in read write semaphore. The logic is similar to the optimistic spinning in mutex but without the MCS lock queueing of the spinner. This provides a better chance for a writer to acquire the lock before being we block it and put it to sleep.
Disabling of pre-emption during optimistic spinning was suggested by Davidlohr Bueso. It improved performance of aim7 for his test suite.
Combined with the patch to avoid unnecesary cmpxchg,
in testing by Davidlohr Bueso on aim7 workloads
on 8 socket 80 cores system, he saw improvements of alltests (+14.5%), custom (+17%), disk (+11%),high_systime(+5%), shared (+15%) and short (+4%), most of them after around 500 users when he implementedi_mmapas rwsem.Signed-off-by Tim Chen <[email protected]>--- Makefile | 2 +- include/linux/rwsem.h | 3 + init/Kconfig | 9 +++ kernel/rwsem.c | 29 +++++++++- lib/rwsem.c | 148 +++++++++++++++++++++++++++++++++++++++++++++---- 5 files changed, 178 insertions(+), 13 deletions(-)Makefile +0
-0
Makefile line 7
Quoted 8 lines
Makefile +1
-1@@ -1,7 +1,7 @@ VERSION = 3 PATCHLEVEL = 10 SUBLEVEL = 0 -EXTRAVERSION = -rc4 +EXTRAVERSION = -rc4-optspin4 NAME = Unicycling Gorilla # *DOCUMENTATION*
I'm fairly sure we don't want to commit this hunk ;-)
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 2/2] rwsem: do optimistic spinning for writer lock acquisition
Quoted 40 lines · Peter Zijlstra
Quoted 39 lines · Tim Chen
Introduce in this patch optimistic spinning for writer lock acquisition in read write semaphore. The logic is similar to the optimistic spinning in mutex but without the MCS lock queueing of the spinner. This provides a better chance for a writer to acquire the lock before being we block it and put it to sleep.
Disabling of pre-emption during optimistic spinning was suggested by Davidlohr Bueso. It improved performance of aim7 for his test suite.
Combined with the patch to avoid unnecesary cmpxchg,
in testing by Davidlohr Bueso on aim7 workloads
on 8 socket 80 cores system, he saw improvements of alltests (+14.5%), custom (+17%), disk (+11%),high_systime(+5%), shared (+15%) and short (+4%), most of them after around 500 users when he implementedi_mmapas rwsem.Signed-off-by Tim Chen <[email protected]>--- Makefile | 2 +- include/linux/rwsem.h | 3 + init/Kconfig | 9 +++ kernel/rwsem.c | 29 +++++++++- lib/rwsem.c | 148 +++++++++++++++++++++++++++++++++++++++++++++---- 5 files changed, 178 insertions(+), 13 deletions(-)Makefile +1
-1@@ -1,7 +1,7 @@ VERSION = 3 PATCHLEVEL = 10 SUBLEVEL = 0 -EXTRAVERSION = -rc4 +EXTRAVERSION = -rc4-optspin4 NAME = Unicycling Gorilla
Quoted 3 lines
Makefile line 7
Quoted 1 line
Makefile +0
-0# *DOCUMENTATION*I'm fairly sure we don't want to commit this hunk ;-)
Fat fingers. Thanks for catching.
Tim
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>