Date: Mon, 6 Aug 2018 08:33:57 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ian Lepore <ian@freebsd.org> Cc: Warner Losh <imp@bsdimp.com>, "Rodney W. Grimes" <rgrimes@freebsd.org>, Conrad Meyer <cem@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r337334 - head/lib/libc/sys Message-ID: <20180806074507.E920@besplex.bde.org> In-Reply-To: <1533478958.9860.18.camel@freebsd.org> References: <201808042208.w74M8OmD057603@repo.freebsd.org> <201808042224.w74MOgLi095274@pdx.rh.CN85.dnsmgr.net> <CANCZdfpWh25X%2BkKsbrdNY697Ex%2BxG95crVRgKqVQpfrqZO_CoA@mail.gmail.com> <1533478958.9860.18.camel@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 5 Aug 2018, Ian Lepore wrote: > On Sat, 2018-08-04 at 16:39 -0600, Warner Losh wrote: >>> [Context lost to corruption of spaces} >> >> Specifically, for compatibility, we store minutes west of UTC on >> settimeofday and we retrieve it for settimeofday. Otherwise it's 100% >> unused by anything else at all in the system. Well, technically, we use it >> for utc_offset, we don't really use that elsewhere (one can find references >> in utc_offset, but usually we set this via adjkerntz, which is actively >> used in the system). tz_minuteswest likely should just be removed, and the >> argument to settimzeofday should just be completely ignored. I already described how adjkerntz sets both tz_minuteswest and other more cryptic variables used in utc_offset(). > utc_offset is used by all RTC drivers, and by the convenience routines > that convert times to/from FAT filesystem format (which I assume are > used by FAT filesystem code, but I haven't checked that). So if the tz > info passed to settimeofday() ends up in utc_offset, how can we be sure > nobody is relying on that? I forgot about FAT times, and only remembered that utc_offset() was used for RTC drivers. I thought that utc_offset() is 0 unless something sets the timezone to nonzero or the RTC is on local time (wall_cmos_clock case). However, since the kernel needs the timezone for FAT times, it must be known even if the RTC is on UTC time. Now I don't see how FAT times can even work unless the wall_cmos_clock. They are just the UTC minus utc_offset(), where utc_offset() is (tz_minuteswest * 60 + (wall_cmos_clock ? adjkerntz : 0)) Here wall_cmos_clock is only for managing the RTC offset. It must be 0 unless the RTC is on local time. But then either FAT times or RTC times must be broken except in the Greenwich meridian. Otherwise, tz_minuteswest must be nonzero to ajust FAT times right, but this makes utc_offset() unusable for the RTC offset. The timezone and setting it using settimeofday() is the correct API for implementing all of this (it only has minutes resolution, but no more is needed). The correct offsets are then: rtc_offset() = wall_cmos_clock ? tz_minuteswest * 60 : 0; fat_offset() = tz_minuteswest * 60; where tz_minuteswest must include the dst offset so it needs a daemon like adjkerntz(8) to manage it even if wall_cmos_clock == 0. I think adjkerntz already does this management, and it works in the wall_cmos_clock != 0 case (except for missing locking of all 3 of the variables, and fundamental races updating them all and also adjusting the RTC on every dst change). FAT times work in this case. I think FAT times just don't work if wall_cmos_clock == 0. I think the extra variables are to try to manage this complexity, but the just get in the way. wall_cmos_clock is obviously needed. But the extra offset can't do anything good. It might have helped on non-preemptible UP systems where locking was not needed. It allowed adjusting the offset without going near settimeofday(). But now non- broken locking would need to use the same lock for settimeofday() and the machdep.adjkerntz sysctl. It is easier to just lock settimeofday(). >> So while hyper technically, one could use this, nobody does, nor has since >> between 4.3 and 4.4 when it was realized that storing the timezone in the >> kernel was a really stupid idea. That's what the language used in the man >> page that you removed was trying to say. adjkerntz(8) uses this. Storing the timezone in the kernel is an essential idea. Otherwise, every FAT time calculation would have to call userland to ask for the timezone. Similarly for RTC offsets, except those don't need to be calculated very often. adjkerntz(8) can be simplified by only using 1 offset, but that offset should be the timezone set by settimeofday() and not a sysctl with even more broken locking. My program for demonstrating overflow bugs and panics in RTC code uses this. It could use either the timezone or machdep.adjkerntz to set a bad offset, but the timezone is easier to use. Bad offsets can be as little as 1 second, to convert the Epoch time (time_t)0 to the invalid time (time_t)-1. The program uses a timezone of +-24 hours to clobber any more reasonable offset in machdep.adjkerntz. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180806074507.E920>