Am 20.06.2013 19:27, schrieb John Stultz:On 06/20/2013 03:15 AM, Alexander Holler wrote:Therefor there now will be hctosys as a kernel command line parameter.
Instead of a kernel config option which can't be changed by 99% of all
Linux users, that option allows ordinary (non kernel compiling) users
to disable hctosys at all.
I agree your suggestion of having a hctosys= boot option (to override
the CONFIG_HCTOSYS_DEVICE value) could be a useful extension.
But we shouldn't expect users to set magic boot flags in order to have a
reliably functioning system. If userland sets the time during init, and
the hctosys functionality isn't supposed to overwrite that value, then
there should be no case where userland sets the time at boot, but we end
up with the RTC time after boot. But currently that race is possible
(though small).
A more concrete case:
On many distros ntpd isn't installed by default. Instead they leave the
kernel to initialize the time from the RTC.
Which still is done, even earlier with the new hctosys (if a RTC is used instead of a persistent clock). Nothing changed there. And if the persistent clock is used, which is true on almost all x86 systems, the race doesn't exist at all, at least as long as the persistent clock still exists and will be used (instead of rtc-cmos).
But ntpd can be installed afterwards, and it would be silly to require
users edit their boot arguments when installing the ntp package.
Something which wasn't possible before without recompiling the kernel.
And, like before, most RTC drivers will be loaded before userspace
calls ntp/ntpdate. If not, the system is already broken.
I'm not sure I'm following how the system is already broken?
Because it isn't determined what does set the time. The race you've described happens because someone wants to use ntp for the hctosys functionality but he doesn't want it, if the date might come first from a RTC (in case the race window would be even hit). So the configuration is broken because it is non-deterministic while someone wants deterministic.
And just in case, I've made that possible window for the above race
very small by checking the flag systime_was_set twice, once before
starting to read the time and a second time right before the time is
set. Ok, the race is still there, but as said before, if that problem
does exist at all, the system would be setup wrong at all.
It just seems that if we really want a to do this, we might as well do
it right, using the timekeeping_settime_first() or whatever function
that can properly check the value and complete the action atomically
while holding the lock.
This is basically what this code is trying to avoid in the first place.
And I'll grant that its a small race window, but it may lead to
irregular behavior.
So either we need to document that this race is theoretically possible,
and explain *why* its safe to ignore it. Or if we really want to do
I would think that documenting hctosys=none should be enough.
Again, I don't think users who install ntpd should have to also change
their boot parameters.
All systems I've seen do load modules very early (before they would
start anything ntp related). And the new hctosys is done inside the
registration of the RTC. So if a system is configured in such a way
that the race really might happen, the system would be broken because
the RTC would not be there when starting ntp. To conclude, I think the
problem is highly academic/artificial and, if possible at all, only
possible on already misconfigured systems during a very, very small
window. Therfor I still believe that no locking mechanism is needed.
Anyway, I don't care, if you want, I will make an accessor-function
with locks and an return code for "set" to indicate if it was already
set.
Sorry if I'm frustrating you here, that's not my intent.
I'm not frustrated, I'm just annoyed.
I don't know why everyone seems to assume that I'm getting frustrated while I just find it totally annoying to invest time to make checkpatch-clean max. 80 chars per line wide patches while having to use 8 chars intendation and meaningfull variable and function names. Besides having to explain and discuss every single line and often get called a fool or similiar.
We could also request and wait for a third opinion on that topic. As
it most likely will not end up in any kernel before the end of this
year (if at all), there is enough time to do so.
I'm in no rush. I think the changes you're proposing are interesting and
novel cases that we should handle. But I also do think we should do it
properly, especially since its relatively easy to do in this case.
Anyway, my intention was to avoid locking stuff in the time-setting functions but it looks like that isn't wanted. So I will implement the locking foo, after we finished the discussion about the other parts and I will have come to the impression that I don't write stuff just for nothing. A quick look at your other comments revealed that I have to explain a lot more in order to not get accused that I want to kill the timekeeping system. ;)