Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755490AbcKWA0y (ORCPT ); Tue, 22 Nov 2016 19:26:54 -0500 Received: from mail-vk0-f41.google.com ([209.85.213.41]:34036 "EHLO mail-vk0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751433AbcKWA0x (ORCPT ); Tue, 22 Nov 2016 19:26:53 -0500 MIME-Version: 1.0 In-Reply-To: References: <1479766370-31311-1-git-send-email-joelaf@google.com> From: Joel Fernandes Date: Tue, 22 Nov 2016 16:26:52 -0800 Message-ID: Subject: Re: [PATCH v2 1/2] timekeeping: Introduce a fast boot clock derived from fast monotonic clock To: Thomas Gleixner Cc: LKML , Steven Rostedt , John Stultz , Ingo Molnar Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2684 Lines: 74 On Tue, Nov 22, 2016 at 4:23 PM, Joel Fernandes wrote: > Hi Thomas, > > On Tue, Nov 22, 2016 at 2:29 PM, Thomas Gleixner wrote: >> >> On Mon, 21 Nov 2016, Joel Fernandes wrote: >> > @@ -56,6 +56,12 @@ static struct timekeeper shadow_timekeeper; >> > struct tk_fast { >> > seqcount_t seq; >> > struct tk_read_base base[2]; >> > + >> > + /* >> > + * first dimension is based on lower seq bit, >> > + * second dimension is for offset type (real, boot, tai) >> > + */ >> >> Can you figure out why there are comments above the struct which explain >> the members in Kernel Doc format and not randomly formatted comments inside >> the struct definition? > > Ok sorry. I can move the comments before the function in the prescribed format. > >> > + ktime_t offsets[2][TK_OFFS_MAX]; >> >> This bloats fast_tk_raw for no value, along with the extra stores in the >> update function for fast_tk_raw which will never use that offset stuff. >> >> Aside of that, I really have to ask the question whether it's really >> necessary to add this extra bloat in storage, update and readout code for >> something which is not used by most people. >> >> What's wrong with adding a tracepoint into the boot offset update function >> and let perf or the tracer inject the value of the boot offset into the >> trace data when starting. The time adjustment can be done in >> postprocessing. > > I agree we're bloating this and probably not very acceptable. > tracepoint adds additional complexity and out of tree patches which > we'd like to avoid. Would you be Ok if we added a relatively simple > function like below that could do the job and not bloat the optimal > case? > > /* > * Fast and NMI safe access to boot time. It may be slightly out of date > * as we're accessing offset without seqcount writes, but is safe to access. s/writes/reads/ > */ > u64 ktime_get_boot_fast_ns(void) > { > struct timekeeper *tk = &tk_core.timekeeper; > return __ktime_get_fast_ns(&tk_fast_mono) + tk->offs_boot; I meant, __ktime_get_fast_ns(&tk_fast_mono) + ktime_to_ns(tk->offs_boot). Was just showing the idea... Joel > } > EXPORT_SYMBOL_GPL(ktime_get_boot_fast_ns); > > >> That should be sufficient for tracing suspend/resume behaviour. If there is >> not a really convincing reason for having that as a real trace clock then I >> prefer to avoid that extra stuff. > > Several clocks are accessible just by simple writing of clock name to > trace_clock in debugfs. This is really cool and doesn't require any > out of tree patches or post processing complexity. > > Thanks, > Joel