Re: [PATCH] timers/nohz: Update nohz load even if tick already stopped

[Peter Zijlstra]

On Mon, Oct 28, 2019 at 04:07:16PM +0100, Frederic Weisbecker wrote:
> From: Scott Wood <swood@xxxxxxxxxx>
> 
> The way loadavg is tracked during nohz only pays attention to the load
> upon entering nohz. This can be particularly noticeable if nohz is
> entered while non-idle, and then the cpu goes idle and stays that way for
> a long time. We've had reports of a loadavg near 150 on a mostly idle
> system.
> 
> Calling calc_load_nohz_start() regardless of whether the tick is already
> stopped addresses the issue when going idle. Tracking load changes when
> not going idle (e.g. multiple SCHED_FIFO tasks coming and going) is not
> addressed by this patch.

Hurph, is that phenomena you describe NOHZ or NOHZ_FULL? Because that
second thing you talk about, multiple SCHED_FIFO tasks running without a
tick is definitely NOHZ_FULL.

I'm thinking all of this is NOHZ_FULL because IIRC we always start the
tick when there is a runnable task. So your example of going idle in
NOHZ already cannot happen for regular NOHZ.

And for loadavg vs NOHZ_FULL I don't much like this patch. It ductapes
one symptom but doesn't address the actual issue. Let me go rediscover
how loadavg-nohz works again, I so hate all this crap :/

Bah, I can't get me head straight, but I'm thinking something like the
below ought to do. Does it actually work?

---
 include/linux/sched/nohz.h |  2 ++
 kernel/sched/core.c        |  3 +++
 kernel/sched/loadavg.c     | 26 +++++++++++++++++++-------
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/include/linux/sched/nohz.h b/include/linux/sched/nohz.h
index 1abe91ff6e4a..a2296004351f 100644
--- a/include/linux/sched/nohz.h
+++ b/include/linux/sched/nohz.h
@@ -16,9 +16,11 @@ static inline void nohz_balance_enter_idle(int cpu) { }
 #ifdef CONFIG_NO_HZ_COMMON
 void calc_load_nohz_start(void);
 void calc_load_nohz_stop(void);
+void calc_load_nohz_remote(int cpu);
 #else
 static inline void calc_load_nohz_start(void) { }
 static inline void calc_load_nohz_stop(void) { }
+static inline void calc_load_nohz_remote(int cpu) { }
 #endif /* CONFIG_NO_HZ_COMMON */
 
 #if defined(CONFIG_NO_HZ_COMMON) && defined(CONFIG_SMP)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index eb42b71faab9..209e50d48f80 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3666,6 +3666,8 @@ static void sched_tick_remote(struct work_struct *work)
 	 * having one too much is no big deal because the scheduler tick updates
 	 * statistics and checks timeslices in a time-independent way, regardless
 	 * of when exactly it is running.
+	 *
+	 * XXX should we be checking tick_nohz_tick_stopped_cpu() under rq->lock ?
 	 */
 	if (idle_cpu(cpu) || !tick_nohz_tick_stopped_cpu(cpu))
 		goto out_requeue;
@@ -3686,6 +3688,7 @@ static void sched_tick_remote(struct work_struct *work)
 	curr->sched_class->task_tick(rq, curr, 0);
 
 out_unlock:
+	calc_load_nohz_remote(cpu);
 	rq_unlock_irq(rq, &rf);
 
 out_requeue:
diff --git a/kernel/sched/loadavg.c b/kernel/sched/loadavg.c
index 28a516575c18..7549bd9b6853 100644
--- a/kernel/sched/loadavg.c
+++ b/kernel/sched/loadavg.c
@@ -231,16 +231,11 @@ static inline int calc_load_read_idx(void)
 	return calc_load_idx & 1;
 }
 
-void calc_load_nohz_start(void)
+static void calc_load_nohz_fold(struct rq *rq)
 {
-	struct rq *this_rq = this_rq();
 	long delta;
 
-	/*
-	 * We're going into NO_HZ mode, if there's any pending delta, fold it
-	 * into the pending NO_HZ delta.
-	 */
-	delta = calc_load_fold_active(this_rq, 0);
+	delta = calc_load_fold_active(rq, 0);
 	if (delta) {
 		int idx = calc_load_write_idx();
 
@@ -248,6 +243,23 @@ void calc_load_nohz_start(void)
 	}
 }
 
+void calc_load_nohz_start(void)
+{
+	/*
+	 * We're going into NO_HZ mode, if there's any pending delta, fold it
+	 * into the pending NO_HZ delta.
+	 */
+	calc_load_nohz_fold(this_rq());
+}
+
+void calc_load_nohz_remote(int cpu)
+{
+	/*
+	 * XXX comment goes here.
+	 */
+	calc_load_nohz_fold(cpu_rq(cpu));
+}
+
 void calc_load_nohz_stop(void)
 {
 	struct rq *this_rq = this_rq();