MIME-Version: 1.0
In-Reply-To: <alpine.DEB.2.20.1611222222370.3495@nanos>
References: <1479766370-31311-1-git-send-email-joelaf@google.com> <alpine.DEB.2.20.1611222222370.3495@nanos>
From: Joel Fernandes <joelaf@google.com>
Date: Tue, 22 Nov 2016 16:23:51 -0800
Message-ID: <CAJWu+oo9ye_rtW943CDcWmTU3A+8L1JS4ZSc4R+6qWHwcqRozg@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] timekeeping: Introduce a fast boot clock derived
 from fast monotonic clock
To: Thomas Gleixner <tglx@linutronix.de>
Cc: LKML <linux-kernel@vger.kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        John Stultz <john.stultz@linaro.org>, Ingo Molnar <mingo@redhat.com>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2393
Lines: 63

Hi Thomas,

On Tue, Nov 22, 2016 at 2:29 PM, Thomas Gleixner <tglx@linutronix.de> 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.
 */
u64 ktime_get_boot_fast_ns(void)
{
        struct timekeeper *tk = &tk_core.timekeeper;
        return __ktime_get_fast_ns(&tk_fast_mono) + tk->offs_boot;
}
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