From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751007AbdE3KHf convert rfc822-to-8bit (ORCPT ); Tue, 30 May 2017 06:07:35 -0400 Received: from relay1.mentorg.com ([192.94.38.131]:39734 "EHLO relay1.mentorg.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750867AbdE3KHe (ORCPT ); Tue, 30 May 2017 06:07:34 -0400 From: "Mirea, Bogdan-Stefan" To: Thomas Gleixner CC: "linux-kernel@vger.kernel.org" , "john.stultz@linaro.org" , "ore@pengutronix.de" , "kernel@pengutronix.de" , "Prarit Bhargava" , Peter Zijlstra , "Ingo Molnar" Subject: RE: [PATCH v3] Added "Preserve Boot Time Support" Thread-Topic: [PATCH v3] Added "Preserve Boot Time Support" Thread-Index: AQHS0IkH7rSEKTmQB0iYLoHoASPbyqH/Qr2AgALSzuD///7MAIABLAxggAMctQCABlkvUA== Date: Tue, 30 May 2017 10:07:21 +0000 Message-ID: <0e3740a22ba74803806fd2ed5e92d407@svr-ies-mbx-01.mgc.mentorg.com> References: <1495188983-47002-1-git-send-email-Bogdan-Stefan_mirea@mentor.com> <7927187342d24ae7bc77ba1997e0aa4b@svr-ies-mbx-01.mgc.mentorg.com> <1136356b7bf043aaae4982ee7865bbe2@svr-ies-mbx-01.mgc.mentorg.com> In-Reply-To: Accept-Language: en-US, en-IE Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [137.202.0.87] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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