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

Groups > linux.kernel > #1441066 > unrolled thread

Re: [patch 10/15] sched/migration: Move calc_load_migrate() into CPU_DYING

Started byAnton Blanchard <anton@samba.org>
First post2016-07-12 07:10 +0200
Last post2016-07-13 15:50 +0200
Articles 6 — 6 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [patch 10/15] sched/migration: Move calc_load_migrate() into  CPU_DYING Anton Blanchard <anton@samba.org> - 2016-07-12 07:10 +0200
    Re: [patch 10/15] sched/migration: Move calc_load_migrate() into  CPU_DYING Thomas Gleixner <tglx@linutronix.de> - 2016-07-12 18:40 +0200
      Re: [patch 10/15] sched/migration: Move calc_load_migrate() into  CPU_DYING Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com> - 2016-07-12 21:00 +0200
      Re: [patch 10/15] sched/migration: Move calc_load_migrate() into  CPU_DYING Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> - 2016-07-12 22:10 +0200
      Re: [patch 10/15] sched/migration: Move calc_load_migrate() into  CPU_DYING Peter Zijlstra <peterz@infradead.org> - 2016-07-13 10:00 +0200
      [tip:sched/urgent] sched/core: Correct off by one bug in load  migration calculation tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2016-07-13 15:50 +0200

#1441066 — Re: [patch 10/15] sched/migration: Move calc_load_migrate() into CPU_DYING

FromAnton Blanchard <anton@samba.org>
Date2016-07-12 07:10 +0200
SubjectRe: [patch 10/15] sched/migration: Move calc_load_migrate() into CPU_DYING
Message-ID<rTY7f-ra-1@gated-at.bofh.it>
Hi Thomas,

> It really does not matter when we fold the load for the outgoing cpu.
> It's almost dead anyway, so there is no harm if we fail to fold the
> few microseconds which are required for going fully away.

We are seeing the load average shoot up when hot unplugging CPUs (+1
for every CPU we offline) on ppc64. This reproduces on bare metal as
well as inside a KVM guest. A bisect points at this commit.

As an example, a completely idle box with 128 CPUS and 112 hot
unplugged:

# uptime
 04:35:30 up  1:23,  2 users,  load average: 112.43, 122.94, 125.54

Anton

[toc] | [next] | [standalone]

#1441548

FromThomas Gleixner <tglx@linutronix.de>
Date2016-07-12 18:40 +0200
Message-ID<rU8T0-7sL-25@gated-at.bofh.it>
In reply to#1441066
Anton,

On Tue, 12 Jul 2016, Anton Blanchard wrote:
> > It really does not matter when we fold the load for the outgoing cpu.
> > It's almost dead anyway, so there is no harm if we fail to fold the
> > few microseconds which are required for going fully away.
> 
> We are seeing the load average shoot up when hot unplugging CPUs (+1
> for every CPU we offline) on ppc64. This reproduces on bare metal as
> well as inside a KVM guest. A bisect points at this commit.
> 
> As an example, a completely idle box with 128 CPUS and 112 hot
> unplugged:
> 
> # uptime
>  04:35:30 up  1:23,  2 users,  load average: 112.43, 122.94, 125.54

Yes, it's an off by one as we now call that from the task which is tearing
down the cpu. Does the patch below fix it?

Thanks,

	tglx

8<----------------------

Subject: sched/migration: Correct off by one in load migration
From: Thomas Gleixner <tglx@linutronix.de>

The move of calc_load_migrate() from CPU_DEAD to CPU_DYING did not take into
account that the function is now called from a thread running on the outgoing
CPU. As a result a cpu unplug leakes a load of 1 into the global load
accounting mechanism.

Fix it by adjusting for the currently running thread which calls
calc_load_migrate().

Fixes: e9cd8fa4fcfd: "sched/migration: Move calc_load_migrate() into CPU_DYING"
Reported-by: Anton Blanchard <anton@samba.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 51d7105f529a..97ee9ac7e97c 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5394,13 +5394,15 @@ void idle_task_exit(void)
 /*
  * Since this CPU is going 'away' for a while, fold any nr_active delta
  * we might have. Assumes we're called after migrate_tasks() so that the
- * nr_active count is stable.
+ * nr_active count is stable. We need to take the teardown thread which
+ * is calling this into account, so we hand in adjust = 1 to the load
+ * calculation.
  *
  * Also see the comment "Global load-average calculations".
  */
 static void calc_load_migrate(struct rq *rq)
 {
-	long delta = calc_load_fold_active(rq);
+	long delta = calc_load_fold_active(rq, 1);
 	if (delta)
 		atomic_long_add(delta, &calc_load_tasks);
 }
