[PATCH] time: Revert ALWAYS_USE_PERSISTENT_CLOCK compile time optimizaitons

[PATCH] time: Revert ALWAYS_USE_PERSISTENT_CLOCK compile time optimizaitons

[John Stultz]

Kay Sievers noted that the ALWAYS_USE_PERSISTENT_CLOCK config,
which enables some minor compile time optimization to avoid
uncessary code in mostly the suspend/resume path could cause
problems for userland.

In particular, the dependency for RTC_HCTOSYS on
!ALWAYS_USE_PERSISTENT_CLOCK, which avoids setting the time
twice and simplifies suspend/resume, has the side effect
of causing the /sys/class/rtc/rtcN/hctosys flag to always be
zero, and this flag is commonly used by udev to setup the
/dev/rtc symlink to /dev/rtcN, which can cause pain for
older applications.

While the udev rules could use some work to be less fragile,
breaking userland should strongly be avoided. Additionally
the compile time optimizations are fairly minor, and the code
being optimized is likely to be reworked in the future, so
lets revert this change.

I suspect this will miss 3.9-final, so marking it for 3.9-stable.

Cc: stable <stable@vger.kernel.org>
CC: Kay Sievers <kay@vrfy.org>
Cc: Feng Tang <feng.tang@intel.com>
Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
CC: Thomas Gleixner <tglx@linutronix.de>
Reported-by: Kay Sievers <kay@vrfy.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 arch/x86/Kconfig     |    1 -
 drivers/rtc/Kconfig  |    2 --
 include/linux/time.h |    4 ----
 kernel/time/Kconfig  |    5 -----
 4 files changed, 12 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 15b5cef..6ef2a37 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -107,7 +107,6 @@ config X86
 	select GENERIC_CLOCKEVENTS_BROADCAST if X86_64 || (X86_32 && X86_LOCAL_APIC)
 	select GENERIC_TIME_VSYSCALL if X86_64
 	select KTIME_SCALAR if X86_32
-	select ALWAYS_USE_PERSISTENT_CLOCK
 	select GENERIC_STRNCPY_FROM_USER
 	select GENERIC_STRNLEN_USER
 	select HAVE_CONTEXT_TRACKING if X86_64
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 79fbe38..9e95473 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -20,7 +20,6 @@ if RTC_CLASS
 config RTC_HCTOSYS
 	bool "Set system time from RTC on startup and resume"
 	default y
-	depends on !ALWAYS_USE_PERSISTENT_CLOCK
 	help
 	  If you say yes here, the system time (wall clock) will be set using
 	  the value read from a specified RTC device. This is useful to avoid
@@ -29,7 +28,6 @@ config RTC_HCTOSYS
 config RTC_SYSTOHC
 	bool "Set the RTC time based on NTP synchronization"
 	default y
-	depends on !ALWAYS_USE_PERSISTENT_CLOCK
 	help
 	  If you say yes here, the system time (wall clock) will be stored
 	  in the RTC specified by RTC_HCTOSYS_DEVICE approximately every 11
diff --git a/include/linux/time.h b/include/linux/time.h
index d4835df..afcdc4b 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -117,14 +117,10 @@ static inline bool timespec_valid_strict(const struct timespec *ts)
 
 extern bool persistent_clock_exist;
 
-#ifdef ALWAYS_USE_PERSISTENT_CLOCK
-#define has_persistent_clock()	true
-#else
 static inline bool has_persistent_clock(void)
 {
 	return persistent_clock_exist;
 }
-#endif
 
 extern void read_persistent_clock(struct timespec *ts);
 extern void read_boot_clock(struct timespec *ts);
diff --git a/kernel/time/Kconfig b/kernel/time/Kconfig
index 24510d8..b696922 100644
--- a/kernel/time/Kconfig
+++ b/kernel/time/Kconfig
@@ -12,11 +12,6 @@ config CLOCKSOURCE_WATCHDOG
 config ARCH_CLOCKSOURCE_DATA
 	bool
 
-# Platforms has a persistent clock
-config ALWAYS_USE_PERSISTENT_CLOCK
-	bool
-	default n
-
 # Timekeeping vsyscall support
 config GENERIC_TIME_VSYSCALL
 	bool
-- 
1.7.10.4

Re: [PATCH] time: Revert ALWAYS_USE_PERSISTENT_CLOCK compile time optimizaitons

[Kay Sievers]

