Re: [PATCH 1/3] printk: Allow architecture-specific timestamping function

[Russell King - ARM Linux admin]

On Mon, Jul 22, 2019 at 01:47:57PM +0100, Marc Zyngier wrote:
> On 22/07/2019 12:25, Petr Mladek wrote:
> > On Mon 2019-07-22 11:33:28, Marc Zyngier wrote:
> >> printk currently relies on local_clock to time-stamp the kernel
> >> messages. In order to allow the timestamping (and only that)
> >> to be overridden by architecture-specific code, let's declare
> >> a new timestamp_clock() function, which gets used by the printk
> >> code. Architectures willing to make use of this facility will
> >> have to define CONFIG_ARCH_HAS_TIMESTAMP_CLOCK.
> >>
> >> The default is of course to return local_clock(), so that the
> >> existing behaviour stays unchanged.
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> >> ---
> >>  include/linux/sched/clock.h | 13 +++++++++++++
> >>  kernel/printk/printk.c      |  4 ++--
> >>  2 files changed, 15 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/linux/sched/clock.h b/include/linux/sched/clock.h
> >> index 867d588314e0..3cf4b2a8ce18 100644
> >> --- a/include/linux/sched/clock.h
> >> +++ b/include/linux/sched/clock.h
> >> @@ -98,4 +98,17 @@ static inline void enable_sched_clock_irqtime(void) {}
> >>  static inline void disable_sched_clock_irqtime(void) {}
> >>  #endif
> >>  
> >> +#ifdef CONFIG_ARCH_HAS_TIMESTAMP_CLOCK
> >> +/* Special need architectures can provide their timestamping function */
> > 
> > The commit message and the above comment should be more specific
> > about what are the special needs.
> > 
> > It must be clear how and why the clock differs from the other
> > clocks, especially from lock_clock().
> 
> Fair enough. How about something along the lines of:
> 
> "An architecture can override the timestamp clock (which defaults to
> local_clock) if local_clock is not significant early enough (sched_clock
> being available too late)."

We have:
1) the standard clocksource
2) the sched_clock, which is _supposed_ to be initialised early
3) persistent_clock

Do we really need another clock?

Why not initialise sched_clock() early (as in, before sched_init(),
which is where the first sched_clock() read occurs) ?

We've already been around the argument that sched_clock() apparently
can't be initialised early enough (which is the argument I had in reply
to the sched_clock() situation on ARM32) then how does inventing
timestamp_clock() solve this problem?

Wouldn't timestamp_clock() also suffer from the very same "we can't
initialise it early enough" issue, and it'll just be setup along side
clocksources, just like sched_clock() has become?

I fail to see what adding yet another architecture specific clock
implementation buys, apart from yet more complexity.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up