On 02/07/2013 12:24 PM, John Stultz wrote: > On Thu, Feb 7, 2013 at 5:29 AM, Prarit Bhargava <pra...@redhat.com> wrote: >> We do not see this manifest on some architectures because they limit changes >> to the rtc to +/-15 minutes of the current value of the rtc (x86, alpha, >> mn10300). Other arches do nothing (cris, mips, sh), and only a few seem to >> show this problem (power, sparc). I can reproduce this reliably on powerpc >> with the latest Fedoras (17, 18, rawhide), as well as an Ubuntu powerpc spin. >> I can also reproduce it "older" OSes such as RHEL6. > > Interesting. > Yea, local RTC time is probably pretty rare outside of x86 (due to windows). > And the +/- 15minute trick has always explicitly masked this issue there. >
I'm not sure I understand the purpose behind the +/-15 minute window? Is it just to prevent a wild swing on the RTC? I can understand that to some degree, however, I'm not sure I agree with it being the default behaviour. Here's a real-world scenario: My RTC on my laptop is set to 1:30PM Jan 7, 2013. I boot, systemd and ntp do their magic, and the system time comes up as Feb 7, 12:48PM. I never will notice that the RTC is wrong. Now I go somewhere and have to work on a plane. I have no internet connection and then boot. Now the system time will be 1:30PM Jan 7, 2013. That's actually happened to me and I remember filing it away for a bug to be looked at. AFAIK, no other OS does that ... if I install Windows or use a Mac in the no-internet connection case, the time is *always* corrected. I tried to see if I could get this to happen on a Mac and I can't. 99.99999% of Linux users out there are using some sort of time protocol (usually NTP, but PTP is starting to catch on) to sync their systems. NTP is a trusted source of timekeeping IMO. How often do we see systems that run NTP but don't trust the numbers that come from it? We should be doing a full sync of the RTC in NTP, or at least it should be an option/CONFIG option (FYI: I want to patch for that ... it'll give me something to do). > >> A few things about the patch. 'sys_time_offset' certainly could have a >> better name and it could be a bool. Also, technically I do not need to add >> the >> 'adjust' struct in sync_cmos_clock() as the value of now.tv_sec is not used >> after being passed into update_persistent_clock(). However, I think the code >> is easier to follow if I do add 'adjust'. >> >> ------8<--------- >> >> Take the timezone offset applied in warp_clock() into account when writing >> the hardware clock in the ntp code. This patch adds sys_time_offset which >> indicates that an offset has been applied in warp_clock(). >> >> Signed-off-by: Prarit Bhargava <pra...@redhat.com> >> Cc: John Stultz <johns...@us.ibm.com> >> Cc: Thomas Gleixner <t...@linutronix.de> >> --- >> include/linux/time.h | 1 + >> kernel/time.c | 8 ++++++++ >> kernel/time/ntp.c | 8 ++++++-- >> 3 files changed, 15 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/time.h b/include/linux/time.h >> index 4d358e9..02754b5 100644 >> --- a/include/linux/time.h >> +++ b/include/linux/time.h >> @@ -117,6 +117,7 @@ static inline bool timespec_valid_strict(const struct >> timespec *ts) >> >> extern void read_persistent_clock(struct timespec *ts); >> extern void read_boot_clock(struct timespec *ts); >> +extern int sys_time_offset; >> extern int update_persistent_clock(struct timespec now); >> void timekeeping_init(void); >> extern int timekeeping_suspended; >> diff --git a/kernel/time.c b/kernel/time.c >> index d226c6a..66533d3 100644 >> --- a/kernel/time.c >> +++ b/kernel/time.c >> @@ -115,6 +115,12 @@ SYSCALL_DEFINE2(gettimeofday, struct timeval __user *, >> tv, >> } >> >> /* >> + * Indicates if there is an offset between the system clock and the hardware >> + * clock/persistent clock/rtc. >> + */ >> +int sys_time_offset; > > So why is this extra flag necessary instead of just using if > (sys_tz.tz_minuteswest) ? sys_tz can be set during runtime via settimeofday() without affecting the current system time. The bug *only* happens if the system clock is "warped" ahead relative to the hardware clock on the first call to settimeofday(), so checking for sys_tz.tz_minuteswest isn't good enough of a test. > > >> + >> +/* >> * Adjust the time obtained from the CMOS to be UTC time instead of >> * local time. >> * >> @@ -135,6 +141,8 @@ static inline void warp_clock(void) >> struct timespec adjust; >> >> adjust = current_kernel_time(); >> + if (sys_tz.tz_minuteswest > 0) >> + sys_time_offset = 1; > > This seems like it wouldn't work for localtimes that are east of GMT. Ah, good point. I had it in my head that minuteswest was always negative. That should be if (sys_tz.tz_minuteswest != 0) I'll fix that in the next !RFE patch. P. -- 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/