diff --git a/kernel/sched/loadavg.c b/kernel/sched/loadavg.c
index b0b93fd33af9..a2d6eb71f06b 100644
--- a/kernel/sched/loadavg.c
+++ b/kernel/sched/loadavg.c
@@ -78,11 +78,11 @@ void get_avenrun(unsigned long *loads, unsigned long offset, int shift)
 	loads[2] = (avenrun[2] + offset) << shift;
 }
 
-long calc_load_fold_active(struct rq *this_rq)
+long calc_load_fold_active(struct rq *this_rq, long adjust)
 {
 	long nr_active, delta = 0;
 
-	nr_active = this_rq->nr_running;
+	nr_active = this_rq->nr_running - adjust;
 	nr_active += (long)this_rq->nr_uninterruptible;
 
 	if (nr_active != this_rq->calc_load_active) {
@@ -188,7 +188,7 @@ void calc_load_enter_idle(void)
 	 * We're going into NOHZ mode, if there's any pending delta, fold it
 	 * into the pending idle delta.
 	 */
-	delta = calc_load_fold_active(this_rq);
+	delta = calc_load_fold_active(this_rq, 0);
 	if (delta) {
 		int idx = calc_load_write_idx();
 
@@ -389,7 +389,7 @@ void calc_global_load_tick(struct rq *this_rq)
 	if (time_before(jiffies, this_rq->calc_load_update))
 		return;
 
-	delta  = calc_load_fold_active(this_rq);
+	delta  = calc_load_fold_active(this_rq, 0);
 	if (delta)
 		atomic_long_add(delta, &calc_load_tasks);
 
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 7cbeb92a1cb9..898c0d2f18fe 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -28,7 +28,7 @@ extern unsigned long calc_load_update;
 extern atomic_long_t calc_load_tasks;
 
 extern void calc_global_load_tick(struct rq *this_rq);
-extern long calc_load_fold_active(struct rq *this_rq);
+extern long calc_load_fold_active(struct rq *this_rq, long adjust);
 
 #ifdef CONFIG_SMP
 extern void cpu_load_update_active(struct rq *this_rq);

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

#1441658

FromVaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
Date2016-07-12 21:00 +0200
Message-ID<rUb4t-o7-5@gated-at.bofh.it>
In reply to#1441548
* Thomas Gleixner <tglx@linutronix.de> [2016-07-12 18:33:56]:

> Anton,
> 
> On Tue, 12 Jul 2016, Anton Blanchard wrote:
> > > It really does not matter when we fold the load for the outgoing cpu.
> > > It's almost dead anyway, so there is no harm if we fail to fold the
> > > few microseconds which are required for going fully away.
> > 
> > We are seeing the load average shoot up when hot unplugging CPUs (+1
> > for every CPU we offline) on ppc64. This reproduces on bare metal as
> > well as inside a KVM guest. A bisect points at this commit.
> > 
> > As an example, a completely idle box with 128 CPUS and 112 hot
> > unplugged:
> > 
> > # uptime
> >  04:35:30 up  1:23,  2 users,  load average: 112.43, 122.94, 125.54
> 
> Yes, it's an off by one as we now call that from the task which is tearing
> down the cpu. Does the patch below fix it?

Hi Thomas,

Yes this patch fixes the issue.  I was able to recreate the problem
and also verify with this patch on 4.7.0-rc7.

> 
> Thanks,
> 
> 	tglx
> 
> 8<----------------------
> 
> Subject: sched/migration: Correct off by one in load migration
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The move of calc_load_migrate() from CPU_DEAD to CPU_DYING did not take into
> account that the function is now called from a thread running on the outgoing
> CPU. As a result a cpu unplug leakes a load of 1 into the global load
> accounting mechanism.
> 
> Fix it by adjusting for the currently running thread which calls
> calc_load_migrate().
> 
> Fixes: e9cd8fa4fcfd: "sched/migration: Move calc_load_migrate() into CPU_DYING"
> Reported-by: Anton Blanchard <anton@samba.org>

Tested-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>

> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>
> ---
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 51d7105f529a..97ee9ac7e97c 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5394,13 +5394,15 @@ void idle_task_exit(void)
>  /*
>   * Since this CPU is going 'away' for a while, fold any nr_active delta
>   * we might have. Assumes we're called after migrate_tasks() so that the
> - * nr_active count is stable.
> + * nr_active count is stable. We need to take the teardown thread which
> + * is calling this into account, so we hand in adjust = 1 to the load
> + * calculation.
>   *
>   * Also see the comment "Global load-average calculations".
>   */
>  static void calc_load_migrate(struct rq *rq)
>  {
> -	long delta = calc_load_fold_active(rq);
> +	long delta = calc_load_fold_active(rq, 1);
>  	if (delta)
>  		atomic_long_add(delta, &calc_load_tasks);
>  }
> diff --git a/kernel/sched/loadavg.c b/kernel/sched/loadavg.c
> index b0b93fd33af9..a2d6eb71f06b 100644
> --- a/kernel/sched/loadavg.c
> +++ b/kernel/sched/loadavg.c
> @@ -78,11 +78,11 @@ void get_avenrun(unsigned long *loads, unsigned long offset, int shift)
>  	loads[2] = (avenrun[2] + offset) << shift;
>  }
> 
> -long calc_load_fold_active(struct rq *this_rq)
> +long calc_load_fold_active(struct rq *this_rq, long adjust)
>  {
>  	long nr_active, delta = 0;
> 
> -	nr_active = this_rq->nr_running;
> +	nr_active = this_rq->nr_running - adjust;
>  	nr_active += (long)this_rq->nr_uninterruptible;

	if (nr_active != this_rq->calc_load_active) {
		delta = nr_active - this_rq->calc_load_active;
		this_rq->calc_load_active = nr_active;
	}

	return delta;

Does the above calculation hold good even if we send adjust=1 and bump
down nr_active? Tested ok though :)

--Vaidy

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

#1441698

FromShilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
Date2016-07-12 22:10 +0200
Message-ID<rUcae-1jn-13@gated-at.bofh.it>
In reply to#1441548
Hi,

On 07/12/2016 10:03 PM, Thomas Gleixner wrote:
> Anton,
> 
> On Tue, 12 Jul 2016, Anton Blanchard wrote:
>>> It really does not matter when we fold the load for the outgoing cpu.
>>> It's almost dead anyway, so there is no harm if we fail to fold the
>>> few microseconds which are required for going fully away.
>>
>> We are seeing the load average shoot up when hot unplugging CPUs (+1
>> for every CPU we offline) on ppc64. This reproduces on bare metal as
>> well as inside a KVM guest. A bisect points at this commit.
>>
>> As an example, a completely idle box with 128 CPUS and 112 hot
>> unplugged:
>>
>> # uptime
>>  04:35:30 up  1:23,  2 users,  load average: 112.43, 122.94, 125.54
> 
> Yes, it's an off by one as we now call that from the task which is tearing
> down the cpu. Does the patch below fix it?

Tested your patch to see that on an idle box on offlinig the cpus I dont see
increase in loadaverage.

# uptime
01:27:44 up 10 min,  1 user,  load average: 0.00, 0.18, 0.18

# lscpu | grep -Ei "on-line|off-line"
On-line CPU(s) list:   0-127

# ppc64_cpu --cores-on=2

# lscpu | grep -Ei "on-line|off-line"
On-line CPU(s) list:   0-15
Off-line CPU(s) list:  16-127

# sleep 60
# uptime
 01:28:52 up 11 min,  1 user,  load average: 0.11, 0.19, 0.18

Thanks and Regards,
Shilpa

> 
> Thanks,
> 
> 	tglx
> 
> 8<----------------------
> 
> Subject: sched/migration: Correct off by one in load migration
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The move of calc_load_migrate() from CPU_DEAD to CPU_DYING did not take into
> account that the function is now called from a thread running on the outgoing
> CPU. As a result a cpu unplug leakes a load of 1 into the global load
> accounting mechanism.
> 
> Fix it by adjusting for the currently running thread which calls
> calc_load_migrate().
> 
> Fixes: e9cd8fa4fcfd: "sched/migration: Move calc_load_migrate() into CPU_DYING"
> Reported-by: Anton Blanchard <anton@samba.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 51d7105f529a..97ee9ac7e97c 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5394,13 +5394,15 @@ void idle_task_exit(void)
>  /*
>   * Since this CPU is going 'away' for a while, fold any nr_active delta
>   * we might have. Assumes we're called after migrate_tasks() so that the
> - * nr_active count is stable.
> + * nr_active count is stable. We need to take the teardown thread which
> + * is calling this into account, so we hand in adjust = 1 to the load
> + * calculation.
>   *
>   * Also see the comment "Global load-average calculations".
>   */
>  static void calc_load_migrate(struct rq *rq)
>  {
> -	long delta = calc_load_fold_active(rq);
> +	long delta = calc_load_fold_active(rq, 1);
>  	if (delta)
>  		atomic_long_add(delta, &calc_load_tasks);
>  }
> diff --git a/kernel/sched/loadavg.c b/kernel/sched/loadavg.c
> index b0b93fd33af9..a2d6eb71f06b 100644
> --- a/kernel/sched/loadavg.c
> +++ b/kernel/sched/loadavg.c
> @@ -78,11 +78,11 @@ void get_avenrun(unsigned long *loads, unsigned long offset, int shift)
>  	loads[2] = (avenrun[2] + offset) << shift;
>  }
> 
> -long calc_load_fold_active(struct rq *this_rq)
> +long calc_load_fold_active(struct rq *this_rq, long adjust)
>  {
>  	long nr_active, delta = 0;
> 
> -	nr_active = this_rq->nr_running;
> +	nr_active = this_rq->nr_running - adjust;
>  	nr_active += (long)this_rq->nr_uninterruptible;
> 
>  	if (nr_active != this_rq->calc_load_active) {
> @@ -188,7 +188,7 @@ void calc_load_enter_idle(void)
>  	 * We're going into NOHZ mode, if there's any pending delta, fold it
>  	 * into the pending idle delta.
>  	 */
> -	delta = calc_load_fold_active(this_rq);
> +	delta = calc_load_fold_active(this_rq, 0);
>  	if (delta) {
>  		int idx = calc_load_write_idx();
> 
> @@ -389,7 +389,7 @@ void calc_global_load_tick(struct rq *this_rq)
>  	if (time_before(jiffies, this_rq->calc_load_update))
>  		return;
> 
> -	delta  = calc_load_fold_active(this_rq);
> +	delta  = calc_load_fold_active(this_rq, 0);
>  	if (delta)
>  		atomic_long_add(delta, &calc_load_tasks);
> 
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 7cbeb92a1cb9..898c0d2f18fe 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -28,7 +28,7 @@ extern unsigned long calc_load_update;
>  extern atomic_long_t calc_load_tasks;
> 
>  extern void calc_global_load_tick(struct rq *this_rq);
> -extern long calc_load_fold_active(struct rq *this_rq);
> +extern long calc_load_fold_active(struct rq *this_rq, long adjust);
> 
>  #ifdef CONFIG_SMP
>  extern void cpu_load_update_active(struct rq *this_rq);
> 
> 
>

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

#1442132

FromPeter Zijlstra <peterz@infradead.org>
Date2016-07-13 10:00 +0200
Message-ID<rUnfj-8tL-5@gated-at.bofh.it>
In reply to#1441548
On Tue, Jul 12, 2016 at 06:33:56PM +0200, Thomas Gleixner wrote:

> Subject: sched/migration: Correct off by one in load migration
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The move of calc_load_migrate() from CPU_DEAD to CPU_DYING did not take into
> account that the function is now called from a thread running on the outgoing
> CPU. As a result a cpu unplug leakes a load of 1 into the global load
> accounting mechanism.
> 
> Fix it by adjusting for the currently running thread which calls
> calc_load_migrate().
> 
> Fixes: e9cd8fa4fcfd: "sched/migration: Move calc_load_migrate() into CPU_DYING"
> Reported-by: Anton Blanchard <anton@samba.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

> +++ b/kernel/sched/loadavg.c
> @@ -78,11 +78,11 @@ void get_avenrun(unsigned long *loads, unsigned long offset, int shift)
>  	loads[2] = (avenrun[2] + offset) << shift;
>  }
>  
> -long calc_load_fold_active(struct rq *this_rq)
> +long calc_load_fold_active(struct rq *this_rq, long adjust)
>  {
>  	long nr_active, delta = 0;
>  
> -	nr_active = this_rq->nr_running;
> +	nr_active = this_rq->nr_running - adjust;
>  	nr_active += (long)this_rq->nr_uninterruptible;
>  
>  	if (nr_active != this_rq->calc_load_active) {

Yeah, I think this is the only sensible approach.

How do you want to route this?

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

#1442466 — [tip:sched/urgent] sched/core: Correct off by one bug in load migration calculation

Fromtip-bot for Thomas Gleixner <tipbot@zytor.com>
Date2016-07-13 15:50 +0200
Subject[tip:sched/urgent] sched/core: Correct off by one bug in load migration calculation
Message-ID<rUsI1-3Kr-7@gated-at.bofh.it>
In reply to#1441548
Commit-ID:  d60585c5766e9620d5d83e2b25dc042c7bdada2c
Gitweb:     http://git.kernel.org/tip/d60585c5766e9620d5d83e2b25dc042c7bdada2c
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Tue, 12 Jul 2016 18:33:56 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 13 Jul 2016 14:58:20 +0200

sched/core: Correct off by one bug in load migration calculation

The move of calc_load_migrate() from CPU_DEAD to CPU_DYING did not take into
account that the function is now called from a thread running on the outgoing
CPU. As a result a cpu unplug leakes a load of 1 into the global load
accounting mechanism.

Fix it by adjusting for the currently running thread which calls
calc_load_migrate().

Reported-by: Anton Blanchard <anton@samba.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
Cc: rt@linutronix.de
Cc: shreyas@linux.vnet.ibm.com
Fixes: e9cd8fa4fcfd: ("sched/migration: Move calc_load_migrate() into CPU_DYING")
Link: http://lkml.kernel.org/r/alpine.DEB.2.11.1607121744350.4083@nanos
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/core.c    | 6 ++++--
 kernel/sched/loadavg.c | 8 ++++----
 kernel/sched/sched.h   | 2 +-
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 51d7105..97ee9ac 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5394,13 +5394,15 @@ void idle_task_exit(void)
 /*
  * Since this CPU is going 'away' for a while, fold any nr_active delta
  * we might have. Assumes we're called after migrate_tasks() so that the
- * nr_active count is stable.
+ * nr_active count is stable. We need to take the teardown thread which
+ * is calling this into account, so we hand in adjust = 1 to the load
+ * calculation.
  *
  * Also see the comment "Global load-average calculations".
  */
 static void calc_load_migrate(struct rq *rq)
 {
-	long delta = calc_load_fold_active(rq);
+	long delta = calc_load_fold_active(rq, 1);
 	if (delta)
 		atomic_long_add(delta, &calc_load_tasks);
 }
diff --git a/kernel/sched/loadavg.c b/kernel/sched/loadavg.c
index b0b93fd..a2d6eb7 100644
--- a/kernel/sched/loadavg.c
+++ b/kernel/sched/loadavg.c
@@ -78,11 +78,11 @@ void get_avenrun(unsigned long *loads, unsigned long offset, int shift)
 	loads[2] = (avenrun[2] + offset) << shift;
 }
 
-long calc_load_fold_active(struct rq *this_rq)
+long calc_load_fold_active(struct rq *this_rq, long adjust)
 {
 	long nr_active, delta = 0;
 
-	nr_active = this_rq->nr_running;
+	nr_active = this_rq->nr_running - adjust;
 	nr_active += (long)this_rq->nr_uninterruptible;
 
 	if (nr_active != this_rq->calc_load_active) {
@@ -188,7 +188,7 @@ void calc_load_enter_idle(void)
 	 * We're going into NOHZ mode, if there's any pending delta, fold it
 	 * into the pending idle delta.
 	 */
-	delta = calc_load_fold_active(this_rq);
+	delta = calc_load_fold_active(this_rq, 0);
 	if (delta) {
 		int idx = calc_load_write_idx();
 
@@ -389,7 +389,7 @@ void calc_global_load_tick(struct rq *this_rq)
 	if (time_before(jiffies, this_rq->calc_load_update))
 		return;
 
-	delta  = calc_load_fold_active(this_rq);
+	delta  = calc_load_fold_active(this_rq, 0);
 	if (delta)
 		atomic_long_add(delta, &calc_load_tasks);
 
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 7cbeb92..898c0d2 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -28,7 +28,7 @@ extern unsigned long calc_load_update;
 extern atomic_long_t calc_load_tasks;
 
 extern void calc_global_load_tick(struct rq *this_rq);
-extern long calc_load_fold_active(struct rq *this_rq);
+extern long calc_load_fold_active(struct rq *this_rq, long adjust);
 
 #ifdef CONFIG_SMP
 extern void cpu_load_update_active(struct rq *this_rq);

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web