Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Mon, Nov 12, 2018 at 02:21:12PM -0800, Paul E. McKenney wrote:
> On Mon, Nov 12, 2018 at 07:17:41PM +0100, Peter Zijlstra wrote:
> > On Mon, Nov 12, 2018 at 05:28:52AM -0800, Paul E. McKenney wrote:
> > > On Mon, Nov 12, 2018 at 10:00:47AM +0100, Peter Zijlstra wrote:
> >
> > > > 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?
> >
> > That does not explicitly state that because RCU-preempt
> > synchornize_rcu() can take _much_ longer, the new synchronize_sched()
> > can now take _much_ longer too.
> >
> > So when someone bisects a problem to this commit; and he reads the
> > Changelog, he might get the impression that was unexpected.
>
> Of course, a preempt_disable() section of code can still be preempted
> by the underlying hypervisor, so in a surprisingly large fraction of
> the installed base, there really isn't that much difference.
>
> > > 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.
> >
> > Yes, it is water under the bridge; but Changelogs should be explicit
> > about behavioural changes.
> >
> > And while the merged RCU has the semantic behaviour required, the timing
> > behaviour did change significantly.
>
> When running on bare metal, potentially. From what I see, preemption
> of RCU read-side critical sections is the exception rather than the rule.
> And again, when running on hypervisors, even irq-disable regions of code
> can be preempted. (And yes, there is work in flight to allow RCU to deal
> with this.)
>
> > > > > > 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?
> >
> > No, I meant to say that your original Changelog only states that
> > sync_rcu now covers rcu-sched behaviour. Which means that the change is
> > sufficient.
> >
> > It completely and utterly fails to explain _why_ you're doing the
> > change. Ie. you do not address why it is necessary.
> >
> > A Changelog should always explain why the change is needed.
> >
> > In this case because you want to get rid of the sync_sched() api.
>
> Right, which is stated in your suggested wording above. So I am still
> not seeing what you want added to this:
>
> "Since synchronize_sched() is now equivalent to synchronize_rcu(),
> replace the synchronize_sched() usage such that we can eventually
> remove the interface."
Finally getting back to this. I removed this commit from the group that
I intend to send in next week's -tip pull request, and updated its commit
log as shown below. Does this work for you?
Thanx, Paul
commit 52ffe7fbe615e8989f054432c76a7e43b8c35607
Author: Paul E. McKenney
Date: Tue Nov 6 19:13:54 2018 -0800
sched: Replace synchronize_sched() with synchronize_rcu()
Now that synchronize_rcu() waits for preempt-disable regions of
code as well as RCU read-side critical sections, synchronize_sched()
can be replaced by synchronize_rcu(), in fact, synchronize_sched()
is now completely equivalent to synchronize_rcu(). This commit
therefore replaces synchronize_sched() with synchronize_rcu() so that
synchronize_sched() can eventually be removed entirely.
Signed-off-by: Paul E. McKenney
Cc: Ingo Molnar
Cc: Peter Zijlstra
diff --git a/kernel/sched/cpufreq.c b/kernel/sched/cpufreq.c
index 5e54cbcae673..90fee8e01280 100644
---
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Mon, Nov 12, 2018 at 02:21:12PM -0800, Paul E. McKenney wrote:
> On Mon, Nov 12, 2018 at 07:17:41PM +0100, Peter Zijlstra wrote:
> > On Mon, Nov 12, 2018 at 05:28:52AM -0800, Paul E. McKenney wrote:
> > > On Mon, Nov 12, 2018 at 10:00:47AM +0100, Peter Zijlstra wrote:
> >
> > > > 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?
> >
> > That does not explicitly state that because RCU-preempt
> > synchornize_rcu() can take _much_ longer, the new synchronize_sched()
> > can now take _much_ longer too.
> >
> > So when someone bisects a problem to this commit; and he reads the
> > Changelog, he might get the impression that was unexpected.
>
> Of course, a preempt_disable() section of code can still be preempted
> by the underlying hypervisor, so in a surprisingly large fraction of
> the installed base, there really isn't that much difference.
>
> > > 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.
> >
> > Yes, it is water under the bridge; but Changelogs should be explicit
> > about behavioural changes.
> >
> > And while the merged RCU has the semantic behaviour required, the timing
> > behaviour did change significantly.
>
> When running on bare metal, potentially. From what I see, preemption
> of RCU read-side critical sections is the exception rather than the rule.
> And again, when running on hypervisors, even irq-disable regions of code
> can be preempted. (And yes, there is work in flight to allow RCU to deal
> with this.)
>
> > > > > > 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?
> >
> > No, I meant to say that your original Changelog only states that
> > sync_rcu now covers rcu-sched behaviour. Which means that the change is
> > sufficient.
> >
> > It completely and utterly fails to explain _why_ you're doing the
> > change. Ie. you do not address why it is necessary.
> >
> > A Changelog should always explain why the change is needed.
> >
> > In this case because you want to get rid of the sync_sched() api.
>
> Right, which is stated in your suggested wording above. So I am still
> not seeing what you want added to this:
>
> "Since synchronize_sched() is now equivalent to synchronize_rcu(),
> replace the synchronize_sched() usage such that we can eventually
> remove the interface."
Finally getting back to this. I removed this commit from the group that
I intend to send in next week's -tip pull request, and updated its commit
log as shown below. Does this work for you?
Thanx, Paul
commit 52ffe7fbe615e8989f054432c76a7e43b8c35607
Author: Paul E. McKenney
Date: Tue Nov 6 19:13:54 2018 -0800
sched: Replace synchronize_sched() with synchronize_rcu()
Now that synchronize_rcu() waits for preempt-disable regions of
code as well as RCU read-side critical sections, synchronize_sched()
can be replaced by synchronize_rcu(), in fact, synchronize_sched()
is now completely equivalent to synchronize_rcu(). This commit
therefore replaces synchronize_sched() with synchronize_rcu() so that
synchronize_sched() can eventually be removed entirely.
Signed-off-by: Paul E. McKenney
Cc: Ingo Molnar
Cc: Peter Zijlstra
diff --git a/kernel/sched/cpufreq.c b/kernel/sched/cpufreq.c
index 5e54cbcae673..90fee8e01280 100644
---
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Mon, Nov 12, 2018 at 07:17:41PM +0100, Peter Zijlstra wrote:
> On Mon, Nov 12, 2018 at 05:28:52AM -0800, Paul E. McKenney wrote:
> > On Mon, Nov 12, 2018 at 10:00:47AM +0100, Peter Zijlstra wrote:
>
> > > 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?
>
> That does not explicitly state that because RCU-preempt
> synchornize_rcu() can take _much_ longer, the new synchronize_sched()
> can now take _much_ longer too.
>
> So when someone bisects a problem to this commit; and he reads the
> Changelog, he might get the impression that was unexpected.
Of course, a preempt_disable() section of code can still be preempted
by the underlying hypervisor, so in a surprisingly large fraction of
the installed base, there really isn't that much difference.
> > 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.
>
> Yes, it is water under the bridge; but Changelogs should be explicit
> about behavioural changes.
>
> And while the merged RCU has the semantic behaviour required, the timing
> behaviour did change significantly.
When running on bare metal, potentially. From what I see, preemption
of RCU read-side critical sections is the exception rather than the rule.
And again, when running on hypervisors, even irq-disable regions of code
can be preempted. (And yes, there is work in flight to allow RCU to deal
with this.)
> > > > > 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?
>
> No, I meant to say that your original Changelog only states that
> sync_rcu now covers rcu-sched behaviour. Which means that the change is
> sufficient.
>
> It completely and utterly fails to explain _why_ you're doing the
> change. Ie. you do not address why it is necessary.
>
> A Changelog should always explain why the change is needed.
>
> In this case because you want to get rid of the sync_sched() api.
Right, which is stated in your suggested wording above. So I am still
not seeing what you want added to this:
"Since synchronize_sched() is now equivalent to synchronize_rcu(),
replace the synchronize_sched() usage such that we can eventually
remove the interface."
Thanx, Paul
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Mon, Nov 12, 2018 at 07:17:41PM +0100, Peter Zijlstra wrote:
> On Mon, Nov 12, 2018 at 05:28:52AM -0800, Paul E. McKenney wrote:
> > On Mon, Nov 12, 2018 at 10:00:47AM +0100, Peter Zijlstra wrote:
>
> > > 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?
>
> That does not explicitly state that because RCU-preempt
> synchornize_rcu() can take _much_ longer, the new synchronize_sched()
> can now take _much_ longer too.
>
> So when someone bisects a problem to this commit; and he reads the
> Changelog, he might get the impression that was unexpected.
Of course, a preempt_disable() section of code can still be preempted
by the underlying hypervisor, so in a surprisingly large fraction of
the installed base, there really isn't that much difference.
> > 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.
>
> Yes, it is water under the bridge; but Changelogs should be explicit
> about behavioural changes.
>
> And while the merged RCU has the semantic behaviour required, the timing
> behaviour did change significantly.
When running on bare metal, potentially. From what I see, preemption
of RCU read-side critical sections is the exception rather than the rule.
And again, when running on hypervisors, even irq-disable regions of code
can be preempted. (And yes, there is work in flight to allow RCU to deal
with this.)
> > > > > 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?
>
> No, I meant to say that your original Changelog only states that
> sync_rcu now covers rcu-sched behaviour. Which means that the change is
> sufficient.
>
> It completely and utterly fails to explain _why_ you're doing the
> change. Ie. you do not address why it is necessary.
>
> A Changelog should always explain why the change is needed.
>
> In this case because you want to get rid of the sync_sched() api.
Right, which is stated in your suggested wording above. So I am still
not seeing what you want added to this:
"Since synchronize_sched() is now equivalent to synchronize_rcu(),
replace the synchronize_sched() usage such that we can eventually
remove the interface."
Thanx, Paul
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Mon, 12 Nov 2018 19:17:41 +0100
Peter Zijlstra wrote:
> On Mon, Nov 12, 2018 at 05:28:52AM -0800, Paul E. McKenney wrote:
> > On Mon, Nov 12, 2018 at 10:00:47AM +0100, Peter Zijlstra wrote:
>
> > > 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?
>
> That does not explicitly state that because RCU-preempt
> synchornize_rcu() can take _much_ longer, the new synchronize_sched()
> can now take _much_ longer too.
I'm curious. Are there measurements to see how much longer they can
take? Saying "_much_ longer" would require that one has done the
timings to see what the actual impact is.
>
> So when someone bisects a problem to this commit; and he reads the
> Changelog, he might get the impression that was unexpected.
It may well be unexpected. What is the timing differences between a
normal synchronize_rcu and a synchronize_sched. Of course, 99% of users
wont see any difference as 99% don't run CONFIG_PREEMPT.
-- Steve
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Mon, 12 Nov 2018 19:17:41 +0100
Peter Zijlstra wrote:
> On Mon, Nov 12, 2018 at 05:28:52AM -0800, Paul E. McKenney wrote:
> > On Mon, Nov 12, 2018 at 10:00:47AM +0100, Peter Zijlstra wrote:
>
> > > 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?
>
> That does not explicitly state that because RCU-preempt
> synchornize_rcu() can take _much_ longer, the new synchronize_sched()
> can now take _much_ longer too.
I'm curious. Are there measurements to see how much longer they can
take? Saying "_much_ longer" would require that one has done the
timings to see what the actual impact is.
>
> So when someone bisects a problem to this commit; and he reads the
> Changelog, he might get the impression that was unexpected.
It may well be unexpected. What is the timing differences between a
normal synchronize_rcu and a synchronize_sched. Of course, 99% of users
wont see any difference as 99% don't run CONFIG_PREEMPT.
-- Steve
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Mon, Nov 12, 2018 at 05:28:52AM -0800, Paul E. McKenney wrote:
> On Mon, Nov 12, 2018 at 10:00:47AM +0100, Peter Zijlstra wrote:
> > 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?
That does not explicitly state that because RCU-preempt
synchornize_rcu() can take _much_ longer, the new synchronize_sched()
can now take _much_ longer too.
So when someone bisects a problem to this commit; and he reads the
Changelog, he might get the impression that was unexpected.
> 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.
Yes, it is water under the bridge; but Changelogs should be explicit
about behavioural changes.
And while the merged RCU has the semantic behaviour required, the timing
behaviour did change significantly.
> > > > 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?
No, I meant to say that your original Changelog only states that
sync_rcu now covers rcu-sched behaviour. Which means that the change is
sufficient.
It completely and utterly fails to explain _why_ you're doing the
change. Ie. you do not address why it is necessary.
A Changelog should always explain why the change is needed.
In this case because you want to get rid of the sync_sched() api.
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Mon, Nov 12, 2018 at 05:28:52AM -0800, Paul E. McKenney wrote:
> On Mon, Nov 12, 2018 at 10:00:47AM +0100, Peter Zijlstra wrote:
> > 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?
That does not explicitly state that because RCU-preempt
synchornize_rcu() can take _much_ longer, the new synchronize_sched()
can now take _much_ longer too.
So when someone bisects a problem to this commit; and he reads the
Changelog, he might get the impression that was unexpected.
> 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.
Yes, it is water under the bridge; but Changelogs should be explicit
about behavioural changes.
And while the merged RCU has the semantic behaviour required, the timing
behaviour did change significantly.
> > > > 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?
No, I meant to say that your original Changelog only states that
sync_rcu now covers rcu-sched behaviour. Which means that the change is
sufficient.
It completely and utterly fails to explain _why_ you're doing the
change. Ie. you do not address why it is necessary.
A Changelog should always explain why the change is needed.
In this case because you want to get rid of the sync_sched() api.
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
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
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
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.
> > 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.
> > 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.
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
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.
> > 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.
> > 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.
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Mon, Nov 12, 2018 at 03:07:10AM +0100, Peter Zijlstra wrote:
> On Sun, Nov 11, 2018 at 05:47:36PM -0800, Paul E. McKenney wrote:
> > On Mon, Nov 12, 2018 at 01:53:29AM +0100, Peter Zijlstra wrote:
> > > On Sun, Nov 11, 2018 at 04:45:28PM -0800, Paul E. McKenney wrote:
> > > > On Mon, Nov 12, 2018 at 01:12:33AM +0100, Peter Zijlstra wrote:
> > > > > On Sun, Nov 11, 2018 at 11:43:52AM -0800, Paul E. McKenney wrote:
> > > > > > Now that synchronize_rcu() waits for preempt-disable regions of code
> > > > > > as well as RCU read-side critical sections, synchronize_sched() can
> > > > > > be
> > > > > > replaced by synchronize_rcu(). This commit therefore makes this
> > > > > > change.
> > > > >
> > > > > Yes, but it also waits for an actual RCU quiestent state, which makes
> > > > > synchoinize_rcu() potentially much more expensive than an actual
> > > > > synchronize_sched().
> > > >
> > > > None of the readers have changed.
> > > >
> > > > For the updaters, if CONFIG_PREEMPT=n, synchronize_rcu() and
> > > > synchronize_sched() always were one and the same. When
> > > > CONFIG_PREEMPT=y,
> > > > synchronize_rcu() and synchronize_sched() are now one and the same.
> > >
> > > The Changelog does not state this; and does the commit that makes that
> > > happen state the regression potential?
> >
> > The Changelog says this:
> >
> > Now that synchronize_rcu() waits for preempt-disable
> > regions of code as well as RCU read-side critical sections,
> > synchronize_sched() can be replaced by synchronize_rcu().
> > This commit therefore makes this change.
> >
> > The "synchronize_rcu() waits for preempt-disable regions of code as
> > well as RCU read-side critical sections" seems pretty unambiguous to me.
> > Exactly what more are you wanting said there?
>
> The quoted bit only states that synchronize_rcu() is sufficient; it does
> not say it is equivalent and the patch is a nop. It also doesn't say
> that the purpose is to get rid of the synchronize_sched() function.
>
> > 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.
> > Normal grace periods are almost always quite long compared to typical
> > read-side critical sections, preempt-disable regions of code, and so on.
> > So in the common case this should be OK. Or are you instead worried
> > about synchronize_sched_expedited()?
>
> No, I still feel expedited should not exist at all ;-)
I figured as much. ;-)
> 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?
> > > > > So why are we doing this?
> > > >
> > > > Given that synchronize_rcu() and synchronize_sched() are now always one
> > > > and the same, this is a distinction without a difference.
> > >
> > > The Changelog did not state a reason for the patch. Therefore it is a
> > > bad patch.
> >
> > ??? Here is the current definition of synchronize_sched() in mainline:
> >
> > static inline void synchronize_sched(void)
> > {
> > synchronize_rcu();
> > }
>
> 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.
Thanx, Paul
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Mon, Nov 12, 2018 at 03:07:10AM +0100, Peter Zijlstra wrote:
> On Sun, Nov 11, 2018 at 05:47:36PM -0800, Paul E. McKenney wrote:
> > On Mon, Nov 12, 2018 at 01:53:29AM +0100, Peter Zijlstra wrote:
> > > On Sun, Nov 11, 2018 at 04:45:28PM -0800, Paul E. McKenney wrote:
> > > > On Mon, Nov 12, 2018 at 01:12:33AM +0100, Peter Zijlstra wrote:
> > > > > On Sun, Nov 11, 2018 at 11:43:52AM -0800, Paul E. McKenney wrote:
> > > > > > Now that synchronize_rcu() waits for preempt-disable regions of code
> > > > > > as well as RCU read-side critical sections, synchronize_sched() can
> > > > > > be
> > > > > > replaced by synchronize_rcu(). This commit therefore makes this
> > > > > > change.
> > > > >
> > > > > Yes, but it also waits for an actual RCU quiestent state, which makes
> > > > > synchoinize_rcu() potentially much more expensive than an actual
> > > > > synchronize_sched().
> > > >
> > > > None of the readers have changed.
> > > >
> > > > For the updaters, if CONFIG_PREEMPT=n, synchronize_rcu() and
> > > > synchronize_sched() always were one and the same. When
> > > > CONFIG_PREEMPT=y,
> > > > synchronize_rcu() and synchronize_sched() are now one and the same.
> > >
> > > The Changelog does not state this; and does the commit that makes that
> > > happen state the regression potential?
> >
> > The Changelog says this:
> >
> > Now that synchronize_rcu() waits for preempt-disable
> > regions of code as well as RCU read-side critical sections,
> > synchronize_sched() can be replaced by synchronize_rcu().
> > This commit therefore makes this change.
> >
> > The "synchronize_rcu() waits for preempt-disable regions of code as
> > well as RCU read-side critical sections" seems pretty unambiguous to me.
> > Exactly what more are you wanting said there?
>
> The quoted bit only states that synchronize_rcu() is sufficient; it does
> not say it is equivalent and the patch is a nop. It also doesn't say
> that the purpose is to get rid of the synchronize_sched() function.
>
> > 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.
> > Normal grace periods are almost always quite long compared to typical
> > read-side critical sections, preempt-disable regions of code, and so on.
> > So in the common case this should be OK. Or are you instead worried
> > about synchronize_sched_expedited()?
>
> No, I still feel expedited should not exist at all ;-)
I figured as much. ;-)
> 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?
> > > > > So why are we doing this?
> > > >
> > > > Given that synchronize_rcu() and synchronize_sched() are now always one
> > > > and the same, this is a distinction without a difference.
> > >
> > > The Changelog did not state a reason for the patch. Therefore it is a
> > > bad patch.
> >
> > ??? Here is the current definition of synchronize_sched() in mainline:
> >
> > static inline void synchronize_sched(void)
> > {
> > synchronize_rcu();
> > }
>
> 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.
Thanx, Paul
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Sun, Nov 11, 2018 at 05:47:36PM -0800, Paul E. McKenney wrote:
> On Mon, Nov 12, 2018 at 01:53:29AM +0100, Peter Zijlstra wrote:
> > On Sun, Nov 11, 2018 at 04:45:28PM -0800, Paul E. McKenney wrote:
> > > On Mon, Nov 12, 2018 at 01:12:33AM +0100, Peter Zijlstra wrote:
> > > > On Sun, Nov 11, 2018 at 11:43:52AM -0800, Paul E. McKenney wrote:
> > > > > Now that synchronize_rcu() waits for preempt-disable regions of code
> > > > > as well as RCU read-side critical sections, synchronize_sched() can be
> > > > > replaced by synchronize_rcu(). This commit therefore makes this
> > > > > change.
> > > >
> > > > Yes, but it also waits for an actual RCU quiestent state, which makes
> > > > synchoinize_rcu() potentially much more expensive than an actual
> > > > synchronize_sched().
> > >
> > > None of the readers have changed.
> > >
> > > For the updaters, if CONFIG_PREEMPT=n, synchronize_rcu() and
> > > synchronize_sched() always were one and the same. When CONFIG_PREEMPT=y,
> > > synchronize_rcu() and synchronize_sched() are now one and the same.
> >
> > The Changelog does not state this; and does the commit that makes that
> > happen state the regression potential?
>
> The Changelog says this:
>
> Now that synchronize_rcu() waits for preempt-disable
> regions of code as well as RCU read-side critical sections,
> synchronize_sched() can be replaced by synchronize_rcu().
> This commit therefore makes this change.
>
> The "synchronize_rcu() waits for preempt-disable regions of code as
> well as RCU read-side critical sections" seems pretty unambiguous to me.
> Exactly what more are you wanting said there?
The quoted bit only states that synchronize_rcu() is sufficient; it does
not say it is equivalent and the patch is a nop. It also doesn't say
that the purpose is to get rid of the synchronize_sched() function.
> 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 :/
> Normal grace periods are almost always quite long compared to typical
> read-side critical sections, preempt-disable regions of code, and so on.
> So in the common case this should be OK. Or are you instead worried
> about synchronize_sched_expedited()?
No, I still feel expedited should not exist at all ;-)
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.
> > > > So why are we doing this?
> > >
> > > Given that synchronize_rcu() and synchronize_sched() are now always one
> > > and the same, this is a distinction without a difference.
> >
> > The Changelog did not state a reason for the patch. Therefore it is a
> > bad patch.
>
> ??? Here is the current definition of synchronize_sched() in mainline:
>
> static inline void synchronize_sched(void)
> {
> synchronize_rcu();
> }
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.
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Sun, Nov 11, 2018 at 05:47:36PM -0800, Paul E. McKenney wrote:
> On Mon, Nov 12, 2018 at 01:53:29AM +0100, Peter Zijlstra wrote:
> > On Sun, Nov 11, 2018 at 04:45:28PM -0800, Paul E. McKenney wrote:
> > > On Mon, Nov 12, 2018 at 01:12:33AM +0100, Peter Zijlstra wrote:
> > > > On Sun, Nov 11, 2018 at 11:43:52AM -0800, Paul E. McKenney wrote:
> > > > > Now that synchronize_rcu() waits for preempt-disable regions of code
> > > > > as well as RCU read-side critical sections, synchronize_sched() can be
> > > > > replaced by synchronize_rcu(). This commit therefore makes this
> > > > > change.
> > > >
> > > > Yes, but it also waits for an actual RCU quiestent state, which makes
> > > > synchoinize_rcu() potentially much more expensive than an actual
> > > > synchronize_sched().
> > >
> > > None of the readers have changed.
> > >
> > > For the updaters, if CONFIG_PREEMPT=n, synchronize_rcu() and
> > > synchronize_sched() always were one and the same. When CONFIG_PREEMPT=y,
> > > synchronize_rcu() and synchronize_sched() are now one and the same.
> >
> > The Changelog does not state this; and does the commit that makes that
> > happen state the regression potential?
>
> The Changelog says this:
>
> Now that synchronize_rcu() waits for preempt-disable
> regions of code as well as RCU read-side critical sections,
> synchronize_sched() can be replaced by synchronize_rcu().
> This commit therefore makes this change.
>
> The "synchronize_rcu() waits for preempt-disable regions of code as
> well as RCU read-side critical sections" seems pretty unambiguous to me.
> Exactly what more are you wanting said there?
The quoted bit only states that synchronize_rcu() is sufficient; it does
not say it is equivalent and the patch is a nop. It also doesn't say
that the purpose is to get rid of the synchronize_sched() function.
> 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 :/
> Normal grace periods are almost always quite long compared to typical
> read-side critical sections, preempt-disable regions of code, and so on.
> So in the common case this should be OK. Or are you instead worried
> about synchronize_sched_expedited()?
No, I still feel expedited should not exist at all ;-)
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.
> > > > So why are we doing this?
> > >
> > > Given that synchronize_rcu() and synchronize_sched() are now always one
> > > and the same, this is a distinction without a difference.
> >
> > The Changelog did not state a reason for the patch. Therefore it is a
> > bad patch.
>
> ??? Here is the current definition of synchronize_sched() in mainline:
>
> static inline void synchronize_sched(void)
> {
> synchronize_rcu();
> }
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.
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Mon, Nov 12, 2018 at 01:53:29AM +0100, Peter Zijlstra wrote:
> On Sun, Nov 11, 2018 at 04:45:28PM -0800, Paul E. McKenney wrote:
> > On Mon, Nov 12, 2018 at 01:12:33AM +0100, Peter Zijlstra wrote:
> > > On Sun, Nov 11, 2018 at 11:43:52AM -0800, Paul E. McKenney wrote:
> > > > Now that synchronize_rcu() waits for preempt-disable regions of code
> > > > as well as RCU read-side critical sections, synchronize_sched() can be
> > > > replaced by synchronize_rcu(). This commit therefore makes this change.
> > >
> > > Yes, but it also waits for an actual RCU quiestent state, which makes
> > > synchoinize_rcu() potentially much more expensive than an actual
> > > synchronize_sched().
> >
> > None of the readers have changed.
> >
> > For the updaters, if CONFIG_PREEMPT=n, synchronize_rcu() and
> > synchronize_sched() always were one and the same. When CONFIG_PREEMPT=y,
> > synchronize_rcu() and synchronize_sched() are now one and the same.
>
> The Changelog does not state this; and does the commit that makes that
> happen state the regression potential?
The Changelog says this:
Now that synchronize_rcu() waits for preempt-disable
regions of code as well as RCU read-side critical sections,
synchronize_sched() can be replaced by synchronize_rcu().
This commit therefore makes this change.
The "synchronize_rcu() waits for preempt-disable regions of code as
well as RCU read-side critical sections" seems pretty unambiguous to me.
Exactly what more are you wanting said there?
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")
Normal grace periods are almost always quite long compared to typical
read-side critical sections, preempt-disable regions of code, and so on.
So in the common case this should be OK. Or are you instead worried
about synchronize_sched_expedited()?
> > > So why are we doing this?
> >
> > Given that synchronize_rcu() and synchronize_sched() are now always one
> > and the same, this is a distinction without a difference.
>
> The Changelog did not state a reason for the patch. Therefore it is a
> bad patch.
??? Here is the current definition of synchronize_sched() in mainline:
static inline void synchronize_sched(void)
{
synchronize_rcu();
}
Thanx, Paul
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Mon, Nov 12, 2018 at 01:53:29AM +0100, Peter Zijlstra wrote:
> On Sun, Nov 11, 2018 at 04:45:28PM -0800, Paul E. McKenney wrote:
> > On Mon, Nov 12, 2018 at 01:12:33AM +0100, Peter Zijlstra wrote:
> > > On Sun, Nov 11, 2018 at 11:43:52AM -0800, Paul E. McKenney wrote:
> > > > Now that synchronize_rcu() waits for preempt-disable regions of code
> > > > as well as RCU read-side critical sections, synchronize_sched() can be
> > > > replaced by synchronize_rcu(). This commit therefore makes this change.
> > >
> > > Yes, but it also waits for an actual RCU quiestent state, which makes
> > > synchoinize_rcu() potentially much more expensive than an actual
> > > synchronize_sched().
> >
> > None of the readers have changed.
> >
> > For the updaters, if CONFIG_PREEMPT=n, synchronize_rcu() and
> > synchronize_sched() always were one and the same. When CONFIG_PREEMPT=y,
> > synchronize_rcu() and synchronize_sched() are now one and the same.
>
> The Changelog does not state this; and does the commit that makes that
> happen state the regression potential?
The Changelog says this:
Now that synchronize_rcu() waits for preempt-disable
regions of code as well as RCU read-side critical sections,
synchronize_sched() can be replaced by synchronize_rcu().
This commit therefore makes this change.
The "synchronize_rcu() waits for preempt-disable regions of code as
well as RCU read-side critical sections" seems pretty unambiguous to me.
Exactly what more are you wanting said there?
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")
Normal grace periods are almost always quite long compared to typical
read-side critical sections, preempt-disable regions of code, and so on.
So in the common case this should be OK. Or are you instead worried
about synchronize_sched_expedited()?
> > > So why are we doing this?
> >
> > Given that synchronize_rcu() and synchronize_sched() are now always one
> > and the same, this is a distinction without a difference.
>
> The Changelog did not state a reason for the patch. Therefore it is a
> bad patch.
??? Here is the current definition of synchronize_sched() in mainline:
static inline void synchronize_sched(void)
{
synchronize_rcu();
}
Thanx, Paul
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Sun, Nov 11, 2018 at 04:45:28PM -0800, Paul E. McKenney wrote: > On Mon, Nov 12, 2018 at 01:12:33AM +0100, Peter Zijlstra wrote: > > On Sun, Nov 11, 2018 at 11:43:52AM -0800, Paul E. McKenney wrote: > > > Now that synchronize_rcu() waits for preempt-disable regions of code > > > as well as RCU read-side critical sections, synchronize_sched() can be > > > replaced by synchronize_rcu(). This commit therefore makes this change. > > > > Yes, but it also waits for an actual RCU quiestent state, which makes > > synchoinize_rcu() potentially much more expensive than an actual > > synchronize_sched(). > > None of the readers have changed. > > For the updaters, if CONFIG_PREEMPT=n, synchronize_rcu() and > synchronize_sched() always were one and the same. When CONFIG_PREEMPT=y, > synchronize_rcu() and synchronize_sched() are now one and the same. The Changelog does not state this; and does the commit that makes that happen state the regression potential? > > So why are we doing this? > > Given that synchronize_rcu() and synchronize_sched() are now always one > and the same, this is a distinction without a difference. The Changelog did not state a reason for the patch. Therefore it is a bad patch.
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Sun, Nov 11, 2018 at 04:45:28PM -0800, Paul E. McKenney wrote: > On Mon, Nov 12, 2018 at 01:12:33AM +0100, Peter Zijlstra wrote: > > On Sun, Nov 11, 2018 at 11:43:52AM -0800, Paul E. McKenney wrote: > > > Now that synchronize_rcu() waits for preempt-disable regions of code > > > as well as RCU read-side critical sections, synchronize_sched() can be > > > replaced by synchronize_rcu(). This commit therefore makes this change. > > > > Yes, but it also waits for an actual RCU quiestent state, which makes > > synchoinize_rcu() potentially much more expensive than an actual > > synchronize_sched(). > > None of the readers have changed. > > For the updaters, if CONFIG_PREEMPT=n, synchronize_rcu() and > synchronize_sched() always were one and the same. When CONFIG_PREEMPT=y, > synchronize_rcu() and synchronize_sched() are now one and the same. The Changelog does not state this; and does the commit that makes that happen state the regression potential? > > So why are we doing this? > > Given that synchronize_rcu() and synchronize_sched() are now always one > and the same, this is a distinction without a difference. The Changelog did not state a reason for the patch. Therefore it is a bad patch.
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Mon, Nov 12, 2018 at 01:12:33AM +0100, Peter Zijlstra wrote: > On Sun, Nov 11, 2018 at 11:43:52AM -0800, Paul E. McKenney wrote: > > Now that synchronize_rcu() waits for preempt-disable regions of code > > as well as RCU read-side critical sections, synchronize_sched() can be > > replaced by synchronize_rcu(). This commit therefore makes this change. > > Yes, but it also waits for an actual RCU quiestent state, which makes > synchoinize_rcu() potentially much more expensive than an actual > synchronize_sched(). None of the readers have changed. For the updaters, if CONFIG_PREEMPT=n, synchronize_rcu() and synchronize_sched() always were one and the same. When CONFIG_PREEMPT=y, synchronize_rcu() and synchronize_sched() are now one and the same. > So why are we doing this? Given that synchronize_rcu() and synchronize_sched() are now always one and the same, this is a distinction without a difference. So we might as well get rid of the _bh and _sched APIs. (See the tail end of current mainline's include/linux/rcupdate.h.) If you are instead asking why the RCU flavors (RCU-bh, RCU-preempt, and RCU-sched) got merged, it was due to a security incident stemming from confusion between two of the flavor, with the resulting bug turning out to be exploitable. Linus therefore requested that I do something to make this not happen again, which I did. Thanx, Paul
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Mon, Nov 12, 2018 at 01:12:33AM +0100, Peter Zijlstra wrote: > On Sun, Nov 11, 2018 at 11:43:52AM -0800, Paul E. McKenney wrote: > > Now that synchronize_rcu() waits for preempt-disable regions of code > > as well as RCU read-side critical sections, synchronize_sched() can be > > replaced by synchronize_rcu(). This commit therefore makes this change. > > Yes, but it also waits for an actual RCU quiestent state, which makes > synchoinize_rcu() potentially much more expensive than an actual > synchronize_sched(). None of the readers have changed. For the updaters, if CONFIG_PREEMPT=n, synchronize_rcu() and synchronize_sched() always were one and the same. When CONFIG_PREEMPT=y, synchronize_rcu() and synchronize_sched() are now one and the same. > So why are we doing this? Given that synchronize_rcu() and synchronize_sched() are now always one and the same, this is a distinction without a difference. So we might as well get rid of the _bh and _sched APIs. (See the tail end of current mainline's include/linux/rcupdate.h.) If you are instead asking why the RCU flavors (RCU-bh, RCU-preempt, and RCU-sched) got merged, it was due to a security incident stemming from confusion between two of the flavor, with the resulting bug turning out to be exploitable. Linus therefore requested that I do something to make this not happen again, which I did. Thanx, Paul
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Sun, Nov 11, 2018 at 11:43:52AM -0800, Paul E. McKenney wrote: > Now that synchronize_rcu() waits for preempt-disable regions of code > as well as RCU read-side critical sections, synchronize_sched() can be > replaced by synchronize_rcu(). This commit therefore makes this change. Yes, but it also waits for an actual RCU quiestent state, which makes synchoinize_rcu() potentially much more expensive than an actual synchronize_sched(). So why are we doing this?
Re: [PATCH tip/core/rcu 23/41] sched: Replace synchronize_sched() with synchronize_rcu()
On Sun, Nov 11, 2018 at 11:43:52AM -0800, Paul E. McKenney wrote: > Now that synchronize_rcu() waits for preempt-disable regions of code > as well as RCU read-side critical sections, synchronize_sched() can be > replaced by synchronize_rcu(). This commit therefore makes this change. Yes, but it also waits for an actual RCU quiestent state, which makes synchoinize_rcu() potentially much more expensive than an actual synchronize_sched(). So why are we doing this?
