Messages in this thread | | | Date | Sun, 15 May 2016 10:47:47 -0400 | From | Waiman Long <> | Subject | Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner field |
| |
On 05/13/2016 01:58 PM, Peter Hurley wrote: > 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
While load tearing in the argument to rwsem_is_reader_owned() isn't an issue as the wrong decision won't do any harm. Store tearing as identified above can be a problem. I will fix that even though the the chance of compiling generating store tearing code is really small.
Cheers, Longman
| |