Messages in this thread | | | Subject | Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner field | From | Peter Hurley <> | Date | Fri, 13 May 2016 10:58:05 -0700 |
| |
On 05/13/2016 08:07 AM, Peter Zijlstra wrote: > On Wed, May 11, 2016 at 03:04:20PM -0700, Peter Hurley wrote: >>> + return !rwsem_is_reader_owned(READ_ONCE(sem->owner)); >> >> It doesn't make sense to force reload sem->owner here; if sem->owner >> is not being reloaded then the loop above will execute forever. >> >> Arguably, this check should be bumped out to the optimistic spin and >> reload/check the owner there? >> > > Note that barrier() and READ_ONCE() have overlapping but not identical > results and the combined use actually makes sense here. > > Yes, a barrier() anywhere in the loop will force a reload of the > variable, _however_ it doesn't force that reload to not suffer from > load tearing. > > Using volatile also forces a reload, but also ensures the load cannot > be torn IFF it is of machine word side and naturally aligned. > > So while the READ_ONCE() here is pointless for forcing the reload; > that's already ensured, we still need to make sure the load isn't torn.
If load tearing a naturally aligned pointer is a real code generation possibility then the rcu list code is broken too (which loads ->next directly; cf. list_for_each_entry_rcu() & list_for_each_entry_lockless()).
For 4.4, Paul added READ_ONCE() checks for list_empty() et al, but iirc that had to do with control dependencies and not load tearing.
OTOH, this patch might actually produce store-tearing:
+static inline void rwsem_set_reader_owned(struct rw_semaphore *sem) +{ + /* + * We check the owner value first to make sure that we will only + * do a write to the rwsem cacheline when it is really necessary + * to minimize cacheline contention. + */ + if (sem->owner != RWSEM_READER_OWNED) + sem->owner = RWSEM_READER_OWNED; +}
Regards, Peter Hurley
| |