clocksource: Load the ACPI PM clocksource asynchronously

From: Arjan van de Ven <[email protected]>
Date: Mon, 30 Jan 2012 20:23:30 -0800
Subject: [PATCH] clocksource: Load the ACPI PM clocksource asynchronously

The ACPI clocksource takes quite some time to initialize, and this increases the boot time of the kernel for a double digit percentage. This while almost all modern systems will be using the HPET already anyway.

This patch turns the clocksource loading into an asynchronous operation; which means it won't hold up the boot while still becoming available normally.

To make this work well, an udelay() had to be turned into an usleep_range() so that on UP systems, we yield the CPU to regular boot tasks instead of spinning.

---
 drivers/clocksource/acpi_pm.c |   24 ++++++++++++++++--------
 1 files changed, 16 insertions(+), 8 deletions(-)

index 6b5cf02..82e8820 100644
@@ -23,6 +23,7 @@
 #include <linux/init.h>
 #include <linux/pci.h>
 #include <linux/delay.h>
+#include <linux/async.h>
 #include <asm/io.h>
 
 /*

@@ -179,17 +180,15 @@ static int verify_pmtmr_rate(void)
 /* Number of reads we try to get two different values */
 #define ACPI_PM_READ_CHECKS 10000
 
-static int __init init_acpi_pm_clocksource(void)
+static void __init acpi_pm_clocksource_async(void *unused, async_cookie_t cookie)
 {
 	cycle_t value1, value2;
 	unsigned int i, j = 0;
 
-	if (!pmtmr_ioport)
-		return -ENODEV;
 
 	/* "verify" this timing source: */
 	for (j = 0; j < ACPI_PM_MONOTONICITY_CHECKS; j++) {
-		udelay(100 * j);
+		usleep_range(100 * j, 100 * j + 100);
 		value1 = clocksource_acpi_pm.read(&clocksource_acpi_pm);
 		for (i = 0; i < ACPI_PM_READ_CHECKS; i++) {
 			value2 = clocksource_acpi_pm.read(&clocksource_acpi_pm);

@@ -203,25 +202,34 @@ static int __init init_acpi_pm_clocksource(void)
 			       " 0x%#llx, 0x%#llx - aborting.\n",
 			       value1, value2);
 			pmtmr_ioport = 0;
-			return -EINVAL;
+			return;
 		}
 		if (i == ACPI_PM_READ_CHECKS) {
 			printk(KERN_INFO "PM-Timer failed consistency check "
 			       " (0x%#llx) - aborting.\n", value1);
 			pmtmr_ioport = 0;
-			return -ENODEV;
+			return;
 		}
 	}
 
 	if (verify_pmtmr_rate() != 0){
 		pmtmr_ioport = 0;
-		return -ENODEV;
+		return;
 	}
 
-	return clocksource_register_hz(&clocksource_acpi_pm,
+	clocksource_register_hz(&clocksource_acpi_pm,
 						PMTMR_TICKS_PER_SEC);
 }
 
+static int __init init_acpi_pm_clocksource(void)
+{
+	if (!pmtmr_ioport)
+		return -ENODEV;
+
+	async_schedule(acpi_pm_clocksource_async, NULL);
+	return 0;
+}
+
 /* We use fs_initcall because we want the PCI fixups to have run
  * but we still need to load before device_initcall
  */

1.7.6.4



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Looks good to me. I've gone ahead and queued it

thanks
-john

This patch became the following commit:

commit b519508298e0292e1771eecf14aaf67755adc39d
Author: Arjan van de Ven <[email protected]>
AuthorDate: Mon Jan 30 20:23:30 2012 -0800
Commit: John Stultz <[email protected]>
CommitDate: Wed Feb 1 18:39:46 2012 -0800

to which I bisected as the culprit for very strange load balance behavior on my machine.

With this patch in place, my CPU is constantly being pegged at 100% (and my CPU monitor sometimes registers NaN%), regardless of the active governor and under conditions when my computer would normally be idling quite placidly.

Reverting this commit does indeed remove the problem from previously problematic builds.

Sincerely,
Michael Witten

Some probably useless information:

* Dell Latitude D810 (from about 2005/2006)

* BIOS Version A05

* /proc/cpuinfo:

      processor       : 0
      vendor_id       : GenuineIntel
      cpu family      : 6
      model           : 13
      model name      : Intel(R) Pentium(R) M processor 2.13GHz
      stepping        : 8
      microcode       : 0x20
      cpu MHz         : 800.000
      cache size      : 2048 KB
      fdiv_bug        : no
      hlt_bug         : no
      f00f_bug        : no
      coma_bug        : no
      fpu             : yes
      fpu_exception   : yes
      cpuid level     : 2
      wp              : yes
      flags           : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov clflush dts acpi mmx fxsr sse sse2 ss tm pbe nx bts est tm2
      bogomips        : 1596.47
      clflush size    : 64

cache_alignment : 64
address sizes : 32 bits physical, 32 bits virtual
power management:

Doh! I should have thought of that.

The dmesg output for a kernel built from your patch's commit yields this interesting line:

PM-Timer running at invalid rate: 113% of normal - aborting.

That certainly sounds unsavory!

Here is some other, probably uninteresting context:

