On Sat, Sep 14, 2019 at 07:35:02AM -0500, Eric W. Biederman wrote: > > The current task on the runqueue is currently read with rcu_dereference(). > > To obtain ordinary rcu semantics for an rcu_dereference of rq->curr it needs > to be paird with rcu_assign_pointer of rq->curr. Which provides the > memory barrier necessary to order assignments to the task_struct > and the assignment to rq->curr. > > Unfortunately the assignment of rq->curr in __schedule is a hot path, > and it has already been show that additional barriers in that code > will reduce the performance of the scheduler. So I will attempt to > describe below why you can effectively have ordinary rcu semantics > without any additional barriers. > > The assignment of rq->curr in init_idle is a slow path called once > per cpu and that can use rcu_assign_pointer() without any concerns. > > As I write this there are effectively two users of rcu_dereference on > rq->curr. There is the membarrier code in kernel/sched/membarrier.c > that only looks at "->mm" after the rcu_dereference. Then there is > task_numa_compare() in kernel/sched/fair.c. My best reading of the > code shows that task_numa_compare only access: "->flags", > "->cpus_ptr", "->numa_group", "->numa_faults[]", > "->total_numa_faults", and "->se.cfs_rq". > > The code in __schedule() essentially does: > rq_lock(...); > smp_mb__after_spinlock(); > > next = pick_next_task(...); > rq->curr = next; > > context_switch(prev, next); > > At the start of the function the rq_lock/smp_mb__after_spinlock > pair provides a full memory barrier. Further there is a full memory barrier > in context_switch(). > > This means that any task that has already run and modified itself (the > common case) has already seen two memory barriers before __schedule() > runs and begins executing. A task that modifies itself then sees a > third full memory barrier pair with the rq_lock(); > > For a brand new task that is enqueued with wake_up_new_task() there > are the memory barriers present from the taking and release the > pi_lock and the rq_lock as the processes is enqueued as well as the > full memory barrier at the start of __schedule() assuming __schedule() > happens on the same cpu. > > This means that by the time we reach the assignment of rq->curr > except for values on the task struct modified in pick_next_task > the code has the same guarantees as if it used rcu_assign_pointer. > > Reading through all of the implementations of pick_next_task it > appears pick_next_task is limited to modifying the task_struct fields > "->se", "->rt", "->dl". These fields are the sched_entity structures > of the varies schedulers. > > Further "->se.cfs_rq" is only changed in cgroup attach/move operations > initialized by userspace. > > Unless I have missed something this means that in practice that the > users of "rcu_dereerence(rq->curr)" get normal rcu semantics of > rcu_dereference() for the fields the care about, despite the > assignment of rq->curr in __schedule() ot using rcu_assign_pointer. > > Link: > https://lore.kernel.org/r/[email protected] > Signed-off-by: "Eric W. Biederman" <[email protected]> > --- > kernel/sched/core.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 69015b7c28da..668262806942 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -3857,7 +3857,11 @@ static void __sched notrace __schedule(bool preempt) > > if (likely(prev != next)) { > rq->nr_switches++; > - rq->curr = next; > + /* > + * RCU users of rcu_dereference(rq->curr) may not see > + * changes to task_struct made by pick_next_task(). > + */ > + RCU_INIT_POINTER(rq->curr, next);
It would be nice to have more explanations in the comments as to why we don't use rcu_assign_pointer() here (the very fast-path issue) and why it is expected to be fine (the rq_lock() + post spinlock barrier) under which condition. Some short summary of the changelog. Because that line implies way too many subtleties. Thanks.
