[Resend PATCH V2] X86/CPU: Avoid 100ms sleep for cpu offline during S3

[Resend PATCH V2] X86/CPU: Avoid 100ms sleep for cpu offline during S3

[Lan Tianyu]

With some bad kernel configures, cpu offline consumes more than 100ms
during S3. This because native_cpu_die() would fall into 100ms
sleep when cpu idle loop thread marked cpu state to DEAD slower. It's
timing related issue. What native_cpu_die() does is that poll cpu
state and wait for 100ms if cpu state hasn't been marked to DEAD.
The 100ms sleep doesn't make sense. To avoid such long sleep, this
patch is to add struct completion to each cpu, wait for the completion
in the native_cpu_die() and wakeup the completion when the cpu state is
marked to DEAD.

Tested on the Intel Xeon server with 48 cores, Ivbridge and Haswell laptops.
the times of cpu offline on these machines are reduced from more than 100ms
to less than 5ms. The system suspend time reduces 2.3s on the servers.

Borislav and Prarit also helped to test the patch on an AMD machine and
a few systems of various sizes and configurations (multi-socket,
single-socket, no hyper threading, etc.). No issues seen.

Acked-by: Borislav Petkov <bp@suse.de>
Tested-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 arch/x86/kernel/smpboot.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 5492798..25a8f17 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -102,6 +102,8 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_llc_shared_map);
 DEFINE_PER_CPU_SHARED_ALIGNED(struct cpuinfo_x86, cpu_info);
 EXPORT_PER_CPU_SYMBOL(cpu_info);
 
+DEFINE_PER_CPU(struct completion, die_complete);
+
 atomic_t init_deasserted;
 
 /*
@@ -1331,7 +1333,7 @@ int native_cpu_disable(void)
 		return ret;
 
 	clear_local_APIC();
-
+	init_completion(&per_cpu(die_complete, smp_processor_id()));
 	cpu_disable_common();
 	return 0;
 }
@@ -1339,18 +1341,14 @@ int native_cpu_disable(void)
 void native_cpu_die(unsigned int cpu)
 {
 	/* We don't do anything here: idle task is faking death itself. */
-	unsigned int i;
+	wait_for_completion_timeout(&per_cpu(die_complete, cpu), HZ);
 
-	for (i = 0; i < 10; i++) {
-		/* They ack this in play_dead by setting CPU_DEAD */
-		if (per_cpu(cpu_state, cpu) == CPU_DEAD) {
-			if (system_state == SYSTEM_RUNNING)
-				pr_info("CPU %u is now offline\n", cpu);
-			return;
-		}
-		msleep(100);
-	}
-	pr_err("CPU %u didn't die...\n", cpu);
+	/* They ack this in play_dead by setting CPU_DEAD */
+	if (per_cpu(cpu_state, cpu) == CPU_DEAD) {
+		if (system_state == SYSTEM_RUNNING)
+			pr_info("CPU %u is now offline\n", cpu);
+	} else
+		pr_err("CPU %u didn't die...\n", cpu);
 }
 
 void play_dead_common(void)
@@ -1362,6 +1360,7 @@ void play_dead_common(void)
 	mb();
 	/* Ack it */
 	__this_cpu_write(cpu_state, CPU_DEAD);
