From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753908AbbKJPlr (ORCPT ); Tue, 10 Nov 2015 10:41:47 -0500 Received: from mail-io0-f175.google.com ([209.85.223.175]:36825 "EHLO mail-io0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753778AbbKJPlp (ORCPT ); Tue, 10 Nov 2015 10:41:45 -0500 MIME-Version: 1.0 In-Reply-To: References: <1447156675-7418-1-git-send-email-stefano.stabellini@eu.citrix.com> <4372800.FdTygmcuyo@wuerfel> Date: Tue, 10 Nov 2015 07:41:44 -0800 Message-ID: Subject: Re: [PATCH v2 1/7] timekeeping: introduce __current_kernel_time64 From: John Stultz To: Thomas Gleixner Cc: Stefano Stabellini , Arnd Bergmann , "linux-arm-kernel@lists.infradead.org" , xen-devel@lists.xensource.com, Ian.Campbell@citrix.com, Peter Zijlstra , lkml , Ingo Molnar Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Nov 10, 2015 at 7:31 AM, Thomas Gleixner wrote: > On Tue, 10 Nov 2015, John Stultz wrote: >> On Tue, Nov 10, 2015 at 7:10 AM, Stefano Stabellini >> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: john.stultz@linaro.org (John Stultz) Date: Tue, 10 Nov 2015 07:41:44 -0800 Subject: [PATCH v2 1/7] timekeeping: introduce __current_kernel_time64 In-Reply-To: References: <1447156675-7418-1-git-send-email-stefano.stabellini@eu.citrix.com> <4372800.FdTygmcuyo@wuerfel> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, Nov 10, 2015 at 7:31 AM, Thomas Gleixner wrote: > On Tue, 10 Nov 2015, John Stultz wrote: >> On Tue, Nov 10, 2015 at 7:10 AM, Stefano Stabellini >> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Stultz Subject: Re: [PATCH v2 1/7] timekeeping: introduce __current_kernel_time64 Date: Tue, 10 Nov 2015 07:41:44 -0800 Message-ID: References: <1447156675-7418-1-git-send-email-stefano.stabellini@eu.citrix.com> <4372800.FdTygmcuyo@wuerfel> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Thomas Gleixner Cc: Stefano Stabellini , Arnd Bergmann , "linux-arm-kernel@lists.infradead.org" , xen-devel@lists.xensource.com, Ian.Campbell@citrix.com, Peter Zijlstra , lkml , Ingo Molnar List-Id: xen-devel@lists.xenproject.org On Tue, Nov 10, 2015 at 7:31 AM, Thomas Gleixner wrote: > On Tue, 10 Nov 2015, John Stultz wrote: >> On Tue, Nov 10, 2015 at 7:10 AM, Stefano Stabellini >> 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