Re: [PATCH] rcu: Make jiffies_till_sched_qs writable

[Byungchul Park]

On Tue, Jul 09, 2019 at 05:41:02AM -0700, Paul E. McKenney wrote:
> > Hi Paul,
> > 
> > IMHO, as much as we want to tune the time for fqs to be initiated, we
> > can also want to tune the time for the help from scheduler to start.
> > I thought only difference between them is a level of urgency. I might be
> > wrong. It would be appreciated if you let me know if I miss something.
> 
> Hello, Byungchul,
> 
> I understand that one hypothetically might want to tune this at runtime,
> but have you had need to tune this at runtime on a real production
> workload?  If so, what problem was happening that caused you to want to
> do this tuning?

Not actually.

> > And it's ok even if the patch is turned down based on your criteria. :)
> 
> If there is a real need, something needs to be provided to meet that
> need.  But in the absence of a real need, past experience has shown
> that speculative tuning knobs usually do more harm than good.  ;-)

It makes sense, "A speculative tuning knobs do more harm than good".

Then, it would be better to leave jiffies_till_{first,next}_fqs tunnable
but jiffies_till_sched_qs until we need it.

However,

(1) In case that jiffies_till_sched_qs is tunnable:

	We might need all of jiffies_till_{first,next}_qs,
	jiffies_till_sched_qs and jiffies_to_sched_qs because
	jiffies_to_sched_qs can be affected by any of them. So we
	should be able to read each value at any time.

(2) In case that jiffies_till_sched_qs is not tunnable:

	I think we don't have to keep the jiffies_till_sched_qs any
	longer since that's only for setting jiffies_to_sched_qs at
	*booting time*, which can be done with jiffies_to_sched_qs too.
	It's meaningless to keep all of tree variables.

The simpler and less knobs that we really need we have, the better.

what do you think about it?

In the following patch, I (1) removed jiffies_till_sched_qs and then
(2) renamed jiffies_*to*_sched_qs to jiffies_*till*_sched_qs because I
think jiffies_till_sched_qs is a much better name for the purpose. I
will resend it with a commit msg after knowing your opinion on it.

Thanks,
Byungchul

---8<---
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index e72c184..94b58f5 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3792,10 +3792,6 @@
 			a value based on the most recent settings
 			of rcutree.jiffies_till_first_fqs
 			and rcutree.jiffies_till_next_fqs.
-			This calculated value may be viewed in
-			rcutree.jiffies_to_sched_qs.  Any attempt to set
-			rcutree.jiffies_to_sched_qs will be cheerfully
-			overwritten.
 
 	rcutree.kthread_prio= 	 [KNL,BOOT]
 			Set the SCHED_FIFO priority of the RCU per-CPU
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index a2f8ba2..ad9dc86 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -421,10 +421,8 @@ static int rcu_is_cpu_rrupt_from_idle(void)
  * How long the grace period must be before we start recruiting
  * quiescent-state help from rcu_note_context_switch().
  */
-static ulong jiffies_till_sched_qs = ULONG_MAX;
+static ulong jiffies_till_sched_qs = ULONG_MAX; /* See adjust_jiffies_till_sched_qs(). */
 module_param(jiffies_till_sched_qs, ulong, 0444);
-static ulong jiffies_to_sched_qs; /* See adjust_jiffies_till_sched_qs(). */
-module_param(jiffies_to_sched_qs, ulong, 0444); /* Display only! */
 
 /*
  * Make sure that we give the grace-period kthread time to detect any
@@ -436,18 +434,13 @@ static void adjust_jiffies_till_sched_qs(void)
 {
 	unsigned long j;
 
-	/* If jiffies_till_sched_qs was specified, respect the request. */
-	if (jiffies_till_sched_qs != ULONG_MAX) {
-		WRITE_ONCE(jiffies_to_sched_qs, jiffies_till_sched_qs);
-		return;
-	}
 	/* Otherwise, set to third fqs scan, but bound below on large system. */
 	j = READ_ONCE(jiffies_till_first_fqs) +
 		      2 * READ_ONCE(jiffies_till_next_fqs);
 	if (j < HZ / 10 + nr_cpu_ids / RCU_JIFFIES_FQS_DIV)
 		j = HZ / 10 + nr_cpu_ids / RCU_JIFFIES_FQS_DIV;
 	pr_info("RCU calculated value of scheduler-enlistment delay is %ld jiffies.\n", j);
-	WRITE_ONCE(jiffies_to_sched_qs, j);
+	WRITE_ONCE(jiffies_till_sched_qs, j);
 }
 
 static int param_set_first_fqs_jiffies(const char *val, const struct kernel_param *kp)
@@ -1033,16 +1026,16 @@ static int rcu_implicit_dynticks_qs(struct rcu_data *rdp)
 
 	/*
 	 * A CPU running for an extended time within the kernel can
-	 * delay RCU grace periods: (1) At age jiffies_to_sched_qs,
-	 * set .rcu_urgent_qs, (2) At age 2*jiffies_to_sched_qs, set
+	 * delay RCU grace periods: (1) At age jiffies_till_sched_qs,
+	 * set .rcu_urgent_qs, (2) At age 2*jiffies_till_sched_qs, set
 	 * both .rcu_need_heavy_qs and .rcu_urgent_qs.  Note that the
 	 * unsynchronized assignments to the per-CPU rcu_need_heavy_qs
 	 * variable are safe because the assignments are repeated if this
 	 * CPU failed to pass through a quiescent state.  This code
-	 * also checks .jiffies_resched in case jiffies_to_sched_qs
+	 * also checks .jiffies_resched in case jiffies_till_sched_qs
 	 * is set way high.
 	 */
-	jtsq = READ_ONCE(jiffies_to_sched_qs);
+	jtsq = READ_ONCE(jiffies_till_sched_qs);
 	ruqp = per_cpu_ptr(&rcu_data.rcu_urgent_qs, rdp->cpu);
 	rnhqp = &per_cpu(rcu_data.rcu_need_heavy_qs, rdp->cpu);
 	if (!READ_ONCE(*rnhqp) &&
@@ -3383,7 +3376,8 @@ static void __init rcu_init_geometry(void)
 		jiffies_till_first_fqs = d;
 	if (jiffies_till_next_fqs == ULONG_MAX)
 		jiffies_till_next_fqs = d;
-	adjust_jiffies_till_sched_qs();
+	if (jiffies_till_sched_qs == ULONG_MAX)
+		adjust_jiffies_till_sched_qs();
 
 	/* If the compile-time values are accurate, just leave. */
 	if (rcu_fanout_leaf == RCU_FANOUT_LEAF &&