Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1576587 > unrolled thread

[PATCH] sched/loadavg: Avoid loadavg spikes caused by delayed NO_HZ accounting

Back to article view | Back to linux.kernel

Contents

  [PATCH] sched/loadavg: Avoid loadavg spikes caused by delayed NO_HZ accounting Matt Fleming <matt@codeblueprint.co.uk> - 2017-02-08 14:40 +0100
    Re: [PATCH] sched/loadavg: Avoid loadavg spikes caused by delayed  NO_HZ accounting kbuild test robot <lkp@intel.com> - 2017-02-08 18:20 +0100
    Re: [PATCH] sched/loadavg: Avoid loadavg spikes caused by delayed  NO_HZ accounting Peter Zijlstra <peterz@infradead.org> - 2017-02-09 17:00 +0100

#1576587 — [PATCH] sched/loadavg: Avoid loadavg spikes caused by delayed NO_HZ accounting

The calculation for the next sample window when exiting NOH_HZ idle
does not handle the fact that we may not have reached the next sample
window yet, i.e. that we came out of idle between sample windows.

If we wake from NO_HZ idle after the pending this_rq->calc_load_update
window time when we want idle but before the next sample window, we
will add an unnecessary LOAD_FREQ delay to the load average
accounting, delaying any update for potentially ~9seconds.

This can result in huge spikes in the load average values due to
per-cpu uninterruptible task counts being out of sync when accumulated
across all CPUs.

It's safe to update the per-cpu active count if we wake between sample
windows because any load that we left in 'calc_load_idle' will have
been zero'd when the idle load was folded in calc_global_load().

This issue is easy to reproduce before,

  commit 9d89c257dfb9 ("sched/fair: Rewrite runnable load and utilization average tracking")

just by forking short-lived process pipelines built from ps(1) and
grep(1) in a loop. I'm unable to reproduce the spikes after that
commit, but the bug still seems to be present from code review.

Fixes: commit 5167e8d ("sched/nohz: Rewrite and fix load-avg computation -- again")
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Mike Galbraith <umgwanakikbuti@gmail.com>
Cc: Morten Rasmussen <morten.rasmussen@arm.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: <stable@vger.kernel.org> # v3.5+
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 kernel/sched/loadavg.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/loadavg.c b/kernel/sched/loadavg.c
index a2d6eb71f06b..a7a6f3646970 100644
--- a/kernel/sched/loadavg.c
+++ b/kernel/sched/loadavg.c
@@ -199,6 +199,7 @@ void calc_load_enter_idle(void)
 void calc_load_exit_idle(void)
 {
 	struct rq *this_rq = this_rq();
+	unsigned long next_window;
 
 	/*
 	 * If we're still before the sample window, we're done.
@@ -210,10 +211,16 @@ void calc_load_exit_idle(void)
 	 * We woke inside or after the sample window, this means we're already
 	 * accounted through the nohz accounting, so skip the entire deal and
 	 * sync up for the next window.
+	 *
+	 * The next window is 'calc_load_update' if we haven't reached it yet,
+	 * and 'calc_load_update + 10' if we're inside the current window.
 	 */
-	this_rq->calc_load_update = calc_load_update;
-	if (time_before(jiffies, this_rq->calc_load_update + 10))
-		this_rq->calc_load_update += LOAD_FREQ;
+	next_window = calc_load_update;
+
+	if (time_in_range_open(jiffies, next_window, next_window + 10)
+		next_window += LOAD_FREQ;
+
+	this_rq->calc_load_update = next_window;
 }
 
 static long calc_load_fold_idle(void)
-- 
2.10.0

[toc] | [next] | [standalone]

#1576733 — Re: [PATCH] sched/loadavg: Avoid loadavg spikes caused by delayed NO_HZ accounting

[Multipart message — attachments visible in raw view] — view raw

Hi Matt,

[auto build test ERROR on tip/sched/core]
[also build test ERROR on v4.10-rc7 next-20170208]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Matt-Fleming/sched-loadavg-Avoid-loadavg-spikes-caused-by-delayed-NO_HZ-accounting/20170208-224135
config: c6x-evmc6678_defconfig (attached as .config)
compiler: c6x-elf-gcc (GCC) 6.2.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=c6x 

All errors (new ones prefixed by >>):

   kernel/sched/loadavg.c: In function 'calc_load_exit_idle':
>> kernel/sched/loadavg.c:221:3: error: expected ')' before 'next_window'
      next_window += LOAD_FREQ;
      ^~~~~~~~~~~
>> kernel/sched/loadavg.c:224:1: error: expected expression before '}' token
    }
    ^