On Wed, Apr 24, 2013 at 8:32 PM, John Stultz <john.stultz@linaro.org> wrote:
> Kay Sievers noted that the ALWAYS_USE_PERSISTENT_CLOCK config,
> which enables some minor compile time optimization to avoid
> uncessary code in mostly the suspend/resume path could cause
> problems for userland.
>
> In particular, the dependency for RTC_HCTOSYS on
> !ALWAYS_USE_PERSISTENT_CLOCK, which avoids setting the time
> twice and simplifies suspend/resume, has the side effect
> of causing the /sys/class/rtc/rtcN/hctosys flag to always be
> zero, and this flag is commonly used by udev to setup the
> /dev/rtc symlink to /dev/rtcN, which can cause pain for
> older applications.

FWIW, in the light of the original change, I've just removed the
/dev/rtc creation from the default udev rules now, so that thing will
be phased out in the future.

Thanks,
Kay

Re: [PATCH] time: Revert ALWAYS_USE_PERSISTENT_CLOCK compile time optimizaitons

[John Stultz]

On 04/24/2013 11:41 AM, Kay Sievers wrote:
> On Wed, Apr 24, 2013 at 8:32 PM, John Stultz <john.stultz@linaro.org> wrote:
>> Kay Sievers noted that the ALWAYS_USE_PERSISTENT_CLOCK config,
>> which enables some minor compile time optimization to avoid
>> uncessary code in mostly the suspend/resume path could cause
>> problems for userland.
>>
>> In particular, the dependency for RTC_HCTOSYS on
>> !ALWAYS_USE_PERSISTENT_CLOCK, which avoids setting the time
>> twice and simplifies suspend/resume, has the side effect
>> of causing the /sys/class/rtc/rtcN/hctosys flag to always be
>> zero, and this flag is commonly used by udev to setup the
>> /dev/rtc symlink to /dev/rtcN, which can cause pain for
>> older applications.
> FWIW, in the light of the original change, I've just removed the
> /dev/rtc creation from the default udev rules now, so that thing will
> be phased out in the future.
Is that actually wanted? What happens to applications that use /dev/rtc?

I think setting up the /dev/rtc link is important. Its just that setting 
it up exclusively by the hctosys flag is maybe more fragile then we'd 
like. Instead the hctosys flag maybe should only be used as a hint if 
there is more then one RTC available.

thanks
-john

Re: [PATCH] time: Revert ALWAYS_USE_PERSISTENT_CLOCK compile time optimizaitons

[Kay Sievers]

On Wed, Apr 24, 2013 at 8:55 PM, John Stultz <john.stultz@linaro.org> wrote:
> On 04/24/2013 11:41 AM, Kay Sievers wrote:
>>
>> On Wed, Apr 24, 2013 at 8:32 PM, John Stultz <john.stultz@linaro.org>
>> wrote:
>>>
>>> Kay Sievers noted that the ALWAYS_USE_PERSISTENT_CLOCK config,
>>> which enables some minor compile time optimization to avoid
>>> uncessary code in mostly the suspend/resume path could cause
>>> problems for userland.
>>>
>>> In particular, the dependency for RTC_HCTOSYS on
>>> !ALWAYS_USE_PERSISTENT_CLOCK, which avoids setting the time
>>> twice and simplifies suspend/resume, has the side effect
>>> of causing the /sys/class/rtc/rtcN/hctosys flag to always be
>>> zero, and this flag is commonly used by udev to setup the
>>> /dev/rtc symlink to /dev/rtcN, which can cause pain for
>>> older applications.
>>
>> FWIW, in the light of the original change, I've just removed the
>> /dev/rtc creation from the default udev rules now, so that thing will
>> be phased out in the future.
>
> Is that actually wanted? What happens to applications that use /dev/rtc?
>
> I think setting up the /dev/rtc link is important. Its just that setting it
> up exclusively by the hctosys flag is maybe more fragile then we'd like.
> Instead the hctosys flag maybe should only be used as a hint if there is
> more then one RTC available.

So far we only created the symlink for an rtc with the hctosys flag
set. It was added as a workaround for ARM, which sometimes has
multiple RTCs. But there is now also logic in udev/systemd anyway to
search for the system's rtc, which does not rely on the symlink. Other
commonly used tools we checked just use /dev/rtc0 directly.

As mentioned in the mail yesterday, I expected that
CONFIG_RTC_HCTOSYS=y was also needed on x86. The current udev logic
would need updating anyway, if we disable CONFIG_RTC_HCTOSYS=y now. So
let's just find out what's really needed here and add it back properly
if something really needs it.

