diff mbox

[1/2] hrtimer: update '->active_bases' before calling hrtimer_force_reprogram()

Message ID c7c8ebcd9ed88bb09d76059c745a1fafb48314e7.1427959032.git.viresh.kumar@linaro.org
State New
Headers show

Commit Message

Viresh Kumar April 2, 2015, 10:51 a.m. UTC 
'active_bases' indicates which clock-base have active timers. While it
is updated (almost) correctly, it is hardly used. Next commit will start
using it to make code more efficient, but before that we need to fix a
problem.

While removing hrtimers, in __remove_hrtimer():
- We first remove the hrtimer from the queue.
- Then reprogram clockevent device if required
  (hrtimer_force_reprogram()).
- And then finally clear 'active_bases', if no more timers are pending
  on the current clock base (from which we are removing the hrtimer).

hrtimer_force_reprogram() needs to loop over all active clock bases to
find the next expiry event, and while doing so it will use
'active_bases' (after next commit). And it will find the current base
active, as we haven't cleared it until now, even if current clock base
has no more hrtimers queued.

To fix this issue, clear active_bases before calling
hrtimer_force_reprogram().

Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/hrtimer.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Viresh Kumar April 2, 2015, 1:53 p.m. UTC | #1 
On 2 April 2015 at 19:17, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Apr 02, 2015 at 04:21:21PM +0530, Viresh Kumar wrote:
>> 'active_bases' indicates which clock-base have active timers. While it
>> is updated (almost) correctly, it is hardly used. Next commit will start
>> using it to make code more efficient, but before that we need to fix a
>> problem.
>>
>> While removing hrtimers, in __remove_hrtimer():
>> - We first remove the hrtimer from the queue.
>> - Then reprogram clockevent device if required
>>   (hrtimer_force_reprogram()).
>> - And then finally clear 'active_bases', if no more timers are pending
>>   on the current clock base (from which we are removing the hrtimer).
>>
>> hrtimer_force_reprogram() needs to loop over all active clock bases to
>> find the next expiry event, and while doing so it will use
>> 'active_bases' (after next commit). And it will find the current base
>> active, as we haven't cleared it until now, even if current clock base
>> has no more hrtimers queued.
>>
>> To fix this issue, clear active_bases before calling
>> hrtimer_force_reprogram().
>
> This is a small inefficiency right? Not an actual bug or anything.

So, what's explained in this patch is a BUG, which isn't harming us today.

But overall both patches combined are about improving on the small
inefficiency :)

Sorry for the political answer :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Viresh Kumar April 2, 2015, 2:23 p.m. UTC | #2 
On 2 April 2015 at 19:46, Peter Zijlstra <peterz@infradead.org> wrote:
> So then I'm not seeing how its a bug. Sure __hrtimer_get_next_event()
> will iterate all the bases again, and it will not skip the just empty
> one. But I don't see how that is anything but an inefficiency. By virtue
> of the base being empty it cannot find an event there, so its a
> pointless check.
>
> What am I missing?

Hmm. It was a bug for me because I was doing this unconditionally:
timer = container_of(timerqueue_getnext(&base->active),
+                                    struct hrtimer, node);

And this will give a container-of over NULL, as timerqueue_getnext() can
return NULL..

And so it will crash in my case.

But I understand your point, its inefficiency only :(
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
diff mbox

Patch

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index bee0c1f78091..3152f327c988 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -879,6 +879,9 @@  static void __remove_hrtimer(struct hrtimer *timer,
 
 	next_timer = timerqueue_getnext(&base->active);
 	timerqueue_del(&base->active, &timer->node);
+	if (!timerqueue_getnext(&base->active))
+		base->cpu_base->active_bases &= ~(1 << base->index);
+
 	if (&timer->node == next_timer) {
 #ifdef CONFIG_HIGH_RES_TIMERS
 		/* Reprogram the clock event device. if enabled */
@@ -892,8 +895,6 @@  static void __remove_hrtimer(struct hrtimer *timer,
 		}
 #endif
 	}
-	if (!timerqueue_getnext(&base->active))
-		base->cpu_base->active_bases &= ~(1 << base->index);
 out:
 	timer->state = newstate;
 }