vim +221 kernel/sched/loadavg.c

   215		 * The next window is 'calc_load_update' if we haven't reached it yet,
   216		 * and 'calc_load_update + 10' if we're inside the current window.
   217		 */
   218		next_window = calc_load_update;
   219	
   220		if (time_in_range_open(jiffies, next_window, next_window + 10)
 > 221			next_window += LOAD_FREQ;
   222	
   223		this_rq->calc_load_update = next_window;
 > 224	}
   225	
   226	static long calc_load_fold_idle(void)
   227	{

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1577722 — Re: [PATCH] sched/loadavg: Avoid loadavg spikes caused by delayed NO_HZ accounting

On Wed, Feb 08, 2017 at 01:29:24PM +0000, Matt Fleming wrote:
> The calculation for the next sample window when exiting NOH_HZ idle
> does not handle the fact that we may not have reached the next sample
> window yet, i.e. that we came out of idle between sample windows.
> 
> If we wake from NO_HZ idle after the pending this_rq->calc_load_update
> window time when we want idle but before the next sample window, we
> will add an unnecessary LOAD_FREQ delay to the load average
> accounting, delaying any update for potentially ~9seconds.
> 
> This can result in huge spikes in the load average values due to
> per-cpu uninterruptible task counts being out of sync when accumulated
> across all CPUs.
> 
> It's safe to update the per-cpu active count if we wake between sample
> windows because any load that we left in 'calc_load_idle' will have
> been zero'd when the idle load was folded in calc_global_load().

Right, so differently put; the problem is that we check against the
'stale' rq->calc_load_update, while the current and effective period
boundary is 'calc_load_update'.

So, when rq->calc_load_update < jiffies < calc_load_update, we end up
setting the next-update to calc_load_update+LOAD_FREQ, where it should
have been calc_load_update.

> diff --git a/kernel/sched/loadavg.c b/kernel/sched/loadavg.c
> index a2d6eb71f06b..a7a6f3646970 100644
> --- a/kernel/sched/loadavg.c
> +++ b/kernel/sched/loadavg.c

> @@ -210,10 +211,16 @@ void calc_load_exit_idle(void)
>  	 * We woke inside or after the sample window, this means we're already
>  	 * accounted through the nohz accounting, so skip the entire deal and
>  	 * sync up for the next window.
> +	 *
> +	 * The next window is 'calc_load_update' if we haven't reached it yet,
> +	 * and 'calc_load_update + 10' if we're inside the current window.
>  	 */
> +	next_window = calc_load_update;
> +
> +	if (time_in_range_open(jiffies, next_window, next_window + 10)
> +		next_window += LOAD_FREQ;
> +
> +	this_rq->calc_load_update = next_window;
>  }

So I don't much like the time_in_range_open() thing. The simpler patch
which you tested to also work was:

diff --git a/kernel/sched/loadavg.c b/kernel/sched/loadavg.c
index 7296b7308eca..cfb47bd0ee50 100644
--- a/kernel/sched/loadavg.c
+++ b/kernel/sched/loadavg.c
@@ -201,6 +201,8 @@ void calc_load_exit_idle(void)
 {
 	struct rq *this_rq = this_rq();
 
+	this_rq->calc_load_update = calc_load_update;
+
 	/*
 	 * If we're still before the sample window, we're done.
 	 */
@@ -212,7 +214,6 @@ void calc_load_exit_idle(void)
 	 * accounted through the nohz accounting, so skip the entire deal and
 	 * sync up for the next window.
 	 */
-	this_rq->calc_load_update = calc_load_update;
 	if (time_before(jiffies, this_rq->calc_load_update + 10))
 		this_rq->calc_load_update += LOAD_FREQ;
 }

But the problem there is that we unconditionally issue that store. Now
I've no idea how much of a problem that is, and it certainly is the
simplest form (+- comments that need updating), so maybe that makes
sense.

Alternatively, something like:

diff --git a/kernel/sched/loadavg.c b/kernel/sched/loadavg.c
index 7296b7308eca..3dd4ce6fe151 100644
--- a/kernel/sched/loadavg.c
+++ b/kernel/sched/loadavg.c
@@ -207,12 +207,15 @@ void calc_load_exit_idle(void)
 	if (time_before(jiffies, this_rq->calc_load_update))
 		return;
 
+	this_rq->calc_load_update = calc_load_update;
+	if (time_before(jiffies, this_rq->calc_load_update))
+		return;
+
 	/*
 	 * We woke inside or after the sample window, this means we're already
 	 * accounted through the nohz accounting, so skip the entire deal and
 	 * sync up for the next window.
 	 */
-	this_rq->calc_load_update = calc_load_update;
 	if (time_before(jiffies, this_rq->calc_load_update + 10))
 		this_rq->calc_load_update += LOAD_FREQ;
 }

might be another solution.

Irrespective the above though; should we not make this:

+	this_rq->calc_load_update = READ_ONCE(calc_load_update);

because if for some reason we do a double load of calc_load_update and
see two different values, weird stuff could happen.

And because, on general principle, a READ_ONCE() should be paired with a
WRITE_ONCE(), that should be done too I suppose.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web