lkml.org 
[lkml]   [2017]   [Aug]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps
On Mon, Aug 07, 2017 at 09:52:10AM -0700, John Stultz wrote:
> On Mon, Aug 7, 2017 at 8:52 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> > +u64 ktime_get_real_log_ts(u64 *offset_real)
> > +{
> > + *offset_real = ktime_to_ns(tk_core.timekeeper.offs_real);
> > +
> > + if (timekeeping_active)
> > + return ktime_get_mono_fast_ns();
> > + else
> > + return local_clock();
> > +}
> > +
> > +u64 ktime_get_boot_log_ts(void)
> > +{
> > + if (timekeeping_active)
> > + return ktime_get_boot_fast_ns();
> > + else
> > + return local_clock();
> > +}
>
> This feels a little tacked on and duplicative. I'd suggest having one
> function that returns the offset_real and offset_boot or have a
> separate get_mono_log_ts() so its at least consistent. Additionally,
> in the commit message, you call out the lack of locking between
> grabing the offs_real and calling get_mono_fast_ns(), but I worry it
> may be particularly problematic on 32bit systems, and you don't have
> any notes in the actual code about it (it looks like an oversight).
>
> Also, when timekeeping_active flips over, and we change from local
> clock to the specified clock, do we see a discontinuity in the log? I
> know folks use to gripe about that back in the day.

Yeah, yuck, this smells. Please don't mix clocks like this.

I expect all you want is to avoid the explosion you get from calling the
fast things too early, right? Please use the below, which should result
in it returning 0.

---
kernel/time/timekeeping.c | 47 +++++++++++++++++++++++++++++++++--------------
1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index cedafa008de5..d111039e0245 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -60,8 +60,39 @@ struct tk_fast {
struct tk_read_base base[2];
};

-static struct tk_fast tk_fast_mono ____cacheline_aligned;
-static struct tk_fast tk_fast_raw ____cacheline_aligned;
+/* Suspend-time cycles value for halted fast timekeeper. */
+static u64 cycles_at_suspend;
+
+static u64 dummy_clock_read(struct clocksource *cs)
+{
+ return cycles_at_suspend;
+}
+
+static struct clocksource dummy_clock = {
+ .read = dummy_clock_read,
+};
+
+static struct tk_fast tk_fast_mono ____cacheline_aligned = {
+ .base = {
+ (struct tk_read_base){
+ .clock = &dummy_clock,
+ },
+ (struct tk_read_base){
+ .clock = &dummy_clock,
+ },
+ },
+};
+
+static struct tk_fast tk_fast_raw ____cacheline_aligned = {
+ .base = {
+ (struct tk_read_base){
+ .clock = &dummy_clock,
+ },
+ (struct tk_read_base){
+ .clock = &dummy_clock,
+ },
+ },
+};

/* flag for if timekeeping is suspended */
int __read_mostly timekeeping_suspended;
@@ -477,18 +508,6 @@ u64 notrace ktime_get_boot_fast_ns(void)
}
EXPORT_SYMBOL_GPL(ktime_get_boot_fast_ns);

-/* Suspend-time cycles value for halted fast timekeeper. */
-static u64 cycles_at_suspend;
-
-static u64 dummy_clock_read(struct clocksource *cs)
-{
- return cycles_at_suspend;
-}
-
-static struct clocksource dummy_clock = {
- .read = dummy_clock_read,
-};
-
/**
* halt_fast_timekeeper - Prevent fast timekeeper from accessing clocksource.
* @tk: Timekeeper to snapshot.
\
 
 \ /
  Last update: 2017-08-07 19:17    [W:0.159 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site