Messages in this thread | ![/](/images/icornerl.gif) | | From | Pavel Tatashin <> | Date | Tue, 19 Jun 2018 17:25:56 -0400 | Subject | Re: [PATCH v10 2/7] time: sync read_boot_clock64() with persistent clock |
| |
On Tue, Jun 19, 2018 at 5:17 PM Thomas Gleixner <tglx@linutronix.de> wrote: > > On Fri, 15 Jun 2018, Pavel Tatashin wrote: > > > read_boot_clock64() returns a boot start timestamp from epoch. Some arches > > may need to access the persistent clock interface in order to calculate the > > epoch offset. The resolution of the persistent clock, however, might be > > low. Therefore, in order to avoid time discrepancies a new argument 'now' > > is added to read_boot_clock64() parameters. Arch may decide to use it > > instead of accessing persistent clock again. > > I kinda know what you are trying to say, but it's all but obvious. > > > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c > > index 4786df904c22..fb6898fab374 100644 > > --- a/kernel/time/timekeeping.c > > +++ b/kernel/time/timekeeping.c > > @@ -1502,9 +1502,13 @@ void __weak read_persistent_clock64(struct timespec64 *ts64) > > * Function to read the exact time the system has been started. > > * Returns a timespec64 with tv_sec=0 and tv_nsec=0 if unsupported. > > * > > + * Argument 'now' contains time from persistent clock to calculate offset from > > + * epoch. May contain zeros if persist ant clock is not available. > > What's a 'persist ant' ?
persistent, but I think spell checker decided that I was writing about some ants. :)
> > > + * > > * XXX - Do be sure to remove it once all arches implement it. > > */ > > -void __weak read_boot_clock64(struct timespec64 *ts) > > +void __weak __init read_boot_clock64(struct timespec64 *now, > > + struct timespec64 *ts) > > { > > ts->tv_sec = 0; > > ts->tv_nsec = 0; > > @@ -1535,7 +1539,7 @@ void __init timekeeping_init(void) > > } else if (now.tv_sec || now.tv_nsec) > > persistent_clock_exists = true; > > > > - read_boot_clock64(&boot); > > + read_boot_clock64(&now, &boot); > > This interface was already bolted on and this extension just makes it > worse. If 'now' is invalid then you need the same checks as after > read_persistent_clock() replicated along with conditionals of some sort. > > 'boot' time is used to adjust the wall to monotonic offset. So looking at > the math behind all of that: > > read_persistent_clock(&now); > read_boot_clock(&boot); > > tk_set_xtime(tk, now) > tk->xtime_sec = now.sec; > tk->xtime_nsec = now.nsec; > > tk_set_wall_to_mono(tk, -boot); > tk->wall_to_mono = boot; > tk->offs_real = -boot; > > timekeeping_update(tk) > tk->tkr_mono = tk->xtime + tk->wall_to_mono; > > tkr_mono.base is guaranteed to be >= 0. So you need to make sure that > > tk->xtime + tk->wall_to_mono >= 0 > > Yet another check which you need to do in that magic function. That's just > wrong. If this grows more users then they all have to copy the same sanity > checks over and over and half of them will be wrong. > > Fortunately there is only a single real user of read_boot_clock() in the > tree: S390. By virtue of being S390 there is no worry about any sanity > checks. It just works. > > ARM has the function, but there is no single subarch which actually > implements the thing, so this is easy to fix by removing it. > > So the right thing to do is the following: > > Provide a new function: > > void __weak read_persistent_wall_and_boot_offset(struct timespec64 *wall_time, > ktime_t *boot_offset) > { > read_persistent_clock(wall_time); > } > > Then add the new function to S390: > > void read_persistent_wall_and_boot_offset(struct timespec64 *wall_time, > ktime_t *boot_offset) > { > unsigned char clk[STORE_CLOCK_EXT_SIZE]; > struct timespec64 boot_time; > __u64 delta; > > read_persistent_clock(wall_time); > > delta = initial_leap_seconds + TOD_UNIX_EPOCH; > memcpy(clk, tod_clock_base, 16); > *(__u64 *) &clk[1] -= delta; > if (*(__u64 *) &clk[1] > delta) > clk[0]--; > ext_to_timespec64(clk, boot_time); > *boot_offset = timespec64_to_ns(timespec64_sub(wall_time, boot_time)); > } > > Then rework timekeeping_init(): > > timekeeping_init() > struct timespec64 wall_time, wall_to_mono, offs; > ktime_t boot_offset = 0; > > read_persistent_wall_and_boot_offset(&walltime, &boot_offset); > > if (!valid(walltime)) > boottime = wall_time.tv_sec = wall_time.tv_nsec = 0; > else > persistent_clock = true; > > if (boot_offset > timespec64_to_nsec(wall_time)) > offs.tv_sec = offs.tv_nsec = 0; > else > offs = ns_to_timespec64(boot_offset); > > wall_to_mono = timespec64_sub(offs, wall_time); > tk_set_wall_to_mono(tk, wall_time); > > > After that remove the read_boot_time() leftovers all over the place. And > then the x86 implementation boils down to: > > void read_persistent_wall_and_boot_offset(struct timespec64 *wall_time, > ktime_t *boot_offset) > { > x86_platform.get_wallclock(ts); > *boot_offset = sched_clock(); > } > > And doing it this way - by adding the extra math to the only architecture > on the planet which has sane timers - is the right thing to do because all > other architectures will have to fall back to tricks similar to x86 by > doing clocksource/schedclock based guestimation of the time where the > machine actually reached the kernel. > > Hmm?
Sounds good, I will redo this and the next patch with your suggestions.
Thank you, Pavel
| ![\](/images/icornerr.gif) |