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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 12 Jul 2016 17:04:17 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Nicolai Stange <nicstange@...il.com>
cc:	Ingo Molnar <mingo@...hat.com>, "H. Peter Anvin" <hpa@...or.com>,
	x86@...nel.org, John Stultz <john.stultz@...aro.org>,
	Borislav Petkov <bp@...e.de>,
	Paolo Bonzini <pbonzini@...hat.com>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>,
	"Peter Zijlstra (Intel)" <peterz@...radead.org>,
	"Christopher S. Hall" <christopher.s.hall@...el.com>,
	Adrian Hunter <adrian.hunter@...el.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/4] kernel/time/clockevents: compensate for monotonic
 clock's dynamic frequency

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