More RCU flavor consolidation cleanup for v4.21/v5.0

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.

Quoted 3 lines · Paul E. McKenney

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?

Quoted 8 lines · Peter Zijlstra

Quoted 3 lines · Paul E. McKenney

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.

Quoted 1 line

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.

Quoted 10 lines · Paul E. McKenney

Quoted 8 lines · Peter Zijlstra

Quoted 3 lines · Paul E. McKenney

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().

Quoted 5 lines

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?

Quoted 4 lines

Quoted 1 line

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.

Quoted 11 lines · Peter Zijlstra

Quoted 10 lines · Paul E. McKenney

Quoted 8 lines · Peter Zijlstra

Quoted 3 lines · Paul E. McKenney

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().

Quoted 8 lines

Quoted 5 lines

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()?

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

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();
	}

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(). Similarly, call_rcu_sched() can be replaced by call_rcu(). This commit therefore makes these changes.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Steven Rostedt <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc <[email protected]>

Quoted 23 lines · Paul E. McKenney

Quoted 11 lines · Peter Zijlstra

Quoted 10 lines · Paul E. McKenney

Quoted 8 lines · Peter Zijlstra

Quoted 3 lines · Paul E. McKenney

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().

Quoted 8 lines

Quoted 5 lines

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:

Quoted 8 lines

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.

Quoted 5 lines

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 :/

Quoted 4 lines

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.

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 6 lines

??? 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.

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.

Signed-off-by Paul E. McKenney <[email protected]>

Cc "Naveen N. Rao" <[email protected]>

Cc Anil S Keshavamurthy <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Masami Hiramatsu <[email protected]>

---
 kernel/kprobes.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

kernel/kprobes.c +5 -5

index 90e98e233647..08e31d863191 100644
@@ -229,7 +229,7 @@ static int collect_garbage_slots(struct kprobe_insn_cache *c)
 	struct kprobe_insn_page *kip, *next;
 
 	/* Ensure no-one is interrupted on the garbages */
-	synchronize_sched();
+	synchronize_rcu();
 
 	list_for_each_entry_safe(kip, next, &c->pages, list) {
 		int i;

@@ -1382,7 +1382,7 @@ static int register_aggr_kprobe(struct kprobe *orig_p, struct kprobe *p)
 			if (ret) {
 				ap->flags |= KPROBE_FLAG_DISABLED;
 				list_del_rcu(&p->list);
-				synchronize_sched();
+				synchronize_rcu();
 			}
 		}
 	}

@@ -1597,7 +1597,7 @@ int register_kprobe(struct kprobe *p)
 		ret = arm_kprobe(p);
 		if (ret) {
 			hlist_del_rcu(&p->hlist);
-			synchronize_sched();
+			synchronize_rcu();
 			goto out;
 		}
 	}

@@ -1776,7 +1776,7 @@ void unregister_kprobes(struct kprobe **kps, int num)
 			kps[i]->addr = NULL;
 	mutex_unlock(&kprobe_mutex);
 
-	synchronize_sched();
+	synchronize_rcu();
 	for (i = 0; i < num; i++)
 		if (kps[i]->addr)
 			__unregister_kprobe_bottom(kps[i]);

@@ -1966,7 +1966,7 @@ void unregister_kretprobes(struct kretprobe **rps, int num)
 			rps[i]->kp.addr = NULL;
 	mutex_unlock(&kprobe_mutex);
 