+	complete(&per_cpu(die_complete, smp_processor_id()));
 
 	/*
 	 * With physical CPU hotplug, we should halt the cpu
-- 
1.8.4.rc0.1.g8f6a3e5.dirty

Re: [Resend PATCH V2] X86/CPU: Avoid 100ms sleep for cpu offline during S3

[Lan Tianyu]

On 2014年08月26日 15:43, Lan Tianyu wrote:
> With some bad kernel configures, cpu offline consumes more than 100ms
> during S3. This because native_cpu_die() would fall into 100ms
> sleep when cpu idle loop thread marked cpu state to DEAD slower. It's
> timing related issue. What native_cpu_die() does is that poll cpu
> state and wait for 100ms if cpu state hasn't been marked to DEAD.
> The 100ms sleep doesn't make sense. To avoid such long sleep, this
> patch is to add struct completion to each cpu, wait for the completion
> in the native_cpu_die() and wakeup the completion when the cpu state is
> marked to DEAD.
> 
> Tested on the Intel Xeon server with 48 cores, Ivbridge and Haswell laptops.
> the times of cpu offline on these machines are reduced from more than 100ms
> to less than 5ms. The system suspend time reduces 2.3s on the servers.
> 
> Borislav and Prarit also helped to test the patch on an AMD machine and
> a few systems of various sizes and configurations (multi-socket,
> single-socket, no hyper threading, etc.). No issues seen.
> 
> Acked-by: Borislav Petkov <bp@suse.de>
> Tested-by: Prarit Bhargava <prarit@redhat.com>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  arch/x86/kernel/smpboot.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 

Hi HPA, Ingo, Thomas & Peter Z:
	Is this patch ok for you?

> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 5492798..25a8f17 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -102,6 +102,8 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_llc_shared_map);
>  DEFINE_PER_CPU_SHARED_ALIGNED(struct cpuinfo_x86, cpu_info);
>  EXPORT_PER_CPU_SYMBOL(cpu_info);
>  
> +DEFINE_PER_CPU(struct completion, die_complete);
> +
>  atomic_t init_deasserted;
>  
>  /*
> @@ -1331,7 +1333,7 @@ int native_cpu_disable(void)
>  		return ret;
>  
>  	clear_local_APIC();
> -
> +	init_completion(&per_cpu(die_complete, smp_processor_id()));
>  	cpu_disable_common();
>  	return 0;
>  }
> @@ -1339,18 +1341,14 @@ int native_cpu_disable(void)
>  void native_cpu_die(unsigned int cpu)
>  {
>  	/* We don't do anything here: idle task is faking death itself. */
> -	unsigned int i;
> +	wait_for_completion_timeout(&per_cpu(die_complete, cpu), HZ);
>  
> -	for (i = 0; i < 10; i++) {
> -		/* They ack this in play_dead by setting CPU_DEAD */
> -		if (per_cpu(cpu_state, cpu) == CPU_DEAD) {
> -			if (system_state == SYSTEM_RUNNING)
> -				pr_info("CPU %u is now offline\n", cpu);
> -			return;
> -		}
> -		msleep(100);
> -	}
> -	pr_err("CPU %u didn't die...\n", cpu);
> +	/* They ack this in play_dead by setting CPU_DEAD */
> +	if (per_cpu(cpu_state, cpu) == CPU_DEAD) {
> +		if (system_state == SYSTEM_RUNNING)
> +			pr_info("CPU %u is now offline\n", cpu);
> +	} else
> +		pr_err("CPU %u didn't die...\n", cpu);
>  }
>  
>  void play_dead_common(void)
> @@ -1362,6 +1360,7 @@ void play_dead_common(void)
>  	mb();
>  	/* Ack it */
>  	__this_cpu_write(cpu_state, CPU_DEAD);
> +	complete(&per_cpu(die_complete, smp_processor_id()));
>  
>  	/*
>  	 * With physical CPU hotplug, we should halt the cpu
> 


-- 
Best regards
Tianyu Lan

Re: [Resend PATCH V2] X86/CPU: Avoid 100ms sleep for cpu offline during S3

[Ingo Molnar]

* Lan Tianyu <tianyu.lan@intel.com> wrote:

> On 2014年08月26日 15:43, Lan Tianyu wrote:
> > With some bad kernel configures, cpu offline consumes more than 100ms
> > during S3. This because native_cpu_die() would fall into 100ms
> > sleep when cpu idle loop thread marked cpu state to DEAD slower. It's
> > timing related issue. What native_cpu_die() does is that poll cpu
> > state and wait for 100ms if cpu state hasn't been marked to DEAD.
> > The 100ms sleep doesn't make sense. To avoid such long sleep, this
> > patch is to add struct completion to each cpu, wait for the completion
> > in the native_cpu_die() and wakeup the completion when the cpu state is
> > marked to DEAD.
> > 
> > Tested on the Intel Xeon server with 48 cores, Ivbridge and Haswell laptops.
> > the times of cpu offline on these machines are reduced from more than 100ms
> > to less than 5ms. The system suspend time reduces 2.3s on the servers.
> > 
> > Borislav and Prarit also helped to test the patch on an AMD machine and
> > a few systems of various sizes and configurations (multi-socket,
> > single-socket, no hyper threading, etc.). No issues seen.
> > 
> > Acked-by: Borislav Petkov <bp@suse.de>
> > Tested-by: Prarit Bhargava <prarit@redhat.com>
> > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> > ---
> >  arch/x86/kernel/smpboot.c | 23 +++++++++++------------
> >  1 file changed, 11 insertions(+), 12 deletions(-)
> > 
> 
> Hi HPA, Ingo, Thomas & Peter Z:
> 	Is this patch ok for you?

Nice patch!

I cleaned up a few small details and will push it out if it 
passes testing.

Thanks,

	Ingo

[tip:x86/cpu] x86/smpboot: Speed up suspend/ resume by avoiding 100ms sleep for CPU offline during S3

[tip-bot for Lan Tianyu]

Commit-ID:  2ed53c0d6cc99fc712f7c037e41d9ec4eb8d6b08
Gitweb:     http://git.kernel.org/tip/2ed53c0d6cc99fc712f7c037e41d9ec4eb8d6b08
Author:     Lan Tianyu <tianyu.lan@intel.com>
AuthorDate: Tue, 26 Aug 2014 15:43:45 +0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 24 Sep 2014 15:02:06 +0200

x86/smpboot: Speed up suspend/resume by avoiding 100ms sleep for CPU offline during S3

With certain kernel configurations, CPU offline consumes more than
100ms during S3.

It's a timing related issue: native_cpu_die() would occasionally fall
into a 100ms sleep when the CPU idle loop thread marked the CPU state
to DEAD too slowly.

What native_cpu_die() does is that it polls the CPU state and waits
for 100ms if CPU state hasn't been marked to DEAD. The 100ms sleep
doesn't make sense and is purely historic.

To avoid such long sleeping, this patch adds a 'struct completion'
to each CPU, waits for the completion in native_cpu_die() and wakes
up the completion when the CPU state is marked to DEAD.

Tested on an Intel Xeon server with 48 cores, Ivybridge and on
Haswell laptops. The CPU offlining cost on these machines is
reduced from more than 100ms to less than 5ms. The system
suspend time is reduced by 2.3s on the servers.

Borislav and Prarit also helped to test the patch on an AMD
machine and a few systems of various sizes and configurations
(multi-socket, single-socket, no hyper threading, etc.). No
issues were seen.

Tested-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
Acked-by: Borislav Petkov <bp@suse.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: srostedt@redhat.com
Cc: toshi.kani@hp.com
Cc: imammedo@redhat.com
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/1409039025-32310-1-git-send-email-tianyu.lan@intel.com
[ Improved a few minor details in the code, cleaned up the changelog. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/smpboot.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 2d872e0..fdbc5fc 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -102,6 +102,8 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_llc_shared_map);
 DEFINE_PER_CPU_SHARED_ALIGNED(struct cpuinfo_x86, cpu_info);
 EXPORT_PER_CPU_SYMBOL(cpu_info);
 
+static DEFINE_PER_CPU(struct completion, die_complete);
+
 atomic_t init_deasserted;
 
 /*
@@ -1323,26 +1325,24 @@ int native_cpu_disable(void)
 		return ret;
 
 	clear_local_APIC();
-
+	init_completion(&per_cpu(die_complete, smp_processor_id()));
 	cpu_disable_common();
+
 	return 0;
 }
 
 void native_cpu_die(unsigned int cpu)
 {
 	/* We don't do anything here: idle task is faking death itself. */
