Re: [PATCH v2 4/4] task: RCUify the assignment of rq->curr

[Frederic Weisbecker <frederic@kernel.org>]

On Wed, Sep 25, 2019 at 08:49:17PM -0500, Eric W. Biederman wrote:
> Frederic Weisbecker <frederic@kernel.org> writes:
> 
> > On Sat, Sep 14, 2019 at 07:35:02AM -0500, Eric W. Biederman wrote:
> >> 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.
> 
> Crucially that line documents the standard rules don't apply,
> and it documents which guarantees a new user of the code can probably
> count on.  I say probably because the comment may go stale before I new
> user of rcu appears.  I have my hopes things are simple enough at that
> location that if the comment needs to be changed it can be.

At least I can't understand that line without referring to the changelog.

> 
> If it is not obvious from reading the code that calls
> "task_rcu_dereference(rq->curr)" now "rcu_dereference(rq->curr)" why we
> don't need the guarantees from rcu_assign_pointet() my sense is that
> it should be those locations that document what guarantees they need.

Both sides should probably have comments.

> 
> Of the several different locations that use this my sense is that they
> all have different requirements.
> 
> - The rcuwait code just needs the lifetime change as it never dereferences
>   rq->curr.
> 
> - The membarrier code just looks at rq->curr->mm for a moment so it
>   hardly needs anything.  I suspect we might be able to make the rcu
>   critical section smaller in that code.
> 
> - I don't know the code in task_numa_compare() well enough even to make an
>   educated guess.  Peter asserts (if I read his reply correctly) it is
>   all just a heuristic so stale values should not matter.
> 
>   My reading of the code strongly suggests that we have the ordinary
>   rcu_assign_pointer() guarantees there.  The few fields that are not
>   covered by the ordinary guarantees do not appear to be read.  So even
>   if Peter is wrong RCU_INIT_POINTER appears safe to me.
> 
>   I also don't think we will have confusion with people reading the
>   code and expecting ordinary rcu_dereference semantics().
> 
> I can't possibly see putting the above several lines in a meaningful
> comment where RCU_INIT_POINTER is called.  Especially in a comment
> that will survive changes to any of those functions.  My experience
> is comments that try that are almost always overlooked when someone
> updates the code.

That's ok, it's the nature of comments, they get out of date. But at
least they provide a link to history so we can rewind to find the initial
how and why for a tricky line.

I bet nobody wants git blame as a base for their text editors.

> 
> I barely found all of the comments that depended upon the details of
> task_rcu_dereference and updated them in my patchset, when I removed
> the need for task_rcu_dereference.
> 
> I don't think it would be wise to put a comment that is a wall of words
> in the middle of __schedule().  I think it will become inaccurate with
> time and because it is a lot of words I think it will be ignored.
> 
> 
> As for the __schedule: It is the heart of the scheduler.  It is
> performance code.  It is clever code.  It is likely to stay that way
> because it is the scheduler.  There are good technical reasons for the
> code is the way it is, and anyone changing the scheduler in a
> responsible manner that includes benchmarking should find those
> technical reasons quickly enough.
> 
> 
> So I think a quick word to the wise is enough.  Comments are certainly
> not enough to prevent people being careless and making foolish mistakes.

Well it's not even about preventing anything, it's only about making
a line of cryptic code understandable for reviewers. No need for thorough
details, indeed anyone making use of that code or modifying it has to dive
into the deep guts anyway.

So how about that:

/*
 * Avoid rcu_dereference() in this very fast path.
 * Instead rely on full barrier implied by rq_lock() + smp_mb__after_spinlock().
 * Warning: In-between writes may be missed by readers (eg: pick_next_task())
 */

Thanks.