linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] clocksource: Load the ACPI PM clocksource asynchronously
@ 2012-01-31  4:51 Arjan van de Ven
  2012-02-02  2:40 ` john stultz
  2012-04-08  5:58 ` Michael Witten
  0 siblings, 2 replies; 6+ messages in thread
From: Arjan van de Ven @ 2012-01-31  4:51 UTC (permalink / raw)
  To: linux-kernel; +Cc: John Stultz, Thomas Gleixner, Len Brown, arjanvandeven

>From ef4b0d23878277dfd75828f13353e605aae2937f Mon Sep 17 00:00:00 2001
From: Arjan van de Ven <arjan@linux.intel.com>
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.

CC: John Stultz <johnstul@us.ibm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Len Brown <lenb@kernel.org>

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 drivers/clocksource/acpi_pm.c |   24 ++++++++++++++++--------
 1 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/clocksource/acpi_pm.c b/drivers/clocksource/acpi_pm.c
index 6b5cf02..82e8820 100644
--- a/drivers/clocksource/acpi_pm.c
+++ b/drivers/clocksource/acpi_pm.c
@@ -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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] clocksource: Load the ACPI PM clocksource asynchronously
  2012-01-31  4:51 [PATCH] clocksource: Load the ACPI PM clocksource asynchronously Arjan van de Ven
@ 2012-02-02  2:40 ` john stultz
  2012-04-08  5:58 ` Michael Witten
  1 sibling, 0 replies; 6+ messages in thread
From: john stultz @ 2012-02-02  2:40 UTC (permalink / raw)
  To: Arjan van de Ven; +Cc: linux-kernel, Thomas Gleixner, Len Brown, arjanvandeven

On Mon, 2012-01-30 at 20:51 -0800, Arjan van de Ven wrote:
> From ef4b0d23878277dfd75828f13353e605aae2937f Mon Sep 17 00:00:00 2001
> From: Arjan van de Ven <arjan@linux.intel.com>
> 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.
> 
> CC: John Stultz <johnstul@us.ibm.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Len Brown <lenb@kernel.org>
> 
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>


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

thanks
-john



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] clocksource: Load the ACPI PM clocksource asynchronously
  2012-01-31  4:51 [PATCH] clocksource: Load the ACPI PM clocksource asynchronously Arjan van de Ven
  2012-02-02  2:40 ` john stultz
@ 2012-04-08  5:58 ` Michael Witten
       [not found]   ` <CADyApD2UZebJd_nSJHAWkjz=PT4_DZfnSr38geqUb1kvJzKi-g@mail.gmail.com>
  1 sibling, 1 reply; 6+ messages in thread
From: Michael Witten @ 2012-04-08  5:58 UTC (permalink / raw)
  To: Arjan van de Ven
  Cc: John Stultz, Thomas Gleixner, Len Brown, Arjan van de Ven, linux-kernel

On Mon, 30 Jan 2012 20:51:20 -0800, Arjan van de Ven wrote:

> ...
>
> 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.
>
> ...

This patch became the following commit:

  commit b519508298e0292e1771eecf14aaf67755adc39d
  Author:     Arjan van de Ven <arjan@linux.intel.com>
  AuthorDate: Mon Jan 30 20:23:30 2012 -0800
  Commit:     John Stultz <john.stultz@linaro.org>
  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:

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] clocksource: Load the ACPI PM clocksource asynchronously
       [not found]   ` <CADyApD2UZebJd_nSJHAWkjz=PT4_DZfnSr38geqUb1kvJzKi-g@mail.gmail.com>
@ 2012-04-11 20:04     ` Michael Witten
  2012-04-11 21:13       ` Thomas Gleixner
  0 siblings, 1 reply; 6+ messages in thread
From: Michael Witten @ 2012-04-11 20:04 UTC (permalink / raw)
  To: Arjan van de Ven
  Cc: Arjan van de Ven, John Stultz, Thomas Gleixner, Len Brown, linux-kernel

On Sun, 8 Apr 2012 19:20:22 -0700, Arjan van de Ven wrote:

>> 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.
>>
>>
> any chance of a dmesg? (or even better, a diff of the dmesg from
> abefore and fter)

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!).

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] clocksource: Load the ACPI PM clocksource asynchronously
  2012-04-11 20:04     ` Michael Witten
@ 2012-04-11 21:13       ` Thomas Gleixner
  2012-04-11 22:15         ` [tip:timers/urgent] Revert "clocksource: Load the ACPI PM clocksource asynchronously" tip-bot for Thomas Gleixner
  0 siblings, 1 reply; 6+ messages in thread
From: Thomas Gleixner @ 2012-04-11 21:13 UTC (permalink / raw)
  To: Michael Witten
  Cc: Arjan van de Ven, Arjan van de Ven, John Stultz, Len Brown, linux-kernel

On Wed, 11 Apr 2012, Michael Witten wrote:
> On Sun, 8 Apr 2012 19:20:22 -0700, Arjan van de Ven wrote:
> 
> >> 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.
> >>
> >>
> > any chance of a dmesg? (or even better, a diff of the dmesg from
> > abefore and fter)
> 
> 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.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [tip:timers/urgent] Revert "clocksource: Load the ACPI PM clocksource asynchronously"
  2012-04-11 21:13       ` Thomas Gleixner
@ 2012-04-11 22:15         ` tip-bot for Thomas Gleixner
  0 siblings, 0 replies; 6+ messages in thread
From: tip-bot for Thomas Gleixner @ 2012-04-11 22:15 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, arjanvandeven, hpa, mingo, johnstul, arjan,
	mfwitten, lenb, tglx

Commit-ID:  d48fc63f6f3f485ed5aa9cf019d8e8e3a7d10263
Gitweb:     http://git.kernel.org/tip/d48fc63f6f3f485ed5aa9cf019d8e8e3a7d10263
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Wed, 11 Apr 2012 23:49:16 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
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.

Bisected-by: Michael Witten <mfwitten@gmail.com>
Cc: Arjan van de Ven <arjanvandeven@gmail.com>
Cc: Arjan van de Ven <arjan@infradead.org>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Len Brown <lenb@kernel.org>
LKML-Reference: <alpine.LFD.2.02.1204112303270.2542@ionos>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/clocksource/acpi_pm.c |   24 ++++++++----------------
 1 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/clocksource/acpi_pm.c b/drivers/clocksource/acpi_pm.c
index 82e8820..6b5cf02 100644
--- a/drivers/clocksource/acpi_pm.c
+++ b/drivers/clocksource/acpi_pm.c
@@ -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
  */

^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2012-04-11 22:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-31  4:51 [PATCH] clocksource: Load the ACPI PM clocksource asynchronously Arjan van de Ven
2012-02-02  2:40 ` john stultz
2012-04-08  5:58 ` Michael Witten
     [not found]   ` <CADyApD2UZebJd_nSJHAWkjz=PT4_DZfnSr38geqUb1kvJzKi-g@mail.gmail.com>
2012-04-11 20:04     ` Michael Witten
2012-04-11 21:13       ` Thomas Gleixner
2012-04-11 22:15         ` [tip:timers/urgent] Revert "clocksource: Load the ACPI PM clocksource asynchronously" tip-bot for Thomas Gleixner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).