diff mbox

ARM: timer: Shutdown clock event device when stopping local timer

Message ID CAH3Oq6QA0DCEDc-Yh+k=3bgUH7xzKxHSC1aWSoL4NzaYCZZHbg@mail.gmail.com
State New, archived
Headers show

Commit Message

ning.n.jiang@gmail.com March 30, 2013, 9:57 a.m. UTC
2013/3/30 Stephen Boyd <sboyd@codeaurora.org>:
> On 03/29/13 02:24, ning.n.jiang@gmail.com wrote:
>> From: Ning Jiang <ning.n.jiang@gmail.com>
>>
>> Currently there are two problems when we try to stop local timer.
>> First, it calls set_mode function directly so mode state is not
>> updated for the clock event device. Second, it makes the device
>> unused instead of shutdown.
>
> What device is this a problem on? I believe this only matters to drivers
> which enable their timer in their set_next_event() callback? But even
> then, does anything actually happen because the interrupt should have
> been disabled in the local timer stop callback.
>

Right. Drivers which enable timer in set_next_event() will have this problem.
It will not have functional problem in my case. But my device cannot enter
low power mode with a pending interrupt even if it is disabled.

>>
>> A subtle error will happen because of it. When a cpu is plugged out
>> it will stop the local timer. It will call tick_nohz_idle_enter()
>> in idle thread afterwards. It will cancel the sched timer and try
>> to reprogram the next event. This is wrong since the local timer
>> is supposed to be stopped.
>>
>> The right way to stop the local timer is to shutdown it by calling
>> clockevents_set_mode(). Thus when we try to reprogram the clock
>> event device, it will return directly without doing anything since
>> the clock mode is CLOCK_EVT_MODE_SHUTDOWN.
>
> While this prevents the set_next_event() callback from being called on a
> dying CPU, wouldn't it be better to fix this problem in the core code
> once instead of fixing it many times in each local timer driver? It
> doesn't seem to make much sense to program an event on a CPU that is
> about to die, so why do we do that?
>

Actually, I was trying to fix it in the core code like this, but I
thought it is not
that good and we still need to fix the local timer driver problem even
with this fix.

        if (dev->mode == CLOCK_EVT_MODE_SHUTDOWN)

> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Russell King - ARM Linux March 30, 2013, 10:04 a.m. UTC | #1
On Sat, Mar 30, 2013 at 05:57:38PM +0800, Ning Jiang wrote:
> 2013/3/30 Stephen Boyd <sboyd@codeaurora.org>:
> > On 03/29/13 02:24, ning.n.jiang@gmail.com wrote:
> >> From: Ning Jiang <ning.n.jiang@gmail.com>
> >>
> >> Currently there are two problems when we try to stop local timer.
> >> First, it calls set_mode function directly so mode state is not
> >> updated for the clock event device. Second, it makes the device
> >> unused instead of shutdown.
> >
> > What device is this a problem on? I believe this only matters to drivers
> > which enable their timer in their set_next_event() callback? But even
> > then, does anything actually happen because the interrupt should have
> > been disabled in the local timer stop callback.
> >
> 
> Right. Drivers which enable timer in set_next_event() will have this problem.
> It will not have functional problem in my case. But my device cannot enter
> low power mode with a pending interrupt even if it is disabled.

You're not telling us what you have discovered.  How does set_next_event()
get called after we've set the mode to UNUSED in the current code?
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
ning.n.jiang@gmail.com March 30, 2013, 11:56 a.m. UTC | #2
2013/3/30 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Sat, Mar 30, 2013 at 05:57:38PM +0800, Ning Jiang wrote:
>> 2013/3/30 Stephen Boyd <sboyd@codeaurora.org>:
>> > On 03/29/13 02:24, ning.n.jiang@gmail.com wrote:
>> >> From: Ning Jiang <ning.n.jiang@gmail.com>
>> >>
>> >> Currently there are two problems when we try to stop local timer.
>> >> First, it calls set_mode function directly so mode state is not
>> >> updated for the clock event device. Second, it makes the device
>> >> unused instead of shutdown.
>> >
>> > What device is this a problem on? I believe this only matters to drivers
>> > which enable their timer in their set_next_event() callback? But even
>> > then, does anything actually happen because the interrupt should have
>> > been disabled in the local timer stop callback.
>> >
>>
>> Right. Drivers which enable timer in set_next_event() will have this problem.
>> It will not have functional problem in my case. But my device cannot enter
>> low power mode with a pending interrupt even if it is disabled.
>
> You're not telling us what you have discovered.  How does set_next_event()
> get called after we've set the mode to UNUSED in the current code?

