[PATCH 2/2] x86/apic: check global clockevent in lapic timer setup

[PATCH 2/2] x86/apic: check global clockevent in lapic timer setup

[Pan, Jacob jun]

>From 5b3b795b42796e326d34713d4785c161b52e04db Mon Sep 17 00:00:00 2001
From: Jacob Pan <jacob.jun.pan@intel.com>
Date: Thu, 17 Dec 2009 08:07:57 -0800
Subject: [PATCH 2/2] x86/apic: check global clockevent in lapic timer setup

Global clockevent is needed to calibrate local apic timer.
This patch makes sure we have a valid global clockevent prior
to lapic timer setup.
Non-pc x86 mid platform with per cpu platform timer may not
have a global clockevent device.

Signed-off-by: Jacob Pan <jacob.jun.pan@intel.com>
---
 arch/x86/kernel/apic/apic.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index aa57c07..7e7aee1 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -724,6 +724,13 @@ static int __init calibrate_APIC_clock(void)
  */
 void __init setup_boot_APIC_clock(void)
 {
+	/* global clockevent is needed for calibration */
+	if (!global_clock_event) {
+		apic_printk(APIC_DEBUG,
+				"no global clockevent for calibration\n");
+		return;
+	}
+
 	/*
 	 * The local apic timer can be disabled via the kernel
 	 * commandline or from the CPU detection code. Register the lapic
-- 
1.6.5.3

Re: [PATCH 2/2] x86/apic: check global clockevent in lapic timer setup

[Cyrill Gorcunov]

On Thu, Dec 17, 2009 at 09:59:23AM -0800, Pan, Jacob jun wrote:
> From: Jacob Pan <jacob.jun.pan@intel.com>
> Date: Thu, 17 Dec 2009 08:07:57 -0800
> Subject: [PATCH 2/2] x86/apic: check global clockevent in lapic timer setup
> 
> Global clockevent is needed to calibrate local apic timer.
> This patch makes sure we have a valid global clockevent prior
> to lapic timer setup.
> Non-pc x86 mid platform with per cpu platform timer may not
> have a global clockevent device.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@intel.com>
> ---
>  arch/x86/kernel/apic/apic.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index aa57c07..7e7aee1 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -724,6 +724,13 @@ static int __init calibrate_APIC_clock(void)
>   */
>  void __init setup_boot_APIC_clock(void)
>  {
> +	/* global clockevent is needed for calibration */
> +	if (!global_clock_event) {
> +		apic_printk(APIC_DEBUG,
> +				"no global clockevent for calibration\n");
> +		return;
> +	}
> +
>  	/*
>  	 * The local apic timer can be disabled via the kernel
>  	 * commandline or from the CPU detection code. Register the lapic
> -- 
> 1.6.5.3
>

Hi Jacob,

Wouldn't be better to operate the same way as in case of "noapictimer"
boot option. I guess the non-pc x86 midplatforms you're mentioning
do not use SMP ever but just to be consistent in code.

Perhaps something like:

void __init setup_boot_APIC_clock(void) {

	if (!global_clock_event) {
		pr_info("No global clockevent for calibration\n");
		disable_apic_timer = 1;
	}

	if (disable_apic_timer) {
		pr_info("Disabling APIC timer\n");
		/* No broadcast on UP ! */
		if (num_possible_cpus() > 1) {
			lapic_clockevent.mult = 1;
			setup_APIC_timer();
		}
		return;
	}

	...
}

Or I miss something?

	-- Cyrill

RE: [PATCH 2/2] x86/apic: check global clockevent in lapic timer setup

[Pan, Jacob jun]

>
>Wouldn't be better to operate the same way as in case of "noapictimer"
>boot option. I guess the non-pc x86 midplatforms you're mentioning
>do not use SMP ever but just to be consistent in code.
>
[[JPAN]] We do use SMP with hyper threading in Moorestown.
In that case we have a per cpu platform timer without global clockevent.
so i think we don't want the dummy lapic event. we don't want to use the
broadcast mechanism as mentioned in the comments before disabling lapic
timer.

For moorestown, I can use x86_init.timers.setup_percpu_clockev
abstraction function so that Moorestown platform does not need to call
setup_boot_APIC_clock() if per cpu platform timer is used. so many IFs :).

But in the case of having constant and always on LAPIC timer, we still do
not want the dummy lapic clockevent and the broadcast. we will just have
per cpu always on local apic timers without global clockevent device.


