Messages in this thread | | | Date | Mon, 29 Apr 2019 13:40:41 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH 1/1] rcu/sync: simplify the state machine |
| |
On Mon, Apr 29, 2019 at 06:06:04PM +0200, Oleg Nesterov wrote: > On 04/28, Paul E. McKenney wrote: > > > > And it still looks good after review, so I have pushed it. > > Thanks! > > > I did add > > READ_ONCE() and WRITE_ONCE() to unprotected uses of ->gp_state, but > > please let me know if I messed anything up. > > Well, at least WRITE_ONCE()'s look certainly unneeded to me, gp_state > is protected by rss_lock. > > WARN_ON_ONCE(gp_state) can read gp_state lockless, but even in this case > I do not understand what READ_ONCE() tries to prevent... > > Nevermind, this won't hurt and as I already said I don't understand the > _ONCE() magic anyway ;)
If I understand correctly, rcu_sync_is_idle() can be inline and returns ->gp_state. Without the READ_ONCE(), the compiler might fuse reads from consecutive calls to rcu_sync_is_idle() or (under register pressure) re-read from it, getting inconsistent results. For example, this:
tmp = rcu_sync_is_idle(rsp); do_something(tmp); do_something_else(tmp);
Might become this:
do_something(rcu_sync_is_idle(rsp)); do_something_else(rcu_sync_is_idle(rsp));
This might actually be harmless given current calls, but it would be at best an accident waiting to happen.
Or am I missing something here?
Thanx, Paul
| |