-	synchronize_sched();
+	synchronize_rcu();
 	for (i = 0; i < num; i++) {
 		if (rps[i]->kp.addr) {
 			__unregister_kprobe_bottom(&rps[i]->kp);

Signature · 1 line

2.17.1

Quoted 3 lines · On Sun, 11 Nov 2018 11:43:49 -0800 "Paul E. McKenney" <[email protected]>…

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.

Would you mean synchronize_rcu() can ensure that any interrupt handler (which should run under preempt-disable state) run out (even on non-preemptive kernel)?

If so, I agree with these changes.

Thank you,

Quoted 62 lines

Signed-off-by Paul E. McKenney <[email protected]>

Cc "Naveen N. Rao" <[email protected]>

Cc Anil S Keshavamurthy <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Masami Hiramatsu <[email protected]>

---
 kernel/kprobes.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

kernel/kprobes.c +5 -5

index 90e98e233647..08e31d863191 100644
@@ -229,7 +229,7 @@ static int collect_garbage_slots(struct kprobe_insn_cache *c)
 	struct kprobe_insn_page *kip, *next;
 
 	/* Ensure no-one is interrupted on the garbages */
-	synchronize_sched();
+	synchronize_rcu();
 
 	list_for_each_entry_safe(kip, next, &c->pages, list) {
 		int i;

@@ -1382,7 +1382,7 @@ static int register_aggr_kprobe(struct kprobe *orig_p, struct kprobe *p)
 			if (ret) {
 				ap->flags |= KPROBE_FLAG_DISABLED;
 				list_del_rcu(&p->list);
-				synchronize_sched();
+				synchronize_rcu();
 			}
 		}
 	}

@@ -1597,7 +1597,7 @@ int register_kprobe(struct kprobe *p)
 		ret = arm_kprobe(p);
 		if (ret) {
 			hlist_del_rcu(&p->hlist);
-			synchronize_sched();
+			synchronize_rcu();
 			goto out;
 		}
 	}

@@ -1776,7 +1776,7 @@ void unregister_kprobes(struct kprobe **kps, int num)
 			kps[i]->addr = NULL;
 	mutex_unlock(&kprobe_mutex);
 
-	synchronize_sched();
+	synchronize_rcu();
 	for (i = 0; i < num; i++)
 		if (kps[i]->addr)
 			__unregister_kprobe_bottom(kps[i]);

@@ -1966,7 +1966,7 @@ void unregister_kretprobes(struct kretprobe **rps, int num)
 			rps[i]->kp.addr = NULL;
 	mutex_unlock(&kprobe_mutex);
 