>Perhaps something like:
>
>void __init setup_boot_APIC_clock(void) {
>
>	if (!global_clock_event) {
>		pr_info("No global clockevent for calibration\n");
>		disable_apic_timer = 1;
>	}
>
>	if (disable_apic_timer) {
>		pr_info("Disabling APIC timer\n");
>		/* No broadcast on UP ! */
>		if (num_possible_cpus() > 1) {
>			lapic_clockevent.mult = 1;
>			setup_APIC_timer();
>		}
>		return;
>	}
>
>	...
>}
>
>Or I miss something?
>
>	-- Cyrill

Re: [PATCH 2/2] x86/apic: check global clockevent in lapic timer setup

[H. Peter Anvin]

On 12/17/2009 02:31 PM, Pan, Jacob jun wrote:
>> Wouldn't be better to operate the same way as in case of "noapictimer"
>> boot option. I guess the non-pc x86 midplatforms you're mentioning
>> do not use SMP ever but just to be consistent in code.
>>
> [[JPAN]] We do use SMP with hyper threading in Moorestown.
> In that case we have a per cpu platform timer without global clockevent.
> so i think we don't want the dummy lapic event. we don't want to use the
> broadcast mechanism as mentioned in the comments before disabling lapic
> timer.
> 
> For moorestown, I can use x86_init.timers.setup_percpu_clockev
> abstraction function so that Moorestown platform does not need to call
> setup_boot_APIC_clock() if per cpu platform timer is used. so many IFs :).
> 
> But in the case of having constant and always on LAPIC timer, we still do
> not want the dummy lapic clockevent and the broadcast. we will just have
> per cpu always on local apic timers without global clockevent device.

OK, I'm not entirely sure I follow this, and I'm not sure someone trying
to understand the code in five years would, either.  I don't really see
the above translating into "we don't have a global clockevent, therefore
we shouldn't initialize (but should still not disable) the local APIC
timer."

	-hpa

RE: [PATCH 2/2] x86/apic: check global clockevent in lapic timer setup

[Pan, Jacob jun]

>-----Original Message-----
>From: H. Peter Anvin [mailto:hpa@linux.intel.com]
>Sent: Thursday, December 17, 2009 2:34 PM
>To: Pan, Jacob jun
>Cc: Cyrill Gorcunov; linux-kernel@vger.kernel.org; x86@kernel.org
>Subject: Re: [PATCH 2/2] x86/apic: check global clockevent in lapic timer setup
>
>On 12/17/2009 02:31 PM, Pan, Jacob jun wrote:
>>> Wouldn't be better to operate the same way as in case of "noapictimer"
>>> boot option. I guess the non-pc x86 midplatforms you're mentioning
>>> do not use SMP ever but just to be consistent in code.
>>>
>> [[JPAN]] We do use SMP with hyper threading in Moorestown.
>> In that case we have a per cpu platform timer without global clockevent.
>> so i think we don't want the dummy lapic event. we don't want to use the
>> broadcast mechanism as mentioned in the comments before disabling lapic
>> timer.
>>
>> For moorestown, I can use x86_init.timers.setup_percpu_clockev
>> abstraction function so that Moorestown platform does not need to call
>> setup_boot_APIC_clock() if per cpu platform timer is used. so many IFs :).
>>
>> But in the case of having constant and always on LAPIC timer, we still do
>> not want the dummy lapic clockevent and the broadcast. we will just have
>> per cpu always on local apic timers without global clockevent device.
>
>OK, I'm not entirely sure I follow this, and I'm not sure someone trying
>to understand the code in five years would, either.  I don't really see
>the above translating into "we don't have a global clockevent, therefore
>we shouldn't initialize (but should still not disable) the local APIC
>timer."
>
>	-hpa
[[JPAN]] There is some thing wrong in my logic.

If we have always running lapic timer, and per cpu platform timers, we would
still want to set up the lapic timer without global clockevent, just without
calibration. perhaps use a platform specific setup_percpu_clockev() function.

So i don't think we need this patch at the moment, maybe we only need to do a
sanity check for global clockevent in calibrate_APIC_clock().

Honestly, i don't fully understand how the dummy lapic event device is related
to the broadcast mechanism. can someone explain why we register the dummy
lapic clockevent?

RE: [PATCH 2/2] x86/apic: check global clockevent in lapic timer setup

[Thomas Gleixner]

