Hi Thomas,
>
> + * @next: time of the next event on this clock base
>
> What initializes that? It's 0 to begin with.
I thought I can skip initialization because I update base->next
in the interrupt or in __remove_hrtimer, like:
- enqueue_timer, base->next is 0
- reprogram device
- device fires -> hrtimer_interrupt
- __run_hrtimer
- __remove_hrtimer
- if last base->next = KTIME_MAX
- otherwise base->next = ktime_sub(hrtimer_get_expires(timer), base->offset)
in hrtimer_interrupt
> > @@ -893,6 +895,10 @@ static int enqueue_hrtimer(struct hrtimer *timer,
> > */
> > timer->state |= HRTIMER_STATE_ENQUEUED;
> >
> > + expires = ktime_sub(hrtimer_get_expires(timer), base->offset);
>
> This does not work when time gets set and the offset changes. We need
> to store the absolute expiry time and subtract the offset at
> evaluation time.
Hm, looking at this code after a while it seems I don't need to update
base->next in enqueue_hrtimer. It's enough to set it to KTIME_MAX in
__remove_hrtimer or to actual value upon breaking from __run_hrtimer
loop in hrtimer_interrupt.
> > @@ -929,8 +935,10 @@ static void __remove_hrtimer(struct hrtimer *timer,
> > }
> > #endif
> > }
> > - if (!timerqueue_getnext(&base->active))
> > + if (!timerqueue_getnext(&base->active)) {
> > base->cpu_base->active_bases &= ~(1 << base->index);
> > + base->next = ktime_set(KTIME_SEC_MAX, 0);
> > + }
>
> And what updates base->next if there are timers pending?
See above, hrtimer_interrupt updates it before breaking or sets to
KTIME_MAX in __remove_hrtimer if it's the last one.
> > + for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++) {
> > + ktime_t expires;
>
> So this adds the third incarnation of finding the next expiring timer
> to the code. Not really helpful.
Didn't really think about all the other places, refactoring may come in
another patch.
> Untested patch which addresses the issues below.
Aside from a small nitpick below, looks reasonable, I'll try to run it on a
couple of machines.
Should I send you a v3 afterwards with the changelog or
tested-by would be enough?
> + while (active) {
> + idx = __ffs(active);
> + active &= ~(1UL << idx);
Is there any reason you did that instead of conventional:
for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++) {
if (!(cpu_base->active_bases & (1 << i)))
continue;
...
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
