hrtimer_start*() family never fails to enqueue a hrtimer to a clock-base. In case the hrtimer was in past and getting added on this_cpu's clock-base, we raise a softirq and exit.
At several places in the kernel, we try to make sure if hrtimer was added properly or not by calling hrtimer_active(), like:
hrtimer_start(timer, expires, mode); if (hrtimer_active(timer)) { /* Added successfully */ } else { /* Was added in the past */ }
As hrtimer_start*() never fails, hrtimer_active() is guaranteed to return 'true'. So, there is no point calling hrtimer_active().
Also this is done in while loop at several places, which isn't required if hrtimer_start*() never fails. Drop those loops as well.
This patch only updates tick-sched.c file currently, others will be fixed if above explanation holds true.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- This was also raised here and didn't attract many: https://www.lkml.org/lkml/2014/7/3/559
Pushed here: git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/linux.git tick/remove-stale-hrtimer_active
kernel/time/tick-sched.c | 45 ++++++++++++++++++--------------------------- 1 file changed, 18 insertions(+), 27 deletions(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 6558b7a..66ca5ab 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -658,9 +658,7 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, if (ts->nohz_mode == NOHZ_MODE_HIGHRES) { hrtimer_start(&ts->sched_timer, expires, HRTIMER_MODE_ABS_PINNED); - /* Check, if the timer was already in the past */ - if (hrtimer_active(&ts->sched_timer)) - goto out; + goto out; } else if (!tick_program_event(expires, 0)) goto out; /* @@ -844,24 +842,25 @@ static void tick_nohz_restart(struct tick_sched *ts, ktime_t now) hrtimer_cancel(&ts->sched_timer); hrtimer_set_expires(&ts->sched_timer, ts->last_tick);
- while (1) { - /* Forward the time to expire in the future */ - hrtimer_forward(&ts->sched_timer, now, tick_period); + /* Forward the time to expire in the future */ + hrtimer_forward(&ts->sched_timer, now, tick_period);
- if (ts->nohz_mode == NOHZ_MODE_HIGHRES) { - hrtimer_start_expires(&ts->sched_timer, - HRTIMER_MODE_ABS_PINNED); - /* Check, if the timer was already in the past */ - if (hrtimer_active(&ts->sched_timer)) - break; - } else { - if (!tick_program_event( - hrtimer_get_expires(&ts->sched_timer), 0)) - break; - } + if (ts->nohz_mode == NOHZ_MODE_HIGHRES) { + hrtimer_start_expires(&ts->sched_timer, + HRTIMER_MODE_ABS_PINNED); + return; + } + + while (1) { + if (!tick_program_event(hrtimer_get_expires(&ts->sched_timer), + 0)) + break; /* Reread time and update jiffies */ now = ktime_get(); tick_do_update_jiffies64(now); + + /* Forward the time to expire in the future */ + hrtimer_forward(&ts->sched_timer, now, tick_period); } }
@@ -1104,7 +1103,6 @@ early_param("skew_tick", skew_tick); void tick_setup_sched_timer(void) { struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched); - ktime_t now = ktime_get();
/* * Emulate tick processing via per-CPU hrtimers: @@ -1123,15 +1121,8 @@ void tick_setup_sched_timer(void) hrtimer_add_expires_ns(&ts->sched_timer, offset); }
- for (;;) { - hrtimer_forward(&ts->sched_timer, now, tick_period); - hrtimer_start_expires(&ts->sched_timer, - HRTIMER_MODE_ABS_PINNED); - /* Check, if the timer was already in the past */ - if (hrtimer_active(&ts->sched_timer)) - break; - now = ktime_get(); - } + hrtimer_forward(&ts->sched_timer, ktime_get(), tick_period); + hrtimer_start_expires(&ts->sched_timer, HRTIMER_MODE_ABS_PINNED);
#ifdef CONFIG_NO_HZ_COMMON if (tick_nohz_enabled) {
On Tue, Jul 08, 2014 at 04:10:47PM +0530, Viresh Kumar wrote:
hrtimer_start*() family never fails to enqueue a hrtimer to a clock-base. In case the hrtimer was in past and getting added on this_cpu's clock-base, we raise a softirq and exit.
At several places in the kernel, we try to make sure if hrtimer was added properly or not by calling hrtimer_active(), like:
hrtimer_start(timer, expires, mode); if (hrtimer_active(timer)) { /* Added successfully */ } else { /* Was added in the past */ }
As hrtimer_start*() never fails, hrtimer_active() is guaranteed to return 'true'. So, there is no point calling hrtimer_active().
Also this is done in while loop at several places, which isn't required if hrtimer_start*() never fails. Drop those loops as well.
This patch only updates tick-sched.c file currently, others will be fixed if above explanation holds true.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Makes sense, assuming I'm not missing something obvious.
But you might want to add a WARN_ON_ONCE(!hrtimer_active(hrtimer)) on the end of these hrtimer_start common parts. In the end of __hrtimer_start_range_ns() maybe.
Thanks.
This was also raised here and didn't attract many: https://www.lkml.org/lkml/2014/7/3/559
Pushed here: git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/linux.git tick/remove-stale-hrtimer_active
kernel/time/tick-sched.c | 45 ++++++++++++++++++--------------------------- 1 file changed, 18 insertions(+), 27 deletions(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 6558b7a..66ca5ab 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -658,9 +658,7 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, if (ts->nohz_mode == NOHZ_MODE_HIGHRES) { hrtimer_start(&ts->sched_timer, expires, HRTIMER_MODE_ABS_PINNED);
/* Check, if the timer was already in the past */if (hrtimer_active(&ts->sched_timer))goto out;
goto out;@@ -844,24 +842,25 @@ static void tick_nohz_restart(struct tick_sched *ts, ktime_t now) hrtimer_cancel(&ts->sched_timer); hrtimer_set_expires(&ts->sched_timer, ts->last_tick);
- while (1) {
/* Forward the time to expire in the future */hrtimer_forward(&ts->sched_timer, now, tick_period);
- /* Forward the time to expire in the future */
- hrtimer_forward(&ts->sched_timer, now, tick_period);
if (ts->nohz_mode == NOHZ_MODE_HIGHRES) {hrtimer_start_expires(&ts->sched_timer,HRTIMER_MODE_ABS_PINNED);/* Check, if the timer was already in the past */if (hrtimer_active(&ts->sched_timer))break;} else {if (!tick_program_event(hrtimer_get_expires(&ts->sched_timer), 0))break;}
- if (ts->nohz_mode == NOHZ_MODE_HIGHRES) {
hrtimer_start_expires(&ts->sched_timer,HRTIMER_MODE_ABS_PINNED);return;- }
- while (1) {
if (!tick_program_event(hrtimer_get_expires(&ts->sched_timer),0))break;/* Forward the time to expire in the future */hrtimer_forward(&ts->sched_timer, now, tick_period);} @@ -1104,7 +1103,6 @@ early_param("skew_tick", skew_tick); void tick_setup_sched_timer(void) { struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched);
- ktime_t now = ktime_get();
/* * Emulate tick processing via per-CPU hrtimers: @@ -1123,15 +1121,8 @@ void tick_setup_sched_timer(void) hrtimer_add_expires_ns(&ts->sched_timer, offset); }
- for (;;) {
hrtimer_forward(&ts->sched_timer, now, tick_period);hrtimer_start_expires(&ts->sched_timer,HRTIMER_MODE_ABS_PINNED);/* Check, if the timer was already in the past */if (hrtimer_active(&ts->sched_timer))break;now = ktime_get();- }
- hrtimer_forward(&ts->sched_timer, ktime_get(), tick_period);
- hrtimer_start_expires(&ts->sched_timer, HRTIMER_MODE_ABS_PINNED);
#ifdef CONFIG_NO_HZ_COMMON if (tick_nohz_enabled) { -- 2.0.0.rc2
On 8 July 2014 18:21, Frederic Weisbecker fweisbec@gmail.com wrote:
Makes sense, assuming I'm not missing something obvious.
Actually looking at the way it is written since the beginning, and the credentials of people that wrote it, I also have serious doubts about this stuff..
So be ready for a rant, I am :)
But you might want to add a WARN_ON_ONCE(!hrtimer_active(hrtimer)) on the end of these hrtimer_start common parts. In the end of __hrtimer_start_range_ns() maybe.
When I first read your reply, it looked a great idea. But thinking about it a bit more, it didn't look that great :)
Isn't it something like:
x = y; WARN_ON_ONCE(x != y);
i.e. we are adding a WARN where we are quite sure nothing can go wrong.
WARN()'s are probably good at places, where we don't expect things to happen in a certain way but they can still happen.
For example in the set_dev_mode() series, we had it on failure from ->set_dev_mode(), as platforms might try to return failures.
Having said that, I will still add this in a separate patch (the last one) just for testing. My trees are automated with kbuild bot and we can figure out if we really need to check for hrtimer_active().
This last one *shouldn't* be upstreamed :)
Let me know if you still want that WARN() to be there..
-- viresh
On Tue, Jul 08, 2014 at 07:08:46PM +0530, Viresh Kumar wrote:
On 8 July 2014 18:21, Frederic Weisbecker fweisbec@gmail.com wrote:
Makes sense, assuming I'm not missing something obvious.
Actually looking at the way it is written since the beginning, and the credentials of people that wrote it, I also have serious doubts about this stuff..
So be ready for a rant, I am :)
But you might want to add a WARN_ON_ONCE(!hrtimer_active(hrtimer)) on the end of these hrtimer_start common parts. In the end of __hrtimer_start_range_ns() maybe.
When I first read your reply, it looked a great idea. But thinking about it a bit more, it didn't look that great :)
Isn't it something like:
x = y; WARN_ON_ONCE(x != y);
Well, every state check eventually sums up to that. Now for a state check to be useful we must indeed keep some "reasonable distance" between it and the potential state write.
But then "reasonable distance" is left to personal appreciation :)
To me a function call is enough.
The best would be to check from all __hrtimer_start_range_ns() callers. So maybe __hrtimer_start_range_ns() could do the check and call __hrtimer_start_range_ns_internal().
i.e. we are adding a WARN where we are quite sure nothing can go wrong.
WARN()'s are probably good at places, where we don't expect things to happen in a certain way but they can still happen.
For example in the set_dev_mode() series, we had it on failure from ->set_dev_mode(), as platforms might try to return failures.
Having said that, I will still add this in a separate patch (the last one) just for testing. My trees are automated with kbuild bot and we can figure out if we really need to check for hrtimer_active().
This last one *shouldn't* be upstreamed :)
Let me know if you still want that WARN() to be there..
Well as you just shown above, you don't feel safe with that. That feeling alone proves that we need a state check.
Also it's very important to have one such that people are warned when they try things that go beyond our hrtimer active assumption.
Plus the check is mostly free.
On 8 July 2014 19:45, Frederic Weisbecker fweisbec@gmail.com wrote:
Well as you just shown above, you don't feel safe with that. That feeling alone proves that we need a state check.
Also it's very important to have one such that people are warned when they try things that go beyond our hrtimer active assumption.
Plus the check is mostly free.
Okay, I have added a commit for that now. Please fetch it again.
On Tue, Jul 08, 2014 at 08:57:58PM +0530, Viresh Kumar wrote:
On 8 July 2014 19:45, Frederic Weisbecker fweisbec@gmail.com wrote:
Well as you just shown above, you don't feel safe with that. That feeling alone proves that we need a state check.
Also it's very important to have one such that people are warned when they try things that go beyond our hrtimer active assumption.
Plus the check is mostly free.
Okay, I have added a commit for that now. Please fetch it again.
Okay.
BTW, I think you should now get over the internal post thing, it's not always a good idea to comply with Thomas wrath. At least not for too long :-)
You're a high-frequency patch submitter and we don't scale enough. Just post your patchsets to LKML as "RFC" until they get some agreement. This way they shouldn't get much attention by Thomas until they reach a mergeable shape.
Sounds good? :-)
On 8 July 2014 21:12, Frederic Weisbecker fweisbec@gmail.com wrote:
BTW, I think you should now get over the internal post thing, it's not always a good idea to comply with Thomas wrath. At least not for too long :-)
:)
You're a high-frequency patch submitter and we don't scale enough. Just post your patchsets to LKML as "RFC" until they get some agreement. This way they shouldn't get much attention by Thomas until they reach a mergeable shape.
Sounds good? :-)
Its more difficult for me than anybody else to stick to a private list. I have to do it, forcefully :)
Obviously, because I made others lost confidence on me with the way things progressed in ONESHOT_STOPPED series.
Yes, it sounds good to me. If you are okay with the series I will send it directly and you can provide your reviewed-by's after pointing out any shortcomings.
-- viresh
On Tue, Jul 08, 2014 at 09:27:48PM +0530, Viresh Kumar wrote:
On 8 July 2014 21:12, Frederic Weisbecker fweisbec@gmail.com wrote:
BTW, I think you should now get over the internal post thing, it's not always a good idea to comply with Thomas wrath. At least not for too long :-)
:)
You're a high-frequency patch submitter and we don't scale enough. Just post your patchsets to LKML as "RFC" until they get some agreement. This way they shouldn't get much attention by Thomas until they reach a mergeable shape.
Sounds good? :-)
Its more difficult for me than anybody else to stick to a private list. I have to do it, forcefully :)
Obviously, because I made others lost confidence on me with the way things progressed in ONESHOT_STOPPED series.
Yes, it sounds good to me. If you are okay with the series I will send it directly and you can provide your reviewed-by's after pointing out any shortcomings.
Sounds good. I can still take the patches if they are ignored. But it's important to keep LKML along so that reviews get visible.
On 07/08/2014 07:45 PM, Frederic Weisbecker wrote:
On Tue, Jul 08, 2014 at 07:08:46PM +0530, Viresh Kumar wrote:
On 8 July 2014 18:21, Frederic Weisbecker fweisbec@gmail.com wrote:
Makes sense, assuming I'm not missing something obvious.
Actually looking at the way it is written since the beginning, and the credentials of people that wrote it, I also have serious doubts about this stuff..
So be ready for a rant, I am :)
But you might want to add a WARN_ON_ONCE(!hrtimer_active(hrtimer)) on the end of these hrtimer_start common parts. In the end of __hrtimer_start_range_ns() maybe.
When I first read your reply, it looked a great idea. But thinking about it a bit more, it didn't look that great :)
Isn't it something like:
x = y; WARN_ON_ONCE(x != y);
Well, every state check eventually sums up to that. Now for a state check to be useful we must indeed keep some "reasonable distance" between it and the potential state write.
But then "reasonable distance" is left to personal appreciation :)
To me a function call is enough.
The best would be to check from all __hrtimer_start_range_ns() callers. So maybe __hrtimer_start_range_ns() could do the check and call __hrtimer_start_range_ns_internal().
i.e. we are adding a WARN where we are quite sure nothing can go wrong.
WARN()'s are probably good at places, where we don't expect things to happen in a certain way but they can still happen.
For example in the set_dev_mode() series, we had it on failure from ->set_dev_mode(), as platforms might try to return failures.
Having said that, I will still add this in a separate patch (the last one) just for testing. My trees are automated with kbuild bot and we can figure out if we really need to check for hrtimer_active().
This last one *shouldn't* be upstreamed :)
Let me know if you still want that WARN() to be there..
Well as you just shown above, you don't feel safe with that. That feeling alone proves that we need a state check.
Also it's very important to have one such that people are warned when they try things that go beyond our hrtimer active assumption.
Plus the check is mostly free.
Is it possible that between the time we start the hrtimer and we check for hrtimer_active(), we can get a timer interrupt which can dequeue the tick sched timer if it were to be in the past? In this case the hrtimer_active() can have a failed return value right?
Regards Preeti U Murthy
On 9 July 2014 10:49, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
Is it possible that between the time we start the hrtimer and we check for hrtimer_active(), we can get a timer interrupt which can dequeue the tick sched timer if it were to be in the past? In this case the hrtimer_active() can have a failed return value right?
Most of the programming for tick-sched is done from interrupt-context and so interrupts might be disabled then.
Also, the checks in tick-sched and other kernel layers are not for checking if hrtimer is still enqueued or not. It is for checking if it was added in past.
And in case it is added in past, we will raise a softirq and quit. And would be handled on next interrupt..
On 07/09/2014 10:57 AM, Viresh Kumar wrote:
On 9 July 2014 10:49, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
Is it possible that between the time we start the hrtimer and we check for hrtimer_active(), we can get a timer interrupt which can dequeue the tick sched timer if it were to be in the past? In this case the hrtimer_active() can have a failed return value right?
Most of the programming for tick-sched is done from interrupt-context and so interrupts might be disabled then.
Also, the checks in tick-sched and other kernel layers are not for checking if hrtimer is still enqueued or not. It is for checking if it was added in past.
My concern is around this comment itself. This function is not actually verifying if the timer was in the past. And such timers are taken care of in hrtimer_start* as you have pointed out.
So as we discussed offline, it would be good to mention in the changelog that currently in the kernel, this comment does not make sense for the above mentioned reason, in addition to saying that it will fail all the time in its find for a non-enqueued hrtimer.
Else the patch looks fine to me. I have added my reviewed by just in case Frederic intends to pick this up through his tree.
Reviewed-by: Preeti U Murthy preeti@linux.vnet.ibm.com
Regards Preeti U Murthy
And in case it is added in past, we will raise a softirq and quit. And would be handled on next interrupt..
On 9 July 2014 12:07, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
My concern is around this comment itself. This function is not actually verifying if the timer was in the past. And such timers are taken care of in hrtimer_start* as you have pointed out.
So as we discussed offline, it would be good to mention in the changelog that currently in the kernel, this comment does not make sense for the above mentioned reason, in addition to saying that it will fail all the time in its find for a non-enqueued hrtimer.
I have added short note about the comment in the relevant patch.
Else the patch looks fine to me. I have added my reviewed by just in case Frederic intends to pick this up through his tree.
Reviewed-by: Preeti U Murthy preeti@linux.vnet.ibm.com
Thanks, I haven't added this to the patch while sending it to LKML as there were some logs update.
Please give your Reviewed-by's for the entire series now :)
linaro-kernel@lists.linaro.org
-
Frederic Weisbecker -
Preeti U Murthy -
Viresh Kumar