Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()

[Paul E. McKenney]

On Mon, Nov 12, 2018 at 10:00:47AM +0100, Peter Zijlstra wrote:
> On Sun, Nov 11, 2018 at 06:24:55PM -0800, Paul E. McKenney wrote:
> 
> > > > There were quite a few commits involved in making this happen.  Perhaps
> > > > the most pertinent are these:
> > > > 
> > > > 3e3100989869 ("rcu: Defer reporting RCU-preempt quiescent states when disabled")
> > > > 45975c7d21a1 ("rcu: Define RCU-sched API in terms of RCU for Tree RCU PREEMPT builds")
> > > 
> > > The latter; it does not mention that this will possible make
> > > synchronize_sched() quite a bit more expensive on PREEMPT=y builds :/
> > 
> > In theory, sure.  In practice, people have switched any number of
> > things from RCU-sched to RCU and back without problems.
> 
> Still, better safe than sorry. It was a rather big change in behaviour,
> so it wouldn't have been strange to call that out.

This guy:

45975c7d21a1 ("rcu: Define RCU-sched API in terms of RCU for Tree RCU PREEMPT builds")

Has a commit log that says:

	Now that RCU-preempt knows about preemption disabling, its
	implementation of synchronize_rcu() works for synchronize_sched(),
	and likewise for the other RCU-sched update-side API members.
	This commit therefore confines the RCU-sched update-side code
	to CONFIG_PREEMPT=n builds, and defines RCU-sched's update-side
	API members in terms of those of RCU-preempt.

That last phrase seems pretty explicit.  What am I missing here?

Not that it matters, given that I know of no way to change a mainlined
commit log.  I suppose I could ask Jon if he would be willing to take
a 2018 RCU API LWN article, if that would help.

> > > But for PREEMPT=y synchronize_sched() can be quite a bit shorter than
> > > synchronize_rcu(), since we don't have to wait for preempted read side
> > > stuff.
> > 
> > Again, there are quite a few places that have managed that transition
> > without issue.  Why do you expect this change to have problems that have
> > not been seen elsewhere?
> 
> I'm not, I'm just taking issue with the Changelog.

OK, good.

> > > Again, the patch didn't say that.
> > > 
> > > If the Changelog would've read something like:
> > > 
> > > "Since synchronize_sched() is now equivalent to synchronize_rcu(),
> > > replace the synchronize_sched() usage such that we can eventually remove
> > > the interface."
> > > 
> > > It would've been clear that the patch is a nop and what the purpose
> > > was.
> > 
> > I can easily make that change.
> 
> Please, sufficient doesn't imply necessary etc.. A changelog should
> always clarify why we do the patch.

???  Did you mean to say "necessary doesn't imply sufficient"?  If so,
what else do you feel is missing?

If not, color me confused.

							Thanx, Paul