RE: [PATCH v3] Added "Preserve Boot Time Support"

[Mirea, Bogdan-Stefan]

Hello Thomas,

On Friday, May 26, 2017 1:05 PM, Thomas Gleixner wrote: 
> On Wed, 24 May 2017, Mirea, Bogdan-Stefan wrote:
> > I am thinking about using timekeeping_inject_sleeptime64(delta) hook to
> 
> That's not a hook. It's a regular function. Please use proper technical
> terms.
> 
> > add a delta time at boot to the CLOCK_BOOTTIME, but the problem that
> > arise here is that this hook is intended to be used on rtc_resume()
> > code (to add the time spent in suspend to CLOCK_BOOTTIME).
> 
> It's not the intention. It _IS_ used for injecting the suspended time.
> 
> > >From my point of view this will do the trick even if the
> > CONFIG_BOOT_TIME_PRESERVE will then depend on PM_SLEEP &&
> RTC_HCTOSYS
> > (needed by timekeeping_inject_sleeptime64 to work).
> 
> First of all there is no point in having this extra CONFIG switch. This can
> be made unconditionally and depend on a kernel command line argument.
> 
> What has this to do with PM_SLEEP and RTC_HCTOSYS? Just because that
> function is currently only available when these config switches are enabled
> it does not mean that it cannot be made available unconditionally.
> 
> What you are trying to do is to cram that new functionality into the
> existing code without actually spending time on thinking about a proper
> design.
> 
> Let's talk about the objective.
> 
>  1) Insert time from system start (power on, hardware reset) to kernel start
>     (timekeeping init) into CLOCK_BOOTTIME, if the system can read out this
>     value.
> 
>  2) Coordinate sched_clock and CLOCK_BOOTTIME
> 
> #1 timekeeping_inject_sleeptime64() provides the required functionality
>    already. We can rename that function to reflect that it's used for both
>    (injecting suspend time and injecting system start time).
> 
>    The more interesting question is how to provide a generic mechanism to
>    retrieve that value. There are two possibilities:
> 
>    A) Value is stored by the early boot code and retrieved from there
> 
>    B) Value is made available via a clocksource after initializing and
>       registering it.
> 
>    #A can be solved by having a weak function
> 
> 	u64 __weak read_system_starttime(void)
> 	{
> 		return 0;
> 	}
> 
>      and let architectures implementing it when required. Though I doubt
>      that we need that right away.
> 
>    #B can be made generic in a very simple way
> 
>      Add a flag CLOCK_SOURCE_STARTTIME to the clocksource flags and let the
>      core code use that flag exactly once to inject the system start time
>      into CLOCK_BOOTTIME.
> 
> #2 depends on #1
> 
>    When the timekeeping core injects the system start time it can tell the
>    sched_clock core to use that offset as well.
> 
>    But, that does not solve the problem that sched_clock and CLOCK_BOOTTIME
>    will drift apart over time which makes the whole thing moot.
> 
>    There was a patch set floating around, which made the clock used for
>    printk selectable so it can actually use CLOCK_MONOTONIC or
>    CLOCK_BOOTTIME via ktime_get_[mono|boot]_fast_ns(), but that never got
>    merged.
> 
>    So instead of trying to provide a half correct solution which does not
>    allow you to coordinate dmesg timestamps, uptime, tracer timestamps
>    etc. reliably due to drift, I rather have that dmesg selectable
>    timestamping patch merged and provide coordinated timestamps that way.
>    (Cc'ed Prarit, who was working on that)
> 
> Thanks,
> 
> 	tglx
> 
> 
Thank you for your valuable and complete answer, I will consider everything you
said and update accordingly. I'll also notify you when this is ready.

Kind Regards,
Bogdan