From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932959AbcGLLLM (ORCPT ); Tue, 12 Jul 2016 07:11:12 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:33090 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932617AbcGLLLB (ORCPT ); Tue, 12 Jul 2016 07:11:01 -0400 From: Nicolai Stange To: Thomas Gleixner Cc: Nicolai Stange , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, John Stultz , Borislav Petkov , Paolo Bonzini , Viresh Kumar , Hidehiro Kawai , "Peter Zijlstra \(Intel\)" , "Christopher S. Hall" , Adrian Hunter , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 4/4] kernel/time/clockevents: compensate for monotonic clock's dynamic frequency References: <20160710193047.18320-1-nicstange@gmail.com> <20160710193047.18320-5-nicstange@gmail.com> <87inwcd6s7.fsf@gmail.com> Date: Tue, 12 Jul 2016 13:10:57 +0200 In-Reply-To: (Thomas Gleixner's message of "Mon, 11 Jul 2016 10:32:04 +0200 (CEST)") Message-ID: <87poqjm7r2.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.0.94 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Thomas Gleixner 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