-	synchronize_sched();
+	synchronize_rcu();
 	for (i = 0; i < num; i++) {
 		if (rps[i]->kp.addr) {
 			__unregister_kprobe_bottom(&rps[i]->kp);

Signature · 1 line

2.17.1

Signature · 1 line

Masami Hiramatsu <[email protected]>

Quoted 8 lines

Quoted 5 lines

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.

Quoted 3 lines

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?

Quoted 4 lines

Again, the patch didn't say that.

If the Changelog would've read something like:

Quoted 6 lines

"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.

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(). Similarly, call_rcu_sched() can be replaced by call_rcu(). This commit therefore makes these changes.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Jessica Yu <[email protected]>

---
kernel/module.c | 14 +++++++-------

1 file changed, 7 insertions(+), 7 deletions(-)

kernel/module.c +4 -4

index 49a405891587..99b46c32d579 100644
@@ -2159,7 +2159,7 @@ static void free_module(struct module *mod)
 	/* Remove this module from bug list, this uses list_del_rcu */
 	module_bug_cleanup(mod);
 	/* Wait for RCU-sched synchronizing before releasing mod->list and buglist. */
-	synchronize_sched();
+	synchronize_rcu();
 	mutex_unlock(&module_mutex);
 
 	/* This may be empty, but that's OK */

@@ -3507,15 +3507,15 @@ static noinline int do_init_module(struct module *mod)
 	/*
 	 * We want to free module_init, but be aware that kallsyms may be
 	 * walking this with preempt disabled.  In all the failure paths, we
-	 * call synchronize_sched(), but we don't want to slow down the success
+	 * call synchronize_rcu(), but we don't want to slow down the success
 	 * path, so use actual RCU here.
 	 * Note that module_alloc() on most architectures creates W+X page
 	 * mappings which won't be cleaned up until do_free_init() runs.  Any
 	 * code such as mark_rodata_ro() which depends on those mappings to
 	 * be cleaned up needs to sync with the queued work - ie
-	 * rcu_barrier_sched()
+	 * rcu_barrier()
 	 */
-	call_rcu_sched(&freeinit->rcu, do_free_init);
+	call_rcu(&freeinit->rcu, do_free_init);
 	mutex_unlock(&module_mutex);
 	wake_up_all(&module_wq);
 

@@ -3526,7 +3526,7 @@ static noinline int do_init_module(struct module *mod)

fail:

	/* Try to protect us from buggy refcounters. */
	mod->state = MODULE_STATE_GOING;

kernel/module.c +1 -1

-	synchronize_sched();
+	synchronize_rcu();

	module_put(mod);
	blocking_notifier_call_chain(&module_notify_list,
				     MODULE_STATE_GOING, mod);

kernel/module.c +2 -2

@@ -3819,7 +3819,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 ddebug_cleanup:
 	ftrace_release_mod(mod);
 	dynamic_debug_remove(mod, info->debug);
-	synchronize_sched();
+	synchronize_rcu();
 	kfree(mod->args);
 free_arch_cleanup:
 	module_arch_cleanup(mod);

@@ -3834,7 +3834,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	mod_tree_remove(mod);
 	wake_up_all(&module_wq);
 	/* Wait for RCU-sched synchronizing before releasing mod->list. */
-	synchronize_sched();
+	synchronize_rcu();
 	mutex_unlock(&module_mutex);
 free_module:
 	/* Free lock-classes; relies on the preceding sync_rcu() */

Signature · 1 line

2.17.1

Hello!

This series does additional cleanup for the RCU flavor consolidation, focusing primarily on uses of old API members, for example, so that call_rcu_bh() becomes call_rcu(). There are also a few straggling internal-to-RCU cleanups.

1. Remove unused rcu_state externs, courtesy of Joel Fernandes.

2. Fix rcu_{node,data} comments about gp_seq_needed, courtesy of
Joel Fernandes.

3. Eliminate synchronize_rcu_mult() and its sole caller.

4. Consolidate the RCU update functions invoked by sync.c.

5-41. Replace old flavorful RCU API calls with the corresponding
vanilla calls.

Quoted 6 lines · Paul E. McKenney

Quoted 6 lines

Quoted 5 lines

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")

Quoted 5 lines

Quoted 2 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 8 lines

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."

Quoted 4 lines

Quoted 2 lines

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.

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.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Corey Minyard <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc <[email protected]>

---
  drivers/char/ipmi/ipmi_si_intf.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/char/ipmi/ipmi_si_intf.c +1 -1

index 677618e6f1f7..dc8603d34320 100644
@@ -2187,7 +2187,7 @@ static void shutdown_smi(void *send_info)
  	 * handlers might have been running before we freed the
  	 * interrupt.
  	 */
-	synchronize_sched();
+	synchronize_rcu();
 
  	/*
  	 * Timeouts are stopped, now make sure the interrupts are off

Quoted 12 lines · ----- On Nov 11, 2018, at 2:41 PM, paulmck [email protected] wrote:

Hello!

This series does additional cleanup for the RCU flavor consolidation, focusing primarily on uses of old API members, for example, so that call_rcu_bh() becomes call_rcu(). There are also a few straggling internal-to-RCU cleanups.

1. Remove unused rcu_state externs, courtesy of Joel Fernandes.

2. Fix rcu_{node,data} comments about gp_seq_needed, courtesy of
Joel Fernandes.

Quoted 6 lines

3. Eliminate synchronize_rcu_mult() and its sole caller.

4. Consolidate the RCU update functions invoked by sync.c.

5-41. Replace old flavorful RCU API calls with the corresponding
vanilla calls.

Hi Paul,

Just a heads up: we might want to spell out warnings in very big letters for anyone trying to backport code using RCU from post-4.21 kernels back to older kernels. I fear that newer code will build just fine on older kernels, but will spectacularly fail in hard-to-debug ways at runtime.

Renaming synchronize_rcu() and call_rcu() to something that did not exist in prior kernels would prevent that. It may not be as pretty though.

Quoted 3 lines · Paul E. McKenney

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.

Assuming the above is true, this looks fine to me.

Acked-by Corey MInyard <[email protected]>

Or I can take it in my tree, either way.

-corey

Quoted 21 lines

Signed-off-by Paul E. McKenney <[email protected]>

Cc Corey Minyard <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc <[email protected]>

---
 drivers/char/ipmi/ipmi_si_intf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/char/ipmi/ipmi_si_intf.c +1 -1

index 677618e6f1f7..dc8603d34320 100644
@@ -2187,7 +2187,7 @@ static void shutdown_smi(void *send_info)
 	 * handlers might have been running before we freed the
 	 * interrupt.
 	 */
-	synchronize_sched();
+	synchronize_rcu();
 	/*
 	 * Timeouts are stopped, now make sure the interrupts are off

Quoted 7 lines · Corey Minyard

Quoted 3 lines · Paul E. McKenney

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.

Assuming the above is true, this looks fine to me.

Acked-by Corey MInyard <[email protected]>

Thank you!

Quoted 1 line

Or I can take it in my tree, either way.

I will carry it unless/until I send it upstream (this coming merge window) or I see it already upstream.

So your choice. ;-)

Thanx, Paul

Quoted 25 lines

-corey

Quoted 21 lines

Signed-off-by Paul E. McKenney <[email protected]>

Cc Corey Minyard <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc <[email protected]>

---
  drivers/char/ipmi/ipmi_si_intf.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/char/ipmi/ipmi_si_intf.c +1 -1

index 677618e6f1f7..dc8603d34320 100644
@@ -2187,7 +2187,7 @@ static void shutdown_smi(void *send_info)
  	 * handlers might have been running before we freed the
  	 * interrupt.
  	 */
-	synchronize_sched();
+	synchronize_rcu();
  	/*
  	 * Timeouts are stopped, now make sure the interrupts are off

+++ Paul E. McKenney [11/11/18 11:43 -0800]:

Quoted 7 lines

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(). Similarly, call_rcu_sched() can be replaced by call_rcu(). This commit therefore makes these changes.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Jessica Yu <[email protected]>

Acked-by Jessica Yu <[email protected]>

Thanks!

Quoted 66 lines

---
kernel/module.c | 14 +++++++-------

1 file changed, 7 insertions(+), 7 deletions(-)

kernel/module.c +4 -4

index 49a405891587..99b46c32d579 100644
@@ -2159,7 +2159,7 @@ static void free_module(struct module *mod)
 	/* Remove this module from bug list, this uses list_del_rcu */
 	module_bug_cleanup(mod);
 	/* Wait for RCU-sched synchronizing before releasing mod->list and buglist. */
-	synchronize_sched();
+	synchronize_rcu();
 	mutex_unlock(&module_mutex);
 
 	/* This may be empty, but that's OK */

@@ -3507,15 +3507,15 @@ static noinline int do_init_module(struct module *mod)
 	/*
 	 * We want to free module_init, but be aware that kallsyms may be
 	 * walking this with preempt disabled.  In all the failure paths, we
-	 * call synchronize_sched(), but we don't want to slow down the success
+	 * call synchronize_rcu(), but we don't want to slow down the success
 	 * path, so use actual RCU here.
 	 * Note that module_alloc() on most architectures creates W+X page
 	 * mappings which won't be cleaned up until do_free_init() runs.  Any
 	 * code such as mark_rodata_ro() which depends on those mappings to
 	 * be cleaned up needs to sync with the queued work - ie
-	 * rcu_barrier_sched()
+	 * rcu_barrier()
 	 */
-	call_rcu_sched(&freeinit->rcu, do_free_init);
+	call_rcu(&freeinit->rcu, do_free_init);
 	mutex_unlock(&module_mutex);
 	wake_up_all(&module_wq);
 

@@ -3526,7 +3526,7 @@ static noinline int do_init_module(struct module *mod)

fail:

	/* Try to protect us from buggy refcounters. */
	mod->state = MODULE_STATE_GOING;

kernel/module.c +1 -1

-	synchronize_sched();
+	synchronize_rcu();

	module_put(mod);
	blocking_notifier_call_chain(&module_notify_list,
				     MODULE_STATE_GOING, mod);

kernel/module.c +0 -0

@@ -3819,7 +3819,7 @@ static int load_module(struct load_info *info, const char __user *uargs,

ddebug_cleanup:

	ftrace_release_mod(mod);
	dynamic_debug_remove(mod, info->debug);

kernel/module.c +1 -1

-	synchronize_sched();
+	synchronize_rcu();

	kfree(mod->args);
free_arch_cleanup:
	module_arch_cleanup(mod);

kernel/module.c +1 -1

@@ -3834,7 +3834,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	mod_tree_remove(mod);
 	wake_up_all(&module_wq);
 	/* Wait for RCU-sched synchronizing before releasing mod->list. */
-	synchronize_sched();
+	synchronize_rcu();
 	mutex_unlock(&module_mutex);

free_module:

	/* Free lock-classes; relies on the preceding sync_rcu() */

Signature · 1 line

2.17.1

Quoted 5 lines · Paul E. McKenney

Quoted 4 lines · Corey Minyard

Quoted 3 lines · Paul E. McKenney

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.

Quoted 3 lines

Quoted 3 lines

Assuming the above is true, this looks fine to me.

Acked-by Corey MInyard <[email protected]>

Dang, that's

Acked-by Corey Minyard <[email protected]>

Quoted 7 lines

Thank you!

Quoted 1 line

Or I can take it in my tree, either way.

I will carry it unless/until I send it upstream (this coming merge window) or I see it already upstream.

So your choice. ;-)

Ok, if you don't mind, you can carry it.

Thanks,

-corey

Quoted 27 lines

Thanx, Paul

Quoted 25 lines

-corey

Quoted 21 lines

Signed-off-by Paul E. McKenney <[email protected]>

Cc Corey Minyard <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc <[email protected]>

---
 drivers/char/ipmi/ipmi_si_intf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/char/ipmi/ipmi_si_intf.c +1 -1

index 677618e6f1f7..dc8603d34320 100644
@@ -2187,7 +2187,7 @@ static void shutdown_smi(void *send_info)
 	 * handlers might have been running before we freed the
 	 * interrupt.
 	 */
-	synchronize_sched();
+	synchronize_rcu();
 	/*
 	 * Timeouts are stopped, now make sure the interrupts are off

Quoted 2 lines · Peter Zijlstra

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.

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

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.

Quoted 4 lines

Quoted 1 line

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?

Quoted 18 lines · Paul E. McKenney

Quoted 2 lines · Peter Zijlstra

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.

Quoted 1 line

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.

Quoted 29 lines · Mathieu Desnoyers

Quoted 12 lines · ----- On Nov 11, 2018, at 2:41 PM, paulmck [email protected] wrote:

Hello!

This series does additional cleanup for the RCU flavor consolidation, focusing primarily on uses of old API members, for example, so that call_rcu_bh() becomes call_rcu(). There are also a few straggling internal-to-RCU cleanups.

1. Remove unused rcu_state externs, courtesy of Joel Fernandes.

2. Fix rcu_{node,data} comments about gp_seq_needed, courtesy of
Joel Fernandes.

Quoted 6 lines

3. Eliminate synchronize_rcu_mult() and its sole caller.

4. Consolidate the RCU update functions invoked by sync.c.

5-41. Replace old flavorful RCU API calls with the corresponding
vanilla calls.

Hi Paul,

Just a heads up: we might want to spell out warnings in very big letters for anyone trying to backport code using RCU from post-4.21 kernels back to older kernels. I fear that newer code will build just fine on older kernels, but will spectacularly fail in hard-to-debug ways at runtime.

Quoted 3 lines

Renaming synchronize_rcu() and call_rcu() to something that did not exist in prior kernels would prevent that. It may not be as pretty though.

From v4.20 rather than v4.21, but yes. Would it make sense to have Sasha automatically flag -stable candidates going back past that boundary that contain call_rcu(), synchronize_rcu(), etc.? Adding Sasha on CC, and I might be able to touch base with him this week.

Quoted 30 lines · Paul E. McKenney

Quoted 29 lines · Mathieu Desnoyers

Quoted 12 lines · ----- On Nov 11, 2018, at 2:41 PM, paulmck [email protected] wrote:

Hello!

This series does additional cleanup for the RCU flavor consolidation, focusing primarily on uses of old API members, for example, so that call_rcu_bh() becomes call_rcu(). There are also a few straggling internal-to-RCU cleanups.

1. Remove unused rcu_state externs, courtesy of Joel Fernandes.

2. Fix rcu_{node,data} comments about gp_seq_needed, courtesy of
Joel Fernandes.

Quoted 6 lines

3. Eliminate synchronize_rcu_mult() and its sole caller.

4. Consolidate the RCU update functions invoked by sync.c.

5-41. Replace old flavorful RCU API calls with the corresponding
vanilla calls.

Hi Paul,

Just a heads up: we might want to spell out warnings in very big letters for anyone trying to backport code using RCU from post-4.21 kernels back to older kernels. I fear that newer code will build just fine on older kernels, but will spectacularly fail in hard-to-debug ways at runtime.

Quoted 8 lines

Quoted 3 lines

Renaming synchronize_rcu() and call_rcu() to something that did not exist in prior kernels would prevent that. It may not be as pretty though.

From v4.20 rather than v4.21, but yes. Would it make sense to have Sasha automatically flag -stable candidates going back past that boundary that contain call_rcu(), synchronize_rcu(), etc.? Adding Sasha on CC, and I might be able to touch base with him this week.

We had a similar issue recently with a vfs change
(https://patchwork.kernel.org/patch/10604339/) leading to potentially the same results as described above, we took it as is to avoid these issues in the future, though this is a much smaller change than what's proposed here.

We can look into an good way to solve this. While I can alert on post-4.20 stable tagged patches that touch rcu, do you really want to be dealing with this for the next 10+ years? It'll also means each of those patches will need a manual backport.

Let's talk at Plumbers :)

Quoted 18 lines · Paul E. McKenney

Quoted 2 lines · Peter Zijlstra

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.

Quoted 1 line

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.

Quoted 3 lines

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.

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

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.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

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.

Now that call_rcu()'s callback is not invoked until after all bh-disable regions of code have completed (in addition to explicitly marked RCU read-side critical sections), call_rcu() can be used in place of call_rcu_bh(). Similarly, rcu_barrier() can be used in place of rcu_barrier_bh(). This commit therefore makes these changes.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Roopa Prabhu <[email protected]>

Cc Nikolay Aleksandrov <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc <[email protected]>

Cc <[email protected]>

---
 net/bridge/br_mdb.c       |  2 +-
 net/bridge/br_multicast.c | 14 +++++++-------
 2 files changed, 8 insertions(+), 8 deletions(-)

Now that call_rcu()'s callback is not invoked until after all preempt-disable regions of code have completed (in addition to explicitly marked RCU read-side critical sections), call_rcu() can be used in place of call_rcu_sched(). This commit therefore makes that change.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Lai Jiangshan <[email protected]>

Now that call_rcu()'s callback is not invoked until after all preempt-disable regions of code have completed (in addition to explicitly marked RCU read-side critical sections), call_rcu() can be used in place of call_rcu_sched(). This commit therefore makes that change.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Ming Lei <[email protected]>

Cc Bart Van Assche <[email protected]>

Cc Jens Axboe <[email protected]>

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, even though it is but a comment.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Dennis Zhou <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Christoph Lameter <[email protected]>

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, even though it is but a comment.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Dennis Zhou <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc "Dennis Zhou (Facebook)" <[email protected]>

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Paul E. McKenney
Sent: Sunday, November 11, 2018 11:44 AM
To: linux-kernel at vger.kernel.org
Cc: tglx at linutronix.de; David S. Miller <[email protected]>; peterz at infradead.org; fweisbec at gmail.com; jiangshanlai at gmail.com; josh at joshtriplett.org; rostedt at goodmis.org; oleg at redhat.com; dhowells at redhat.com; edumazet at google.com;
mathieu.desnoyers at efficios.com; dipankar at in.ibm.com;
joel at joelfernandes.org; intel-wired-lan at lists.osuosl.org; akpm at linux- foundation.org; Paul E. McKenney <[email protected]>;
mingo at kernel.org
Subject: [Intel-wired-lan] [PATCH tip/core/rcu 13/41] ethernet/intel/ixgbe:
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(). This commit therefore makes this change.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Jeff Kirsher <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc <[email protected]>

---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Quoted 7 lines · Masami Hiramatsu

Quoted 3 lines · On Sun, 11 Nov 2018 11:43:49 -0800 "Paul E. McKenney" <[email protected]>…

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.

Quoted 3 lines

Would you mean synchronize_rcu() can ensure that any interrupt handler (which should run under preempt-disable state) run out (even on non-preemptive kernel)?

Yes, but only as of this merge window. See this commit:

3e3100989869 ("rcu: Defer reporting RCU-preempt quiescent states when disabled")

Don't try this in v4.19 or earlier, but v4.20 and later is OK. ;-)

Thanx, Paul

Quoted 71 lines

If so, I agree with these changes.

Thank you,

Quoted 62 lines

Signed-off-by Paul E. McKenney <[email protected]>

Cc "Naveen N. Rao" <[email protected]>

Cc Anil S Keshavamurthy <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Masami Hiramatsu <[email protected]>

---
 kernel/kprobes.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

kernel/kprobes.c +5 -5

index 90e98e233647..08e31d863191 100644
@@ -229,7 +229,7 @@ static int collect_garbage_slots(struct kprobe_insn_cache *c)
 	struct kprobe_insn_page *kip, *next;
 
 	/* Ensure no-one is interrupted on the garbages */
-	synchronize_sched();
+	synchronize_rcu();
 
 	list_for_each_entry_safe(kip, next, &c->pages, list) {
 		int i;

@@ -1382,7 +1382,7 @@ static int register_aggr_kprobe(struct kprobe *orig_p, struct kprobe *p)
 			if (ret) {
 				ap->flags |= KPROBE_FLAG_DISABLED;
 				list_del_rcu(&p->list);
-				synchronize_sched();
+				synchronize_rcu();
 			}
 		}
 	}

@@ -1597,7 +1597,7 @@ int register_kprobe(struct kprobe *p)
 		ret = arm_kprobe(p);
 		if (ret) {
 			hlist_del_rcu(&p->hlist);
-			synchronize_sched();
+			synchronize_rcu();
 			goto out;
 		}
 	}

@@ -1776,7 +1776,7 @@ void unregister_kprobes(struct kprobe **kps, int num)
 			kps[i]->addr = NULL;
 	mutex_unlock(&kprobe_mutex);
 
-	synchronize_sched();
+	synchronize_rcu();
 	for (i = 0; i < num; i++)
 		if (kps[i]->addr)
 			__unregister_kprobe_bottom(kps[i]);

@@ -1966,7 +1966,7 @@ void unregister_kretprobes(struct kretprobe **rps, int num)
 			rps[i]->kp.addr = NULL;
 	mutex_unlock(&kprobe_mutex);
 
-	synchronize_sched();
+	synchronize_rcu();
 	for (i = 0; i < num; i++) {
 		if (rps[i]->kp.addr) {
 			__unregister_kprobe_bottom(&rps[i]->kp);

Signature · 1 line

2.17.1

Signature · 1 line

Masami Hiramatsu <[email protected]>

Quoted 8 lines · Paul E. McKenney

Now that call_rcu()'s callback is not invoked until after all preempt-disable regions of code have completed (in addition to explicitly marked RCU read-side critical sections), call_rcu() can be used in place of call_rcu_sched(). This commit therefore makes that change.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Lai Jiangshan <[email protected]>

Acked-by Tejun Heo <[email protected]>

Quoted 14 lines · On Sun, 11 Nov 2018 19:19:16 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 10 lines · Masami Hiramatsu

Quoted 3 lines · On Sun, 11 Nov 2018 11:43:49 -0800 "Paul E. McKenney" <[email protected]>…

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.

Would you mean synchronize_rcu() can ensure that any interrupt handler (which should run under preempt-disable state) run out (even on non-preemptive kernel)?

Yes, but only as of this merge window. See this commit:

Quoted 1 line

3e3100989869 ("rcu: Defer reporting RCU-preempt quiescent states when disabled")

OK, I also found that now those are same.

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

Acked-by Masami Hiramatsu <[email protected]>

Thank you!

Quoted 77 lines

Don't try this in v4.19 or earlier, but v4.20 and later is OK. ;-)

Thanx, Paul

Quoted 71 lines

If so, I agree with these changes.

Thank you,

Quoted 62 lines

Signed-off-by Paul E. McKenney <[email protected]>

Cc "Naveen N. Rao" <[email protected]>

Cc Anil S Keshavamurthy <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Masami Hiramatsu <[email protected]>

---
 kernel/kprobes.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

kernel/kprobes.c +5 -5

index 90e98e233647..08e31d863191 100644
@@ -229,7 +229,7 @@ static int collect_garbage_slots(struct kprobe_insn_cache *c)
 	struct kprobe_insn_page *kip, *next;
 
 	/* Ensure no-one is interrupted on the garbages */
-	synchronize_sched();
+	synchronize_rcu();
 
 	list_for_each_entry_safe(kip, next, &c->pages, list) {
 		int i;

@@ -1382,7 +1382,7 @@ static int register_aggr_kprobe(struct kprobe *orig_p, struct kprobe *p)
 			if (ret) {
 				ap->flags |= KPROBE_FLAG_DISABLED;
 				list_del_rcu(&p->list);
-				synchronize_sched();
+				synchronize_rcu();
 			}
 		}
 	}

@@ -1597,7 +1597,7 @@ int register_kprobe(struct kprobe *p)
 		ret = arm_kprobe(p);
 		if (ret) {
 			hlist_del_rcu(&p->hlist);
-			synchronize_sched();
+			synchronize_rcu();
 			goto out;
 		}
 	}