On Thu, 17 Dec 2009, Pan, Jacob jun wrote:
> >-----Original Message-----
> >From: H. Peter Anvin [mailto:hpa@linux.intel.com]
> >Sent: Thursday, December 17, 2009 2:34 PM
> >To: Pan, Jacob jun
> >Cc: Cyrill Gorcunov; linux-kernel@vger.kernel.org; x86@kernel.org
> >Subject: Re: [PATCH 2/2] x86/apic: check global clockevent in lapic timer setup
> >
> >On 12/17/2009 02:31 PM, Pan, Jacob jun wrote:
> >>> Wouldn't be better to operate the same way as in case of "noapictimer"
> >>> boot option. I guess the non-pc x86 midplatforms you're mentioning
> >>> do not use SMP ever but just to be consistent in code.
> >>>
> >> [[JPAN]] We do use SMP with hyper threading in Moorestown.
> >> In that case we have a per cpu platform timer without global clockevent.
> >> so i think we don't want the dummy lapic event. we don't want to use the
> >> broadcast mechanism as mentioned in the comments before disabling lapic
> >> timer.
> >>
> >> For moorestown, I can use x86_init.timers.setup_percpu_clockev
> >> abstraction function so that Moorestown platform does not need to call
> >> setup_boot_APIC_clock() if per cpu platform timer is used. so many IFs :).
> >>
> >> But in the case of having constant and always on LAPIC timer, we still do
> >> not want the dummy lapic clockevent and the broadcast. we will just have
> >> per cpu always on local apic timers without global clockevent device.
> >
> >OK, I'm not entirely sure I follow this, and I'm not sure someone trying
> >to understand the code in five years would, either.  I don't really see
> >the above translating into "we don't have a global clockevent, therefore
> >we shouldn't initialize (but should still not disable) the local APIC
> >timer."
> >
> >	-hpa
> [[JPAN]] There is some thing wrong in my logic.
> 
> If we have always running lapic timer, and per cpu platform timers, we would
> still want to set up the lapic timer without global clockevent, just without
> calibration. perhaps use a platform specific setup_percpu_clockev() function.
> 
> So i don't think we need this patch at the moment, maybe we only need to do a
> sanity check for global clockevent in calibrate_APIC_clock().

No, we need to fix the whole lapic timer calibration logic first.

There is no reason why we don't calibrate the lapic at the same time
as we calibrate the TSC.

Another question is why there is no way to read out the lapic clock
frequency from some config registers or wherever the chip designers
decided to hide that. There is no real reason why the lapic timer
calibration needs to be extremly precise.

> Honestly, i don't fully understand how the dummy lapic event device
> is related to the broadcast mechanism. can someone explain why we
> register the dummy lapic clockevent?

The broadcast mechanism is there in the first place to work around the
APIC stops in deeper C-states idiocy. 

Then we need to support the disable lapic timer command line option
(even on SMP) so we make use of the existing broadcast mechanism and
register the dummy device to have a per cpu clock event device.

Thanks,

	tglx

RE: [PATCH 2/2] x86/apic: check global clockevent in lapic timer setup

[Pan, Jacob jun]

