lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20181112132852.GH4170@linux.ibm.com>
Date:   Mon, 12 Nov 2018 05:28:52 -0800
From:   "Paul E. McKenney" <paulmck@...ux.ibm.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     linux-kernel@...r.kernel.org, mingo@...nel.org,
        jiangshanlai@...il.com, dipankar@...ibm.com,
        akpm@...ux-foundation.org, mathieu.desnoyers@...icios.com,
        josh@...htriplett.org, tglx@...utronix.de, rostedt@...dmis.org,
        dhowells@...hat.com, edumazet@...gle.com, fweisbec@...il.com,
        oleg@...hat.com, joel@...lfernandes.org,
        Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched()
 with synchronize_rcu()

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