Re: [PATCH v3 3/3] arm64: Early boot time stamps

[Marc Zyngier]

On Thu, 03 Jan 2019 19:58:25 +0000,
Pavel Tatashin <pasha.tatashin@xxxxxxxxxx> wrote:
> 
> > I still think this approach is flawed. You provide the kernel with a
> > potentially broken sched_clock that may jump back and forth until the
> > workaround kicks in. Nobody expects this.
> >
> > Instead, I'd suggest you allow for a something other than local_clock()
> > to be used for the time stamping until a properly working sched_clock
> > gets registered.
> >
> > This way, you'll only impact the timestamps when running on a broken system.
> 
> I think, given that on other platforms sched_clock() is already used
> early, it is not a good idea to invent a different clock just for time
> stamps.

Square pegs vs round holes. Mimicking other architectures isn't always
the right thing to do when faced with a different problem. We put a
lot of effort in working around timer errata for a good reason, and
feeding the rest of the system bogus timing information doesn't sound
great.

> We could limit arm64 approach only for chips where cntvct_el0 is
> working: i.e. frequency is known, and the clock is stable, meaning
> cannot go backward. Perhaps we would start early clock a little later,
> but at least it will be available for the sane chips. The only
> question, where during boot time this is known.

How do you propose we do that? Defective timers can be a property of
the implementation, of the integration, or both. In any case, it
requires firmware support (DT, ACPI). All that is only available quite
late, and moving it earlier is not easily doable.

> Another approach is to modify sched_clock() in
> kernel/time/sched_clock.c to never return backward value during boot.
> 
> 1. Rename  current implementation of sched_clock() to sched_clock_raw()
> 2. New sched_clock() would look like this:
> 
> u64 sched_clock(void)
> {
>    if (static_branch(early_unstable_clock))
>       return sched_clock_unstable();
>    else
>       return sched_clock_raw();
> }
> 
> 3. sched_clock_unstable() would look like this:
> 
> u64 sched_clock_unstable(void)
> {
> again:
>   static u64 old_clock;
>   u64 new_clock = sched_clock_raw();
>   static u64 old_clock_read =   READ_ONCE(old_clock);
>   /* It is ok if time does not progress, but don't allow to go backward */
>   if (new_clock < old_clock_read)
>     return old_clock_read;
>    /* update the old_clock value */
>    if (cmpxchg64(&old_clock, old_clock_read, new_clock) != old_clock_read)
>       goto again;
>    return new_clock;
> }

You now have an "unstable" clock that is only allowed to move forward,
until you switch to the real one. And at handover time, anything can
happen.

It is one thing to allow for the time stamping to be imprecise. But
imposing the same behaviour on other parts of the kernel that have so
far relied on a strictly monotonic sched_clock feels like a bad idea.

What I'm proposing is that we allow architectures to override the hard
tie between local_clock/sched_clock and kernel log time stamping, with
the default being of course what we have today. This gives a clean
separation between the two when the architecture needs to delay the
availability of sched_clock until implementation requirements are
discovered. It also keep sched_clock simple and efficient.

To illustrate what I'm trying to argue for, I've pushed out a couple
of proof of concept patches here[1]. I've briefly tested them in a
guest, and things seem to work OK.

Thanks,

	M.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=arm64/tsclock

-- 
Jazz is not dead, it just smell funny.