On Mon, Jul 13, 2020 at 01:12:46PM +0100, Qais Yousef wrote: > On 07/13/20 13:21, Peter Zijlstra wrote: > > It's monday, and I cannot get my brain working.. I cannot decipher the > > comments you have with the smp_[rw]mb(), what actual ordering do they > > enforce? > > It was a bit of a paranoia to ensure that readers on other cpus see the new > value after this point. IIUC that's not something any barrier can provide. Barriers can only order between (at least) two memory operations: X = 1; y = Y; smp_wmb(); smp_rmb(); Y = 1; x = X; guarantees that if y == 1, then x must also be 1. Because the left hand side orders the store of Y after the store of X, while the right hand side order the load of X after the load of Y. Therefore, if the first load observes the last store, the second load must observe the first store. Without a second variable, barriers can't guarantee _anything_. Which is why any barrier comment should refer to at least two variables. > > Also, your synchronize_rcu() relies on write_lock() beeing > > non-preemptible, which isn't true on PREEMPT_RT. > > > > The below seems simpler... > Hmm maybe I am missing something obvious, but beside the race with fork; I was > worried about another race and that's what the synchronize_rcu() is trying to > handle. > > It's the classic preemption in the middle of RMW operation race. > > copy_process() sysctl_uclamp > > sched_post_fork() > __uclamp_sync_rt() > // read sysctl > // PREEMPT > for_each_process_thread() > // RESUME > // write syctl to p > > 2. sysctl_uclamp happens *during* sched_post_fork() > > There's the risk of the classic preemption in the middle of RMW where another > CPU could have changed the shared variable after the current CPU has already > read it, but before writing it back. Aah.. I see. > I protect this with rcu_read_lock() which as far as I know synchronize_rcu() > will ensure if we do the update during this section; we'll wait for it to > finish. New forkees entering the rcu_read_lock() section will be okay because > they should see the new value. > > spinlocks() and mutexes seemed inferior to this approach. Well, didn't we just write in another patch that p->uclamp_* was protected by both rq->lock and p->pi_lock?
