remove redundant compare, cmpxchg already does it
RFC PATCH 1/1 remove redundant compare, cmpxchg already does it
remove a redundant comparision
--- kernel/locking/rwsem-xadd.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
kernel/locking/rwsem-xadd.c +1 -2
@@ -249,8 +249,7 @@ static inline bool rwsem_try_write_lock(long count, 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 , +if (cmpxchg (&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);
Signature · 1 line
1.9.1
Quoted 12 lines · Pranith Kumar
remove a redundant comparision
Signed-off-by Pranith Kumar <[email protected]>--- kernel/locking/rwsem-xadd.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)kernel/locking/rwsem-xadd.c +0
-0
kernel/locking/rwsem-xadd.c static inline bool rwsem_try_write_lock(long count, struct rw_semaphore *sem) · line 253
Quoted 7 lines
kernel/locking/rwsem-xadd.c +1
-2@@ -249,8 +249,7 @@ static inline bool rwsem_try_write_lock(long count, 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 , +if (cmpxchg (&sem->count,RWSEM_WAITING_BIAS ,RWSEM_ACTIVE_WRITE_BIAS ) ==RWSEM_WAITING_BIAS ) {
This was mainly done to avoid the cost of a cmpxchg in case where they are not equal. Not sure if it really makes a difference though.
Signature · 1 line
Andev
Peter Zijlstra
maintainerQuoted 8 lines · Andev
Quoted 7 lines · Pranith Kumar
remove a redundant comparision
Signed-off-by Pranith Kumar <[email protected]>--- kernel/locking/rwsem-xadd.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Quoted 15 lines
kernel/locking/rwsem-xadd.c static inline bool rwsem_try_write_lock(long count, struct rw_semaphore *sem) · line 253
Quoted 12 lines
kernel/locking/rwsem-xadd.c +1
-2@@ -249,8 +249,7 @@ static inline bool rwsem_try_write_lock(long count, 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 , +if (cmpxchg (&sem->count,RWSEM_WAITING_BIAS ,RWSEM_ACTIVE_WRITE_BIAS ) ==RWSEM_WAITING_BIAS ) {This was mainly done to avoid the cost of a cmpxchg in case where they are not equal. Not sure if it really makes a difference though.
It does, a cache hot cmpxchg instruction is 24 cycles (as is pretty much any other LOCKed ins, as measured on my WSM-EP), not to mention that cmpxchg is a RMW so it needs to grab the cacheline in exclusive mode.
A read, which allows the cacheline to remain in shared, and non LOCKed ops are way faster.
Quoted 25 lines · Peter Zijlstra
Quoted 8 lines · Andev
Quoted 7 lines · Pranith Kumar
remove a redundant comparision
Signed-off-by Pranith Kumar <[email protected]>--- kernel/locking/rwsem-xadd.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)Quoted 15 lines
kernel/locking/rwsem-xadd.c static inline bool rwsem_try_write_lock(long count, struct rw_semaphore *sem) · line 253
Quoted 12 lines
kernel/locking/rwsem-xadd.c +1
-2@@ -249,8 +249,7 @@ static inline bool rwsem_try_write_lock(long count, 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 , +if (cmpxchg (&sem->count,RWSEM_WAITING_BIAS ,RWSEM_ACTIVE_WRITE_BIAS ) ==RWSEM_WAITING_BIAS ) {This was mainly done to avoid the cost of a cmpxchg in case where they are not equal. Not sure if it really makes a difference though.
Quoted 6 lines
It does, a cache hot cmpxchg instruction is 24 cycles (as is pretty much any other LOCKed ins, as measured on my WSM-EP), not to mention that cmpxchg is a RMW so it needs to grab the cacheline in exclusive mode.
A read, which allows the cacheline to remain in shared, and non LOCKed ops are way faster.
Yep, and we also do it in mutexes. The numbers and benefits on larger systems speaks for themselves. It would, perhaps, be worth adding a comment as it does seem redundant if you're not thinking about the cacheline when reading the code.
Quoted 30 lines · Davidlohr Bueso
Quoted 29 lines · Peter Zijlstra
Quoted 8 lines · Andev
Quoted 7 lines · Pranith Kumar
remove a redundant comparision
Signed-off-by Pranith Kumar <[email protected]>--- kernel/locking/rwsem-xadd.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)Quoted 15 lines
kernel/locking/rwsem-xadd.c static inline bool rwsem_try_write_lock(long count, struct rw_semaphore *sem) · line 253
Quoted 12 lines
kernel/locking/rwsem-xadd.c +1
-2@@ -249,8 +249,7 @@ static inline bool rwsem_try_write_lock(long count, 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 , +if (cmpxchg (&sem->count,RWSEM_WAITING_BIAS ,RWSEM_ACTIVE_WRITE_BIAS ) ==RWSEM_WAITING_BIAS ) {This was mainly done to avoid the cost of a cmpxchg in case where they are not equal. Not sure if it really makes a difference though.
It does, a cache hot cmpxchg instruction is 24 cycles (as is pretty much any other LOCKed ins, as measured on my WSM-EP), not to mention that cmpxchg is a RMW so it needs to grab the cacheline in exclusive mode.
Quoted 7 lines
Quoted 2 lines
A read, which allows the cacheline to remain in shared, and non LOCKed ops are way faster.
Yep, and we also do it in mutexes. The numbers and benefits on larger systems speaks for themselves. It would, perhaps, be worth adding a comment as it does seem redundant if you're not thinking about the cacheline when reading the code.
I knew I had formally read this technique somewhere:
http://pdos.csail.mit.edu/6.828/2010/readings/mcs.pdf (part 2.1).
Peter, what do you think of adding a new cmp_cmpxchg() or dcmpxchg() call for such scenarios?
Peter Zijlstra
maintainerQuoted 5 lines · Davidlohr Bueso
I knew I had formally read this technique somewhere:
http://pdos.csail.mit.edu/6.828/2010/readings/mcs.pdf (part 2.1).Peter, what do you think of adding a new
cmp_cmpxchg()ordcmpxchg()call for such scenarios?
Don't like dcmpxchg(), too easy to confuse with double-cmpxchg or somesuch.
That said, I'm not entirely sure we want this primitive, the thing is, people might use it ;-)
And its somewhat dangerous in that it explicitly does not provide any kind of memory barrier on the fail path, where cmpxchg() is an unconditional full memory barrier.
Also, you really don't want to use it in loops.
So I think it makes more sense to leave things as are and simply apply the pattern where safe and meaningful.
Quoted 25 lines · Peter Zijlstra
Quoted 8 lines · Andev
Quoted 7 lines · Pranith Kumar
remove a redundant comparision
Signed-off-by Pranith Kumar <[email protected]>--- kernel/locking/rwsem-xadd.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)Quoted 15 lines
kernel/locking/rwsem-xadd.c static inline bool rwsem_try_write_lock(long count, struct rw_semaphore *sem) · line 253
Quoted 12 lines
kernel/locking/rwsem-xadd.c +1
-2@@ -249,8 +249,7 @@ static inline bool rwsem_try_write_lock(long count, 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 , +if (cmpxchg (&sem->count,RWSEM_WAITING_BIAS ,RWSEM_ACTIVE_WRITE_BIAS ) ==RWSEM_WAITING_BIAS ) {This was mainly done to avoid the cost of a cmpxchg in case where they are not equal. Not sure if it really makes a difference though.
Quoted 6 lines
It does, a cache hot cmpxchg instruction is 24 cycles (as is pretty much any other LOCKed ins, as measured on my WSM-EP), not to mention that cmpxchg is a RMW so it needs to grab the cacheline in exclusive mode.
A read, which allows the cacheline to remain in shared, and non LOCKed ops are way faster.
Ok, this means that we need to use more of such swaps on highly contended paths. As Davidlohr suggested later on, I think it would be a good idea to document this and add an API.
Signature · 1 line
Pranith