In the current code we did not set the mode to UNUSED but only call
set_mode callback function for the clock event device. This normally
disables current clock event device. The dying CPU eventually will
switch to idle thread, call tick_nohz_idle_enter(), try to cancel the
sched_timer and reprogram the next event. Then set_next_event() gets
called. The call stack will be like:

tick_nohz_idle_enter
  -> __tick_nohz_idle_enter
    -> tick_nohz_stop_sched_tick
      -> hrtimer_cancel
        -> hrtimer_try_to_cancel
          -> remove_hrtimer
            -> __remove_hrtimer
              -> hrtimer_force_reprogram
                -> tick_program_event
                  -> clockevents_program_event
                    -> set_next_event

In set_next_event() we'll re-enable and re-program the clock event device.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
ning.n.jiang@gmail.com March 31, 2013, 1:11 p.m. UTC | #3
2013/3/30 Ning Jiang <ning.n.jiang@gmail.com>:
> 2013/3/30 Russell King - ARM Linux <linux@arm.linux.org.uk>:
>> On Sat, Mar 30, 2013 at 05:57:38PM +0800, Ning Jiang wrote:
>>> 2013/3/30 Stephen Boyd <sboyd@codeaurora.org>:
>>> > On 03/29/13 02:24, ning.n.jiang@gmail.com wrote:
>>> >> From: Ning Jiang <ning.n.jiang@gmail.com>
>>> >>
>>> >> Currently there are two problems when we try to stop local timer.
>>> >> First, it calls set_mode function directly so mode state is not
>>> >> updated for the clock event device. Second, it makes the device
>>> >> unused instead of shutdown.
>>> >
>>> > What device is this a problem on? I believe this only matters to drivers
>>> > which enable their timer in their set_next_event() callback? But even
>>> > then, does anything actually happen because the interrupt should have
>>> > been disabled in the local timer stop callback.
>>> >
>>>
>>> Right. Drivers which enable timer in set_next_event() will have this problem.
>>> It will not have functional problem in my case. But my device cannot enter
>>> low power mode with a pending interrupt even if it is disabled.
>>
>> You're not telling us what you have discovered.  How does set_next_event()
>> get called after we've set the mode to UNUSED in the current code?
>
> In the current code we did not set the mode to UNUSED but only call
> set_mode callback function for the clock event device. This normally
> disables current clock event device. The dying CPU eventually will
> switch to idle thread, call tick_nohz_idle_enter(), try to cancel the
> sched_timer and reprogram the next event. Then set_next_event() gets
> called. The call stack will be like:
>
> tick_nohz_idle_enter
>   -> __tick_nohz_idle_enter
>     -> tick_nohz_stop_sched_tick
>       -> hrtimer_cancel
>         -> hrtimer_try_to_cancel
>           -> remove_hrtimer
>             -> __remove_hrtimer
>               -> hrtimer_force_reprogram
>                 -> tick_program_event
>                   -> clockevents_program_event
>                     -> set_next_event
>
> In set_next_event() we'll re-enable and re-program the clock event device.

I think there are two problems here:
1. We should use clockevents_set_mode() instead of calling set_mode
callback directly. This is the issue my patch was trying to fix.
2. We shouldn't program a clock event device for a dying CPU anyway. I
can submit another patch if agreed.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index c6d6400..e22e268 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -210,6 +210,9 @@  int clockevents_program_event(struct
clock_event_device *dev, ktime_t expires,
                return -ETIME;
        }

+       if (cpu_is_offline(smp_processor_id()))
+               return 0;
+
        dev->next_event = expires;