Messages in this thread | | | Date | Mon, 23 May 2016 13:15:02 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE |
| |
On Mon, May 23, 2016 at 12:44:16PM -0700, Davidlohr Bueso wrote: > On Mon, 23 May 2016, Jason Low wrote: > > >On Sat, 2016-05-21 at 09:04 -0700, Peter Hurley wrote: > >>On 05/18/2016 12:58 PM, Jason Low wrote: > >>> It should be fine to use the standard READ_ONCE here, even if it's just > >>> for documentation, as it's probably not going to cost anything in > >>> practice. It would be better to avoid adding any special macros for this > >>> which may just add more complexity. > >> > >>See, I don't understand this line of reasoning at all. > >> > >>I read this as "it's ok to be non-optimal here where were spinning CPU > >>time but not ok to be non-optimal generally elsewhere where it's > >>way less important like at init time". > > > >So I think there is a difference between using it during init time and > >using it here where we're spinning. During init time, initializing the > >owner field locklessly is normal. No other thread should be concurrently > >be writing to the field, since the structure is just getting > >initialized, so there are no surprises there. > > > >Our access of the owner field in this function is special in that we're > >using a bit of "lockless magic" to read and write to a field that gets > >concurrently accessed without any serialization. Since we're not taking > >the wait_lock in a scenario where we'd normally would take a lock, it > >would be good to have this documented. > > > >>And by the way, it's not just "here" but _everywhere_. > >>What about reading ->on_cpu locklessly? > > > >Sure, we could also use READ_ONCE when reading ->on_cpu :) > > Locking wise we should be covered with ->on_cpu as we're always under rcu_read_lock > (barrier, preempt_disable). But I'm not sure if this rule applies throughout the > scheduler, however, like it does in, say thread_group_cputime(). cpu_clock_sample() > (from posix timers) seems to mix and match being done under rcu. So ultimately I > think you're right.
But rcu_read_lock() does not exclude updates, which is one reason why pointer reads use rcu_dereference() rather than normal assignments.
So I do not believe that rcu_read_lock() is helping you in this case.
That said, it is a bit hard to imagine the compiler tearing a load from an int... But compilers have uncovered weaknesses in my imagination more than once in the past.
Thanx, Paul
| |