--- good-dmesg.txt
+++ bad-dmesg.txt
[...]
Initializing cgroup subsys cpu
-Linux version 3.3.0-rc1-good-12d6d41276def096cb3f7dc36f438db9ed6a0a8d+ [...]
+Linux version 3.3.0-rc1-bad-b519508298e0292e1771eecf14aaf67755adc39d+ [...]
[...]
Fast TSC calibration using PIT
-Detected 798.093 MHz processor.
+Detected 798.071 MHz processor.
-Calibrating delay loop (skipped), value calculated using timer frequency.. 1596.18 BogoMIPS (lpj=798093) +Calibrating delay loop (skipped), value calculated using timer frequency.. 1596.14 BogoMIPS (lpj=798071)
pid_max: default: 32768 minimum: 301
Mount-cache hash table entries: 512
Initializing cgroup subsys cpuacct
[...]
system 00:0c: Plug and Play ACPI device, IDs PNP0c01 (active)
pnp: PnP ACPI: found 13 devices
ACPI: ACPI bus type pnp unregistered
-Switching to clocksource acpi_pm
[...]
+PM-Timer running at invalid rate: 113% of normal - aborting.

Sincerely,
Michael Witten

P.S.

I'm terribly sorry for the delayed reply; Gmail somehow managed to throw away this thread entirely (actually, I probably threw it away, but I'm content to blame my tools; after all, they can't deny it!).

Duh. The original code does this with interrupts disabled and not from the context of some random worker thread.

Aside of that, this will blow up if something else touches the CTC channel 2, which is possible as this code runs async to other init stuff.

Arjan, did you ever verify that patch extensivly on hardware which relies on the PM-Timer? I doubt it. :(

There is no other choice than reverting it.

Thanks,

tglx

Commit-ID:  d48fc63f6f3f485ed5aa9cf019d8e8e3a7d10263
Gitweb:     http://git.kernel.org/tip/d48fc63f6f3f485ed5aa9cf019d8e8e3a7d10263
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Wed, 11 Apr 2012 23:49:16 +0200
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Thu, 12 Apr 2012 00:05:05 +0200

Revert "clocksource: Load the ACPI PM clocksource asynchronously"

This reverts commit b519508298e0292e1771eecf14aaf67755adc39d.

The reason for this revert is that making the frequency verification preemptible and interruptible is not working reliably. Michaels machine failed to use PM-timer with the message:

PM-Timer running at invalid rate: 113% of normal - aborting.

That's not a surprise as the frequency verification does rely on interrupts being disabled. With a async scheduled thread there is no guarantee to achieve the same result. Also some driver might fiddle with the CTC channel 2 during the verification period, which makes the result even more random and unpredictable.

This can be solved by using the same mechanism as we use in the deferred TSC validation code, but that only will work if we verified a working HPET _BEFORE_ trying to do the PM-Timer lazy validation.

So for now reverting is the safe option.

LKML-Reference: <alpine.LFD.2.02.1204112303270.2542@ionos>

---
 drivers/clocksource/acpi_pm.c |   24 ++++++++----------------
 1 files changed, 8 insertions(+), 16 deletions(-)

index 82e8820..6b5cf02 100644
@@ -23,7 +23,6 @@
 #include <linux/init.h>
 #include <linux/pci.h>
 #include <linux/delay.h>
-#include <linux/async.h>
 #include <asm/io.h>
 
 /*

@@ -180,15 +179,17 @@ static int verify_pmtmr_rate(void)
 /* Number of reads we try to get two different values */
 #define ACPI_PM_READ_CHECKS 10000
 
-static void __init acpi_pm_clocksource_async(void *unused, async_cookie_t cookie)
+static int __init init_acpi_pm_clocksource(void)
 {
 	cycle_t value1, value2;
 	unsigned int i, j = 0;
 
+	if (!pmtmr_ioport)
+		return -ENODEV;
 
 	/* "verify" this timing source: */
 	for (j = 0; j < ACPI_PM_MONOTONICITY_CHECKS; j++) {
-		usleep_range(100 * j, 100 * j + 100);
+		udelay(100 * j);
 		value1 = clocksource_acpi_pm.read(&clocksource_acpi_pm);
 		for (i = 0; i < ACPI_PM_READ_CHECKS; i++) {
 			value2 = clocksource_acpi_pm.read(&clocksource_acpi_pm);

@@ -202,34 +203,25 @@ static void __init acpi_pm_clocksource_async(void *unused, async_cookie_t cookie
 			       " 0x%#llx, 0x%#llx - aborting.\n",
 			       value1, value2);
 			pmtmr_ioport = 0;
-			return;
+			return -EINVAL;
 		}
 		if (i == ACPI_PM_READ_CHECKS) {
 			printk(KERN_INFO "PM-Timer failed consistency check "
 			       " (0x%#llx) - aborting.\n", value1);
 			pmtmr_ioport = 0;
-			return;
+			return -ENODEV;
 		}
 	}
 
 	if (verify_pmtmr_rate() != 0){
 		pmtmr_ioport = 0;
-		return;
+		return -ENODEV;
 	}
 
-	clocksource_register_hz(&clocksource_acpi_pm,
+	return clocksource_register_hz(&clocksource_acpi_pm,
 						PMTMR_TICKS_PER_SEC);
 }
 
-static int __init init_acpi_pm_clocksource(void)
-{
-	if (!pmtmr_ioport)
-		return -ENODEV;
-
-	async_schedule(acpi_pm_clocksource_async, NULL);
-	return 0;
-}
-
 /* We use fs_initcall because we want the PCI fixups to have run
  * but we still need to load before device_initcall
  */