Re: [rtc-linux] Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
Am 20.06.2013 21:28, schrieb John Stultz: But ntpd can be installed afterwards, and it would be silly to require users edit their boot arguments when installing the ntp package. This point you left un-addressed, and is the key problem I see with requiring boot arguments. There is no requirement for an additional boot argument. That would only be necessary if you start ntpdate without using a persistent clock and before loading of (working) rtc-modules would have finished. And even then it would only be necessary if you use ntpdate (once) while a driver for the rtc is in it's registration phase and wants to set the clock or if you use ntp and the rtc which registers while ntp sets the time, has a time which would force ntp to refuse further adjustments. And in both cases only, if you don't have disabled the proposed hctosys with using a boot argument. Just because a boot argument makes it possible to disable hctosys by RTC doesn't mean it's necessary. Sorry for beeing pedantic, but I have to defend my decision to avoid locking just because of that possibility. I was fully aware of that unlikely race condition. Anyway, I've already accomplished to add locking to prevent that case. Regards, Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [rtc-linux] Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
On 06/20/2013 11:45 AM, Alexander Holler wrote: 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). Yea, eventually I'd like to push the persistent clock functionality into the RTC core, or remove its use for time initialization and only use the persistent clock for suspend/resume timing. But just because the race doesn't exist on x86, doesn't mean we can ignore it for all the various other arches. But ntpd can be installed afterwards, and it would be silly to require users edit their boot arguments when installing the ntp package. This point you left un-addressed, and is the key problem I see with requiring boot arguments. 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. I still don't see this. As it stands with the current kernel, the HCTOSYS functionality runs prior to init starting, so it may initialize time, but any userland setting of time will have the final say. You're patches allow for the HCTOSYS functionality to happen after init starts. And the systime_was_set flag you proposed seems to address this change in behavior the following way: Assuming userland has not set the clock, allow the HCTOSYS functionality to set the clock after userland has run. This seems like a reasonable balance. However, with your implementation there is a small race possible, such that the hctosys might set the time to RTC time after userland has set the time. You seem to be saying the race is only possible if the system doesn't use the hctosys= boot argument you're also proposing. But machines don't need a boot argument now, and aren't broken, so why do we want to add an option that requires everyone to use it? Personally I think requiring a boot argument is unnecessary (though having a boot option can be helpful in some cases - don't mistake me for thinking the option is a bad idea, I just don't think it should be required) and is actually problematic for distros to handle properly. 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.
Re: [rtc-linux] Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
Am 20.06.2013 19:27, schrieb John Stultz:
On 06/20/2013 03:15 AM, Alexander Holler wrote:
Am 17.06.2013 20:10, schrieb John Stultz:
On 06/14/2013 11:01 PM, Alexander Holler wrote:
What do you think I should write?
void set_systime_was_set(void) and void clear_systime_was_set(void)?
And both functions would have to be exported in order to be usable
from modules?
Or do you think I should write something like that:
extern bool foo;
inline void set_foo(void) { foo = true};
inline void clear_foo(void) { foo = false };
That's just silly, sorry to call it such.
No no. I'm only asking that the boolean be static to timekeeping.c and
an accessor function be used to read it. Since the timekeeping core
should be managing this value, there should be no reason for any other
users to be setting or clearing the value.
First you can't make the value static (semantically) because it has to
be set and cleared from different parts of the kernel. And adding
accessor functions doesn't help in any way, everyone will still be
able to set or clear the value (this still is C and no C++ with
classes or other encapsulation features). The only thing what will
happen with such an accessor function is that a small overhead through
the then necessary function call is introduced.
Why would it be set and cleared from different parts of the kernel?
We're checking if the system time was set. The system time can be set
only from a limited number functions in timekeeping.c. It seems
reasonable it should be static to timekeeping.c
Even so, this is all a tangent. I really think the flag value is racy
and should be dropped for a timekeeping_setime_if_not_set() - or better
named - function that can act atomically.
Of course, I might be wrong and there might be a use case where
multiple things do set the system time concurrently and nothing else
did set system time before, but I found that extremly unlikely.
Yea, the condition check and the action won't be both be done under a
lock, so its likely going to be racy anyway.
And if there ever will be a race for the first timesource to set this
flag (the first time), and something does care about the outtake, the
system would be completly broken.
In order to keep it simple, I just tread userspace like a RTC of type
X and will call them all timesources of type x where a the type is
defined by the driver.
Let us go through the possible cases:
- 2 or more timesources of different type:
If the order is undefined and they have to race for which clock might
be used for hctosys (and thus for adjusting the time after resume
too), the only reason one would want such is for HA purposes. And in
case of HA, both clocks must have the same time, so nobody does care
about which one will win the race (=> no race, no lock or atomic_*
needed).
If the purpose isn't for HA and someone does care about which
timesource should be used, the way to do this is to use hctosys=type
(or hctosys=none in case of userspace) to define which timesource
should be used for hctosys (=> no race, no lock or atomic_* needed).
- 2 or more timesources of the same type:
There is no possibility to define which one should win the race. Such
a system configuration is only usable for HA purposes, so if such
exists, nobody cares about the outtake of the race (=> no race, no
lock or atomic_* needed).
The race I'm thinking of is you have a system that normally sets the
time via ntpdate at bootup. Thus they expect the system to always be
started w/ NTP time (even if the system time was initially set via
hctosys).
Then because of of some delay in the driver (or because the RTC device
was plugged in during boot), the hctosys functionality runs just as
ntpdate is being called. hctosys sees time has not yet been set and
reads the RTC hardware time. At this point, ntpdate sets the time to NTP
time. Then hctosys completes, setting the time to the RTC time. This
results in the system clock being wrong from the user's perspective (as
they expect it to be set to NTP time).
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
Re: [rtc-linux] Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
On 06/20/2013 03:15 AM, Alexander Holler wrote:
Am 17.06.2013 20:10, schrieb John Stultz:
On 06/14/2013 11:01 PM, Alexander Holler wrote:
What do you think I should write?
void set_systime_was_set(void) and void clear_systime_was_set(void)?
And both functions would have to be exported in order to be usable
from modules?
Or do you think I should write something like that:
extern bool foo;
inline void set_foo(void) { foo = true};
inline void clear_foo(void) { foo = false };
That's just silly, sorry to call it such.
No no. I'm only asking that the boolean be static to timekeeping.c and
an accessor function be used to read it. Since the timekeeping core
should be managing this value, there should be no reason for any other
users to be setting or clearing the value.
First you can't make the value static (semantically) because it has to
be set and cleared from different parts of the kernel. And adding
accessor functions doesn't help in any way, everyone will still be
able to set or clear the value (this still is C and no C++ with
classes or other encapsulation features). The only thing what will
happen with such an accessor function is that a small overhead through
the then necessary function call is introduced.
Why would it be set and cleared from different parts of the kernel?
We're checking if the system time was set. The system time can be set
only from a limited number functions in timekeeping.c. It seems
reasonable it should be static to timekeeping.c
Even so, this is all a tangent. I really think the flag value is racy
and should be dropped for a timekeeping_setime_if_not_set() - or better
named - function that can act atomically.
Of course, I might be wrong and there might be a use case where
multiple things do set the system time concurrently and nothing else
did set system time before, but I found that extremly unlikely.
Yea, the condition check and the action won't be both be done under a
lock, so its likely going to be racy anyway.
And if there ever will be a race for the first timesource to set this
flag (the first time), and something does care about the outtake, the
system would be completly broken.
In order to keep it simple, I just tread userspace like a RTC of type
X and will call them all timesources of type x where a the type is
defined by the driver.
Let us go through the possible cases:
- 2 or more timesources of different type:
If the order is undefined and they have to race for which clock might
be used for hctosys (and thus for adjusting the time after resume
too), the only reason one would want such is for HA purposes. And in
case of HA, both clocks must have the same time, so nobody does care
about which one will win the race (=> no race, no lock or atomic_*
needed).
If the purpose isn't for HA and someone does care about which
timesource should be used, the way to do this is to use hctosys=type
(or hctosys=none in case of userspace) to define which timesource
should be used for hctosys (=> no race, no lock or atomic_* needed).
- 2 or more timesources of the same type:
There is no possibility to define which one should win the race. Such
a system configuration is only usable for HA purposes, so if such
exists, nobody cares about the outtake of the race (=> no race, no
lock or atomic_* needed).
The race I'm thinking of is you have a system that normally sets the
time via ntpdate at bootup. Thus they expect the system to always be
started w/ NTP time (even if the system time was initially set via
hctosys).
Then because of of some delay in the driver (or because the RTC device
was plugged in during boot), the hctosys functionality runs just as
ntpdate is being called. hctosys sees time has not yet been set and
reads the RTC hardware time. At this point, ntpdate sets the time to NTP
time. Then hctosys completes, setting the time to the RTC time. This
results in the system clock being wrong from the user's perspective (as
they expect it to be set to NTP time).
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.
But ntpd can be installed afterwards, and it would be silly to require
users edit
Re: [rtc-linux] Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
Am 17.06.2013 20:10, schrieb John Stultz:
On 06/14/2013 11:01 PM, Alexander Holler wrote:
Am 14.06.2013 20:28, schrieb John Stultz:
On 06/14/2013 11:05 AM, Alexander Holler wrote:
Am 14.06.2013 19:41, schrieb John Stultz:
On 06/14/2013 09:52 AM, Alexander Holler wrote:
In order to let an RTC set the time at boot without the problem
that a
second RTC overwrites it, the flag systime_was_set is introduced.
systime_was_set will be true, if a persistent clock sets the time at
boot,
or if do_settimeofday() is called (e.g. by the RTC subsystem or
userspace).
Signed-off-by: Alexander Holler
---
include/linux/time.h | 6 ++
kernel/time/timekeeping.c | 10 +-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/linux/time.h b/include/linux/time.h
index d5d229b..888280f 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -129,6 +129,12 @@ extern int update_persistent_clock(struct
timespec now);
void timekeeping_init(void);
extern int timekeeping_suspended;
+/*
+ * Will be true if the system time was set at least once by
+ * a persistent clock, RTC or userspace.
+ */
+extern bool systime_was_set;
+
Probably should make this static to timekeeping.c and create an
accessor
function so you don't have to export locking rules on this.
unsigned long get_seconds(void);
struct timespec current_kernel_time(void);
struct timespec __current_kernel_time(void); /* does not take
xtime_lock */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index baeeb5c..07d8531 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -37,6 +37,9 @@ int __read_mostly timekeeping_suspended;
/* Flag for if there is a persistent clock on this platform */
bool __read_mostly persistent_clock_exist = false;
+/* Flag for if the system time was set at least once */
+bool __read_mostly systime_was_set;
+
Probably should also move this to be part of the timekeeper structure
(since it will be protected by the timekeeper lock.
I wanted to avoid locks for this silly flag at all. It is only set
once at boot (and resume) and set to 0 at suspend. And I don't see any
possible race condition which could make a lock necessary. Therefor
I've decided to not use a lock or atomic_* in order to skip any delay
in setting the time.
Even so, having random flag variables with special rules being exported
out is likely to cause eventual trouble (someone will mis-use or
overload some meaning on it).
So at least providing a accessor function for non-timekeeping.c uses
would be good.
It's rather hard to misuse a bool (even if a bool in C is just a define).
I'm trying to avoid allowing non-timekeeping users of the value to be
able to set it.
By putting the value behind a timekeeping_systime_was_set() accessor,
and making the boolean value static, we can make sure its properly
managed by the timekeeping code alone.
What do you think I should write?
void set_systime_was_set(void) and void clear_systime_was_set(void)?
And both functions would have to be exported in order to be usable
from modules?
Or do you think I should write something like that:
extern bool foo;
inline void set_foo(void) { foo = true};
inline void clear_foo(void) { foo = false };
That's just silly, sorry to call it such.
No no. I'm only asking that the boolean be static to timekeeping.c and
an accessor function be used to read it. Since the timekeeping core
should be managing this value, there should be no reason for any other
users to be setting or clearing the value.
First you can't make the value static (semantically) because it has to
be set and cleared from different parts of the kernel. And adding
accessor functions doesn't help in any way, everyone will still be able
to set or clear the value (this still is C and no C++ with classes or
other encapsulation features). The only thing what will happen with such
an accessor function is that a small overhead through the then necessary
function call is introduced.
Of course, I might be wrong and there might be a use case where
multiple things do set the system time concurrently and nothing else
did set system time before, but I found that extremly unlikely.
Yea, the condition check and the action won't be both be done under a
lock, so its likely going to be racy anyway.
And if there ever will be a race for the first timesource to set this
flag (the first time), and something does care about the outtake, the
system would be completly broken.
In order to keep it simple, I just tread userspace like a RTC of type
X and will call them all timesources of type x where a the type is
defined by the driver.
Let us go through the possible cases:
- 2 or more timesources of different type:
If the order is undefined and they have to race for which clock might
be used for hctosys (and thus for adjusting the time after resume
too), the only reason one would want such is for HA purposes. And in
case of HA, both clocks
Re: [rtc-linux] Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
Am 17.06.2013 20:10, schrieb John Stultz:
On 06/14/2013 11:01 PM, Alexander Holler wrote:
Am 14.06.2013 20:28, schrieb John Stultz:
On 06/14/2013 11:05 AM, Alexander Holler wrote:
Am 14.06.2013 19:41, schrieb John Stultz:
On 06/14/2013 09:52 AM, Alexander Holler wrote:
In order to let an RTC set the time at boot without the problem
that a
second RTC overwrites it, the flag systime_was_set is introduced.
systime_was_set will be true, if a persistent clock sets the time at
boot,
or if do_settimeofday() is called (e.g. by the RTC subsystem or
userspace).
Signed-off-by: Alexander Holler hol...@ahsoftware.de
---
include/linux/time.h | 6 ++
kernel/time/timekeeping.c | 10 +-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/linux/time.h b/include/linux/time.h
index d5d229b..888280f 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -129,6 +129,12 @@ extern int update_persistent_clock(struct
timespec now);
void timekeeping_init(void);
extern int timekeeping_suspended;
+/*
+ * Will be true if the system time was set at least once by
+ * a persistent clock, RTC or userspace.
+ */
+extern bool systime_was_set;
+
Probably should make this static to timekeeping.c and create an
accessor
function so you don't have to export locking rules on this.
unsigned long get_seconds(void);
struct timespec current_kernel_time(void);
struct timespec __current_kernel_time(void); /* does not take
xtime_lock */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index baeeb5c..07d8531 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -37,6 +37,9 @@ int __read_mostly timekeeping_suspended;
/* Flag for if there is a persistent clock on this platform */
bool __read_mostly persistent_clock_exist = false;
+/* Flag for if the system time was set at least once */
+bool __read_mostly systime_was_set;
+
Probably should also move this to be part of the timekeeper structure
(since it will be protected by the timekeeper lock.
I wanted to avoid locks for this silly flag at all. It is only set
once at boot (and resume) and set to 0 at suspend. And I don't see any
possible race condition which could make a lock necessary. Therefor
I've decided to not use a lock or atomic_* in order to skip any delay
in setting the time.
Even so, having random flag variables with special rules being exported
out is likely to cause eventual trouble (someone will mis-use or
overload some meaning on it).
So at least providing a accessor function for non-timekeeping.c uses
would be good.
It's rather hard to misuse a bool (even if a bool in C is just a define).
I'm trying to avoid allowing non-timekeeping users of the value to be
able to set it.
By putting the value behind a timekeeping_systime_was_set() accessor,
and making the boolean value static, we can make sure its properly
managed by the timekeeping code alone.
What do you think I should write?
void set_systime_was_set(void) and void clear_systime_was_set(void)?
And both functions would have to be exported in order to be usable
from modules?
Or do you think I should write something like that:
extern bool foo;
inline void set_foo(void) { foo = true};
inline void clear_foo(void) { foo = false };
That's just silly, sorry to call it such.
No no. I'm only asking that the boolean be static to timekeeping.c and
an accessor function be used to read it. Since the timekeeping core
should be managing this value, there should be no reason for any other
users to be setting or clearing the value.
First you can't make the value static (semantically) because it has to
be set and cleared from different parts of the kernel. And adding
accessor functions doesn't help in any way, everyone will still be able
to set or clear the value (this still is C and no C++ with classes or
other encapsulation features). The only thing what will happen with such
an accessor function is that a small overhead through the then necessary
function call is introduced.
Of course, I might be wrong and there might be a use case where
multiple things do set the system time concurrently and nothing else
did set system time before, but I found that extremly unlikely.
Yea, the condition check and the action won't be both be done under a
lock, so its likely going to be racy anyway.
And if there ever will be a race for the first timesource to set this
flag (the first time), and something does care about the outtake, the
system would be completly broken.
In order to keep it simple, I just tread userspace like a RTC of type
X and will call them all timesources of type x where a the type is
defined by the driver.
Let us go through the possible cases:
- 2 or more timesources of different type:
If the order is undefined and they have to race for which clock might
be used for hctosys (and thus for adjusting the time after resume
too), the only reason one would want such is for HA purposes. And in
case of
Re: [rtc-linux] Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
On 06/20/2013 03:15 AM, Alexander Holler wrote:
Am 17.06.2013 20:10, schrieb John Stultz:
On 06/14/2013 11:01 PM, Alexander Holler wrote:
What do you think I should write?
void set_systime_was_set(void) and void clear_systime_was_set(void)?
And both functions would have to be exported in order to be usable
from modules?
Or do you think I should write something like that:
extern bool foo;
inline void set_foo(void) { foo = true};
inline void clear_foo(void) { foo = false };
That's just silly, sorry to call it such.
No no. I'm only asking that the boolean be static to timekeeping.c and
an accessor function be used to read it. Since the timekeeping core
should be managing this value, there should be no reason for any other
users to be setting or clearing the value.
First you can't make the value static (semantically) because it has to
be set and cleared from different parts of the kernel. And adding
accessor functions doesn't help in any way, everyone will still be
able to set or clear the value (this still is C and no C++ with
classes or other encapsulation features). The only thing what will
happen with such an accessor function is that a small overhead through
the then necessary function call is introduced.
Why would it be set and cleared from different parts of the kernel?
We're checking if the system time was set. The system time can be set
only from a limited number functions in timekeeping.c. It seems
reasonable it should be static to timekeeping.c
Even so, this is all a tangent. I really think the flag value is racy
and should be dropped for a timekeeping_setime_if_not_set() - or better
named - function that can act atomically.
Of course, I might be wrong and there might be a use case where
multiple things do set the system time concurrently and nothing else
did set system time before, but I found that extremly unlikely.
Yea, the condition check and the action won't be both be done under a
lock, so its likely going to be racy anyway.
And if there ever will be a race for the first timesource to set this
flag (the first time), and something does care about the outtake, the
system would be completly broken.
In order to keep it simple, I just tread userspace like a RTC of type
X and will call them all timesources of type x where a the type is
defined by the driver.
Let us go through the possible cases:
- 2 or more timesources of different type:
If the order is undefined and they have to race for which clock might
be used for hctosys (and thus for adjusting the time after resume
too), the only reason one would want such is for HA purposes. And in
case of HA, both clocks must have the same time, so nobody does care
about which one will win the race (= no race, no lock or atomic_*
needed).
If the purpose isn't for HA and someone does care about which
timesource should be used, the way to do this is to use hctosys=type
(or hctosys=none in case of userspace) to define which timesource
should be used for hctosys (= no race, no lock or atomic_* needed).
- 2 or more timesources of the same type:
There is no possibility to define which one should win the race. Such
a system configuration is only usable for HA purposes, so if such
exists, nobody cares about the outtake of the race (= no race, no
lock or atomic_* needed).
The race I'm thinking of is you have a system that normally sets the
time via ntpdate at bootup. Thus they expect the system to always be
started w/ NTP time (even if the system time was initially set via
hctosys).
Then because of of some delay in the driver (or because the RTC device
was plugged in during boot), the hctosys functionality runs just as
ntpdate is being called. hctosys sees time has not yet been set and
reads the RTC hardware time. At this point, ntpdate sets the time to NTP
time. Then hctosys completes, setting the time to the RTC time. This
results in the system clock being wrong from the user's perspective (as
they expect it to be set to NTP time).
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.
But ntpd can be installed afterwards, and it would be silly to require
users edit their
Re: [rtc-linux] Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
Am 20.06.2013 19:27, schrieb John Stultz:
On 06/20/2013 03:15 AM, Alexander Holler wrote:
Am 17.06.2013 20:10, schrieb John Stultz:
On 06/14/2013 11:01 PM, Alexander Holler wrote:
What do you think I should write?
void set_systime_was_set(void) and void clear_systime_was_set(void)?
And both functions would have to be exported in order to be usable
from modules?
Or do you think I should write something like that:
extern bool foo;
inline void set_foo(void) { foo = true};
inline void clear_foo(void) { foo = false };
That's just silly, sorry to call it such.
No no. I'm only asking that the boolean be static to timekeeping.c and
an accessor function be used to read it. Since the timekeeping core
should be managing this value, there should be no reason for any other
users to be setting or clearing the value.
First you can't make the value static (semantically) because it has to
be set and cleared from different parts of the kernel. And adding
accessor functions doesn't help in any way, everyone will still be
able to set or clear the value (this still is C and no C++ with
classes or other encapsulation features). The only thing what will
happen with such an accessor function is that a small overhead through
the then necessary function call is introduced.
Why would it be set and cleared from different parts of the kernel?
We're checking if the system time was set. The system time can be set
only from a limited number functions in timekeeping.c. It seems
reasonable it should be static to timekeeping.c
Even so, this is all a tangent. I really think the flag value is racy
and should be dropped for a timekeeping_setime_if_not_set() - or better
named - function that can act atomically.
Of course, I might be wrong and there might be a use case where
multiple things do set the system time concurrently and nothing else
did set system time before, but I found that extremly unlikely.
Yea, the condition check and the action won't be both be done under a
lock, so its likely going to be racy anyway.
And if there ever will be a race for the first timesource to set this
flag (the first time), and something does care about the outtake, the
system would be completly broken.
In order to keep it simple, I just tread userspace like a RTC of type
X and will call them all timesources of type x where a the type is
defined by the driver.
Let us go through the possible cases:
- 2 or more timesources of different type:
If the order is undefined and they have to race for which clock might
be used for hctosys (and thus for adjusting the time after resume
too), the only reason one would want such is for HA purposes. And in
case of HA, both clocks must have the same time, so nobody does care
about which one will win the race (= no race, no lock or atomic_*
needed).
If the purpose isn't for HA and someone does care about which
timesource should be used, the way to do this is to use hctosys=type
(or hctosys=none in case of userspace) to define which timesource
should be used for hctosys (= no race, no lock or atomic_* needed).
- 2 or more timesources of the same type:
There is no possibility to define which one should win the race. Such
a system configuration is only usable for HA purposes, so if such
exists, nobody cares about the outtake of the race (= no race, no
lock or atomic_* needed).
The race I'm thinking of is you have a system that normally sets the
time via ntpdate at bootup. Thus they expect the system to always be
started w/ NTP time (even if the system time was initially set via
hctosys).
Then because of of some delay in the driver (or because the RTC device
was plugged in during boot), the hctosys functionality runs just as
ntpdate is being called. hctosys sees time has not yet been set and
reads the RTC hardware time. At this point, ntpdate sets the time to NTP
time. Then hctosys completes, setting the time to the RTC time. This
results in the system clock being wrong from the user's perspective (as
they expect it to be set to NTP time).
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
Re: [rtc-linux] Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
On 06/20/2013 11:45 AM, Alexander Holler wrote: 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). Yea, eventually I'd like to push the persistent clock functionality into the RTC core, or remove its use for time initialization and only use the persistent clock for suspend/resume timing. But just because the race doesn't exist on x86, doesn't mean we can ignore it for all the various other arches. But ntpd can be installed afterwards, and it would be silly to require users edit their boot arguments when installing the ntp package. This point you left un-addressed, and is the key problem I see with requiring boot arguments. 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. I still don't see this. As it stands with the current kernel, the HCTOSYS functionality runs prior to init starting, so it may initialize time, but any userland setting of time will have the final say. You're patches allow for the HCTOSYS functionality to happen after init starts. And the systime_was_set flag you proposed seems to address this change in behavior the following way: Assuming userland has not set the clock, allow the HCTOSYS functionality to set the clock after userland has run. This seems like a reasonable balance. However, with your implementation there is a small race possible, such that the hctosys might set the time to RTC time after userland has set the time. You seem to be saying the race is only possible if the system doesn't use the hctosys= boot argument you're also proposing. But machines don't need a boot argument now, and aren't broken, so why do we want to add an option that requires everyone to use it? Personally I think requiring a boot argument is unnecessary (though having a boot option can be helpful in some cases - don't mistake me for thinking the option is a bad idea, I just don't think it should be required) and is actually problematic for distros to handle properly. 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.
Re: [rtc-linux] Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
Am 20.06.2013 21:28, schrieb John Stultz: But ntpd can be installed afterwards, and it would be silly to require users edit their boot arguments when installing the ntp package. This point you left un-addressed, and is the key problem I see with requiring boot arguments. There is no requirement for an additional boot argument. That would only be necessary if you start ntpdate without using a persistent clock and before loading of (working) rtc-modules would have finished. And even then it would only be necessary if you use ntpdate (once) while a driver for the rtc is in it's registration phase and wants to set the clock or if you use ntp and the rtc which registers while ntp sets the time, has a time which would force ntp to refuse further adjustments. And in both cases only, if you don't have disabled the proposed hctosys with using a boot argument. Just because a boot argument makes it possible to disable hctosys by RTC doesn't mean it's necessary. Sorry for beeing pedantic, but I have to defend my decision to avoid locking just because of that possibility. I was fully aware of that unlikely race condition. Anyway, I've already accomplished to add locking to prevent that case. Regards, Alexander Holler -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [rtc-linux] Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
On 06/14/2013 11:01 PM, Alexander Holler wrote:
Am 14.06.2013 20:28, schrieb John Stultz:
On 06/14/2013 11:05 AM, Alexander Holler wrote:
Am 14.06.2013 19:41, schrieb John Stultz:
On 06/14/2013 09:52 AM, Alexander Holler wrote:
In order to let an RTC set the time at boot without the problem
that a
second RTC overwrites it, the flag systime_was_set is introduced.
systime_was_set will be true, if a persistent clock sets the time at
boot,
or if do_settimeofday() is called (e.g. by the RTC subsystem or
userspace).
Signed-off-by: Alexander Holler
---
include/linux/time.h | 6 ++
kernel/time/timekeeping.c | 10 +-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/linux/time.h b/include/linux/time.h
index d5d229b..888280f 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -129,6 +129,12 @@ extern int update_persistent_clock(struct
timespec now);
void timekeeping_init(void);
extern int timekeeping_suspended;
+/*
+ * Will be true if the system time was set at least once by
+ * a persistent clock, RTC or userspace.
+ */
+extern bool systime_was_set;
+
Probably should make this static to timekeeping.c and create an
accessor
function so you don't have to export locking rules on this.
unsigned long get_seconds(void);
struct timespec current_kernel_time(void);
struct timespec __current_kernel_time(void); /* does not take
xtime_lock */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index baeeb5c..07d8531 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -37,6 +37,9 @@ int __read_mostly timekeeping_suspended;
/* Flag for if there is a persistent clock on this platform */
bool __read_mostly persistent_clock_exist = false;
+/* Flag for if the system time was set at least once */
+bool __read_mostly systime_was_set;
+
Probably should also move this to be part of the timekeeper structure
(since it will be protected by the timekeeper lock.
I wanted to avoid locks for this silly flag at all. It is only set
once at boot (and resume) and set to 0 at suspend. And I don't see any
possible race condition which could make a lock necessary. Therefor
I've decided to not use a lock or atomic_* in order to skip any delay
in setting the time.
Even so, having random flag variables with special rules being exported
out is likely to cause eventual trouble (someone will mis-use or
overload some meaning on it).
So at least providing a accessor function for non-timekeeping.c uses
would be good.
It's rather hard to misuse a bool (even if a bool in C is just a define).
I'm trying to avoid allowing non-timekeeping users of the value to be
able to set it.
By putting the value behind a timekeeping_systime_was_set() accessor,
and making the boolean value static, we can make sure its properly
managed by the timekeeping code alone.
What do you think I should write?
void set_systime_was_set(void) and void clear_systime_was_set(void)?
And both functions would have to be exported in order to be usable
from modules?
Or do you think I should write something like that:
extern bool foo;
inline void set_foo(void) { foo = true};
inline void clear_foo(void) { foo = false };
That's just silly, sorry to call it such.
No no. I'm only asking that the boolean be static to timekeeping.c and
an accessor function be used to read it. Since the timekeeping core
should be managing this value, there should be no reason for any other
users to be setting or clearing the value.
Of course, I might be wrong and there might be a use case where
multiple things do set the system time concurrently and nothing else
did set system time before, but I found that extremly unlikely.
Yea, the condition check and the action won't be both be done under a
lock, so its likely going to be racy anyway.
And if there ever will be a race for the first timesource to set this
flag (the first time), and something does care about the outtake, the
system would be completly broken.
In order to keep it simple, I just tread userspace like a RTC of type
X and will call them all timesources of type x where a the type is
defined by the driver.
Let us go through the possible cases:
- 2 or more timesources of different type:
If the order is undefined and they have to race for which clock might
be used for hctosys (and thus for adjusting the time after resume
too), the only reason one would want such is for HA purposes. And in
case of HA, both clocks must have the same time, so nobody does care
about which one will win the race (=> no race, no lock or atomic_*
needed).
If the purpose isn't for HA and someone does care about which
timesource should be used, the way to do this is to use hctosys=type
(or hctosys=none in case of userspace) to define which timesource
should be used for hctosys (=> no race, no lock or atomic_* needed).
- 2 or more timesources of the same type:
There is no possibility to define which one should
Re: [rtc-linux] Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
On 06/14/2013 11:01 PM, Alexander Holler wrote:
Am 14.06.2013 20:28, schrieb John Stultz:
On 06/14/2013 11:05 AM, Alexander Holler wrote:
Am 14.06.2013 19:41, schrieb John Stultz:
On 06/14/2013 09:52 AM, Alexander Holler wrote:
In order to let an RTC set the time at boot without the problem
that a
second RTC overwrites it, the flag systime_was_set is introduced.
systime_was_set will be true, if a persistent clock sets the time at
boot,
or if do_settimeofday() is called (e.g. by the RTC subsystem or
userspace).
Signed-off-by: Alexander Holler hol...@ahsoftware.de
---
include/linux/time.h | 6 ++
kernel/time/timekeeping.c | 10 +-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/linux/time.h b/include/linux/time.h
index d5d229b..888280f 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -129,6 +129,12 @@ extern int update_persistent_clock(struct
timespec now);
void timekeeping_init(void);
extern int timekeeping_suspended;
+/*
+ * Will be true if the system time was set at least once by
+ * a persistent clock, RTC or userspace.
+ */
+extern bool systime_was_set;
+
Probably should make this static to timekeeping.c and create an
accessor
function so you don't have to export locking rules on this.
unsigned long get_seconds(void);
struct timespec current_kernel_time(void);
struct timespec __current_kernel_time(void); /* does not take
xtime_lock */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index baeeb5c..07d8531 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -37,6 +37,9 @@ int __read_mostly timekeeping_suspended;
/* Flag for if there is a persistent clock on this platform */
bool __read_mostly persistent_clock_exist = false;
+/* Flag for if the system time was set at least once */
+bool __read_mostly systime_was_set;
+
Probably should also move this to be part of the timekeeper structure
(since it will be protected by the timekeeper lock.
I wanted to avoid locks for this silly flag at all. It is only set
once at boot (and resume) and set to 0 at suspend. And I don't see any
possible race condition which could make a lock necessary. Therefor
I've decided to not use a lock or atomic_* in order to skip any delay
in setting the time.
Even so, having random flag variables with special rules being exported
out is likely to cause eventual trouble (someone will mis-use or
overload some meaning on it).
So at least providing a accessor function for non-timekeeping.c uses
would be good.
It's rather hard to misuse a bool (even if a bool in C is just a define).
I'm trying to avoid allowing non-timekeeping users of the value to be
able to set it.
By putting the value behind a timekeeping_systime_was_set() accessor,
and making the boolean value static, we can make sure its properly
managed by the timekeeping code alone.
What do you think I should write?
void set_systime_was_set(void) and void clear_systime_was_set(void)?
And both functions would have to be exported in order to be usable
from modules?
Or do you think I should write something like that:
extern bool foo;
inline void set_foo(void) { foo = true};
inline void clear_foo(void) { foo = false };
That's just silly, sorry to call it such.
No no. I'm only asking that the boolean be static to timekeeping.c and
an accessor function be used to read it. Since the timekeeping core
should be managing this value, there should be no reason for any other
users to be setting or clearing the value.
Of course, I might be wrong and there might be a use case where
multiple things do set the system time concurrently and nothing else
did set system time before, but I found that extremly unlikely.
Yea, the condition check and the action won't be both be done under a
lock, so its likely going to be racy anyway.
And if there ever will be a race for the first timesource to set this
flag (the first time), and something does care about the outtake, the
system would be completly broken.
In order to keep it simple, I just tread userspace like a RTC of type
X and will call them all timesources of type x where a the type is
defined by the driver.
Let us go through the possible cases:
- 2 or more timesources of different type:
If the order is undefined and they have to race for which clock might
be used for hctosys (and thus for adjusting the time after resume
too), the only reason one would want such is for HA purposes. And in
case of HA, both clocks must have the same time, so nobody does care
about which one will win the race (= no race, no lock or atomic_*
needed).
If the purpose isn't for HA and someone does care about which
timesource should be used, the way to do this is to use hctosys=type
(or hctosys=none in case of userspace) to define which timesource
should be used for hctosys (= no race, no lock or atomic_* needed).
- 2 or more timesources of the same type:
There is no possibility to define
Re: [rtc-linux] Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
Am 14.06.2013 20:28, schrieb John Stultz:
On 06/14/2013 11:05 AM, Alexander Holler wrote:
Am 14.06.2013 19:41, schrieb John Stultz:
On 06/14/2013 09:52 AM, Alexander Holler wrote:
In order to let an RTC set the time at boot without the problem that a
second RTC overwrites it, the flag systime_was_set is introduced.
systime_was_set will be true, if a persistent clock sets the time at
boot,
or if do_settimeofday() is called (e.g. by the RTC subsystem or
userspace).
Signed-off-by: Alexander Holler
---
include/linux/time.h | 6 ++
kernel/time/timekeeping.c | 10 +-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/linux/time.h b/include/linux/time.h
index d5d229b..888280f 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -129,6 +129,12 @@ extern int update_persistent_clock(struct
timespec now);
void timekeeping_init(void);
extern int timekeeping_suspended;
+/*
+ * Will be true if the system time was set at least once by
+ * a persistent clock, RTC or userspace.
+ */
+extern bool systime_was_set;
+
Probably should make this static to timekeeping.c and create an accessor
function so you don't have to export locking rules on this.
unsigned long get_seconds(void);
struct timespec current_kernel_time(void);
struct timespec __current_kernel_time(void); /* does not take
xtime_lock */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index baeeb5c..07d8531 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -37,6 +37,9 @@ int __read_mostly timekeeping_suspended;
/* Flag for if there is a persistent clock on this platform */
bool __read_mostly persistent_clock_exist = false;
+/* Flag for if the system time was set at least once */
+bool __read_mostly systime_was_set;
+
Probably should also move this to be part of the timekeeper structure
(since it will be protected by the timekeeper lock.
I wanted to avoid locks for this silly flag at all. It is only set
once at boot (and resume) and set to 0 at suspend. And I don't see any
possible race condition which could make a lock necessary. Therefor
I've decided to not use a lock or atomic_* in order to skip any delay
in setting the time.
Even so, having random flag variables with special rules being exported
out is likely to cause eventual trouble (someone will mis-use or
overload some meaning on it).
So at least providing a accessor function for non-timekeeping.c uses
would be good.
It's rather hard to misuse a bool (even if a bool in C is just a define).
What do you think I should write?
void set_systime_was_set(void) and void clear_systime_was_set(void)?
And both functions would have to be exported in order to be usable from
modules?
Or do you think I should write something like that:
extern bool foo;
inline void set_foo(void) { foo = true};
inline void clear_foo(void) { foo = false };
That's just silly, sorry to call it such.
So now I'm unsure if I'm able to continue work on this series. I seem to
be unable to think and code like maintainers do want I should think and
code, whatever that might be.
Of course, I might be wrong and there might be a use case where
multiple things do set the system time concurrently and nothing else
did set system time before, but I found that extremly unlikely.
Yea, the condition check and the action won't be both be done under a
lock, so its likely going to be racy anyway.
And if there ever will be a race for the first timesource to set this
flag (the first time), and something does care about the outtake, the
system would be completly broken.
In order to keep it simple, I just tread userspace like a RTC of type X
and will call them all timesources of type x where a the type is defined
by the driver.
Let us go through the possible cases:
- 2 or more timesources of different type:
If the order is undefined and they have to race for which clock might be
used for hctosys (and thus for adjusting the time after resume too), the
only reason one would want such is for HA purposes. And in case of HA,
both clocks must have the same time, so nobody does care about which one
will win the race (=> no race, no lock or atomic_* needed).
If the purpose isn't for HA and someone does care about which timesource
should be used, the way to do this is to use hctosys=type (or
hctosys=none in case of userspace) to define which timesource should be
used for hctosys (=> no race, no lock or atomic_* needed).
- 2 or more timesources of the same type:
There is no possibility to define which one should win the race. Such a
system configuration is only usable for HA purposes, so if such exists,
nobody cares about the outtake of the race (=> no race, no lock or
atomic_* needed).
Regards,
Alexander Holler
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at
Re: [rtc-linux] Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
Am 14.06.2013 20:28, schrieb John Stultz:
On 06/14/2013 11:05 AM, Alexander Holler wrote:
Am 14.06.2013 19:41, schrieb John Stultz:
On 06/14/2013 09:52 AM, Alexander Holler wrote:
In order to let an RTC set the time at boot without the problem that a
second RTC overwrites it, the flag systime_was_set is introduced.
systime_was_set will be true, if a persistent clock sets the time at
boot,
or if do_settimeofday() is called (e.g. by the RTC subsystem or
userspace).
Signed-off-by: Alexander Holler hol...@ahsoftware.de
---
include/linux/time.h | 6 ++
kernel/time/timekeeping.c | 10 +-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/linux/time.h b/include/linux/time.h
index d5d229b..888280f 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -129,6 +129,12 @@ extern int update_persistent_clock(struct
timespec now);
void timekeeping_init(void);
extern int timekeeping_suspended;
+/*
+ * Will be true if the system time was set at least once by
+ * a persistent clock, RTC or userspace.
+ */
+extern bool systime_was_set;
+
Probably should make this static to timekeeping.c and create an accessor
function so you don't have to export locking rules on this.
unsigned long get_seconds(void);
struct timespec current_kernel_time(void);
struct timespec __current_kernel_time(void); /* does not take
xtime_lock */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index baeeb5c..07d8531 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -37,6 +37,9 @@ int __read_mostly timekeeping_suspended;
/* Flag for if there is a persistent clock on this platform */
bool __read_mostly persistent_clock_exist = false;
+/* Flag for if the system time was set at least once */
+bool __read_mostly systime_was_set;
+
Probably should also move this to be part of the timekeeper structure
(since it will be protected by the timekeeper lock.
I wanted to avoid locks for this silly flag at all. It is only set
once at boot (and resume) and set to 0 at suspend. And I don't see any
possible race condition which could make a lock necessary. Therefor
I've decided to not use a lock or atomic_* in order to skip any delay
in setting the time.
Even so, having random flag variables with special rules being exported
out is likely to cause eventual trouble (someone will mis-use or
overload some meaning on it).
So at least providing a accessor function for non-timekeeping.c uses
would be good.
It's rather hard to misuse a bool (even if a bool in C is just a define).
What do you think I should write?
void set_systime_was_set(void) and void clear_systime_was_set(void)?
And both functions would have to be exported in order to be usable from
modules?
Or do you think I should write something like that:
extern bool foo;
inline void set_foo(void) { foo = true};
inline void clear_foo(void) { foo = false };
That's just silly, sorry to call it such.
So now I'm unsure if I'm able to continue work on this series. I seem to
be unable to think and code like maintainers do want I should think and
code, whatever that might be.
Of course, I might be wrong and there might be a use case where
multiple things do set the system time concurrently and nothing else
did set system time before, but I found that extremly unlikely.
Yea, the condition check and the action won't be both be done under a
lock, so its likely going to be racy anyway.
And if there ever will be a race for the first timesource to set this
flag (the first time), and something does care about the outtake, the
system would be completly broken.
In order to keep it simple, I just tread userspace like a RTC of type X
and will call them all timesources of type x where a the type is defined
by the driver.
Let us go through the possible cases:
- 2 or more timesources of different type:
If the order is undefined and they have to race for which clock might be
used for hctosys (and thus for adjusting the time after resume too), the
only reason one would want such is for HA purposes. And in case of HA,
both clocks must have the same time, so nobody does care about which one
will win the race (= no race, no lock or atomic_* needed).
If the purpose isn't for HA and someone does care about which timesource
should be used, the way to do this is to use hctosys=type (or
hctosys=none in case of userspace) to define which timesource should be
used for hctosys (= no race, no lock or atomic_* needed).
- 2 or more timesources of the same type:
There is no possibility to define which one should win the race. Such a
system configuration is only usable for HA purposes, so if such exists,
nobody cares about the outtake of the race (= no race, no lock or
atomic_* needed).
Regards,
Alexander Holler
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at
Re: [rtc-linux] Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
On 06/14/2013 11:05 AM, Alexander Holler wrote:
Am 14.06.2013 19:41, schrieb John Stultz:
On 06/14/2013 09:52 AM, Alexander Holler wrote:
In order to let an RTC set the time at boot without the problem that a
second RTC overwrites it, the flag systime_was_set is introduced.
systime_was_set will be true, if a persistent clock sets the time at
boot,
or if do_settimeofday() is called (e.g. by the RTC subsystem or
userspace).
Signed-off-by: Alexander Holler
---
include/linux/time.h | 6 ++
kernel/time/timekeeping.c | 10 +-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/linux/time.h b/include/linux/time.h
index d5d229b..888280f 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -129,6 +129,12 @@ extern int update_persistent_clock(struct
timespec now);
void timekeeping_init(void);
extern int timekeeping_suspended;
+/*
+ * Will be true if the system time was set at least once by
+ * a persistent clock, RTC or userspace.
+ */
+extern bool systime_was_set;
+
Probably should make this static to timekeeping.c and create an accessor
function so you don't have to export locking rules on this.
unsigned long get_seconds(void);
struct timespec current_kernel_time(void);
struct timespec __current_kernel_time(void); /* does not take
xtime_lock */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index baeeb5c..07d8531 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -37,6 +37,9 @@ int __read_mostly timekeeping_suspended;
/* Flag for if there is a persistent clock on this platform */
bool __read_mostly persistent_clock_exist = false;
+/* Flag for if the system time was set at least once */
+bool __read_mostly systime_was_set;
+
Probably should also move this to be part of the timekeeper structure
(since it will be protected by the timekeeper lock.
I wanted to avoid locks for this silly flag at all. It is only set
once at boot (and resume) and set to 0 at suspend. And I don't see any
possible race condition which could make a lock necessary. Therefor
I've decided to not use a lock or atomic_* in order to skip any delay
in setting the time.
Even so, having random flag variables with special rules being exported
out is likely to cause eventual trouble (someone will mis-use or
overload some meaning on it).
So at least providing a accessor function for non-timekeeping.c uses
would be good.
Of course, I might be wrong and there might be a use case where
multiple things do set the system time concurrently and nothing else
did set system time before, but I found that extremly unlikely.
Yea, the condition check and the action won't be both be done under a
lock, so its likely going to be racy anyway.
static inline void tk_normalize_xtime(struct timekeeper *tk)
{
while (tk->xtime_nsec >= ((u64)NSEC_PER_SEC << tk->shift)) {
@@ -498,6 +501,9 @@ int do_settimeofday(const struct timespec *tv)
raw_spin_lock_irqsave(_lock, flags);
write_seqcount_begin(_seq);
+systime_was_set = true;
+
+
timekeeping_forward_now(tk);
xt = tk_xtime(tk);
Might also want to add the flag to inject_offset as well, since that
could be used to set the time.
I wasn't sure about that because I had only a quick look at
inject_offset() and had the impression it's only able to inject a
relative small offset (so not usable at boot). And, as written
sometimes before, I haven't had a deep look at suspend/resume, which
might be the only place where it is really used to set the clock when
systime_was_set is false.
Not via suspend/resume, since those modify the boottime to account for
the sleep time that has past.
I'm thinking via adjtimex ADJ_SETOFFSET (which is relatively new, and
not widely used). See the do_adjtimex() path in timekeeping.c
thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [rtc-linux] Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
Am 14.06.2013 19:41, schrieb John Stultz:
On 06/14/2013 09:52 AM, Alexander Holler wrote:
In order to let an RTC set the time at boot without the problem that a
second RTC overwrites it, the flag systime_was_set is introduced.
systime_was_set will be true, if a persistent clock sets the time at
boot,
or if do_settimeofday() is called (e.g. by the RTC subsystem or
userspace).
Signed-off-by: Alexander Holler
---
include/linux/time.h | 6 ++
kernel/time/timekeeping.c | 10 +-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/linux/time.h b/include/linux/time.h
index d5d229b..888280f 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -129,6 +129,12 @@ extern int update_persistent_clock(struct
timespec now);
void timekeeping_init(void);
extern int timekeeping_suspended;
+/*
+ * Will be true if the system time was set at least once by
+ * a persistent clock, RTC or userspace.
+ */
+extern bool systime_was_set;
+
Probably should make this static to timekeeping.c and create an accessor
function so you don't have to export locking rules on this.
unsigned long get_seconds(void);
struct timespec current_kernel_time(void);
struct timespec __current_kernel_time(void); /* does not take
xtime_lock */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index baeeb5c..07d8531 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -37,6 +37,9 @@ int __read_mostly timekeeping_suspended;
/* Flag for if there is a persistent clock on this platform */
bool __read_mostly persistent_clock_exist = false;
+/* Flag for if the system time was set at least once */
+bool __read_mostly systime_was_set;
+
Probably should also move this to be part of the timekeeper structure
(since it will be protected by the timekeeper lock.
I wanted to avoid locks for this silly flag at all. It is only set once
at boot (and resume) and set to 0 at suspend. And I don't see any
possible race condition which could make a lock necessary. Therefor I've
decided to not use a lock or atomic_* in order to skip any delay in
setting the time.
Of course, I might be wrong and there might be a use case where multiple
things do set the system time concurrently and nothing else did set
system time before, but I found that extremly unlikely.
static inline void tk_normalize_xtime(struct timekeeper *tk)
{
while (tk->xtime_nsec >= ((u64)NSEC_PER_SEC << tk->shift)) {
@@ -498,6 +501,9 @@ int do_settimeofday(const struct timespec *tv)
raw_spin_lock_irqsave(_lock, flags);
write_seqcount_begin(_seq);
+systime_was_set = true;
+
+
timekeeping_forward_now(tk);
xt = tk_xtime(tk);
Might also want to add the flag to inject_offset as well, since that
could be used to set the time.
I wasn't sure about that because I had only a quick look at
inject_offset() and had the impression it's only able to inject a
relative small offset (so not usable at boot). And, as written sometimes
before, I haven't had a deep look at suspend/resume, which might be the
only place where it is really used to set the clock when
systime_was_set is false.
Regards,
Alexander
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
On 06/14/2013 09:52 AM, Alexander Holler wrote:
In order to let an RTC set the time at boot without the problem that a
second RTC overwrites it, the flag systime_was_set is introduced.
systime_was_set will be true, if a persistent clock sets the time at boot,
or if do_settimeofday() is called (e.g. by the RTC subsystem or userspace).
Signed-off-by: Alexander Holler
---
include/linux/time.h | 6 ++
kernel/time/timekeeping.c | 10 +-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/linux/time.h b/include/linux/time.h
index d5d229b..888280f 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -129,6 +129,12 @@ extern int update_persistent_clock(struct timespec now);
void timekeeping_init(void);
extern int timekeeping_suspended;
+/*
+ * Will be true if the system time was set at least once by
+ * a persistent clock, RTC or userspace.
+ */
+extern bool systime_was_set;
+
Probably should make this static to timekeeping.c and create an accessor
function so you don't have to export locking rules on this.
unsigned long get_seconds(void);
struct timespec current_kernel_time(void);
struct timespec __current_kernel_time(void); /* does not take xtime_lock */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index baeeb5c..07d8531 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -37,6 +37,9 @@ int __read_mostly timekeeping_suspended;
/* Flag for if there is a persistent clock on this platform */
bool __read_mostly persistent_clock_exist = false;
+/* Flag for if the system time was set at least once */
+bool __read_mostly systime_was_set;
+
Probably should also move this to be part of the timekeeper structure
(since it will be protected by the timekeeper lock.
static inline void tk_normalize_xtime(struct timekeeper *tk)
{
while (tk->xtime_nsec >= ((u64)NSEC_PER_SEC << tk->shift)) {
@@ -498,6 +501,9 @@ int do_settimeofday(const struct timespec *tv)
raw_spin_lock_irqsave(_lock, flags);
write_seqcount_begin(_seq);
+ systime_was_set = true;
+
+
timekeeping_forward_now(tk);
xt = tk_xtime(tk);
Might also want to add the flag to inject_offset as well, since that
could be used to set the time.
thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
In order to let an RTC set the time at boot without the problem that a
second RTC overwrites it, the flag systime_was_set is introduced.
systime_was_set will be true, if a persistent clock sets the time at boot,
or if do_settimeofday() is called (e.g. by the RTC subsystem or userspace).
Signed-off-by: Alexander Holler
---
include/linux/time.h | 6 ++
kernel/time/timekeeping.c | 10 +-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/linux/time.h b/include/linux/time.h
index d5d229b..888280f 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -129,6 +129,12 @@ extern int update_persistent_clock(struct timespec now);
void timekeeping_init(void);
extern int timekeeping_suspended;
+/*
+ * Will be true if the system time was set at least once by
+ * a persistent clock, RTC or userspace.
+ */
+extern bool systime_was_set;
+
unsigned long get_seconds(void);
struct timespec current_kernel_time(void);
struct timespec __current_kernel_time(void); /* does not take xtime_lock */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index baeeb5c..07d8531 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -37,6 +37,9 @@ int __read_mostly timekeeping_suspended;
/* Flag for if there is a persistent clock on this platform */
bool __read_mostly persistent_clock_exist = false;
+/* Flag for if the system time was set at least once */
+bool __read_mostly systime_was_set;
+
static inline void tk_normalize_xtime(struct timekeeper *tk)
{
while (tk->xtime_nsec >= ((u64)NSEC_PER_SEC << tk->shift)) {
@@ -498,6 +501,9 @@ int do_settimeofday(const struct timespec *tv)
raw_spin_lock_irqsave(_lock, flags);
write_seqcount_begin(_seq);
+ systime_was_set = true;
+
+
timekeeping_forward_now(tk);
xt = tk_xtime(tk);
@@ -781,8 +787,10 @@ void __init timekeeping_init(void)
" Check your CMOS/BIOS settings.\n");
now.tv_sec = 0;
now.tv_nsec = 0;
- } else if (now.tv_sec || now.tv_nsec)
+ } else if (now.tv_sec || now.tv_nsec) {
persistent_clock_exist = true;
+ systime_was_set = true;
+ }
read_boot_clock();
if (!timespec_valid_strict()) {
--
1.8.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
In order to let an RTC set the time at boot without the problem that a
second RTC overwrites it, the flag systime_was_set is introduced.
systime_was_set will be true, if a persistent clock sets the time at boot,
or if do_settimeofday() is called (e.g. by the RTC subsystem or userspace).
Signed-off-by: Alexander Holler hol...@ahsoftware.de
---
include/linux/time.h | 6 ++
kernel/time/timekeeping.c | 10 +-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/linux/time.h b/include/linux/time.h
index d5d229b..888280f 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -129,6 +129,12 @@ extern int update_persistent_clock(struct timespec now);
void timekeeping_init(void);
extern int timekeeping_suspended;
+/*
+ * Will be true if the system time was set at least once by
+ * a persistent clock, RTC or userspace.
+ */
+extern bool systime_was_set;
+
unsigned long get_seconds(void);
struct timespec current_kernel_time(void);
struct timespec __current_kernel_time(void); /* does not take xtime_lock */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index baeeb5c..07d8531 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -37,6 +37,9 @@ int __read_mostly timekeeping_suspended;
/* Flag for if there is a persistent clock on this platform */
bool __read_mostly persistent_clock_exist = false;
+/* Flag for if the system time was set at least once */
+bool __read_mostly systime_was_set;
+
static inline void tk_normalize_xtime(struct timekeeper *tk)
{
while (tk-xtime_nsec = ((u64)NSEC_PER_SEC tk-shift)) {
@@ -498,6 +501,9 @@ int do_settimeofday(const struct timespec *tv)
raw_spin_lock_irqsave(timekeeper_lock, flags);
write_seqcount_begin(timekeeper_seq);
+ systime_was_set = true;
+
+
timekeeping_forward_now(tk);
xt = tk_xtime(tk);
@@ -781,8 +787,10 @@ void __init timekeeping_init(void)
Check your CMOS/BIOS settings.\n);
now.tv_sec = 0;
now.tv_nsec = 0;
- } else if (now.tv_sec || now.tv_nsec)
+ } else if (now.tv_sec || now.tv_nsec) {
persistent_clock_exist = true;
+ systime_was_set = true;
+ }
read_boot_clock(boot);
if (!timespec_valid_strict(boot)) {
--
1.8.1.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
On 06/14/2013 09:52 AM, Alexander Holler wrote:
In order to let an RTC set the time at boot without the problem that a
second RTC overwrites it, the flag systime_was_set is introduced.
systime_was_set will be true, if a persistent clock sets the time at boot,
or if do_settimeofday() is called (e.g. by the RTC subsystem or userspace).
Signed-off-by: Alexander Holler hol...@ahsoftware.de
---
include/linux/time.h | 6 ++
kernel/time/timekeeping.c | 10 +-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/linux/time.h b/include/linux/time.h
index d5d229b..888280f 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -129,6 +129,12 @@ extern int update_persistent_clock(struct timespec now);
void timekeeping_init(void);
extern int timekeeping_suspended;
+/*
+ * Will be true if the system time was set at least once by
+ * a persistent clock, RTC or userspace.
+ */
+extern bool systime_was_set;
+
Probably should make this static to timekeeping.c and create an accessor
function so you don't have to export locking rules on this.
unsigned long get_seconds(void);
struct timespec current_kernel_time(void);
struct timespec __current_kernel_time(void); /* does not take xtime_lock */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index baeeb5c..07d8531 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -37,6 +37,9 @@ int __read_mostly timekeeping_suspended;
/* Flag for if there is a persistent clock on this platform */
bool __read_mostly persistent_clock_exist = false;
+/* Flag for if the system time was set at least once */
+bool __read_mostly systime_was_set;
+
Probably should also move this to be part of the timekeeper structure
(since it will be protected by the timekeeper lock.
static inline void tk_normalize_xtime(struct timekeeper *tk)
{
while (tk-xtime_nsec = ((u64)NSEC_PER_SEC tk-shift)) {
@@ -498,6 +501,9 @@ int do_settimeofday(const struct timespec *tv)
raw_spin_lock_irqsave(timekeeper_lock, flags);
write_seqcount_begin(timekeeper_seq);
+ systime_was_set = true;
+
+
timekeeping_forward_now(tk);
xt = tk_xtime(tk);
Might also want to add the flag to inject_offset as well, since that
could be used to set the time.
thanks
-john
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [rtc-linux] Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
Am 14.06.2013 19:41, schrieb John Stultz:
On 06/14/2013 09:52 AM, Alexander Holler wrote:
In order to let an RTC set the time at boot without the problem that a
second RTC overwrites it, the flag systime_was_set is introduced.
systime_was_set will be true, if a persistent clock sets the time at
boot,
or if do_settimeofday() is called (e.g. by the RTC subsystem or
userspace).
Signed-off-by: Alexander Holler hol...@ahsoftware.de
---
include/linux/time.h | 6 ++
kernel/time/timekeeping.c | 10 +-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/linux/time.h b/include/linux/time.h
index d5d229b..888280f 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -129,6 +129,12 @@ extern int update_persistent_clock(struct
timespec now);
void timekeeping_init(void);
extern int timekeeping_suspended;
+/*
+ * Will be true if the system time was set at least once by
+ * a persistent clock, RTC or userspace.
+ */
+extern bool systime_was_set;
+
Probably should make this static to timekeeping.c and create an accessor
function so you don't have to export locking rules on this.
unsigned long get_seconds(void);
struct timespec current_kernel_time(void);
struct timespec __current_kernel_time(void); /* does not take
xtime_lock */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index baeeb5c..07d8531 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -37,6 +37,9 @@ int __read_mostly timekeeping_suspended;
/* Flag for if there is a persistent clock on this platform */
bool __read_mostly persistent_clock_exist = false;
+/* Flag for if the system time was set at least once */
+bool __read_mostly systime_was_set;
+
Probably should also move this to be part of the timekeeper structure
(since it will be protected by the timekeeper lock.
I wanted to avoid locks for this silly flag at all. It is only set once
at boot (and resume) and set to 0 at suspend. And I don't see any
possible race condition which could make a lock necessary. Therefor I've
decided to not use a lock or atomic_* in order to skip any delay in
setting the time.
Of course, I might be wrong and there might be a use case where multiple
things do set the system time concurrently and nothing else did set
system time before, but I found that extremly unlikely.
static inline void tk_normalize_xtime(struct timekeeper *tk)
{
while (tk-xtime_nsec = ((u64)NSEC_PER_SEC tk-shift)) {
@@ -498,6 +501,9 @@ int do_settimeofday(const struct timespec *tv)
raw_spin_lock_irqsave(timekeeper_lock, flags);
write_seqcount_begin(timekeeper_seq);
+systime_was_set = true;
+
+
timekeeping_forward_now(tk);
xt = tk_xtime(tk);
Might also want to add the flag to inject_offset as well, since that
could be used to set the time.
I wasn't sure about that because I had only a quick look at
inject_offset() and had the impression it's only able to inject a
relative small offset (so not usable at boot). And, as written sometimes
before, I haven't had a deep look at suspend/resume, which might be the
only place where it is really used to set the clock when
systime_was_set is false.
Regards,
Alexander
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [rtc-linux] Re: [PATCH 4/9 RESEND] RFC: timekeeping: introduce flag systime_was_set
On 06/14/2013 11:05 AM, Alexander Holler wrote:
Am 14.06.2013 19:41, schrieb John Stultz:
On 06/14/2013 09:52 AM, Alexander Holler wrote:
In order to let an RTC set the time at boot without the problem that a
second RTC overwrites it, the flag systime_was_set is introduced.
systime_was_set will be true, if a persistent clock sets the time at
boot,
or if do_settimeofday() is called (e.g. by the RTC subsystem or
userspace).
Signed-off-by: Alexander Holler hol...@ahsoftware.de
---
include/linux/time.h | 6 ++
kernel/time/timekeeping.c | 10 +-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/linux/time.h b/include/linux/time.h
index d5d229b..888280f 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -129,6 +129,12 @@ extern int update_persistent_clock(struct
timespec now);
void timekeeping_init(void);
extern int timekeeping_suspended;
+/*
+ * Will be true if the system time was set at least once by
+ * a persistent clock, RTC or userspace.
+ */
+extern bool systime_was_set;
+
Probably should make this static to timekeeping.c and create an accessor
function so you don't have to export locking rules on this.
unsigned long get_seconds(void);
struct timespec current_kernel_time(void);
struct timespec __current_kernel_time(void); /* does not take
xtime_lock */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index baeeb5c..07d8531 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -37,6 +37,9 @@ int __read_mostly timekeeping_suspended;
/* Flag for if there is a persistent clock on this platform */
bool __read_mostly persistent_clock_exist = false;
+/* Flag for if the system time was set at least once */
+bool __read_mostly systime_was_set;
+
Probably should also move this to be part of the timekeeper structure
(since it will be protected by the timekeeper lock.
I wanted to avoid locks for this silly flag at all. It is only set
once at boot (and resume) and set to 0 at suspend. And I don't see any
possible race condition which could make a lock necessary. Therefor
I've decided to not use a lock or atomic_* in order to skip any delay
in setting the time.
Even so, having random flag variables with special rules being exported
out is likely to cause eventual trouble (someone will mis-use or
overload some meaning on it).
So at least providing a accessor function for non-timekeeping.c uses
would be good.
Of course, I might be wrong and there might be a use case where
multiple things do set the system time concurrently and nothing else
did set system time before, but I found that extremly unlikely.
Yea, the condition check and the action won't be both be done under a
lock, so its likely going to be racy anyway.
static inline void tk_normalize_xtime(struct timekeeper *tk)
{
while (tk-xtime_nsec = ((u64)NSEC_PER_SEC tk-shift)) {
@@ -498,6 +501,9 @@ int do_settimeofday(const struct timespec *tv)
raw_spin_lock_irqsave(timekeeper_lock, flags);
write_seqcount_begin(timekeeper_seq);
+systime_was_set = true;
+
+
timekeeping_forward_now(tk);
xt = tk_xtime(tk);
Might also want to add the flag to inject_offset as well, since that
could be used to set the time.
I wasn't sure about that because I had only a quick look at
inject_offset() and had the impression it's only able to inject a
relative small offset (so not usable at boot). And, as written
sometimes before, I haven't had a deep look at suspend/resume, which
might be the only place where it is really used to set the clock when
systime_was_set is false.
Not via suspend/resume, since those modify the boottime to account for
the sleep time that has past.
I'm thinking via adjtimex ADJ_SETOFFSET (which is relatively new, and
not widely used). See the do_adjtimex() path in timekeeping.c
thanks
-john
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
