* Re: [tip:irq/core] x86: Sanitize apb timer interrupt handling
[not found] <tip-a5ef2e70405c8a9ee380b5ff33a008c75454791f@git.kernel.org>
@ 2010-10-12 21:20 ` Jacob Pan
2010-10-12 21:28 ` Thomas Gleixner
0 siblings, 1 reply; 6+ messages in thread
From: Jacob Pan @ 2010-10-12 21:20 UTC (permalink / raw)
To: linux-kernel, mingo, hpa, tglx, jacob.jun.pan, mingo; +Cc: linux-tip-commits
>x86: Sanitize apb timer interrupt handling
>
>Disable the interrupt in CPU_DEAD where it belongs.
My main concern is the performance cost. The power management code for
Moorestown system make use of the cpu hotplug code (disable_nonboot_cpus)
but much more frequently. The system low power states are call S0 idle
state (s0ix).
Leaving the irq enabled at the chip and desc level between S0ix states
might give some performance benefit. That was my original thought.
Will it cause problems?
Thanks,
Jacob
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [tip:irq/core] x86: Sanitize apb timer interrupt handling
2010-10-12 21:20 ` [tip:irq/core] x86: Sanitize apb timer interrupt handling Jacob Pan
@ 2010-10-12 21:28 ` Thomas Gleixner
2010-10-12 22:12 ` Jacob Pan
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Gleixner @ 2010-10-12 21:28 UTC (permalink / raw)
To: Jacob Pan; +Cc: linux-kernel, mingo, hpa, mingo, linux-tip-commits
On Tue, 12 Oct 2010, Jacob Pan wrote:
>
> >x86: Sanitize apb timer interrupt handling
> >
> >Disable the interrupt in CPU_DEAD where it belongs.
> My main concern is the performance cost. The power management code for
> Moorestown system make use of the cpu hotplug code (disable_nonboot_cpus)
> but much more frequently. The system low power states are call S0 idle
> state (s0ix).
>
> Leaving the irq enabled at the chip and desc level between S0ix states
> might give some performance benefit. That was my original thought.
> Will it cause problems?
Errm. I merily moved it to the place where it should be. You do the
disable/enable dance already today.
Thanks,
tglx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [tip:irq/core] x86: Sanitize apb timer interrupt handling
2010-10-12 21:28 ` Thomas Gleixner
@ 2010-10-12 22:12 ` Jacob Pan
2010-10-12 22:26 ` Thomas Gleixner
0 siblings, 1 reply; 6+ messages in thread
From: Jacob Pan @ 2010-10-12 22:12 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: linux-kernel, mingo, hpa, mingo, linux-tip-commits
Thomas Gleixner Tue, 12 Oct 2010 23:28:19 +0200 (CEST)
>On Tue, 12 Oct 2010, Jacob Pan wrote:
>
>>
>> >x86: Sanitize apb timer interrupt handling
>> >
>> >Disable the interrupt in CPU_DEAD where it belongs.
>> My main concern is the performance cost. The power management code for
>> Moorestown system make use of the cpu hotplug code (disable_nonboot_cpus)
>> but much more frequently. The system low power states are call S0 idle
>> state (s0ix).
>>
>> Leaving the irq enabled at the chip and desc level between S0ix states
>> might give some performance benefit. That was my original thought.
>> Will it cause problems?
>
>Errm. I merily moved it to the place where it should be. You do the
>disable/enable dance already today.
>
I think I only do disable/enable at the timer HW level today during cpu hp
notification, not calling disable_irq(). Am i missing something?
Thanks,
Jacob
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [tip:irq/core] x86: Sanitize apb timer interrupt handling
2010-10-12 22:12 ` Jacob Pan
@ 2010-10-12 22:26 ` Thomas Gleixner
2010-10-12 22:44 ` Jacob Pan
2010-10-13 18:10 ` Jacob Pan
0 siblings, 2 replies; 6+ messages in thread
From: Thomas Gleixner @ 2010-10-12 22:26 UTC (permalink / raw)
To: Jacob Pan; +Cc: linux-kernel, mingo, hpa, mingo, linux-tip-commits
On Tue, 12 Oct 2010, Jacob Pan wrote:
> Thomas Gleixner Tue, 12 Oct 2010 23:28:19 +0200 (CEST)
> >On Tue, 12 Oct 2010, Jacob Pan wrote:
> >
> >>
> >> >x86: Sanitize apb timer interrupt handling
> >> >
> >> >Disable the interrupt in CPU_DEAD where it belongs.
> >> My main concern is the performance cost. The power management code for
> >> Moorestown system make use of the cpu hotplug code (disable_nonboot_cpus)
> >> but much more frequently. The system low power states are call S0 idle
> >> state (s0ix).
> >>
> >> Leaving the irq enabled at the chip and desc level between S0ix states
> >> might give some performance benefit. That was my original thought.
> >> Will it cause problems?
> >
> >Errm. I merily moved it to the place where it should be. You do the
> >disable/enable dance already today.
> >
> I think I only do disable/enable at the timer HW level today during cpu hp
> notification, not calling disable_irq(). Am i missing something?
apbt_setup_irq()
{
...
---> disable_irq(adev->irq);
desc->status |= IRQ_MOVE_PCNTXT;
irq_set_affinity(adev->irq, cpumask_of(adev->cpu));
/* APB timer irqs are set up as mp_irqs, timer is edge triggerred */
set_irq_chip_and_handler_name(adev->irq, chip, handle_edge_irq, "edge");
---> enable_irq(adev->irq);
if (system_state == SYSTEM_BOOTING)
...
}
So that's a disable/enable pair on every cpu hotplug, right ?
Now I moved the disable to CPU_DEAD where it belongs and the enable
stayed at the same place.
Thanks,
tglx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [tip:irq/core] x86: Sanitize apb timer interrupt handling
2010-10-12 22:26 ` Thomas Gleixner
@ 2010-10-12 22:44 ` Jacob Pan
2010-10-13 18:10 ` Jacob Pan
1 sibling, 0 replies; 6+ messages in thread
From: Jacob Pan @ 2010-10-12 22:44 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: linux-kernel, mingo, hpa, mingo, linux-tip-commits
Thomas Gleixner Wed, 13 Oct 2010 00:26:08 +0200 (CEST)
>On Tue, 12 Oct 2010, Jacob Pan wrote:
>
>> Thomas Gleixner Tue, 12 Oct 2010 23:28:19 +0200 (CEST)
>> >On Tue, 12 Oct 2010, Jacob Pan wrote:
>> >
>> >>
>> >> >x86: Sanitize apb timer interrupt handling
>> >> >
>> >> >Disable the interrupt in CPU_DEAD where it belongs.
>> >> My main concern is the performance cost. The power management code for
>> >> Moorestown system make use of the cpu hotplug code (disable_nonboot_cpus)
>> >> but much more frequently. The system low power states are call S0 idle
>> >> state (s0ix).
>> >>
>> >> Leaving the irq enabled at the chip and desc level between S0ix states
>> >> might give some performance benefit. That was my original thought.
>> >> Will it cause problems?
>> >
>> >Errm. I merily moved it to the place where it should be. You do the
>> >disable/enable dance already today.
>> >
>> I think I only do disable/enable at the timer HW level today during cpu hp
>> notification, not calling disable_irq(). Am i missing something?
>
>apbt_setup_irq()
>{
> ...
>
>---> disable_irq(adev->irq);
> desc->status |= IRQ_MOVE_PCNTXT;
> irq_set_affinity(adev->irq, cpumask_of(adev->cpu));
> /* APB timer irqs are set up as mp_irqs, timer is edge triggerred */
> set_irq_chip_and_handler_name(adev->irq, chip, handle_edge_irq, "edge");
>---> enable_irq(adev->irq);
> if (system_state == SYSTEM_BOOTING)
> ...
>}
>
>So that's a disable/enable pair on every cpu hotplug, right ?
>
>Now I moved the disable to CPU_DEAD where it belongs and the enable
>stayed at the same place.
apbt_setup_irq() does not get called in cpu hotplug code,
but CPU_DEAD notification will happen every time disable_nonboot_cpus()
are called. Since Moorestown OSPM code uses disable_nonboot_cpus often,
that is why i am concerned about the overhead of disable_irq().
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [tip:irq/core] x86: Sanitize apb timer interrupt handling
2010-10-12 22:26 ` Thomas Gleixner
2010-10-12 22:44 ` Jacob Pan
@ 2010-10-13 18:10 ` Jacob Pan
1 sibling, 0 replies; 6+ messages in thread
From: Jacob Pan @ 2010-10-13 18:10 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: linux-kernel, mingo, hpa, mingo, linux-tip-commits
Hi tglx,
You are right, i am already doing the enable/disable dance.
Thanks for pointing it out, patiently.
Jacob
Thomas Gleixner Wed, 13 Oct 2010 00:26:08 +0200 (CEST)
>On Tue, 12 Oct 2010, Jacob Pan wrote:
>
>> Thomas Gleixner Tue, 12 Oct 2010 23:28:19 +0200 (CEST)
>> >On Tue, 12 Oct 2010, Jacob Pan wrote:
>> >
>> >>
>> >> >x86: Sanitize apb timer interrupt handling
>> >> >
>> >> >Disable the interrupt in CPU_DEAD where it belongs.
>> >> My main concern is the performance cost. The power management code for
>> >> Moorestown system make use of the cpu hotplug code (disable_nonboot_cpus)
>> >> but much more frequently. The system low power states are call S0 idle
>> >> state (s0ix).
>> >>
>> >> Leaving the irq enabled at the chip and desc level between S0ix states
>> >> might give some performance benefit. That was my original thought.
>> >> Will it cause problems?
>> >
>> >Errm. I merily moved it to the place where it should be. You do the
>> >disable/enable dance already today.
>> >
>> I think I only do disable/enable at the timer HW level today during cpu hp
>> notification, not calling disable_irq(). Am i missing something?
>
>apbt_setup_irq()
>{
> ...
>
>---> disable_irq(adev->irq);
> desc->status |= IRQ_MOVE_PCNTXT;
> irq_set_affinity(adev->irq, cpumask_of(adev->cpu));
> /* APB timer irqs are set up as mp_irqs, timer is edge triggerred */
> set_irq_chip_and_handler_name(adev->irq, chip, handle_edge_irq, "edge");
>---> enable_irq(adev->irq);
> if (system_state == SYSTEM_BOOTING)
> ...
>}
>
>So that's a disable/enable pair on every cpu hotplug, right ?
>
>Now I moved the disable to CPU_DEAD where it belongs and the enable
>stayed at the same place.
>
>Thanks,
>
> tglx
yo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-10-13 18:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <tip-a5ef2e70405c8a9ee380b5ff33a008c75454791f@git.kernel.org>
2010-10-12 21:20 ` [tip:irq/core] x86: Sanitize apb timer interrupt handling Jacob Pan
2010-10-12 21:28 ` Thomas Gleixner
2010-10-12 22:12 ` Jacob Pan
2010-10-12 22:26 ` Thomas Gleixner
2010-10-12 22:44 ` Jacob Pan
2010-10-13 18:10 ` Jacob Pan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox