When expiry is set to KTIME_MAX, we cancel the 'tick-sched' hrtimer in highres mode and skip reprogramming clockevent device in lowres mode. But, the clockevent device is already reprogrammed from tick-handler.
We will get interrupted atleast one more time.
In highres mode, as there is no hrtimer to service, hrtimer_interrupt() will return without calling tick-handler.
But the problem is somewhat bigger in lowres mode. We unconditionally reschedule tick everytime tick_nohz_handler() is called and so even if tick_stopped is set, we never enter full dynticks mode.
To fix this, skip rescheduling next tick from tick-handler when we are running tickless.
OTOH, when expires isn't equal to KTIME_MAX, we avoid reprogramming the clkdev from the tick when it is stopped because it's going to be reprogrammed from irq_exit() anyway. So we could avoid one useless device write.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org ---
kernel/time/tick-sched.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 6558b7a..a4a45e0 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -956,6 +956,12 @@ static void tick_nohz_handler(struct clock_event_device *dev) tick_sched_do_timer(now); tick_sched_handle(ts, regs);
+ /* + * Skip reprogramming next event if we are running tickless. + */ + if (ts->tick_stopped) + return; + while (tick_nohz_reprogram(ts, now)) { now = ktime_get(); tick_do_update_jiffies64(now);
Hi Viresh,
On 06/17/2014 01:50 PM, Viresh Kumar wrote:
When expiry is set to KTIME_MAX, we cancel the 'tick-sched' hrtimer in highres mode and skip reprogramming clockevent device in lowres mode. But, the clockevent device is already reprogrammed from tick-handler.
We will get interrupted atleast one more time.
In highres mode, as there is no hrtimer to service, hrtimer_interrupt() will return without calling tick-handler.
But the problem is somewhat bigger in lowres mode. We unconditionally reschedule tick everytime tick_nohz_handler() is called and so even if tick_stopped is set, we never enter full dynticks mode.
To fix this, skip rescheduling next tick from tick-handler when we are running tickless.
OTOH, when expires isn't equal to KTIME_MAX, we avoid reprogramming the clkdev from the tick when it is stopped because it's going to be reprogrammed from irq_exit() anyway. So we could avoid one useless device write.
I am sceptical about what this patch does when expires is not equal to KTIME_MAX. This, being the primary timer handler function in the nohz low resolution mode, it should skip programming *only* when the tick is stopped, indicating that the cpu was in idle (which this patch takes care of) and there were no pending timers queued between the time this cpu went idle and now. If there is a pending timer, it should program the clock device even if the tick was stopped.
I realise that in the irq_exit path, the periodic tick is re-started if there are pending timers, but is it right to rely on the irq exit path for this? What if the code in the irq_exit path optimizes some parts and removes restart of periodic ticks? In which case this code will break. My point is the timer handler function should be self sufficient as much as possible in taking care of programming the clock device.
Correct me if I am wrong but I don't think any other timer handler function relies on the irq_exit path to reprogram the clock device.
Regards Preeti U Murthy
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
kernel/time/tick-sched.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 6558b7a..a4a45e0 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -956,6 +956,12 @@ static void tick_nohz_handler(struct clock_event_device *dev) tick_sched_do_timer(now); tick_sched_handle(ts, regs);
- /*
* Skip reprogramming next event if we are running tickless.*/- if (ts->tick_stopped)
return;- while (tick_nohz_reprogram(ts, now)) { now = ktime_get(); tick_do_update_jiffies64(now);
On 30 June 2014 16:28, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
I am sceptical about what this patch does when expires is not equal to KTIME_MAX. This, being the primary timer handler function in the nohz low resolution mode, it should skip programming *only* when the tick is stopped, indicating that the cpu was in idle (which this patch takes
or in full dynticks mode..
care of) and there were no pending timers queued between the time this cpu went idle and now. If there is a pending timer, it should program the clock device even if the tick was stopped.
No, we don't need tick just for running timers. We might want to go in full dynticks mode here (not infinitely though), and if the timer is required only after say 100 ticks, we will program clkevt device for this late.
Actually frederic suggested to do something similar in High resolution mode as well, so that we can avoid double set-event for clockevent device. As we will surely get to tick_nohz_stop_sched_tick() as well..
I realise that in the irq_exit path, the periodic tick is re-started if there are pending timers, but is it right to rely on the irq exit path
Sure. I don't think we restart periodic tick unless its required. We might be reprogramming clkevt device to fire after some ticks-interval though.
for this? What if the code in the irq_exit path optimizes some parts and removes restart of periodic ticks? In which case this code will break.
It can't.
We already depend *heavily* on tick_nohz_irq_exit() for both idle and full dyntick modes. And this is considered the right place to check if ticks are required or not.
My point is the timer handler function should be self sufficient as much as possible in taking care of programming the clock device.
We probably can't handle everything from tick/timer routines. For example, suppose after tick-interrupt some task is started on the isolated/idle CPU. And by that time tick routines are already called.
And probably so its handled late in irq_exit()..
Correct me if I am wrong but I don't think any other timer handler function relies on the irq_exit path to reprogram the clock device.
I am not sure if I understood what you are pointing at completely.
There are only two timer-handler routines available, leaving periodic ones: - Lowres - The one I updated. - Highres - We already cancel hrtimer when not required.
So, yes we are depending on irq_exit() badly :)
Let me know if I am wrong somewhere :)
Hi Viresh,
On 06/30/2014 04:53 PM, Viresh Kumar wrote:
On 30 June 2014 16:28, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
I am sceptical about what this patch does when expires is not equal to KTIME_MAX. This, being the primary timer handler function in the nohz low resolution mode, it should skip programming *only* when the tick is stopped, indicating that the cpu was in idle (which this patch takes
or in full dynticks mode..
care of) and there were no pending timers queued between the time this cpu went idle and now. If there is a pending timer, it should program the clock device even if the tick was stopped.
No, we don't need tick just for running timers. We might want to go in full dynticks mode here (not infinitely though), and if the timer is required only after say 100 ticks, we will program clkevt device for this late.
Actually frederic suggested to do something similar in High resolution mode as well, so that we can avoid double set-event for clockevent device. As we will surely get to tick_nohz_stop_sched_tick() as well..
In high resolution mode, we will still need to program clock device for any pending hrtimers in hrtimer_interrupt() right? Only the timer wheel is verified in tick_nohz_stop_sched_tick(), isn't it?
I realise that in the irq_exit path, the periodic tick is re-started if there are pending timers, but is it right to rely on the irq exit path
Sure. I don't think we restart periodic tick unless its required. We might be reprogramming clkevt device to fire after some ticks-interval though.
for this? What if the code in the irq_exit path optimizes some parts and removes restart of periodic ticks? In which case this code will break.
It can't.
We already depend *heavily* on tick_nohz_irq_exit() for both idle and full dyntick modes. And this is considered the right place to check if ticks are required or not.
My point is the timer handler function should be self sufficient as much as possible in taking care of programming the clock device.
We probably can't handle everything from tick/timer routines. For example, suppose after tick-interrupt some task is started on the isolated/idle CPU. And by that time tick routines are already called.
And probably so its handled late in irq_exit()..
Correct me if I am wrong but I don't think any other timer handler function relies on the irq_exit path to reprogram the clock device.
I am not sure if I understood what you are pointing at completely.
There are only two timer-handler routines available, leaving periodic ones:
- Lowres - The one I updated.
- Highres - We already cancel hrtimer when not required.
So, yes we are depending on irq_exit() badly :)
Hmm ok, I see the flow now. tick_nohz_handler() becomes relevant for programming as long as the cpu is busy. Else the tick_nohz_stop_sched_tick() has to take care of programming for several ticks away.
Thanks for the explanation!
Regards Preeti U Murthy
Let me know if I am wrong somewhere :)
On 30-Jun-2014, at 5:14 pm, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
Hi Viresh,
On 06/30/2014 04:53 PM, Viresh Kumar wrote:
On 30 June 2014 16:28, Preeti U Murthy preeti@linux.vnet.ibm.com wrote: I am sceptical about what this patch does when expires is not equal to KTIME_MAX. This, being the primary timer handler function in the nohz low resolution mode, it should skip programming *only* when the tick is stopped, indicating that the cpu was in idle (which this patch takes
or in full dynticks mode..
care of) and there were no pending timers queued between the time this cpu went idle and now. If there is a pending timer, it should program the clock device even if the tick was stopped.
No, we don't need tick just for running timers. We might want to go in full dynticks mode here (not infinitely though), and if the timer is required only after say 100 ticks, we will program clkevt device for this late.
Actually frederic suggested to do something similar in High resolution mode as well, so that we can avoid double set-event for clockevent device. As we will surely get to tick_nohz_stop_sched_tick() as well..
In high resolution mode, we will still need to program clock device for any pending hrtimers in hrtimer_interrupt() right? Only the timer wheel is verified in tick_nohz_stop_sched_tick(), isn't it?
Yes. So?
On 06/30/2014 05:48 PM, Viresh Kumar wrote:
On 30-Jun-2014, at 5:14 pm, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
Hi Viresh,
On 06/30/2014 04:53 PM, Viresh Kumar wrote:
On 30 June 2014 16:28, Preeti U Murthy preeti@linux.vnet.ibm.com wrote: I am sceptical about what this patch does when expires is not equal to KTIME_MAX. This, being the primary timer handler function in the nohz low resolution mode, it should skip programming *only* when the tick is stopped, indicating that the cpu was in idle (which this patch takes
or in full dynticks mode..
care of) and there were no pending timers queued between the time this cpu went idle and now. If there is a pending timer, it should program the clock device even if the tick was stopped.
No, we don't need tick just for running timers. We might want to go in full dynticks mode here (not infinitely though), and if the timer is required only after say 100 ticks, we will program clkevt device for this late.
Actually frederic suggested to do something similar in High resolution mode as well, so that we can avoid double set-event for clockevent device. As we will surely get to tick_nohz_stop_sched_tick() as well..
In high resolution mode, we will still need to program clock device for any pending hrtimers in hrtimer_interrupt() right? Only the timer wheel is verified in tick_nohz_stop_sched_tick(), isn't it?
Yes. So?
I was wondering what Frederic suggested with regard to the high resolution timers in this aspect.
Regards Preeti U Murthy
On 06/17/2014 01:50 PM, Viresh Kumar wrote:
When expiry is set to KTIME_MAX, we cancel the 'tick-sched' hrtimer in highres mode and skip reprogramming clockevent device in lowres mode. But, the clockevent device is already reprogrammed from tick-handler.
We will get interrupted atleast one more time.
In highres mode, as there is no hrtimer to service, hrtimer_interrupt() will return without calling tick-handler.
But the problem is somewhat bigger in lowres mode. We unconditionally reschedule tick everytime tick_nohz_handler() is called and so even if tick_stopped is set, we never enter full dynticks mode.
To fix this, skip rescheduling next tick from tick-handler when we are running tickless.
OTOH, when expires isn't equal to KTIME_MAX, we avoid reprogramming the clkdev from the tick when it is stopped because it's going to be reprogrammed from irq_exit() anyway. So we could avoid one useless device write.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
kernel/time/tick-sched.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 6558b7a..a4a45e0 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -956,6 +956,12 @@ static void tick_nohz_handler(struct clock_event_device *dev) tick_sched_do_timer(now); tick_sched_handle(ts, regs);
- /*
* Skip reprogramming next event if we are running tickless.*/- if (ts->tick_stopped)
return;- while (tick_nohz_reprogram(ts, now)) { now = ktime_get(); tick_do_update_jiffies64(now);
Reviewed-by: Preeti U Murthy preeti@linux.vnet.ibm.com
On 06/30/2014 05:26 PM, Preeti U Murthy wrote:
On 06/17/2014 01:50 PM, Viresh Kumar wrote:
When expiry is set to KTIME_MAX, we cancel the 'tick-sched' hrtimer in highres mode and skip reprogramming clockevent device in lowres mode. But, the clockevent device is already reprogrammed from tick-handler.
We will get interrupted atleast one more time.
In highres mode, as there is no hrtimer to service, hrtimer_interrupt() will return without calling tick-handler.
But the problem is somewhat bigger in lowres mode. We unconditionally reschedule tick everytime tick_nohz_handler() is called and so even if tick_stopped is set, we never enter full dynticks mode.
To fix this, skip rescheduling next tick from tick-handler when we are running tickless.
OTOH, when expires isn't equal to KTIME_MAX, we avoid reprogramming the clkdev from the tick when it is stopped because it's going to be reprogrammed from irq_exit() anyway. So we could avoid one useless device write.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
kernel/time/tick-sched.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 6558b7a..a4a45e0 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -956,6 +956,12 @@ static void tick_nohz_handler(struct clock_event_device *dev) tick_sched_do_timer(now); tick_sched_handle(ts, regs);
- /*
* Skip reprogramming next event if we are running tickless.*/- if (ts->tick_stopped)
return;- while (tick_nohz_reprogram(ts, now)) { now = ktime_get(); tick_do_update_jiffies64(now);
Reviewed-by: Preeti U Murthy preeti@linux.vnet.ibm.com
While I was re-verifying this piece of code I came across commit 79bf2bb3. As we discussed offline, commit 79bf2bb3 had this check when support to dynamic tick was being added.
But as you pointed out, the commit fb02fbc removed this check to keep jiffies updated for the sake of long running softirqs but was later again reverted in ae99286b due to spurious wakeups. However it was not entirely reverted. The long running softirqs were asked to fix themselves.
So adding back this check in here makes sense for the sake of the problem explained in the changelog. Besides, you might want to have the history as mentioned in the previous paragraph in this changelog too so that people do not wonder why the check was removed in the first place.
Regards Preeti U Murthy
On 2 July 2014 17:08, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
While I was re-verifying this piece of code I came across commit 79bf2bb3. As we discussed offline, commit 79bf2bb3 had this check when support to dynamic tick was being added.
But as you pointed out, the commit fb02fbc removed this check to keep jiffies updated for the sake of long running softirqs but was later again reverted in ae99286b due to spurious wakeups. However it was not entirely reverted. The long running softirqs were asked to fix themselves.
So adding back this check in here makes sense for the sake of the problem explained in the changelog. Besides, you might want to have the history as mentioned in the previous paragraph in this changelog too so that people do not wonder why the check was removed in the first place.
Added this to bottom of existing log.
Similar piece of code was present initially when dynticks functionality was first added: 79bf2bb3. But was later commit fb02fbc removed this check to keep jiffies updated for the sake of long running softirqs but was later again reverted in ae99286b due to spurious wakeups. However it was not entirely reverted. The long running softirqs were asked to fix themselves.
linaro-kernel@lists.linaro.org
-
Preeti U Murthy -
Viresh Kumar