Message-ID: <alpine.DEB.2.20.1705261030060.1902@nanos>
Date:   Fri, 26 May 2017 12:04:30 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     "Mirea, Bogdan-Stefan" <Bogdan-Stefan_Mirea@...tor.com>
cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "john.stultz@...aro.org" <john.stultz@...aro.org>,
        "ore@...gutronix.de" <ore@...gutronix.de>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        Prarit Bhargava <prarit@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>
Subject: RE: [PATCH v3] Added "Preserve Boot Time Support"

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

   

Powered by blists - more mailing lists