While we are at it, the interface to read and set the persistent clock
from userspace, the clock the kernel internally uses, is still to just
use the /dev/rtc0 device with the ioctls? There is no other native
kernel timer interface or something else for that, right?

Thanks,
Kay

Re: [PATCH] time: Revert ALWAYS_USE_PERSISTENT_CLOCK compile time optimizaitons

[Kay Sievers]

On Wed, Apr 24, 2013 at 8:32 PM, John Stultz <john.stultz@linaro.org> wrote:
> Kay Sievers noted that the ALWAYS_USE_PERSISTENT_CLOCK config,
> which enables some minor compile time optimization to avoid
> uncessary code in mostly the suspend/resume path could cause
> problems for userland.
>
> In particular, the dependency for RTC_HCTOSYS on
> !ALWAYS_USE_PERSISTENT_CLOCK, which avoids setting the time
> twice and simplifies suspend/resume, has the side effect
> of causing the /sys/class/rtc/rtcN/hctosys flag to always be
> zero, and this flag is commonly used by udev to setup the
> /dev/rtc symlink to /dev/rtcN, which can cause pain for
> older applications.

An alternative to reverting this could be to set that flag
unconditionally on the rtc that matches the persistent clock the
kernel uses internally?

I mean regardless of the pretty weird config option
CONFIG_RTC_HCTOSYS_DEVICE, which internally just does a strcmp() with
the given device name when the flag is queried. :)

Can't we just have the same interface and your original optimization
on x86, even without CONFIG_RTC_HCTOSYS_DEVICE enabled at all?

Kay

Re: [PATCH] time: Revert ALWAYS_USE_PERSISTENT_CLOCK compile time optimizaitons

[John Stultz]

On 04/24/2013 12:18 PM, Kay Sievers wrote:
> On Wed, Apr 24, 2013 at 8:55 PM, John Stultz <john.stultz@linaro.org> wrote:
>> On 04/24/2013 11:41 AM, Kay Sievers wrote:
>>> On Wed, Apr 24, 2013 at 8:32 PM, John Stultz <john.stultz@linaro.org>
>>> wrote:
>>>> Kay Sievers noted that the ALWAYS_USE_PERSISTENT_CLOCK config,
>>>> which enables some minor compile time optimization to avoid
>>>> uncessary code in mostly the suspend/resume path could cause
>>>> problems for userland.
>>>>
>>>> In particular, the dependency for RTC_HCTOSYS on
>>>> !ALWAYS_USE_PERSISTENT_CLOCK, which avoids setting the time
>>>> twice and simplifies suspend/resume, has the side effect
>>>> of causing the /sys/class/rtc/rtcN/hctosys flag to always be
>>>> zero, and this flag is commonly used by udev to setup the
>>>> /dev/rtc symlink to /dev/rtcN, which can cause pain for
>>>> older applications.
>>> FWIW, in the light of the original change, I've just removed the
>>> /dev/rtc creation from the default udev rules now, so that thing will
>>> be phased out in the future.
>> Is that actually wanted? What happens to applications that use /dev/rtc?
>>
>> I think setting up the /dev/rtc link is important. Its just that setting it
>> up exclusively by the hctosys flag is maybe more fragile then we'd like.
>> Instead the hctosys flag maybe should only be used as a hint if there is
>> more then one RTC available.
> So far we only created the symlink for an rtc with the hctosys flag
> set. It was added as a workaround for ARM, which sometimes has
> multiple RTCs. But there is now also logic in udev/systemd anyway to
> search for the system's rtc, which does not rely on the symlink. Other
> commonly used tools we checked just use /dev/rtc0 directly.

Yea, but there are likely a number of legacy applications that will 
still need the /dev/rtc link. I remember lots of pain when the /dev/rtcN 
devices showed up, and I suspect that's all been quiet due to the 
/dev/rtc links.

Dropping that link seems way too risky to me, thus why I'm submitting a 
revert. As I didn't realize the link was dependent on the hctosys flag.


> As mentioned in the mail yesterday, I expected that
> CONFIG_RTC_HCTOSYS=y was also needed on x86. The current udev logic
> would need updating anyway, if we disable CONFIG_RTC_HCTOSYS=y now. So
> let's just find out what's really needed here and add it back properly
> if something really needs it.

