Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1548194 > unrolled thread
| Started by | Frederic Weisbecker <fweisbec@gmail.com> |
|---|---|
| First post | 2016-12-29 17:50 +0100 |
| Last post | 2017-01-02 08:40 +0100 |
| Articles | 3 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH] x86/apic: Implement set_state_oneshot_stopped() callback Frederic Weisbecker <fweisbec@gmail.com> - 2016-12-29 17:50 +0100
Re: [PATCH] x86/apic: Implement set_state_oneshot_stopped() callback Wanpeng Li <kernellwp@gmail.com> - 2016-12-30 02:30 +0100
Re: [PATCH] x86/apic: Implement set_state_oneshot_stopped() callback Viresh Kumar <viresh.kumar@linaro.org> - 2017-01-02 08:40 +0100
| From | Frederic Weisbecker <fweisbec@gmail.com> |
|---|---|
| Date | 2016-12-29 17:50 +0100 |
| Subject | [PATCH] x86/apic: Implement set_state_oneshot_stopped() callback |
| Message-ID | <sTM3T-AQ-15@gated-at.bofh.it> |
When clock_event_device::set_state_oneshot_stopped() is not implemented,
hrtimer_cancel() can't stop the clock when there is no more timer in
the queue. So the ghost of the freshly cancelled hrtimer haunts us back
later with an extra interrupt:
<idle>-0 [002] d..2 2248.557659: hrtimer_cancel: hrtimer=ffff88021fa92d80
<idle>-0 [002] d.h1 2249.303659: local_timer_entry: vector=239
So let's implement this missing callback for the lapic clock. This
consist in calling its set_state_shutdown() callback. There don't seem
to be a lighter way to stop the clock. Simply writing 0 to APIC_TMICT
won't be enough to stop the clock and avoid the extra interrupt, as
opposed to what is specified in the specs. We must also mask the
timer interrupt in the device.
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Wanpeng Li <wanpeng.li@hotmail.com>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
arch/x86/kernel/apic/apic.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 5b7e43e..a8e90db 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -529,18 +529,19 @@ static void lapic_timer_broadcast(const struct cpumask *mask)
* The local apic timer can be used for any function which is CPU local.
*/
static struct clock_event_device lapic_clockevent = {
- .name = "lapic",
- .features = CLOCK_EVT_FEAT_PERIODIC |
- CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP
- | CLOCK_EVT_FEAT_DUMMY,
- .shift = 32,
- .set_state_shutdown = lapic_timer_shutdown,
- .set_state_periodic = lapic_timer_set_periodic,
- .set_state_oneshot = lapic_timer_set_oneshot,
- .set_next_event = lapic_next_event,
- .broadcast = lapic_timer_broadcast,
- .rating = 100,
- .irq = -1,
+ .name = "lapic",
+ .features = CLOCK_EVT_FEAT_PERIODIC |
+ CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP
+ | CLOCK_EVT_FEAT_DUMMY,
+ .shift = 32,
+ .set_state_shutdown = lapic_timer_shutdown,
+ .set_state_periodic = lapic_timer_set_periodic,
+ .set_state_oneshot = lapic_timer_set_oneshot,
+ .set_state_oneshot_stopped = lapic_timer_shutdown,
+ .set_next_event = lapic_next_event,
+ .broadcast = lapic_timer_broadcast,
+ .rating = 100,
+ .irq = -1,
};
static DEFINE_PER_CPU(struct clock_event_device, lapic_events);
--
2.7.4
[toc] | [next] | [standalone]
| From | Wanpeng Li <kernellwp@gmail.com> |
|---|---|
| Date | 2016-12-30 02:30 +0100 |
| Message-ID | <sTUb8-63e-11@gated-at.bofh.it> |
| In reply to | #1548194 |
2016-12-30 0:45 GMT+08:00 Frederic Weisbecker <fweisbec@gmail.com>:
> When clock_event_device::set_state_oneshot_stopped() is not implemented,
> hrtimer_cancel() can't stop the clock when there is no more timer in
> the queue. So the ghost of the freshly cancelled hrtimer haunts us back
> later with an extra interrupt:
>
> <idle>-0 [002] d..2 2248.557659: hrtimer_cancel: hrtimer=ffff88021fa92d80
> <idle>-0 [002] d.h1 2249.303659: local_timer_entry: vector=239
>
> So let's implement this missing callback for the lapic clock. This
> consist in calling its set_state_shutdown() callback. There don't seem
> to be a lighter way to stop the clock. Simply writing 0 to APIC_TMICT
> won't be enough to stop the clock and avoid the extra interrupt, as
> opposed to what is specified in the specs. We must also mask the
> timer interrupt in the device.
>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Wanpeng Li <wanpeng.li@hotmail.com>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
Reviewed-by: Wanpeng Li <wanpeng.li@hotmail.com>
> arch/x86/kernel/apic/apic.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 5b7e43e..a8e90db 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -529,18 +529,19 @@ static void lapic_timer_broadcast(const struct cpumask *mask)
> * The local apic timer can be used for any function which is CPU local.
> */
> static struct clock_event_device lapic_clockevent = {
> - .name = "lapic",
> - .features = CLOCK_EVT_FEAT_PERIODIC |
> - CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP
> - | CLOCK_EVT_FEAT_DUMMY,
> - .shift = 32,
> - .set_state_shutdown = lapic_timer_shutdown,
> - .set_state_periodic = lapic_timer_set_periodic,
> - .set_state_oneshot = lapic_timer_set_oneshot,
> - .set_next_event = lapic_next_event,
> - .broadcast = lapic_timer_broadcast,
> - .rating = 100,
> - .irq = -1,
> + .name = "lapic",
> + .features = CLOCK_EVT_FEAT_PERIODIC |
> + CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP
> + | CLOCK_EVT_FEAT_DUMMY,
> + .shift = 32,
> + .set_state_shutdown = lapic_timer_shutdown,
> + .set_state_periodic = lapic_timer_set_periodic,
> + .set_state_oneshot = lapic_timer_set_oneshot,
> + .set_state_oneshot_stopped = lapic_timer_shutdown,
> + .set_next_event = lapic_next_event,
> + .broadcast = lapic_timer_broadcast,
> + .rating = 100,
> + .irq = -1,
> };
> static DEFINE_PER_CPU(struct clock_event_device, lapic_events);
>
> --
> 2.7.4
>
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2017-01-02 08:40 +0100 |
| Message-ID | <sV5nP-3rs-11@gated-at.bofh.it> |
| In reply to | #1548194 |
On 29-12-16, 17:45, Frederic Weisbecker wrote: > When clock_event_device::set_state_oneshot_stopped() is not implemented, > hrtimer_cancel() can't stop the clock when there is no more timer in > the queue. So the ghost of the freshly cancelled hrtimer haunts us back > later with an extra interrupt: > > <idle>-0 [002] d..2 2248.557659: hrtimer_cancel: hrtimer=ffff88021fa92d80 > <idle>-0 [002] d.h1 2249.303659: local_timer_entry: vector=239 > > So let's implement this missing callback for the lapic clock. This > consist in calling its set_state_shutdown() callback. There don't seem > to be a lighter way to stop the clock. Simply writing 0 to APIC_TMICT > won't be enough to stop the clock and avoid the extra interrupt, as > opposed to what is specified in the specs. We must also mask the > timer interrupt in the device. > > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Wanpeng Li <wanpeng.li@hotmail.com> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> > --- > arch/x86/kernel/apic/apic.c | 25 +++++++++++++------------ > 1 file changed, 13 insertions(+), 12 deletions(-) I didn't try updating anything other than the ARM subsystem as I was afraid of getting crushed by Thomas :) Though, I do remember from one of the email (from Thomas), where he mentioned that disabling the timer on some platforms may have bigger penalties. Perhaps its fast enough for apic.. Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org> -- viresh
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web