@@ -1776,7 +1776,7 @@ void unregister_kprobes(struct kprobe **kps, int num)
 			kps[i]->addr = NULL;
 	mutex_unlock(&kprobe_mutex);
 
-	synchronize_sched();
+	synchronize_rcu();
 	for (i = 0; i < num; i++)
 		if (kps[i]->addr)
 			__unregister_kprobe_bottom(kps[i]);

@@ -1966,7 +1966,7 @@ void unregister_kretprobes(struct kretprobe **rps, int num)
 			rps[i]->kp.addr = NULL;
 	mutex_unlock(&kprobe_mutex);
 
-	synchronize_sched();
+	synchronize_rcu();
 	for (i = 0; i < num; i++) {
 		if (rps[i]->kp.addr) {
 			__unregister_kprobe_bottom(&rps[i]->kp);

Signature · 1 line

2.17.1

Signature · 1 line

Masami Hiramatsu <[email protected]>

Signature · 1 line

Masami Hiramatsu <[email protected]>

Now that synchronize_rcu() waits for bh-disable regions of code as well as RCU read-side critical sections, the synchronize_rcu_bh() in pcrypt_cpumask_change_notify() can be replaced by synchronize_rcu(). This commit therefore makes this change.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Steffen Klassert <[email protected]>

Cc <[email protected]>

Quoted 19 lines · Peter Zijlstra

Quoted 18 lines · Paul E. McKenney

Quoted 2 lines · Peter Zijlstra

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.

Quoted 8 lines

Quoted 1 line

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.

Quoted 9 lines

Quoted 3 lines

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.)

Quoted 28 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

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.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 1 line

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."

Quoted 8 lines · Paul E. McKenney

Now that synchronize_rcu() waits for bh-disable regions of code as well as RCU read-side critical sections, the synchronize_rcu_bh() in pcrypt_cpumask_change_notify() can be replaced by synchronize_rcu(). This commit therefore makes this change.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Steffen Klassert <[email protected]>

Cc <[email protected]>

Acked-by Herbert Xu <[email protected]>