From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754523AbbCQLZK (ORCPT ); Tue, 17 Mar 2015 07:25:10 -0400 Received: from casper.infradead.org ([85.118.1.10]:48336 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753576AbbCQLZH (ORCPT ); Tue, 17 Mar 2015 07:25:07 -0400 Date: Tue, 17 Mar 2015 12:24:53 +0100 From: Peter Zijlstra To: John Stultz Cc: =?utf-8?B?UGF3ZcWC?= Moll , Adrian Hunter , Ingo Molnar , Stephane Eranian , lkml , Arnaldo Carvalho de Melo , David Ahern , =?iso-8859-1?Q?Fr=E9d=E9ric?= Weisbecker , Jiri Olsa , Namhyung Kim , Paul Mackerras , Thomas Gleixner , Steven Rostedt , Sonny Rao , "ak@linux.intel.com" , vincent.weaver@maine.edu Subject: Re: [RFC][PATCH 1/2] time: Add ktime_get_mono_raw_fast_ns() Message-ID: <20150317112453.GK21418@twins.programming.kicks-ass.net> References: <20150220142930.013968488@infradead.org> <20150220143754.792503379@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Feb 20, 2015 at 11:49:49AM -0800, John Stultz wrote: > On Fri, Feb 20, 2015 at 6:29 AM, Peter Zijlstra wrote: > > @@ -489,7 +512,12 @@ static void timekeeping_update(struct ti > > memcpy(&shadow_timekeeper, &tk_core.timekeeper, > > sizeof(tk_core.timekeeper)); > > > > - update_fast_timekeeper(&tk->tkr); > > + update_fast_timekeeper(&tk_fast_mono, &tk->tkr); > > + > > + tkr = tk->tkr; > > + tkr.mult = tk->tkr.clock->mult; > > + tkr.shift = tk->tkr.clock->shift; > > + update_fast_timekeeper(&tk_fast_mono_raw, &tkr); > > So this is sort of sneaky and subtle here, which will surely cause > problems later on. You're copying the original mult/shift pair into a > copy of the tkr, so you get similar results from timekeeping_get_ns() > as you'd want from timekeeping_get_ns_raw(). This results in multiple > ways of getting the raw clock. > > I think it would be better to either add a new tkr structure for the > raw clock in the timekeeper, so you can directly copy it over, or > extend the tkr structure so it can contain the raw values as well. > > Also, there's no real reason to have fast/non-fast versions of the raw > clock. Since it isn't affected by frequency changes, it can't have the > inconsistency issues the monotonic clock can see (which are documented > in the comment near ktime_get_mono_fast_ns()). So we can probably > condense these and avoid duplicative code. So I was looking at this again; and I think we do indeed need a second way to read mono_raw. So the immediate problem is the tk_core.seq loop around timekeeping_get_ns_raw(); if the NMI happens during the write side of that we're stuck. Now the reason we need that seqcount is because of tk->tkr.cycle_last, which, afaict, is the only bit that mono_raw needs that changes. With the possible exception of the suspend/resume paths; we also need to deal with NMIs requesting time during those. And we need this silly cycle_last business because of short clocks, which is something the generic code needs to deal with. And because we have a bit of an error margin on that we can indeed use a 'stale' cycle_last. As to the above suggestions: - we cannot add another tkr structure to struct timekeeper because that would duplicate a number of fields, including the above mentioned cycle_last, and having two of those is bound to cause confusion; and, - we cannot extend tkr because that would make it overflow the cacheline. So as it is, I do not see any other way than the already proposed patch; we even need the update from timekeeping_update() because we need an up-to-date cycle_last. So all I can really do is write a better changelog.