EXP rcu: Revert expedited GP parallelization cleverness

DISCUSSION Paul E. McKenney 2 participants 3 messages Nov 1, 2018 – Nov 8, 2018

Share & tools

Paul E. McKenney

author

2018-11-01 23:30 UTC #0 lore raw

Quoted 27 lines

(Commit 258ba8e089db23f760139266c232f01bad73f85c from linux-rcu)
This commit reverts a series of commits starting with fcc635436501 ("rcu:
Make expedited GPs handle CPU 0 being offline") and its successors, thus queueing each rcu_node structure's expedited grace-period initialization work on the first CPU of that rcu_node structure.
Suggested-by Sebastian Andrzej Siewior <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
Signed-off-by Sebastian Andrzej Siewior <[email protected]>
kernel/rcu/tree_exp.h +0 -1
index 0b2c2ad69629..a0486414edb4 100644
@@ -472,7 +472,6 @@ static void sync_rcu_exp_select_node_cpus(struct work_struct *wp)
 static void sync_rcu_exp_select_cpus(struct rcu_state *rsp,
 				     smp_call_func_t func)
 {
-	int cpu;
 	struct rcu_node *rnp;
 
 	trace_rcu_exp_grace_period(rsp->name, rcu_exp_gp_seq_endval(rsp), TPS("reset"));
@@ -494,13 +493,7 @@ static void sync_rcu_exp_select_cpus(struct rcu_state *rsp,
 			continue;
 		}
 		INIT_WORK(&rnp->rew.rew_work, sync_rcu_exp_select_node_cpus);

kernel/rcu/tree_exp.h static void sync_rcu_exp_select_cpus(struct rcu_state *rsp, · line 498

Quoted 10 lines

kernel/rcu/tree_exp.h +1 -7

-		preempt_disable();
-		cpu = cpumask_next(rnp->grplo - 1, cpu_online_mask);
-		/* If all offline, queue the work on an unbound CPU. */
-		if (unlikely(cpu > rnp->grphi))
-			cpu = WORK_CPU_UNBOUND;
-		queue_work_on(cpu, rcu_par_gp_wq, &rnp->rew.rew_work);
-		preempt_enable();
+		queue_work_on(rnp->grplo, rcu_par_gp_wq, &rnp->rew.rew_work);
 		rnp->exp_need_flush = true;
 	}

How about instead changing the earlier "if" statement to read as follows?

		if (!READ_ONCE(rcu_par_gp_wq) ||
		    rcu_scheduler_active != RCU_SCHEDULER_RUNNING ||
		    rcu_is_last_leaf_node(rnp) ||
		    IS_ENABLED(CONFIG_PREEMPT_RT_FULL)) {
			/* No workqueues yet or last leaf, do direct call. */
			sync_rcu_exp_select_node_cpus(&rnp->rew.rew_work);
			continue;
		}

This just adds the "|| IS_ENABLED(CONFIG_PREEMPT_RT_FULL)" to the "if" condition.

The advantage of this approach is that it leaves the parallelization alone for mainline, and avoids the overhead of the workqueues for -rt.

Thanx, Paul

Sebastian Andrzej Siewior

2018-11-08 16:58 UTC #1 lore raw

Quoted 49 lines · Paul E. McKenney

Quoted 27 lines
(Commit 258ba8e089db23f760139266c232f01bad73f85c from linux-rcu)
This commit reverts a series of commits starting with fcc635436501 ("rcu:
Make expedited GPs handle CPU 0 being offline") and its successors, thus queueing each rcu_node structure's expedited grace-period initialization work on the first CPU of that rcu_node structure.
Suggested-by Sebastian Andrzej Siewior <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
Signed-off-by Sebastian Andrzej Siewior <[email protected]>
kernel/rcu/tree_exp.h +0 -1
index 0b2c2ad69629..a0486414edb4 100644
@@ -472,7 +472,6 @@ static void sync_rcu_exp_select_node_cpus(struct work_struct *wp)
 static void sync_rcu_exp_select_cpus(struct rcu_state *rsp,
 				     smp_call_func_t func)
 {
-	int cpu;
 	struct rcu_node *rnp;
 
 	trace_rcu_exp_grace_period(rsp->name, rcu_exp_gp_seq_endval(rsp), TPS("reset"));
@@ -494,13 +493,7 @@ static void sync_rcu_exp_select_cpus(struct rcu_state *rsp,
 			continue;
 		}
 		INIT_WORK(&rnp->rew.rew_work, sync_rcu_exp_select_node_cpus);
kernel/rcu/tree_exp.h static void sync_rcu_exp_select_cpus(struct rcu_state *rsp, · line 498
Quoted 10 lines
kernel/rcu/tree_exp.h +1 -7
-		preempt_disable();
-		cpu = cpumask_next(rnp->grplo - 1, cpu_online_mask);
-		/* If all offline, queue the work on an unbound CPU. */
-		if (unlikely(cpu > rnp->grphi))
-			cpu = WORK_CPU_UNBOUND;
-		queue_work_on(cpu, rcu_par_gp_wq, &rnp->rew.rew_work);
-		preempt_enable();
+		queue_work_on(rnp->grplo, rcu_par_gp_wq, &rnp->rew.rew_work);
 		rnp->exp_need_flush = true;
 	}
How about instead changing the earlier "if" statement to read as follows?
		if (!READ_ONCE(rcu_par_gp_wq) ||
		    rcu_scheduler_active != RCU_SCHEDULER_RUNNING ||
		    rcu_is_last_leaf_node(rnp) ||
		    IS_ENABLED(CONFIG_PREEMPT_RT_FULL)) {
			/* No workqueues yet or last leaf, do direct call. */
			sync_rcu_exp_select_node_cpus(&rnp->rew.rew_work);
			continue;
		}

Quoted 5 lines

This just adds the "|| IS_ENABLED(CONFIG_PREEMPT_RT_FULL)" to the "if" condition.
The advantage of this approach is that it leaves the parallelization alone for mainline, and avoids the overhead of the workqueues for -rt.

I don't oppose to the workqueue approach. It is just preempt_disable() + workqueue don't work on -RT. And if I remember correctly, we can't take CPU hotplug lock for other reasons (which woould make the preempt_disable() go away). Also the original argument why that patch went in was not solid so I though removing the extra complexity would be a good thing.
However using sync_rcu_exp_select_node_cpus() (based von v4.20-rc1) should work on -RT from what I can see. And performance wise it should not matter for -RT because the whole synchronize_.*_expedited() is disabled on -RT anyway. So it should be used only during boot-up.

Quoted 1 line

Thanx, Paul

Sebastian

Paul E. McKenney

author

2018-11-08 17:33 UTC #2 lore raw

Quoted 56 lines · Sebastian Andrzej Siewior

Quoted 49 lines · Paul E. McKenney
Quoted 27 lines
(Commit 258ba8e089db23f760139266c232f01bad73f85c from linux-rcu)
This commit reverts a series of commits starting with fcc635436501 ("rcu:
Make expedited GPs handle CPU 0 being offline") and its successors, thus queueing each rcu_node structure's expedited grace-period initialization work on the first CPU of that rcu_node structure.
Suggested-by Sebastian Andrzej Siewior <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
Signed-off-by Sebastian Andrzej Siewior <[email protected]>
kernel/rcu/tree_exp.h +0 -1
index 0b2c2ad69629..a0486414edb4 100644
@@ -472,7 +472,6 @@ static void sync_rcu_exp_select_node_cpus(struct work_struct *wp)
 static void sync_rcu_exp_select_cpus(struct rcu_state *rsp,
 				     smp_call_func_t func)
 {
-	int cpu;
 	struct rcu_node *rnp;
 
 	trace_rcu_exp_grace_period(rsp->name, rcu_exp_gp_seq_endval(rsp), TPS("reset"));
@@ -494,13 +493,7 @@ static void sync_rcu_exp_select_cpus(struct rcu_state *rsp,
 			continue;
 		}
 		INIT_WORK(&rnp->rew.rew_work, sync_rcu_exp_select_node_cpus);
kernel/rcu/tree_exp.h static void sync_rcu_exp_select_cpus(struct rcu_state *rsp, · line 498
Quoted 10 lines
kernel/rcu/tree_exp.h +1 -7
-		preempt_disable();
-		cpu = cpumask_next(rnp->grplo - 1, cpu_online_mask);
-		/* If all offline, queue the work on an unbound CPU. */
-		if (unlikely(cpu > rnp->grphi))
-			cpu = WORK_CPU_UNBOUND;
-		queue_work_on(cpu, rcu_par_gp_wq, &rnp->rew.rew_work);
-		preempt_enable();
+		queue_work_on(rnp->grplo, rcu_par_gp_wq, &rnp->rew.rew_work);
 		rnp->exp_need_flush = true;
 	}
How about instead changing the earlier "if" statement to read as follows?
		if (!READ_ONCE(rcu_par_gp_wq) ||
		    rcu_scheduler_active != RCU_SCHEDULER_RUNNING ||
		    rcu_is_last_leaf_node(rnp) ||
		    IS_ENABLED(CONFIG_PREEMPT_RT_FULL)) {
			/* No workqueues yet or last leaf, do direct call. */
			sync_rcu_exp_select_node_cpus(&rnp->rew.rew_work);
			continue;
		}
Quoted 5 lines
This just adds the "|| IS_ENABLED(CONFIG_PREEMPT_RT_FULL)" to the "if" condition.
The advantage of this approach is that it leaves the parallelization alone for mainline, and avoids the overhead of the workqueues for -rt.

Quoted 8 lines

I don't oppose to the workqueue approach. It is just preempt_disable() + workqueue don't work on -RT. And if I remember correctly, we can't take CPU hotplug lock for other reasons (which woould make the preempt_disable() go away). Also the original argument why that patch went in was not solid so I though removing the extra complexity would be a good thing.
From what I can see, always using the unbound workqueue can serialize

things on some platforms, which kind of defeats the whole purpose of using the workqueues in the first place.

Quoted 4 lines

However using sync_rcu_exp_select_node_cpus() (based von v4.20-rc1) should work on -RT from what I can see. And performance wise it should not matter for -RT because the whole synchronize_.*_expedited() is disabled on -RT anyway. So it should be used only during boot-up.

Agreed, which was why I proposed making -RT use the boot-time code path, given that -RT only uses this code during boot. ;-)

Thanx, Paul