2017-12-06 21:50:46

by Jason Wessel

[permalink] [raw]
Subject: Re: [PATCH] kdb: use __ktime_get_real_seconds instead of __current_kernel_time

On 10/12/2017 09:06 AM, Arnd Bergmann wrote:
> kdb is the only user of the __current_kernel_time() interface, which is
> not y2038 safe and should be removed at some point.
>
> The kdb code also goes to great lengths to print the time in a
> human-readable format from 'struct timespec', again using a non-y2038-safe
> re-implementation of the generic time_to_tm() code.
>
> Using __current_kernel_time() here is necessary since the regular
> accessors that require a sequence lock might hang when called during the
> xtime update. However, this is safe in the particular case since kdb is
> only interested in the tv_sec field that is updated atomically.
>
> In order to make this y2038-safe, I'm converting the code to the generic
> time64_to_tm helper, but that introduces the problem that we have no
> interface like __current_kernel_time() that provides a 64-bit timestamp
> in a lockless, safe and architecture-independent way. I have multiple
> ideas for how to solve that:
>
> - __ktime_get_real_seconds() is lockless, but can return
> incorrect results on 32-bit architectures in the special case that
> we are in the process of changing the time across the epoch, either
> during the timer tick that overflows the seconds in 2038, or while
> calling settimeofday.
>
> - ktime_get_real_fast_ns() would work in this context, but does
> require a call into the clocksource driver to return a high-resolution
> timestamp. This may have undesired side-effects in the debugger,
> since we want to limit the interactions with the rest of the kernel.
>
> - Adding a ktime_get_real_fast_seconds() based on tk_fast_mono
> plus tkr->base_real without the tk_clock_read() delta. Not sure about
> the value of adding yet another interface here.
>
> - Changing the existing ktime_get_real_seconds() to use
> tk_fast_mono on 32-bit architectures rather than xtime_sec. I think
> this could work, but am not entirely sure if this is an improvement.
>
> I picked the first of those for simplicity here. It's technically
> not correct but probably good enough as the time is only used for the
> debugging output and the race will likely never be hit in practice.
> Another downside is having to move the declaration into a public header
> file.
>
> Let me know if anyone has a different preference.


It all seems reasonable to me. Separately I created the same patch because I didn't see this mail first. The only difference was that I added a comment about the __ktime_get_real_seconds() not taking the lock because it was done that way in other places in the header file.

===
extern time64_t ktime_get_real_seconds(void);
+/* does not take xtime_lock */
+extern time64_t __ktime_get_real_seconds(void);
===

Acked-by: Jason Wessel <[email protected]>

Thanks for your work on the 2038 problems. :-)

Cheers,
Jason.