Hey,
what's the intention of:
e90c83f757fffdacec8b3c5eee5617dcc038338f ?
x86: Select HAS_PERSISTENT_CLOCK on x86
It unconditionally sets:
HAS_PERSISTENT_CLOCK
but:
RTC_SYSTOHC
got a depends on !HAS_PERSISTENT_CLOCK
This makes it impossible to sync the system time from the RTC on x86.
What's going on here?
Thanks,
Kay
On 04/23/2013 06:34 PM, Kay Sievers wrote:
> Hey,
>
> what's the intention of:
> e90c83f757fffdacec8b3c5eee5617dcc038338f ?
> x86: Select HAS_PERSISTENT_CLOCK on x86
>
> It unconditionally sets:
> HAS_PERSISTENT_CLOCK
> but:
> RTC_SYSTOHC
> got a depends on !HAS_PERSISTENT_CLOCK
>
> This makes it impossible to sync the system time from the RTC on x86.
> What's going on here?
So I suspect just some confusion, but let me know if thats wrong and
you're actually seeing an issue.
SYSTOHC is what *sets the RTC* to the system time when we're synced with
NTP.
HCTOSYS is what sets the system time from the RTC.
In the past, we've used the update_persistent_clock() interface to set
the CMOS clock when we're synced with NTP, but now we've got an
interface that can sync the RTC interface for systems that don't support
update_persistent_clock().
We've always used the persistent_clock code (or what persistent_clock
abstracted) on x86, but for awhile there was also ways to configure the
kernel to also use the generic RTC infrastructure, which could cause odd
behavior like the time being set/adjusted twice at boot and resume. This
was mostly avoided in code, but we figured we could also avoid these
duplicative paths at build time. Thus if HAS_PERSISTENT_CLOCK is
enabled, we can remove some extra checks that can slow down the resume path.
Now, the persistent_clock and RTC infrastructure are annoyingly
duplicitive, but have very different constraints. We're hopefully (and
likely slowly) working to consolidate some of the logic here.
My goal is eventually to remove the use of the persistent_clock() for
time initialization, instead using the generic RTC infrastructure.
Then I want to morph the persistent_clock infrastructure to really be
only for measuring suspend time by the timekeeping core at resume. This
can be done via clocksources that continue to count via suspend, or via
the RTC layer, however we need a special RTC hook to allow us to read
the RTC time when interrupts are disabled (not all RTCs support this).
Then only should both of those options fail, fall back to the late (and
more error prone) rtc resume hook for suspend timing.
But of course, I don't have patches for this transition yet, and I'm not
sure when in the near term I'll get time to focus on this cleanup.
However, if you are seeing a issue/regression with the existing code in
your config, please let me know and I'll make sure we address it.
thanks
-john
On Wed, Apr 24, 2013 at 4:43 AM, John Stultz <[email protected]> wrote:
> On 04/23/2013 06:34 PM, Kay Sievers wrote:
>>
>> Hey,
>>
>> what's the intention of:
>> e90c83f757fffdacec8b3c5eee5617dcc038338f ?
>> x86: Select HAS_PERSISTENT_CLOCK on x86
>>
>> It unconditionally sets:
>> HAS_PERSISTENT_CLOCK
>> but:
>> RTC_SYSTOHC
>> got a depends on !HAS_PERSISTENT_CLOCK
>>
>> This makes it impossible to sync the system time from the RTC on x86.
>> What's going on here?
>
>
> So I suspect just some confusion, but let me know if thats wrong and you're
> actually seeing an issue.
>
> SYSTOHC is what *sets the RTC* to the system time when we're synced with
> NTP.
>
> HCTOSYS is what sets the system time from the RTC.
Right, and RTC_HCTOSYS is not NTP related. It just reads the time from
the RTC_HCTOSYS_DEVICE at bootup so we do not boot in 1970 time mode.
We need that it in all cases, at every bootup on x86. But it's no
longer there with the above commits. :)
Kay
On 04/23/2013 08:05 PM, Kay Sievers wrote:
> On Wed, Apr 24, 2013 at 4:43 AM, John Stultz <[email protected]> wrote:
>> On 04/23/2013 06:34 PM, Kay Sievers wrote:
>>> Hey,
>>>
>>> what's the intention of:
>>> e90c83f757fffdacec8b3c5eee5617dcc038338f ?
>>> x86: Select HAS_PERSISTENT_CLOCK on x86
>>>
>>> It unconditionally sets:
>>> HAS_PERSISTENT_CLOCK
>>> but:
>>> RTC_SYSTOHC
>>> got a depends on !HAS_PERSISTENT_CLOCK
>>>
>>> This makes it impossible to sync the system time from the RTC on x86.
>>> What's going on here?
>>
>> So I suspect just some confusion, but let me know if thats wrong and you're
>> actually seeing an issue.
>>
>> SYSTOHC is what *sets the RTC* to the system time when we're synced with
>> NTP.
>>
>> HCTOSYS is what sets the system time from the RTC.
> Right, and RTC_HCTOSYS is not NTP related. It just reads the time from
> the RTC_HCTOSYS_DEVICE at bootup so we do not boot in 1970 time mode.
> We need that it in all cases, at every bootup on x86. But it's no
> longer there with the above commits. :)
On x86 the persistent_clock() is backed by the
CMOS/EFI/kvm-wall/xen/vrtc clock (all via x86_platform.get_wallclock)
should be present and we'll initialize the time in timekeeping_init() there.
Its only systems where there isn't a persistent_clock is where the RTC
layer and the HCTOSYS is helpful.
Again, if you're having a problem where an x86 system isn't getting its
time initialized correctly, please let me know the details of the system.
thanks
-john
On Wed, Apr 24, 2013 at 5:19 AM, John Stultz <[email protected]> wrote:
> On 04/23/2013 08:05 PM, Kay Sievers wrote:
>>
>> On Wed, Apr 24, 2013 at 4:43 AM, John Stultz <[email protected]>
>> wrote:
>>>
>>> On 04/23/2013 06:34 PM, Kay Sievers wrote:
>>>>
>>>> Hey,
>>>>
>>>> what's the intention of:
>>>> e90c83f757fffdacec8b3c5eee5617dcc038338f ?
>>>> x86: Select HAS_PERSISTENT_CLOCK on x86
>>>>
>>>> It unconditionally sets:
>>>> HAS_PERSISTENT_CLOCK
>>>> but:
>>>> RTC_SYSTOHC
>>>> got a depends on !HAS_PERSISTENT_CLOCK
>>>>
>>>> This makes it impossible to sync the system time from the RTC on x86.
>>>> What's going on here?
>>>
>>>
>>> So I suspect just some confusion, but let me know if thats wrong and
>>> you're
>>> actually seeing an issue.
>>>
>>> SYSTOHC is what *sets the RTC* to the system time when we're synced with
>>> NTP.
>>>
>>> HCTOSYS is what sets the system time from the RTC.
>>
>> Right, and RTC_HCTOSYS is not NTP related. It just reads the time from
>> the RTC_HCTOSYS_DEVICE at bootup so we do not boot in 1970 time mode.
>> We need that it in all cases, at every bootup on x86. But it's no
>> longer there with the above commits. :)
>
> On x86 the persistent_clock() is backed by the CMOS/EFI/kvm-wall/xen/vrtc
> clock (all via x86_platform.get_wallclock) should be present and we'll
> initialize the time in timekeeping_init() there.
>
> Its only systems where there isn't a persistent_clock is where the RTC layer
> and the HCTOSYS is helpful.
>
> Again, if you're having a problem where an x86 system isn't getting its time
> initialized correctly, please let me know the details of the system.
Until the above commits we always needed:
CONFIG_RTC_HCTOSYS=y
CONFIG_RTC_HCTOSYS_DEVICE="rtc0"
to get the system time correctly initialized at bootup on x86.
These options are gone now and cannot be selected anymore. You are
saying that this is all fine, that they are gone, but all initial
clock syncing should still work?
Also:
$ cat /sys/class/rtc/rtc0/hctosys
0
always carried "1", and this now breaks setups which expect an
automatically created symlink /dev/rtc to the actual "system rtc".
There was also always a line in dmesg that told the rtc_cmos time it
wrote to the system clock. This is also gone?
I haven't looked what goes wrong, I expected the make(1) errors with
"time in the future" after bootup before the network is up, which I
see since a week or two, to be a fallout of that.
Kay
On Wed, Apr 24, 2013 at 5:33 AM, Kay Sievers <[email protected]> wrote:
> Also:
> $ cat /sys/class/rtc/rtc0/hctosys
> 0
> always carried "1", and this now breaks setups which expect an
> automatically created symlink /dev/rtc to the actual "system rtc".
We used to do this in upstream standard udev rules:
SUBSYSTEM=="rtc", ATTR{hctosys}=="1", MODE="0644"
http://cgit.freedesktop.org/systemd/systemd/tree/rules/50-udev-default.rules#n18
If that information is expected to be gone now, we need to update some
tools. Whats' the proper way now to find the "system rtc" to use?
Kay
Hello,
Am 24.04.2013 05:19, schrieb John Stultz:
> On x86 the persistent_clock() is backed by the
> CMOS/EFI/kvm-wall/xen/vrtc clock (all via x86_platform.get_wallclock)
> should be present and we'll initialize the time in timekeeping_init()
> there.
>
> Its only systems where there isn't a persistent_clock is where the RTC
> layer and the HCTOSYS is helpful.
I'm a bit confused too. ;)
Doesn't this remove the users choice of RTC on x86 systems?
Why is there a difference made between the CMOS/EFI/... clocks and other
RTCs?
And why is RTC_SYSTOHC now gone on x86?
Regards,
Alexander
On 04/23/2013 10:12 PM, Alexander Holler wrote:
> Hello,
>
> Am 24.04.2013 05:19, schrieb John Stultz:
>
>> On x86 the persistent_clock() is backed by the
>> CMOS/EFI/kvm-wall/xen/vrtc clock (all via x86_platform.get_wallclock)
>> should be present and we'll initialize the time in timekeeping_init()
>> there.
>>
>> Its only systems where there isn't a persistent_clock is where the RTC
>> layer and the HCTOSYS is helpful.
>
> I'm a bit confused too. ;)
>
> Doesn't this remove the users choice of RTC on x86 systems?
So the userland interfaces for the RTCs are still present.
>
> Why is there a difference made between the CMOS/EFI/... clocks and
> other RTCs?
Its a mostly history. Way way back, timekeeping was mostly arch
specific, and we initialized time with arch specific RTC code. After the
generic timekeeping went in, the persistent_clock() interface was
created as a generic interface to CMOS and other RTCs. The catch being,
since we access the persistent_clock() in very early init as well as
very late suspend and very early resume, the persistent_clock has to be
able to function when interrupts are off.
Now, somewhere near this time (my memory is foggy), the generic RTC
driver infrastructure was also created, pulling most of the RTC drivers
out of arch specific code and into the generic driver code. The problem
there was there were many RTCs that were accessed over buses that
required interrupts to be enabled. So there was no way for the generic
timekeeping code to use the generic RTC code.
Additionally, since the generic RTC code didn't provide what was needed
for the persistent_clock interface, we ended up with duplicated code for
things like the CMOS clock in both drivers/rtc/rtc-cmos.c, and
arch/x86/kernel/rtc.c
For those (not too common) systems that didn't have a persistent_clock,
the RTC framework gained the HCTOSYS option, that would set the clock at
driver init time, as well as on resume.
Now this HCTOSYS approach had some problems that didn't really become
too apparent until ARM started to get much more popular. First of all,
since it just set the time on resume we didn't properly measure suspend
time. So we had to add some special timekeeping interfaces and changes
to measure the time from the RTC device suspend to RTC device resume.
Unfortuantely this still has a problem, because we have to stop and
start timekeeping very late in the suspend and early in the resume path.
Thus any latency between rtc device suspend -> timekeeping suspend and
timekeeping resume -> rtc device resume, introduces time error. Some
tweaks have been added to try to bound this error, but its still not ideal.
Now, the persistent_clock() code is nicer for this, since we access it
in timekeeping suspend and resume, which reduces the introduced error.
Also, because the persistent_clock it provides an nsec interface rather
then a sec granular interface, so we can utilize finer grained hardware
to avoid adding extra error when measuring suspend, where that's available.
Now, on systems that had a persistent_clock, enabling HCTOSYS caused
some (minor trouble), since we would then repeatedly set the time twice
at boot up (once in timekeeping_init, and then again in RTC init paths).
Additionally, during suspend and resume, we would measure suspend in
timekeeping_resume and get things correct, and then the rtc resume would
set the time again, causing error. When rtc suspend/resume was tweaked
to measure suspend time, then we had to be extra careful, since there
were now two systems measuring suspend and trying to update the clock,
so we could end up with time moving forward 2x suspend time.
Since that point, the code has basically ignored the HCTOSYS path on
resume if the persistent_clock was present, which is always true on x86.
> And why is RTC_SYSTOHC now gone on x86?
So summarizing the above, because as much as I'm aware, its always been
redundant and unnecessary on x86. Thus being able at build time to mark
it as unnecessary was attractive, since it reduced the code paths
running at suspend/resume.
That said, Kay's concerns about userland implications (basically the
userland side effects of SYSTOHC being enabled) give me pause, so I may
revert the HAS_PERSISTENT_CLOCK on x86 change.
And again, after recent discussions with both Feng Tang and Jason
Gunthorpe, there's a visible path forward that will hopefully remove
some of the redundancy above:
* Adding special no-irq safe accessors to the RTC driver infrastrucutre,
so the persistent_clock can use the RTC code directly (allowing the
removal of duplicated code)
* Separating the persistent_clock functionality into separate time
initialization and suspend timing functions.
Though I'm not sure when in the near term I'll have to start
implementing this, so I'd be happy to work with interested parties to
get this cleaned up.
thanks
-john
On 04/23/2013 08:33 PM, Kay Sievers wrote:
> On Wed, Apr 24, 2013 at 5:19 AM, John Stultz <[email protected]> wrote:
>> On 04/23/2013 08:05 PM, Kay Sievers wrote:
>>> On Wed, Apr 24, 2013 at 4:43 AM, John Stultz <[email protected]>
>>> wrote:
>>>> On 04/23/2013 06:34 PM, Kay Sievers wrote:
>>>>> Hey,
>>>>>
>>>>> what's the intention of:
>>>>> e90c83f757fffdacec8b3c5eee5617dcc038338f ?
>>>>> x86: Select HAS_PERSISTENT_CLOCK on x86
>>>>>
>>>>> It unconditionally sets:
>>>>> HAS_PERSISTENT_CLOCK
>>>>> but:
>>>>> RTC_SYSTOHC
>>>>> got a depends on !HAS_PERSISTENT_CLOCK
>>>>>
>>>>> This makes it impossible to sync the system time from the RTC on x86.
>>>>> What's going on here?
>>>>
>>>> So I suspect just some confusion, but let me know if thats wrong and
>>>> you're
>>>> actually seeing an issue.
>>>>
>>>> SYSTOHC is what *sets the RTC* to the system time when we're synced with
>>>> NTP.
>>>>
>>>> HCTOSYS is what sets the system time from the RTC.
>>> Right, and RTC_HCTOSYS is not NTP related. It just reads the time from
>>> the RTC_HCTOSYS_DEVICE at bootup so we do not boot in 1970 time mode.
>>> We need that it in all cases, at every bootup on x86. But it's no
>>> longer there with the above commits. :)
>> On x86 the persistent_clock() is backed by the CMOS/EFI/kvm-wall/xen/vrtc
>> clock (all via x86_platform.get_wallclock) should be present and we'll
>> initialize the time in timekeeping_init() there.
>>
>> Its only systems where there isn't a persistent_clock is where the RTC layer
>> and the HCTOSYS is helpful.
>>
>> Again, if you're having a problem where an x86 system isn't getting its time
>> initialized correctly, please let me know the details of the system.
> Until the above commits we always needed:
> CONFIG_RTC_HCTOSYS=y
> CONFIG_RTC_HCTOSYS_DEVICE="rtc0"
> to get the system time correctly initialized at bootup on x86.
So... always needed to get system time correctly initialized? I'm not
sure I agree with this. On non-x86 (mostly embedded) platforms that do
not have persistent_clock support, yes, the above is needed.
But I'm unaware of the above actually being necessary on x86, as its
always had persistent_clock support.
> These options are gone now and cannot be selected anymore. You are
> saying that this is all fine, that they are gone, but all initial
> clock syncing should still work?
Yes, we're just removing a duplicative initialization of time, and
compiling out code in the suspend/resume path that would never trigger
when persistent_clocks were present.
> Also:
> $ cat /sys/class/rtc/rtc0/hctosys
> 0
> always carried "1", and this now breaks setups which expect an
> automatically created symlink /dev/rtc to the actual "system rtc".
Sigh. So just turning off HCTOSYS on those systems causes them to break?
That is sort of obnoxiously fragile. I've always been somewhat
skeptical of the multi-rtc configs - as they're all the "system's" RTCs
after all. 99% probably only have one rtc device, so checking the
hctosys in that case is a little silly.
But the terribly annoying interface breakage when /dev/rtc went to
/dev/rtcN with the generic rtc layer landing shouldn't have happened, so
I won't begrudge too much the userland hacks needed to fix that up.
So I'll send Thomas a revert for the HCTOSYS optimization. In the kernel
we'll still avoid using HCTOSYS when the persistent_clock is there, but
at least userland will still have some /sys/class/rtc/rtcN that has the
"offical" flag.
> There was also always a line in dmesg that told the rtc_cmos time it
> wrote to the system clock. This is also gone?
More worrisome, I'll turn the question around: is that an expected
interface never to break?
> I haven't looked what goes wrong, I expected the make(1) errors with
> "time in the future" after bootup before the network is up, which I
> see since a week or two, to be a fallout of that.
I'd be very interested if you can narrow this suspicion down, as it
would mean there's likely a problem with the persistent_clock code that
needs fixing.
Thanks again for raising the flag on the userland expectations bit.
-john
On Wed, Apr 24, 2013 at 6:07 PM, John Stultz <[email protected]> wrote:
> So summarizing the above, because as much as I'm aware, its always been
> redundant and unnecessary on x86. Thus being able at build time to mark it
> as unnecessary was attractive, since it reduced the code paths running at
> suspend/resume.
>
> That said, Kay's concerns about userland implications (basically the
> userland side effects of SYSTOHC being enabled) give me pause, so I may
> revert the HAS_PERSISTENT_CLOCK on x86 change.
Thanks a lot for all the missing details!
No, I think that all makes too much sense to revert it. Let's just
find a way to solve it properly. I don't think it is of any pressing
importance to keep the old behaviour, if we can still provide the
functionality today.
I'll continue replying in the later mail ...
Kay
On 04/23/2013 08:51 PM, Kay Sievers wrote:
> On Wed, Apr 24, 2013 at 5:33 AM, Kay Sievers <[email protected]> wrote:
>
>> Also:
>> $ cat /sys/class/rtc/rtc0/hctosys
>> 0
>> always carried "1", and this now breaks setups which expect an
>> automatically created symlink /dev/rtc to the actual "system rtc".
> We used to do this in upstream standard udev rules:
> SUBSYSTEM=="rtc", ATTR{hctosys}=="1", MODE="0644"
> http://cgit.freedesktop.org/systemd/systemd/tree/rules/50-udev-default.rules#n18
>
> If that information is expected to be gone now, we need to update some
> tools. Whats' the proper way now to find the "system rtc" to use?
No we'll revert. Can't break userland.
But as to your question, if there's only one rtc, I'd consider it the
"system rtc", only adjusting the link if a second RTC appears and has
the hctosys flag set.
thanks
-john
On 04/24/2013 09:32 AM, Kay Sievers wrote:
> On Wed, Apr 24, 2013 at 6:07 PM, John Stultz <[email protected]> wrote:
>
>> So summarizing the above, because as much as I'm aware, its always been
>> redundant and unnecessary on x86. Thus being able at build time to mark it
>> as unnecessary was attractive, since it reduced the code paths running at
>> suspend/resume.
>>
>> That said, Kay's concerns about userland implications (basically the
>> userland side effects of SYSTOHC being enabled) give me pause, so I may
>> revert the HAS_PERSISTENT_CLOCK on x86 change.
> Thanks a lot for all the missing details!
>
> No, I think that all makes too much sense to revert it. Let's just
> find a way to solve it properly. I don't think it is of any pressing
> importance to keep the old behaviour, if we can still provide the
> functionality today.
So some compile time optimizations for code that likely needs to be
reworked anyway aren't worth the risk of breaking userland to me. Let me
pull these out (on the kernel side, the same code paths will run, we
just avoid some extra checks) so the hctosys flag doesn't change in
distro configs that expect it.
thanks
-john
On Wed, Apr 24, 2013 at 6:30 PM, John Stultz <[email protected]> wrote:
>> Until the above commits we always needed:
>> CONFIG_RTC_HCTOSYS=y
>> CONFIG_RTC_HCTOSYS_DEVICE="rtc0"
>> to get the system time correctly initialized at bootup on x86.
> So... always needed to get system time correctly initialized? I'm not sure I
> agree with this. On non-x86 (mostly embedded) platforms that do not have
> persistent_clock support, yes, the above is needed.
Yeah, right, that's an "always" like the "forever" in "we support that
for forever" :)
> But I'm unaware of the above actually being necessary on x86, as its always
> had persistent_clock support.
Maybe it goes back longer, I remember that we needed to run hwclock in
userspace otherwise we had the 1970 state.
Here is the Fedora bug from 2009 to enable it:
https://bugzilla.redhat.com/show_bug.cgi?id=489494
>> These options are gone now and cannot be selected anymore. You are
>> saying that this is all fine, that they are gone, but all initial
>> clock syncing should still work?
>
> Yes, we're just removing a duplicative initialization of time, and compiling
> out code in the suspend/resume path that would never trigger when
> persistent_clocks were present.
I see, makes sense.
>> Also:
>> $ cat /sys/class/rtc/rtc0/hctosys
>> 0
>> always carried "1", and this now breaks setups which expect an
>> automatically created symlink /dev/rtc to the actual "system rtc".
>
>
> Sigh. So just turning off HCTOSYS on those systems causes them to break?
Well, the symlink is no longer there, which is visible. People asked
where it is gone now. That's the "breakage" which might not deserve
that word, if nothing really breaks that way. Stuff we looked at so
far, falls back to /dev/rtc0 which covers that.
> That is sort of obnoxiously fragile. I've always been somewhat skeptical of
> the multi-rtc configs - as they're all the "system's" RTCs after all. 99%
> probably only have one rtc device, so checking the hctosys in that case is a
> little silly.
Yeah, ARM is as a mess, they often have rtc1 as the "system rtc", that
is why all this symlink game was "invented".
> But the terribly annoying interface breakage when /dev/rtc went to /dev/rtcN
> with the generic rtc layer landing shouldn't have happened, so I won't
> begrudge too much the userland hacks needed to fix that up.
Right.
> So I'll send Thomas a revert for the HCTOSYS optimization. In the kernel
> we'll still avoid using HCTOSYS when the persistent_clock is there, but at
> least userland will still have some /sys/class/rtc/rtcN that has the
> "offical" flag.
So in case you really revert it, x86 should not enable any of that RTC
stuff by default, right?
>> There was also always a line in dmesg that told the rtc_cmos time it
>> wrote to the system clock. This is also gone?
>
> More worrisome, I'll turn the question around: is that an expected interface
> never to break?
No, sure not. I was just noticing that, when looking what was going
on, and I couldn't make sense out of it before you explained the
details.
Kay
Am 24.04.2013 18:07, schrieb John Stultz:
>> And why is RTC_SYSTOHC now gone on x86?
>
> So summarizing the above, because as much as I'm aware, its always been
> redundant and unnecessary on x86. Thus being able at build time to mark
> it as unnecessary was attractive, since it reduced the code paths
> running at suspend/resume.
Hmm, I thought RTC_SYSTOHC was there to update the used RTC clock with
the time from NTP (and liked that). Therefor I don't understand why it
is redundant and unnecessary on x86. Of course, most systems do have
something in userspace to set the RTC on shutdown, so it isn't really
needed.
Anyway, thanks a lot for the great overview. I was totally unaware about
the persistent_clock framework on x86.
Regards,
Alexander
On Thu, Apr 25, 2013 at 9:11 AM, Alexander Holler <[email protected]> wrote:
> Hmm, I thought RTC_SYSTOHC was there to update the used RTC clock with the
> time from NTP (and liked that).
That seems to have the nice self-explaining name CONFIG_GENERIC_CMOS_UPDATE. :)
> Therefor I don't understand why it is
> redundant and unnecessary on x86.
Because the x86 native RTC/cmos is updated with platform code, not
generic rtc code:
arch/x86/kernel/rtc.c
> Of course, most systems do have something
> in userspace to set the RTC on shutdown, so it isn't really needed.
That is gone on most systems today. Systems without NTP or something
else running have no clue about the time, and should not touch the
hardware clock with a boot cycle. Only if a reliable time source like
NTP is available, it should update the hardware clock accordingly.
> Anyway, thanks a lot for the great overview.
Yeah, thanks John, from my side too.
Kay
On 04/25/2013 12:11 AM, Alexander Holler wrote:
> Am 24.04.2013 18:07, schrieb John Stultz:
>
>>> And why is RTC_SYSTOHC now gone on x86?
>>
>> So summarizing the above, because as much as I'm aware, its always been
>> redundant and unnecessary on x86. Thus being able at build time to mark
>> it as unnecessary was attractive, since it reduced the code paths
>> running at suspend/resume.
>
> Hmm, I thought RTC_SYSTOHC was there to update the used RTC clock with
> the time from NTP (and liked that). Therefor I don't understand why it
> is redundant and unnecessary on x86. Of course, most systems do have
> something in userspace to set the RTC on shutdown, so it isn't really
> needed.
Prior to SYSTOHC being introduced, we only synced system time to the RTC
via update_persistent_clock() on systems that had that interface.
SYSTOHC is relatively new and lets the system sync to RTCs that don't
have the persistent clock.
thanks
-john
On Thu, Apr 25, 2013 at 09:13:32AM -0700, John Stultz wrote:
> On 04/25/2013 12:11 AM, Alexander Holler wrote:
> >Am 24.04.2013 18:07, schrieb John Stultz:
> >
> >>>And why is RTC_SYSTOHC now gone on x86?
> >>
> >>So summarizing the above, because as much as I'm aware, its always been
> >>redundant and unnecessary on x86. Thus being able at build time to mark
> >>it as unnecessary was attractive, since it reduced the code paths
> >>running at suspend/resume.
> >
> >Hmm, I thought RTC_SYSTOHC was there to update the used RTC clock
> >with the time from NTP (and liked that). Therefor I don't
> >understand why it is redundant and unnecessary on x86. Of course,
> >most systems do have something in userspace to set the RTC on
> >shutdown, so it isn't really needed.
>
> Prior to SYSTOHC being introduced, we only synced system time to the
> RTC via update_persistent_clock() on systems that had that
> interface. SYSTOHC is relatively new and lets the system sync to
> RTCs that don't have the persistent clock.
Right,
Per John's comments on SYSTOHC, when enabled, NTP still always prefers
the update_persistent_clock clock path if it is available, to avoid
any possible unforseen breakage.
In most cases GENERIC_CMOS_UPDATE and SYSTOHC would ultimately call
exactly the same code, but x86 is very complex in this area and they
can call different code. GENERIC_CMOS_UPDATE calls
arch/x86/kernel/rtc.c:mach_set_rtc_mmss() and SYSTOHC calls
include/asm-generic/rtc.h:__set_rtc_time() (AFAICT)
I would think the SYSTOHC path is the better choice because it is the
path used by userspace /dev/rtc* access and is certainly more tested -
but I don't know for sure. For instance, the NTP path was once
specially optimized to align the second tick of the RTC to the system
clock and I can't tell if both functions have that property.
Anyhow, I think at this point update_persistent_clock and
CONFIG_GENERIC_CMOS_UPDATE are archaic - I left them in when I made
the SYSTOHC patch because untangling everything is a big job.
John mentioned they might be kept for embedded - eg size reduction.
The issue with that idea is if you do not enable the class RTC
subsystem then there is no way for a small embedded userspace to set
the RTC. The /dev/rtc* device obviously goes away, but it also turns
out that that CONFIG_GENERIC_CMOS_UPDATE only works when combined with
a heavy weight userspace NTPD that runs the kernel PLL properly. The
kernel NTP code is enormously conservative and it is actually quite
hard to get it to write to the RTC. An RTC that cannot be set is
useless, so these days I feel CONFIG_RTC is pragmatically mandatory -
and my space constrained embedded systems do set it, for these
reasons.
So, my conclusion is nobody with a RTC looking for space savings, will
disable CONFIG_RTC, which means we can safely rely on
CONFIG_RTC_SYSTOHC to do this work. To that end, I would encourage
everyone who sets CONFIG_GENERIC_CMOS_UPDATE to also set
CONFIG_RTC_SYSTOHC - that will let update_persistent_clock to be
ripped out over time without impacting users.
Regards,
Jason
On Thu, Apr 25, 2013 at 8:33 PM, Jason Gunthorpe
<[email protected]> wrote:
> So, my conclusion is nobody with a RTC looking for space savings, will
> disable CONFIG_RTC, which means we can safely rely on
> CONFIG_RTC_SYSTOHC to do this work. To that end, I would encourage
> everyone who sets CONFIG_GENERIC_CMOS_UPDATE to also set
> CONFIG_RTC_SYSTOHC - that will let update_persistent_clock to be
> ripped out over time without impacting users.
John's original patch forcefully disabled CONFIG_RTC_SYSTOHC on x86,
which is quite the opposite of what you recommend here. :)
Could you guys both sort that out and give an idea what the
recommended setup should look like today, ignoring all space saving
and possible hctosys API changes caused by this. Should
CONFIG_RTC_SYSTOHC be enabled or not?
Kay
On 04/25/2013 12:45 PM, Kay Sievers wrote:
> On Thu, Apr 25, 2013 at 8:33 PM, Jason Gunthorpe
> <[email protected]> wrote:
>> So, my conclusion is nobody with a RTC looking for space savings, will
>> disable CONFIG_RTC, which means we can safely rely on
>> CONFIG_RTC_SYSTOHC to do this work. To that end, I would encourage
>> everyone who sets CONFIG_GENERIC_CMOS_UPDATE to also set
>> CONFIG_RTC_SYSTOHC - that will let update_persistent_clock to be
>> ripped out over time without impacting users.
> John's original patch forcefully disabled CONFIG_RTC_SYSTOHC on x86,
> which is quite the opposite of what you recommend here. :)
>
> Could you guys both sort that out and give an idea what the
> recommended setup should look like today, ignoring all space saving
> and possible hctosys API changes caused by this. Should
> CONFIG_RTC_SYSTOHC be enabled or not?
Its fine if its enabled. We have logic in the kernel to do the right
thing when we're on a system that has the persistent clock and also has
SYSTOHC enabled.
My patch disabled SYSTOHC just to simplify some of the logic at build
time, and in my mind, simplify the config choices.
But as much as I dislike needless config choices, I realize config churn
isn't great either.
So I do think as we eventually consolidate the RTC and persistent_clock
code, using SYSTOHC in the end is probably a good way to go (although we
may drop the config and just always enable that logic).
That said, I suspect we need RTC equivalents to the xen/kvm/vrtc logic
in the x86 persistent_clock code before we'll be able to tear out the
persistent_clock code (I think just cmos and efi have RTC drivers).
thanks
-john
On 04/25/2013 11:33 AM, Jason Gunthorpe wrote:
> John mentioned they might be kept for embedded - eg size reduction.
> The issue with that idea is if you do not enable the class RTC
> subsystem then there is no way for a small embedded userspace to set
> the RTC. The /dev/rtc* device obviously goes away, but it also turns
> out that that CONFIG_GENERIC_CMOS_UPDATE only works when combined with
> a heavy weight userspace NTPD that runs the kernel PLL properly. The
> kernel NTP code is enormously conservative and it is actually quite
> hard to get it to write to the RTC. An RTC that cannot be set is
> useless, so these days I feel CONFIG_RTC is pragmatically mandatory -
> and my space constrained embedded systems do set it, for these
> reasons.
So I mentioned that the size-reduciton focused folks might not like the
generic rtc core over the persistent_clock code, but I'm not convinced
that's a reason to keep the persistent clock code (which isn't trivial
size-wise itself). Instead either we can shrink the rtc core for those
restricted uses or let them compile out the rtc core all together and
let them manage time initialization completely in userland.
I only noted it, because it has come up prior as a complaint when
switching to the RTC core was proposed.
thanks
-john
On Thu, Apr 25, 2013 at 12:54:15PM -0700, John Stultz wrote:
> That said, I suspect we need RTC equivalents to the xen/kvm/vrtc
> logic in the x86 persistent_clock code before we'll be able to tear
> out the persistent_clock code (I think just cmos and efi have RTC
> drivers).
Aha! Maybe this is why my Xen servers always seem to have the wrong
time in the RTC :)
I'm not sure what is going on with Xen, my Xenserver installs have a
/sys/class/rtc/rtc0 in dom0 which is rtc_cmos (bound via a PNP device),
but the Xen platform code seem to route dom0 set_wallclock to a
hypervisor call.. So, like on normal x86, not sure why userspace and
NTP auto-sync use different code.
I have a feeling the set_wallclock method doesn't actually work,
because I have had several hard crashes on Xensever boxes over the
years and the RTC was always wrong on reboot, this suggests to me the
NTP update of the RTC via set_wallclock perhaps is not working..
Xen Folks: If xen_set_wallclock is bogus, or if using the set method
via in rtc_cmos is OK, please return -ENODEV from dom0
xen_set_wallclock and rely on the new CONFIG_RTC_SYSTOHC path for NTP
synchronization.
I wonder if arch/x86/platform/mrst/vrtc.c and rtc-mrst.c are the same
thing?
Jason
On Thu, Apr 25, 2013 at 01:03:00PM -0700, John Stultz wrote:
> On 04/25/2013 11:33 AM, Jason Gunthorpe wrote:
> >John mentioned they might be kept for embedded - eg size reduction.
> >The issue with that idea is if you do not enable the class RTC
> >subsystem then there is no way for a small embedded userspace to set
> >the RTC. The /dev/rtc* device obviously goes away, but it also turns
> >out that that CONFIG_GENERIC_CMOS_UPDATE only works when combined with
> >a heavy weight userspace NTPD that runs the kernel PLL properly. The
> >kernel NTP code is enormously conservative and it is actually quite
> >hard to get it to write to the RTC. An RTC that cannot be set is
> >useless, so these days I feel CONFIG_RTC is pragmatically mandatory -
> >and my space constrained embedded systems do set it, for these
> >reasons.
>
> So I mentioned that the size-reduciton focused folks might not like
> the generic rtc core over the persistent_clock code, but I'm not
> convinced that's a reason to keep the persistent clock code (which
What I mean is you can't actually choose to use persistent_clock over
rtc core, that is not a choice. The only choice these days it to omit
the user space interface to the RTC (eg rtc core). On some platforms
the RTC will still get *read* during boot via persisent_clock, but no
platform has a way for userspace to *set* via
persisent_clock. update_persisent_clock is not connected to userspace
anymore.
An unsettable RTC is useless, IMHO.
> I only noted it, because it has come up prior as a complaint when
> switching to the RTC core was proposed.
Sure, but, AFAIK, that was a general concern of /dev/rtc vs /dev/rtc0
- however since then we have lost /dev/rtc completely. That means
there is no longer any way to access the persistent_clock functions
from userspace.
Jason