Date: Tue, 22 Nov 2016 23:29:49 +0100 (CET)
From: Thomas Gleixner <tglx@linutronix.de>
To: Joel Fernandes <joelaf@google.com>
cc: linux-kernel@vger.kernel.org, Steven Rostedt <rostedt@goodmis.org>,
        John Stultz <john.stultz@linaro.org>, Ingo Molnar <mingo@redhat.com>
Subject: Re: [PATCH v2 1/2] timekeeping: Introduce a fast boot clock derived
 from fast monotonic clock
In-Reply-To: <1479766370-31311-1-git-send-email-joelaf@google.com>
Message-ID: <alpine.DEB.2.20.1611222222370.3495@nanos>
References: <1479766370-31311-1-git-send-email-joelaf@google.com>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1287
Lines: 36

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?

> +	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.

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.

Thanks,

	tglx