>-----Original Message-----
>From: Thomas Gleixner [mailto:tglx@linutronix.de]
>Sent: Friday, December 18, 2009 8:36 AM
>To: Pan, Jacob jun
>Cc: H. Peter Anvin; Cyrill Gorcunov; linux-kernel@vger.kernel.org;
>x86@kernel.org
>Subject: RE: [PATCH 2/2] x86/apic: check global clockevent in lapic timer setup
>
>On Thu, 17 Dec 2009, Pan, Jacob jun wrote:
>> >-----Original Message-----
>> >From: H. Peter Anvin [mailto:hpa@linux.intel.com]
>> >Sent: Thursday, December 17, 2009 2:34 PM
>> >To: Pan, Jacob jun
>> >Cc: Cyrill Gorcunov; linux-kernel@vger.kernel.org; x86@kernel.org
>> >Subject: Re: [PATCH 2/2] x86/apic: check global clockevent in lapic timer
>setup
>> >
>> >On 12/17/2009 02:31 PM, Pan, Jacob jun wrote:
>> >>> Wouldn't be better to operate the same way as in case of "noapictimer"
>> >>> boot option. I guess the non-pc x86 midplatforms you're mentioning
>> >>> do not use SMP ever but just to be consistent in code.
>> >>>
>> >> [[JPAN]] We do use SMP with hyper threading in Moorestown.
>> >> In that case we have a per cpu platform timer without global clockevent.
>> >> so i think we don't want the dummy lapic event. we don't want to use the
>> >> broadcast mechanism as mentioned in the comments before disabling lapic
>> >> timer.
>> >>
>> >> For moorestown, I can use x86_init.timers.setup_percpu_clockev
>> >> abstraction function so that Moorestown platform does not need to call
>> >> setup_boot_APIC_clock() if per cpu platform timer is used. so many IFs :).
>> >>
>> >> But in the case of having constant and always on LAPIC timer, we still do
>> >> not want the dummy lapic clockevent and the broadcast. we will just have
>> >> per cpu always on local apic timers without global clockevent device.
>> >
>> >OK, I'm not entirely sure I follow this, and I'm not sure someone trying
>> >to understand the code in five years would, either.  I don't really see
>> >the above translating into "we don't have a global clockevent, therefore
>> >we shouldn't initialize (but should still not disable) the local APIC
>> >timer."
>> >
>> >	-hpa
>> [[JPAN]] There is some thing wrong in my logic.
>>
>> If we have always running lapic timer, and per cpu platform timers, we would
>> still want to set up the lapic timer without global clockevent, just without
>> calibration. perhaps use a platform specific setup_percpu_clockev() function.
>>
>> So i don't think we need this patch at the moment, maybe we only need to do a
>> sanity check for global clockevent in calibrate_APIC_clock().
>
>No, we need to fix the whole lapic timer calibration logic first.
>
>There is no reason why we don't calibrate the lapic at the same time
>as we calibrate the TSC.
[[JPAN]] that seems to be much more efficient and we can have platform specific
way of calibration too with the x86_init abstraction.
>
>Another question is why there is no way to read out the lapic clock
>frequency from some config registers or wherever the chip designers
>decided to hide that. There is no real reason why the lapic timer
>calibration needs to be extremly precise.
>
[[JPAN]] x86 does have MSR_FSB_FREQ to read bus frequency then the DCR to figure
out LAPIC timer freq. but i guess not all CPU models have that. so having
the abstraction would be a plus for those do have reliable reporting of lapic
freq.

>> Honestly, i don't fully understand how the dummy lapic event device
>> is related to the broadcast mechanism. can someone explain why we
>> register the dummy lapic clockevent?
>
>The broadcast mechanism is there in the first place to work around the
>APIC stops in deeper C-states idiocy.
>
>Then we need to support the disable lapic timer command line option
>(even on SMP) so we make use of the existing broadcast mechanism and
>register the dummy device to have a per cpu clock event device.
>
[[JPAN]] thanks for the explanation. so if we have per cpu timer that is
always-on, and don't have a broadcast timer, then the dummy device would not
be needed, correct?


>Thanks,
>
>	tglx

Re: [PATCH 2/2] x86/apic: check global clockevent in lapic timer setup

[Cyrill Gorcunov]

On Fri, Dec 18, 2009 at 10:13:52AM -0800, Pan, Jacob jun wrote:
...
> >
> >No, we need to fix the whole lapic timer calibration logic first.
> >
> >There is no reason why we don't calibrate the lapic at the same time
> >as we calibrate the TSC.
> [[JPAN]] that seems to be much more efficient and we can have platform specific
> way of calibration too with the x86_init abstraction.

Good idea I think!

> >
> >Another question is why there is no way to read out the lapic clock
> >frequency from some config registers or wherever the chip designers
> >decided to hide that. There is no real reason why the lapic timer
> >calibration needs to be extremly precise.
> >
> [[JPAN]] x86 does have MSR_FSB_FREQ to read bus frequency then the DCR to figure
> out LAPIC timer freq. but i guess not all CPU models have that. so having
> the abstraction would be a plus for those do have reliable reporting of lapic
> freq.

IIRC old apics may use independent clock signal too, though I dont think that we
ever switch (espec novadays) to use it due to obsolescense of such chips :)

> 
> >> Honestly, i don't fully understand how the dummy lapic event device
> >> is related to the broadcast mechanism. can someone explain why we
> >> register the dummy lapic clockevent?
> >
> >The broadcast mechanism is there in the first place to work around the
> >APIC stops in deeper C-states idiocy.
> >
> >Then we need to support the disable lapic timer command line option
> >(even on SMP) so we make use of the existing broadcast mechanism and
> >register the dummy device to have a per cpu clock event device.
> >
> [[JPAN]] thanks for the explanation. so if we have per cpu timer that is
> always-on, and don't have a broadcast timer, then the dummy device would not
> be needed, correct?
> 
> 

Hmm... We may be using nmi detector, so I think we still need dummy clockevent
device to send broadcast "time" IPI, or per-cpu timer interrupt handler have
to call the local apic interrupt routine. At least that is how I imagine this
scheme :)

> >Thanks,
> >
> >	tglx
> 
	-- Cyrill