Messages in this thread | | | Date | Tue, 10 Oct 2023 13:19:02 +0200 | From | Frederic Weisbecker <> | Subject | Re: [PATCH v8 08/25] timer: Rework idle logic |
| |
On Tue, Oct 10, 2023 at 12:15:09AM +0200, Thomas Gleixner wrote: > On Wed, Oct 04 2023 at 14:34, Anna-Maria Behnsen wrote: > > > > - if (time_before_eq(nextevt, basej)) { > > - expires = basem; > > - base->is_idle = false; > > - } else { > > - if (base->timers_pending) > > - expires = basem + (u64)(nextevt - basej) * TICK_NSEC; > > - /* > > - * If we expect to sleep more than a tick, mark the base idle. > > - * Also the tick is stopped so any added timer must forward > > - * the base clk itself to keep granularity small. This idle > > - * logic is only maintained for the BASE_STD base, deferrable > > - * timers may still see large granularity skew (by design). > > - */ > > - if ((expires - basem) > TICK_NSEC) > > - base->is_idle = true; > > + /* > > + * Base is idle if the next event is more than a tick away. Also > > + * the tick is stopped so any added timer must forward the base clk > > + * itself to keep granularity small. This idle logic is only > > + * maintained for the BASE_STD base, deferrable timers may still > > + * see large granularity skew (by design). > > + */ > > + base->is_idle = time_after(nextevt, basej + 1); > > This is wrongly ordered. base->is_idle must be updated _after_ > evaluating base->timers_pending because the below can change nextevt, > no? > > > + if (base->timers_pending) { > > + /* If we missed a tick already, force 0 delta */ > > + if (time_before(nextevt, basej)) > > + nextevt = basej; > > + expires = basem + (u64)(nextevt - basej) * TICK_NSEC;
I suspect it doesn't matter in pratice: base->is_idle will remain false if it's before/equal jiffies.
Still it hurts the eyes so I agree the re-ordering should happen here and this will even simplify a bit the next patch.
Thanks.
> Thanks, > > tglx
| |