locking/mutex: Set and clear owner using WRITE_ONCE()
PATCH 1/1 locking/mutex: Set and clear owner using WRITE_ONCE()
The mutex owner can get read and written to without the wait_lock. Use WRITE_ONCE when setting and clearing the owner field in order to avoid optimizations such as store tearing. This avoids situations where the owner field gets written to with multiple stores and another thread could concurrently read and use a partially written owner value.
--- kernel/locking/mutex.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
kernel/locking/mutex.h +8 -2
@@ -17,14 +17,20 @@ __list_del((waiter)->list.prev, (waiter)->list.next)#ifdef CONFIG_MUTEX_SPIN_ON_OWNER +/* +* The mutex owner can get read and written to locklessly. +* We should use WRITE_ONCE() when writing the owner value to +* avoid store tearing, otherwise, a thread could potentially +* read a partially written and incomplete owner value. +*/ static inline void mutex_set_owner (struct mutex *lock) { - lock->owner = current; +WRITE_ONCE (lock->owner, current); }static inline void mutex_clear_owner (struct mutex *lock) { - lock->owner =NULL ; +WRITE_ONCE (lock->owner,NULL ); }#else static inline void mutex_set_owner (struct mutex *lock)
Signature · 1 line
2.1.4
Quoted 6 lines · Jason Low
The mutex owner can get read and written to without the
wait_lock. UseWRITE_ONCEwhen setting and clearing the owner field in order to avoid optimizations such as store tearing. This avoids situations where the owner field gets written to with multiple stores and another thread could concurrently read and use a partially written owner value.
Right, I also ran into this with rtmutexes when avoiding the schedule() and the spin on owner thingie.
Quoted 1 line
Signed-off-by Jason Low <[email protected]>
Quoted 29 lines · Jason Low
The mutex owner can get read and written to without the
wait_lock. UseWRITE_ONCEwhen setting and clearing the owner field in order to avoid optimizations such as store tearing. This avoids situations where the owner field gets written to with multiple stores and another thread could concurrently read and use a partially written owner value.Signed-off-by Jason Low<[email protected]>--- kernel/locking/mutex.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)kernel/locking/mutex.h +6
-1@@ -17,14 +17,20 @@ __list_del((waiter)->list.prev, (waiter)->list.next)#ifdef CONFIG_MUTEX_SPIN_ON_OWNER +/* +* The mutex owner can get read and written to locklessly. +* We should use WRITE_ONCE() when writing the owner value to +* avoid store tearing, otherwise, a thread could potentially +* read a partially written and incomplete owner value. +*/ static inline void mutex_set_owner (struct mutex *lock) { - lock->owner = current;
kernel/locking/mutex.h line 36
Quoted 10 lines
kernel/locking/mutex.h +2
-1+ WRITE_ONCE(lock->owner, current); }static inline void mutex_clear_owner (struct mutex *lock) { - lock->owner =NULL ; +WRITE_ONCE (lock->owner,NULL ); }#else static inline void mutex_set_owner (struct mutex *lock)
I think mutex-debug.h also needs similar changes for completeness.
Cheers,
Longman
Quoted 33 lines · Waiman Long
Quoted 32 lines · Jason Low
The mutex owner can get read and written to without the
wait_lock. UseWRITE_ONCEwhen setting and clearing the owner field in order to avoid optimizations such as store tearing. This avoids situations where the owner field gets written to with multiple stores and another thread could concurrently read and use a partially written owner value.Signed-off-by Jason Low<[email protected]>--- kernel/locking/mutex.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)kernel/locking/mutex.h +7
-1@@ -17,14 +17,20 @@ __list_del((waiter)->list.prev, (waiter)->list.next)#ifdef CONFIG_MUTEX_SPIN_ON_OWNER +/* +* The mutex owner can get read and written to locklessly. +* We should use WRITE_ONCE() when writing the owner value to +* avoid store tearing, otherwise, a thread could potentially +* read a partially written and incomplete owner value. +*/ static inline void mutex_set_owner (struct mutex *lock) { - lock->owner = current; +WRITE_ONCE (lock->owner, current); }
Quoted 9 lines
kernel/locking/mutex.h line 36
Quoted 7 lines
kernel/locking/mutex.h +1
-1static inline void mutex_clear_owner (struct mutex *lock) { - lock->owner =NULL ; +WRITE_ONCE (lock->owner,NULL ); }#else static inline void mutex_set_owner (struct mutex *lock)I think
mutex-debug.halso needs similar changes for completeness.
Good point, I will add the changes to those functions in the debug case to this patch.
Thanks,
Jason
Quoted 1 line · Waiman Long
I think
mutex-debug.halso needs similar changes for completeness.
Maybe, but given that with debug the wait_lock is unavoidable, doesn't this send the wrong message?
Thanks,
Davidlohr
Quoted 9 lines · Davidlohr Bueso
Quoted 1 line · Waiman Long
I think
mutex-debug.halso needs similar changes for completeness.Maybe, but given that with debug the
wait_lockis unavoidable, doesn't this send the wrong message?Thanks,
Davidlohr
You are right. Optimistic spinning is disabled when DEBUG_MUTEXES is true. So WRITE_ONCE() is not really needed. However, I think we should have at least a comment in mutex-debug.h as to why the owner field is handled differently from mutex.h.
Cheers,
Longman
Quoted 6 lines · Davidlohr Bueso
Quoted 1 line · Waiman Long
I think
mutex-debug.halso needs similar changes for completeness.Maybe, but given that with debug the
wait_lockis unavoidable, doesn't this send the wrong message?
The mutex_set_owner() and mutex_clear_owner() can still get called in the fastpath without the wait_lock for the debug case too correct? If that's the case, then the WRITE_ONCE would still make sense to have in the debug case.
Quoted 10 lines · Jason Low
Quoted 6 lines · Davidlohr Bueso
Quoted 1 line · Waiman Long
I think
mutex-debug.halso needs similar changes for completeness.Maybe, but given that with debug the
wait_lockis unavoidable, doesn't this send the wrong message?The
mutex_set_owner()andmutex_clear_owner()can still get called in the fastpath without thewait_lockfor the debug case too correct?
The fastpath is a nop, see mutex-null:
#define __mutex_fastpath_lock(count, fail_fn) fail_fn(count)
In anycase, if there were a fastpath, that would at least have an ACQUIRE barrier (unless you set ->owner it before it or something weird). But this isn't the case, so debug should be safe afaict.
Thanks,
Davidlohr
Quoted 10 lines · Davidlohr Bueso
Quoted 8 lines · Jason Low
Quoted 6 lines · Davidlohr Bueso
Quoted 1 line · Waiman Long
I think
mutex-debug.halso needs similar changes for completeness.Maybe, but given that with debug the
wait_lockis unavoidable, doesn't this send the wrong message?
Quoted 6 lines
Quoted 2 lines
The
mutex_set_owner()andmutex_clear_owner()can still get called in the fastpath without thewait_lockfor the debug case too correct?The fastpath is a nop, see mutex-null:
#define
__mutex_fastpath_lock(count, fail_fn)fail_fn(count)
Oh right, so then the mutex_set_owner() call in mutex_lock() doesn't really matter. I'll update the patch to remove the use of WRITE_ONCE for the debug case then, and add the comments about why it is different as suggested.
Thanks,
Jason