From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755231AbbBTULv (ORCPT ); Fri, 20 Feb 2015 15:11:51 -0500 Received: from bombadil.infradead.org ([198.137.202.9]:48140 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755002AbbBTULu (ORCPT ); Fri, 20 Feb 2015 15:11:50 -0500 Date: Fri, 20 Feb 2015 21:11:37 +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: <20150220201137.GE23367@worktop.ger.corp.intel.com> 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.22.1 (2013-10-16) 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: > > +u64 notrace ktime_get_mono_raw_fast_ns(void) > > +{ > > + struct tk_read_base *tkr; > > + unsigned int seq; > > + u64 now; > > + > > + do { > > + seq = raw_read_seqcount(&tk_fast_mono_raw.seq); > > + tkr = tk_fast_mono_raw.base + (seq & 0x01); > > + now = ktime_to_ns(tkr->base_mono) + timekeeping_get_ns(tkr); > > > So this doesn't look right. I think you want to use tk->base_raw and > timekeeping_get_ns_raw() here? No, the problem is that timekeeping_get_ns_raw() dereferences tkr->clock. The idea was to, like ktime_get_mono_fast_ns() only touch the _1_ cacheline. Clearly I've messed it up, but I didn't want it to go dereference tkr->clock and pull all kinds of stuff from that second line. > > + 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, OK, this then. > or > extend the tkr structure so it can contain the raw values as well. Can't it would push tk_fast over the _1_ cacheline. > 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. The typical timekeeping_get_ns_raw() still got a seqcount around it which can fail from NMI (inf loop for all). So we do nee the tk_fast dual copy seqcount thing just the same, also as per the above, cacheline cacheline cacheline :) But yes, I think you're right in that we should be able to condense that somewhat.