Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[John Stultz]

On Mon, Aug 7, 2017 at 11:04 AM, Prarit Bhargava <prarit@xxxxxxxxxx> wrote:
> On 08/07/2017 12:52 PM, John Stultz wrote:
>> Still not quite following why you're updating all the defconfigs. I'd
>> make sure the Kconfig default settings are right, and leave updating
>> the defconfig to arch/device maintainers. It adds a lot of noise to
>> the patch.
>
> Hmm ... I thought it was up to the patch submitter to make sure that
> 'make defconfig' still worked?  Are you sure I can leave that broken?
>
> /me *really* doesn't want to get yelled at by every arch maintainer.

No. Don't break systems, but at the same time, can't you use the
default value in Kconfig to set it properly so the old defconfig
settings don't really matter?

Apologies if I've not followed the issue properly, but it is odd, as
I'm not sure I can think of a patch I've seen before that had so much
defconfig noise in it.  Again, I've not looked into it closely, so it
may just be my own ignorance, but it makes me suspect there is a
better way.


>>> +u64 ktime_get_real_log_ts(u64 *offset_real)
>>> +{
>>> +       *offset_real = ktime_to_ns(tk_core.timekeeper.offs_real);
>>> +
>>> +       if (timekeeping_active)
>>> +               return ktime_get_mono_fast_ns();
>>> +       else
>>> +               return local_clock();
>>> +}
>>> +
>>> +u64 ktime_get_boot_log_ts(void)
>>> +{
>>> +       if (timekeeping_active)
>>> +               return ktime_get_boot_fast_ns();
>>> +       else
>>> +               return local_clock();
>>> +}
>>
>> This feels a little tacked on and duplicative.  I'd suggest having one
>> function that returns the offset_real and offset_boot or have a
>> separate get_mono_log_ts() so its at least consistent.
>
> I have a better suggestion that I was toying with -- exporting
> timekeeping_active and using the existing ktime_get_mono|boot|real|_fast_ns()
> functions with a function pointer would simplify this code.

Yea. That sounds cleaner.


>> Additionally,
>> in the commit message, you call out the lack of locking between
>> grabing the offs_real and calling get_mono_fast_ns(), but I worry it
>> may be particularly problematic on 32bit systems, and you don't have
>> any notes in the actual code about it (it looks like an oversight).
>>
>
> I was told to move that comment to the kdoc description by Luis R. Rodriguez.

You can have it both ways. :)  Its just without any mention near the
function, it just looks buggy (well, because it technically is), so
either we should fix it properly or at least document that its
intentionally buggy (which again, doesn't feel great here - we already
have lots of caveats around the _fast_ns() accessors, so this is
layering subtle breakage on top of other subtle behavior).


>> Also, when timekeeping_active flips over, and we change from local
>> clock to the specified clock, do we see a discontinuity in the log? I
>> know folks use to gripe about that back in the day.
>>
>
> I have tested this across many systems and haven't seen a discontinunity
> yet.  I've done both large and small cpu footprint systems and haven't seen
> anything like that.

Ok.

thanks
-john