linux-kernel - Re: [PATCH v2 1/2] timekeeping: Introduce a fast boot clock derived from fast monotonic clock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.20.1611222222370.3495@nanos>
Date:   Tue, 22 Nov 2016 23:29:49 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Joel Fernandes <joelaf@...gle.com>
cc:     linux-kernel@...r.kernel.org, Steven Rostedt <rostedt@...dmis.org>,
        John Stultz <john.stultz@...aro.org>,
        Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH v2 1/2] timekeeping: Introduce a fast boot clock derived
 from fast monotonic clock

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