I'm really fine with the revert, and would like to avoid any userland 
changes. There's more changes to come in that area of the kernel, so if 
there is any userland implications (which I'd actually prefer to avoid), 
lets make sure they are well thought out and not rushed.

> While we are at it, the interface to read and set the persistent clock
> from userspace, the clock the kernel internally uses, is still to just
> use the /dev/rtc0 device with the ioctls? There is no other native
> kernel timer interface or something else for that, right?

Right, the persistent_clock is only useful to the kernel over the RTC in 
the case where interrupts are disabled. Userland should stick to the 
RTC, and we shouldn't expose the persistent_clock out to userland. I'm 
hoping we'll be able to push the persistent_clock functionality into the 
RTC code (though I suspect the micro embedded folks will complain about 
having to compile the generic rtc core in as well - though we can always 
leave time initialization to userland for those crazier cases).

thanks
-john

Re: [PATCH] time: Revert ALWAYS_USE_PERSISTENT_CLOCK compile time optimizaitons

[John Stultz]

On 04/24/2013 12:44 PM, Kay Sievers wrote:
> On Wed, Apr 24, 2013 at 8:32 PM, John Stultz <john.stultz@linaro.org> wrote:
>> Kay Sievers noted that the ALWAYS_USE_PERSISTENT_CLOCK config,
>> which enables some minor compile time optimization to avoid
>> uncessary code in mostly the suspend/resume path could cause
>> problems for userland.
>>
>> In particular, the dependency for RTC_HCTOSYS on
>> !ALWAYS_USE_PERSISTENT_CLOCK, which avoids setting the time
>> twice and simplifies suspend/resume, has the side effect
>> of causing the /sys/class/rtc/rtcN/hctosys flag to always be
>> zero, and this flag is commonly used by udev to setup the
>> /dev/rtc symlink to /dev/rtcN, which can cause pain for
>> older applications.
> An alternative to reverting this could be to set that flag
> unconditionally on the rtc that matches the persistent clock the
> kernel uses internally?
>
> I mean regardless of the pretty weird config option
> CONFIG_RTC_HCTOSYS_DEVICE, which internally just does a strcmp() with
> the given device name when the flag is queried. :)
>
> Can't we just have the same interface and your original optimization
> on x86, even without CONFIG_RTC_HCTOSYS_DEVICE enabled at all?

This might be possible, but I'd rather it be part of the unification 
work w/ the persistent_clock and the RTC code, rather then a quick hack.

thanks
-john

Re: [PATCH] time: Revert ALWAYS_USE_PERSISTENT_CLOCK compile time optimizaitons

[Feng Tang]

On Wed, Apr 24, 2013 at 11:32:56AM -0700, John Stultz wrote:
> Kay Sievers noted that the ALWAYS_USE_PERSISTENT_CLOCK config,
> which enables some minor compile time optimization to avoid
> uncessary code in mostly the suspend/resume path could cause
> problems for userland.
> 
> In particular, the dependency for RTC_HCTOSYS on
> !ALWAYS_USE_PERSISTENT_CLOCK, which avoids setting the time
> twice and simplifies suspend/resume, has the side effect
> of causing the /sys/class/rtc/rtcN/hctosys flag to always be
> zero, and this flag is commonly used by udev to setup the
> /dev/rtc symlink to /dev/rtcN, which can cause pain for
> older applications.
> 
> While the udev rules could use some work to be less fragile,
> breaking userland should strongly be avoided. Additionally
> the compile time optimizations are fairly minor, and the code
> being optimized is likely to be reworked in the future, so
> lets revert this change.
> 
> I suspect this will miss 3.9-final, so marking it for 3.9-stable.

thanks

Acked-by: Feng Tang <feng.tang@intel.com>

Re: [PATCH] time: Revert ALWAYS_USE_PERSISTENT_CLOCK compile time optimizaitons

[Kay Sievers]

On Wed, Apr 24, 2013 at 8:55 PM, John Stultz <john.stultz@linaro.org> wrote:

>> FWIW, in the light of the original change, I've just removed the
>> /dev/rtc creation from the default udev rules now, so that thing will
>> be phased out in the future.
>
> Is that actually wanted? What happens to applications that use /dev/rtc?
>
> I think setting up the /dev/rtc link is important. Its just that setting it
> up exclusively by the hctosys flag is maybe more fragile then we'd like.
> Instead the hctosys flag maybe should only be used as a hint if there is
> more then one RTC available.

Ok, convinced.

I've changed the udev rules now to first "search" for the rtc with
"hctosys" flag set, and if none is found, just fall back to /dev/rtc0.

It should work reliably on most boxes, and still do the right thing in
most cases if none of the rtcs has that flag.

Thanks,
Kay

