lkml.org 
[lkml]   [2014]   [Jun]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] hrtimers: calculate expires_next after all timers are executed
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;

...
}


\
 
 \ /
  Last update: 2014-06-23 13:41    [W:0.101 / U:4.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site