Re: [PATCH v2 4/4] kernel/time/clockevents: compensate for monotonic clock's dynamic frequency

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, 12 Jul 2016, Nicolai Stange wrote:
> 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.

Ok.
 
> 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;

Does that really matter much?

> 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.

I completely fail to parse the above paragraph. 
 
> 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?

Can you provide an initial patch which does the adjustment w/o all the related
churn so we can see how intrusive that gets?

Thanks,

	tglx