CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[Kay Sievers]

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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[John Stultz]

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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[Kay Sievers]

On Wed, Apr 24, 2013 at 4:43 AM, John Stultz <john.stultz@linaro.org> 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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[John Stultz]

On 04/23/2013 08:05 PM, Kay Sievers wrote:
> On Wed, Apr 24, 2013 at 4:43 AM, John Stultz <john.stultz@linaro.org> 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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[Kay Sievers]

On Wed, Apr 24, 2013 at 5:19 AM, John Stultz <john.stultz@linaro.org> wrote:
> On 04/23/2013 08:05 PM, Kay Sievers wrote:
>>
>> On Wed, Apr 24, 2013 at 4:43 AM, John Stultz <john.stultz@linaro.org>
>> 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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[Kay Sievers]

On Wed, Apr 24, 2013 at 5:33 AM, Kay Sievers <kay@vrfy.org> 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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[John Stultz]

On 04/23/2013 08:51 PM, Kay Sievers wrote:
> On Wed, Apr 24, 2013 at 5:33 AM, Kay Sievers <kay@vrfy.org> 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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[John Stultz]

On 04/23/2013 08:33 PM, Kay Sievers wrote:
> On Wed, Apr 24, 2013 at 5:19 AM, John Stultz <john.stultz@linaro.org> wrote:
>> On 04/23/2013 08:05 PM, Kay Sievers wrote:
>>> On Wed, Apr 24, 2013 at 4:43 AM, John Stultz <john.stultz@linaro.org>
>>> 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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[Kay Sievers]

On Wed, Apr 24, 2013 at 6:30 PM, John Stultz <john.stultz@linaro.org> 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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[Alexander Holler]

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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[John Stultz]

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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[Kay Sievers]

On Wed, Apr 24, 2013 at 6:07 PM, John Stultz <john.stultz@linaro.org> 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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[John Stultz]

On 04/24/2013 09:32 AM, Kay Sievers wrote:
> On Wed, Apr 24, 2013 at 6:07 PM, John Stultz <john.stultz@linaro.org> 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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[Alexander Holler]

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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[Kay Sievers]

On Thu, Apr 25, 2013 at 9:11 AM, Alexander Holler <holler@ahsoftware.de> 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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[John Stultz]

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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[Jason Gunthorpe]

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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[Kay Sievers]

On Thu, Apr 25, 2013 at 8:33 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> 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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[John Stultz]

On 04/25/2013 12:45 PM, Kay Sievers wrote:
> On Thu, Apr 25, 2013 at 8:33 PM, Jason Gunthorpe
> <jgunthorpe@obsidianresearch.com> 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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[Jason Gunthorpe]

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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[John Stultz]

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

Re: CONFIG_RTC_HCTOSYS lost on x86 with ALWAYS_USE_PERSISTENT_CLOCK changes?

[Jason Gunthorpe]

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