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

[Marc Zyngier]

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)."

> Also the first mail says that timestamp_clock() might be
> unstable. Is this true only during the early boot or all
> the time?

With the current proposal, the instability period only exists until
sched_clock gets initialized, at which point it takes over and the
timestamping becomes stable.

Note that this is the arm64 implementation, and not something that is
currently guaranteed by just selecting CONFIG_ARCH_HAS_TIMESTAMP_CLOCK.

> 
> The timestamp helps to order the events. An unstable clock
> might be better than nothing during the boot. But it would
> look strange to use it all the time, especially when it was
> unrelated to any other clock used by the system.

And that's exactly what patch #3 implements. Once local_clock() returns
something significant, we use that.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...