[tip:timers/urgent] time: Revert ALWAYS_USE_PERSISTENT_CLOCK compile time optimizaitons

[tip-bot for John Stultz]

Commit-ID:  b4f711ee03d28f776fd2324fd0bd999cc428e4d2
Gitweb:     http://git.kernel.org/tip/b4f711ee03d28f776fd2324fd0bd999cc428e4d2
Author:     John Stultz <john.stultz@linaro.org>
AuthorDate: Wed, 24 Apr 2013 11:32:56 -0700
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 14 May 2013 20:54:06 +0200

time: Revert ALWAYS_USE_PERSISTENT_CLOCK compile time optimizaitons

Kay Sievers noted that the ALWAYS_USE_PERSISTENT_CLOCK config,
which enables some minor compile time optimization to avoid
uncessary code in mostly the suspend/resume path could cause
problems for userland.

In particular, the dependency for RTC_HCTOSYS on
!ALWAYS_USE_PERSISTENT_CLOCK, which avoids setting the time
twice and simplifies suspend/resume, has the side effect
of causing the /sys/class/rtc/rtcN/hctosys flag to always be
zero, and this flag is commonly used by udev to setup the
/dev/rtc symlink to /dev/rtcN, which can cause pain for
older applications.

While the udev rules could use some work to be less fragile,
breaking userland should strongly be avoided. Additionally
the compile time optimizations are fairly minor, and the code
being optimized is likely to be reworked in the future, so
lets revert this change.

Reported-by: Kay Sievers <kay@vrfy.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
Cc: stable <stable@vger.kernel.org> #3.9
Cc: Feng Tang <feng.tang@intel.com>
Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Link: http://lkml.kernel.org/r/1366828376-18124-1-git-send-email-john.stultz@linaro.org
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/Kconfig     | 1 -
 drivers/rtc/Kconfig  | 2 --
 include/linux/time.h | 4 ----
 kernel/time/Kconfig  | 5 -----
 4 files changed, 12 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 5db2117..45c4124 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -108,7 +108,6 @@ config X86
 	select GENERIC_CLOCKEVENTS_BROADCAST if X86_64 || (X86_32 && X86_LOCAL_APIC)
 	select GENERIC_TIME_VSYSCALL if X86_64
 	select KTIME_SCALAR if X86_32
-	select ALWAYS_USE_PERSISTENT_CLOCK
 	select GENERIC_STRNCPY_FROM_USER
 	select GENERIC_STRNLEN_USER
 	select HAVE_CONTEXT_TRACKING if X86_64
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 0c81915..b983813 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -20,7 +20,6 @@ if RTC_CLASS
 config RTC_HCTOSYS
 	bool "Set system time from RTC on startup and resume"
 	default y
-	depends on !ALWAYS_USE_PERSISTENT_CLOCK
 	help
 	  If you say yes here, the system time (wall clock) will be set using
 	  the value read from a specified RTC device. This is useful to avoid
@@ -29,7 +28,6 @@ config RTC_HCTOSYS
 config RTC_SYSTOHC
 	bool "Set the RTC time based on NTP synchronization"
 	default y
-	depends on !ALWAYS_USE_PERSISTENT_CLOCK
 	help
 	  If you say yes here, the system time (wall clock) will be stored
 	  in the RTC specified by RTC_HCTOSYS_DEVICE approximately every 11
diff --git a/include/linux/time.h b/include/linux/time.h
index 22d81b3..d5d229b 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -117,14 +117,10 @@ static inline bool timespec_valid_strict(const struct timespec *ts)
 
 extern bool persistent_clock_exist;
 
-#ifdef ALWAYS_USE_PERSISTENT_CLOCK
-#define has_persistent_clock()	true
-#else
 static inline bool has_persistent_clock(void)
 {
 	return persistent_clock_exist;
 }
-#endif
 
 extern void read_persistent_clock(struct timespec *ts);
 extern void read_boot_clock(struct timespec *ts);
diff --git a/kernel/time/Kconfig b/kernel/time/Kconfig
index 24510d8..b696922 100644
--- a/kernel/time/Kconfig
+++ b/kernel/time/Kconfig
@@ -12,11 +12,6 @@ config CLOCKSOURCE_WATCHDOG
 config ARCH_CLOCKSOURCE_DATA
 	bool
 
-# Platforms has a persistent clock
-config ALWAYS_USE_PERSISTENT_CLOCK
-	bool
-	default n
-
 # Timekeeping vsyscall support
 config GENERIC_TIME_VSYSCALL
 	bool