| Message ID | 1447156675-7418-1-git-send-email-stefano.stabellini@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 10, 2015 at 11:57:49AM +0000, Stefano Stabellini wrote: > __current_kernel_time64 returns a struct timespec64, without taking the > xtime lock. Mirrors __current_kernel_time/current_kernel_time. It always helps if you include a reason why you want a patch.
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 ;-) Arnd
On Tue, 10 Nov 2015, Peter Zijlstra wrote: > On Tue, Nov 10, 2015 at 11:57:49AM +0000, Stefano Stabellini wrote: > > __current_kernel_time64 returns a struct timespec64, without taking the > > xtime lock. Mirrors __current_kernel_time/current_kernel_time. > > It always helps if you include a reason why you want a patch. You are right, sorry. I need to get the current_kernel_time from a pvclock_gtod callback function, which cannot take the lock again. On x86 we are just calling __current_kernel_time() (see arch/x86/xen/time.c:xen_pvclock_gtod_notify). I was introducing the same functionality on ARM, when the maintainers pointed out that it might be better to use struct timespec64 and related functions for future-proofness. To do that I need a version of __current_kernel_time which returns a struct timespec64.
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.
On Tue, Nov 10, 2015 at 7:10 AM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> 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? thanks -john
On Tue, 10 Nov 2015, John Stultz wrote: > On Tue, Nov 10, 2015 at 7:10 AM, Stefano Stabellini > <stefano.stabellini@eu.citrix.com> 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. Thanks, tglx
On Tue, Nov 10, 2015 at 7:31 AM, Thomas Gleixner <tglx@linutronix.de> wrote: > On Tue, 10 Nov 2015, John Stultz wrote: >> On Tue, Nov 10, 2015 at 7:10 AM, Stefano Stabellini >> <stefano.stabellini@eu.citrix.com> 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
On Tue, 10 Nov 2015, John Stultz wrote: > 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. That usage of __current_kernel_time() in that xen notifier is silly to begin with. The notifier gets already called with a pointer to the time keeper. That xen implementation just does not use it. We extract exactly that information in the vdso updates without calling back into the core code. So for solving that xen thing we do not need a 64 bit variant of __current_kernel_time() at all. The notifier has the pointer to the timekeeper and can just grab data from there. > There's one useage in kdb thats maybe problematic, so maybe this will > need a deeper cleanup. That one is silly as well. It only wants to know the seconds portion. Thanks, tglx
On Tue, 10 Nov 2015, Thomas Gleixner wrote: > On Tue, 10 Nov 2015, John Stultz wrote: > > 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. > > That usage of __current_kernel_time() in that xen notifier is silly to > begin with. The notifier gets already called with a pointer to the > time keeper. That xen implementation just does not use it. > > We extract exactly that information in the vdso updates without > calling back into the core code. So for solving that xen thing we do > not need a 64 bit variant of __current_kernel_time() at all. The > notifier has the pointer to the timekeeper and can just grab data from > there. Many thanks for the suggestion, I'll do that. Should I open code tk_xtime in the xen notifier, or should I export it in timekeeper_internal.h?
On Wednesday 11 November 2015 11:51:26 Stefano Stabellini wrote: > On Tue, 10 Nov 2015, Thomas Gleixner wrote: > > On Tue, 10 Nov 2015, John Stultz wrote: > > > 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. > > > > That usage of __current_kernel_time() in that xen notifier is silly to > > begin with. The notifier gets already called with a pointer to the > > time keeper. That xen implementation just does not use it. > > > > We extract exactly that information in the vdso updates without > > calling back into the core code. So for solving that xen thing we do > > not need a 64 bit variant of __current_kernel_time() at all. The > > notifier has the pointer to the timekeeper and can just grab data from > > there. > > Many thanks for the suggestion, I'll do that. > Should I open code tk_xtime in the xen notifier, or should I export it > in timekeeper_internal.h? tk_xtime is a 'static inline' function, I don't see a good way to make that accessible, and you really want the elements separately, so I'd open-code it without going through timespec64. Arnd
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); static inline struct timespec current_kernel_time(void) diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c index b1356b7..c1221c2 100644 --- a/kernel/time/timekeeping.c +++ b/kernel/time/timekeeping.c @@ -1881,6 +1881,13 @@ struct timespec __current_kernel_time(void) return timespec64_to_timespec(tk_xtime(tk)); } +struct timespec64 __current_kernel_time64(void) +{ + struct timekeeper *tk = &tk_core.timekeeper; + + return tk_xtime(tk); +} + struct timespec64 current_kernel_time64(void) { struct timekeeper *tk = &tk_core.timekeeper;
__current_kernel_time64 returns a struct timespec64, without taking the xtime lock. Mirrors __current_kernel_time/current_kernel_time. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> CC: arnd@arndb.de CC: john.stultz@linaro.org CC: tglx@linutronix.de CC: mingo@kernel.org CC: peterz@infradead.org --- include/linux/timekeeping.h | 3 ++- kernel/time/timekeeping.c | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-)