Re: [PATCH 2/2 v7] printk: Add monotonic, boottime, and realtime timestamps

[Prarit Bhargava]

On 08/17/2017 11:30 AM, Mark Salyzyn wrote:
> On 08/17/2017 06:15 AM, Prarit Bhargava wrote:
>> printk.time=1/CONFIG_PRINTK_TIME=1 adds a unmodified local hardware clock
>> timestamp to printk messages.  The local hardware clock loses time each
>> day making it difficult to determine exactly when an issue has occurred in
>> the kernel log, and making it difficult to determine how kernel and
>> hardware issues relate to each other in real time.
> Congrats, greatly eases merges into older kernels, and has eliminated the churn
> this could place on all the configured systems out there.
> 
> Sadly, one of my suggestions did not quite go the way I expected ;-} easy to
> correct, and fix (I missed a spot in my original suggestion, as code changed
> underfoot over the set ;-/). (see bottom)

Added.  I will send out v8 shortly.

> 
> <discussion type="maybe out of the scope for this patch">
> 
> I am not convinced that user space is entirely at a disadvantage with this
> 'feature' enabled. Before interpreting it can read
> /sys/module/printk/parameters/time, then sniff for the flowing content for time
> breaks (watch for printk: timestamp set to <string>). Of course, the value in
> 'time' is current, so it would be _wrong_ during flow of previous content until
> the first time break shows up if it really was being switched that often.
> 
> (echo local ; echo disabled ; echo boottime ; echo monotonic ; echo realtime ;
> echo local ) > /sys/module/printk/parameters/time
> 
> [  473.589169] printk: timestamp set to local
> printk: timestamp set to disabled
> [  473.545384] printk: timestamp set to boottime
> [  473.549924] printk: timestamp set to monotonic
> [1502957708.055265] printk: timestamp set to realtime
> [  473.612024] printk: timestamp set to local
> 
> A 'fix' would be to add a letter after the timestamp if not local. For example:
> 
> [  473.589169] printk: timestamp set to local
> printk: timestamp set to disabled
> [  473.545384b] printk: timestamp set to boottime
> [  473.549924m] printk: timestamp set to monotonic
> [1502957708.055265U] printk: timestamp set to realtime
> [  473.612024] printk: timestamp set to local
> 
> (I used U instead of r, because it is actually UTC, and did not add 'l' because
> it is a long standing default)
> 
> But there would be concern over a change in time format API, so maybe it should
> be relegated to a CONFIG_PRINTK_TIME_DEBUG 'feature' only to add the timebase
> letters?

... then you wouldn't be able to use dmesg, etc., to get proper timestamp
conversions. :(  I'm not going to code dmesg for that because, as you point out
it is outside of the scope of this patch.  Maybe we can do something in future
for this?

P.