Web lists-archives.com

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 <jason.wessel@xxxxxxxxxxxxx>

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

Cheers,
Jason.