Re: [Xen-devel] [PATCH v2 1/7] timekeeping: introduce __current_kernel_time64

[John Stultz <john.stultz@xxxxxxxxxx>]

On Tue, Nov 10, 2015 at 7:31 AM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> On Tue, 10 Nov 2015, John Stultz wrote:
>> On Tue, Nov 10, 2015 at 7:10 AM, Stefano Stabellini
>> <stefano.stabellini@xxxxxxxxxxxxx> wrote:
>> > On Tue, 10 Nov 2015, Arnd Bergmann wrote:
>> >> On Tuesday 10 November 2015 11:57:49 Stefano Stabellini wrote:
>> >> > __current_kernel_time64 returns a struct timespec64, without taking the
>> >> > xtime lock. Mirrors __current_kernel_time/current_kernel_time.
>> >> >
>> >>
>> >> Actually it doesn't mirror __current_kernel_time/current_kernel_time
>> >>
>> >> > diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
>> >> > index ec89d84..b5802bf 100644
>> >> > --- a/include/linux/timekeeping.h
>> >> > +++ b/include/linux/timekeeping.h
>> >> > @@ -19,7 +19,8 @@ extern int do_sys_settimeofday(const struct timespec 
>> >> > *tv,
>> >> >   */
>> >> >  unsigned long get_seconds(void);
>> >> >  struct timespec64 current_kernel_time64(void);
>> >> > -/* does not take xtime_lock */
>> >> > +/* do not take xtime_lock */
>> >> > +struct timespec64 __current_kernel_time64(void);
>> >> >  struct timespec __current_kernel_time(void);
>> >>
>> >> Please change __current_kernel_time into a static inline function
>> >> while you are introducing the new one, to match the patch description ;-)
>> >
>> > The implementation is:
>> >
>> >         struct timekeeper *tk = &tk_core.timekeeper;
>> >
>> >         return timespec64_to_timespec(tk_xtime(tk));
>> >
>> > which cannot be easily made into a static inline, unless we start
>> > exporting tk_core.
>>
>> So the timekeeper is passed to the notifier. So you probably want something 
>> like
>>
>> struct timespec64 __current_kernel_time64(struct timekeeper *tk)
>> {
>>  return timespec64_to_timespec(tk_xtime(tk));
>> }
>>
>> Then you can cast the priv pointer in the notifier to a timekeeper and
>> use it that way?
>
> Err no. Look at commit 8758a240e2d74c5932ab51a73377e6507b7fd441
>
> i.e. Add the new 64bit function and make the existing one a static
> inline which does the timespec64 to timespec conversion.

So yea. The style there is what should be done.

I'm sort of objecting to a different issue, where the
__current_kernel_time() implementation probably shouldn't be grabbing
the tk_core.timekeeper directly, and instead should take a passed
pointer to a timekeeper. The vdso/pv_clock usage should have a
timekeeper passed to them that they could use.

There's one useage in kdb thats maybe problematic, so maybe this will
need a deeper cleanup.

thanks
-john

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel