On 07/07/20 12:30, Valentin Schneider wrote: > > On 07/07/20 10:34, Qais Yousef wrote: > > On 07/06/20 16:49, Valentin Schneider wrote: > >> > >> On 06/07/20 15:28, Qais Yousef wrote: > >> > CC: linux-fsdevel@xxxxxxxxxxxxxxx > >> > --- > >> > > >> > Peter > >> > > >> > I didn't do the > >> > > >> > read_lock(&taslist_lock); > >> > smp_mb__after_spinlock(); > >> > read_unlock(&tasklist_lock); > >> > > >> > dance you suggested on IRC as it didn't seem necessary. But maybe I missed > >> > something. > >> > > >> > >> So the annoying bit with just uclamp_fork() is that it happens *before* the > >> task is appended to the tasklist. This means without too much care we > >> would have (if we'd do a sync at uclamp_fork()): > >> > >> CPU0 (sysctl write) CPU1 (concurrent forker) > >> > >> copy_process() > >> uclamp_fork() > >> p.uclamp_min = state > >> state = foo > >> > >> for_each_process_thread(p, t) > >> update_state(t); > >> list_add(p) > >> > >> i.e. that newly forked process would entirely sidestep the update. Now, > >> with Peter's suggested approach we can be in a much better situation. If we > >> have this in the sysctl update: > >> > >> state = foo; > >> > >> read_lock(&taslist_lock); > >> smp_mb__after_spinlock(); > >> read_unlock(&tasklist_lock); > >> > >> for_each_process_thread(p, t) > >> update_state(t); > >> > >> While having this in the fork: > >> > >> write_lock(&tasklist_lock); > >> list_add(p); > >> write_unlock(&tasklist_lock); > >> > >> sched_post_fork(p); // state re-read here; probably wants an mb first > >> > >> Then we can no longer miss an update. If the forked p doesn't see the new > >> value, it *must* have been added to the tasklist before the updater loops > >> over it, so the loop will catch it. If it sees the new value, we're done. > > > > uclamp_fork() has nothing to do with the race. If copy_process() duplicates the > > task_struct of an RT task, it'll copy the old value. > > > > Quite so; my point was if we were to use uclamp_fork() as to re-read the value. > > > I'd expect the newly introduced sched_post_fork() (also in copy_process() after > > the list update) to prevent this race altogether. > > > > Now we could end up with a problem if for_each_process_thread() doesn't see the > > newly forked task _after_ sched_post_fork(). Hence my question to Peter. > > > > > >> > >> AIUI, the above strategy doesn't require any use of RCU. The update_state() > >> and sched_post_fork() can race, but as per the above they should both be > >> writing the same value. > > > > for_each_process_thread() must be protected by either tasklist_lock or > > rcu_read_lock(). > > > > Right > > > The other RCU logic I added is not to protect against the race above. I > > describe the other race condition in a comment. > > I take it that's the one in uclamp_sync_util_min_rt_default()? Correct. > > __setscheduler_uclamp() can't be preempted as we hold task_rq_lock(). It > can indeed race with the sync though, but again with the above suggested > setup it would either: > - see the old value, but be guaranteed to be iterated over later by the > updater > - see the new value AFAIU rcu_read_lock() is light weight. So having the protection applied is more robust against future changes. > > sched_post_fork() being preempted out is a bit more annoying, but what > prevents us from making that bit preempt-disabled? preempt_disable() is not friendly to RT and heavy handed approach IMO. > > I have to point out I'm assuming here updaters are serialized, which does > seem to be see the case (cf. uclamp_mutex). Correct. Thanks -- Qais Yousef
