Messages in this thread | | | From | Nicolai Stange <> | Subject | Re: [PATCH v2 4/4] kernel/time/clockevents: compensate for monotonic clock's dynamic frequency | Date | Tue, 12 Jul 2016 13:10:57 +0200 |
| |
Thomas Gleixner <tglx@linutronix.de> writes:
> On Mon, 11 Jul 2016, Nicolai Stange wrote: >> > + raw = ((u64)interval >> 32) * raw_mult; /* Upper half of interval */ >> > + if (raw >> 32) >> > + return KTIME_MAX; >> > + raw <<= 32; >> > + tmp = ((u64)interval & U32_MAX) * raw_mult; /* Lower half of interval */ >> > + if (U64_MAX - raw < tmp) >> > + return KTIME_MAX; >> > + raw += tmp; >> > + >> > + /* Finally, do raw /= mono_mult with proper rounding. */ >> > + if (U64_MAX - raw < mono_mult / 2) >> > + return KTIME_MAX; >> > + raw += mono_mult / 2; >> > + do_div(raw, mono_mult); >> > + >> > + return (s64)raw; > > That's a complete insanity. No way that we are going to do all this math in > the CE programming path. > > If you want to address the issue, then you need to find a way to adjust the > mult/shift factors of the clock event device occasionally.
I tried adjusting the clock event device's ->mult, triggered by timekeeping_apply_adjustment() and it works well.
I think that in order to avoid error accumulation, it is best not to do any incremental updates to ->mult, but introduce a new ->mult_mono and recalculate the latter from the former.
Now, the ->mult_mono needs to get updated when the driver updates ->mult. Certainly, hooking into clockevents_register_device() and clockevents_update_freq() is the method of choice here. However, there are a handful of drivers which set ->mult from ->set_state_oneshot() either by direct assignment or through clockevents_config(): drivers/clocksource/sh_cmt.c drivers/clocksource/sh_tmu.c drivers/clocksource/em_sti.c drivers/clocksource/h8300_timer8.c Converting these to clockevents_update_freq() seems straightforward though.
Another issue is that ->min_delta_ns and ->max_delta_ns are measured in raw clock time while the delta in clockevents_program_event() would now be interpreted as being in monotonic clock time: clc = ((unsigned long long) delta * dev->mult_mono) >> dev->shift;
Ideally, I'd like to get rid of ->min_delta_ns and ->max_delta_ns alltogether and consistently use the ->min_delta_ticks and ->max_delta_ticks instead. AFAICS, ->min_delta_ns is really needed only for setting dev->next_event in clockevents_program_min_delta(). dev->next_event is read only from __clockevents_update_freq() for reprogramming purposes and thus, assuming 0 for ->delta_min_ns in clockevents_program_min_delta() would probably work: a reprogramming would invoke clockevents_program_min_delta() once again.
The downside of this approach is that a quick grep reveals 40 clockevent device drivers whose initialization code would need to get touched in order to convert them from min_delta_ns/max_delta_ns to min_delta_ticks/max_delta_ticks.
So, the question is whether I should do all of this or whether the doubled timer interrupts aren't annoying enough to justify such a big change?
Thanks,
Nicolai
| |