-	unsigned int i;
+	wait_for_completion_timeout(&per_cpu(die_complete, cpu), HZ);
 
-	for (i = 0; i < 10; i++) {
-		/* They ack this in play_dead by setting CPU_DEAD */
-		if (per_cpu(cpu_state, cpu) == CPU_DEAD) {
-			if (system_state == SYSTEM_RUNNING)
-				pr_info("CPU %u is now offline\n", cpu);
-			return;
-		}
-		msleep(100);
+	/* They ack this in play_dead() by setting CPU_DEAD */
+	if (per_cpu(cpu_state, cpu) == CPU_DEAD) {
+		if (system_state == SYSTEM_RUNNING)
+			pr_info("CPU %u is now offline\n", cpu);
+	} else {
+		pr_err("CPU %u didn't die...\n", cpu);
 	}
-	pr_err("CPU %u didn't die...\n", cpu);
 }
 
 void play_dead_common(void)
@@ -1354,6 +1354,7 @@ void play_dead_common(void)
 	mb();
 	/* Ack it */
 	__this_cpu_write(cpu_state, CPU_DEAD);
+	complete(&per_cpu(die_complete, smp_processor_id()));
 
 	/*
 	 * With physical CPU hotplug, we should halt the cpu

[tip:x86/urgent] x86/smpboot: Move data structure to its primary usage scope

[tip-bot for Ingo Molnar]

Commit-ID:  db6a00b4bed3abbb038077ba4fdc5be481fe5559
Gitweb:     http://git.kernel.org/tip/db6a00b4bed3abbb038077ba4fdc5be481fe5559
Author:     Ingo Molnar <mingo@kernel.org>
AuthorDate: Sun, 19 Oct 2014 11:41:52 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sun, 19 Oct 2014 11:44:49 +0200

x86/smpboot: Move data structure to its primary usage scope

Makes the code more readable by moving variable and usage closer
to each other, which also avoids this build warning in the
!CONFIG_HOTPLUG_CPU case:

  arch/x86/kernel/smpboot.c:105:42: warning: ‘die_complete’ defined but not used [-Wunused-variable]

Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Lan Tianyu <tianyu.lan@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: srostedt@redhat.com
Cc: toshi.kani@hp.com
Cc: imammedo@redhat.com
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/1409039025-32310-1-git-send-email-tianyu.lan@intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/smpboot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 2d5200e..4d2128a 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -102,8 +102,6 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_llc_shared_map);
 DEFINE_PER_CPU_SHARED_ALIGNED(struct cpuinfo_x86, cpu_info);
 EXPORT_PER_CPU_SYMBOL(cpu_info);
 
-static DEFINE_PER_CPU(struct completion, die_complete);
-
 atomic_t init_deasserted;
 
 /*
@@ -1318,6 +1316,8 @@ void cpu_disable_common(void)
 	fixup_irqs();
 }
 
+static DEFINE_PER_CPU(struct completion, die_complete);
+
 int native_cpu_disable(void)
 {
 	int ret;