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]

Date:   Tue, 22 Nov 2016 16:26:52 -0800
From:   Joel Fernandes <joelaf@...gle.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <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 Tue, Nov 22, 2016 at 4:23 PM, Joel Fernandes <joelaf@...gle.com> wrote:
> Hi Thomas,
>
> On Tue, Nov 22, 2016 at 2:29 PM, Thomas Gleixner <tglx@...utronix.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.

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