Date: Tue, 21 May 2013 19:18:46 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Alexey Dokuchaev <danfe@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Warner Losh <imp@FreeBSD.org>, Warner Losh <imp@bsdimp.com> Subject: Re: svn commit: r247086 - head/sys/x86/isa Message-ID: <20130521172744.U1667@besplex.bde.org> In-Reply-To: <20130521061051.GA51568@FreeBSD.org> References: <201302210638.r1L6cOVx006678@svn.freebsd.org> <20130221064912.GA20360@FreeBSD.org> <20130520022100.GA82181@FreeBSD.org> <73F6A8D9-3366-47AB-9DE9-E570006E3A3F@bsdimp.com> <20130521061051.GA51568@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 21 May 2013, Alexey Dokuchaev wrote: > On Mon, May 20, 2013 at 07:20:48AM -0600, Warner Losh wrote: >> On May 19, 2013, at 8:21 PM, Alexey Dokuchaev wrote: >>> Apparently not: I've been running FreeBSD 8.4-PRERELEASE without pmtimer >>> for a while, and noticed that my laptop stops keeping time during suspend. >>> I've never noticed that behavior before (presumably, with pmtimer). >>> >>> I will soon rebuild the kernel and put pmtimer back to see it fixes time >>> keeping for me. If it will, apparently it is still useful for i386, and >>> not just for APM, but ACPI as well... >> >> It fights ACPI in what it does... It was needed for APM... Let me know how >> it turns out... > > Just rebuilt vanilla r250824; timekeeping works fine (with pmtimer now). My > kernel config and loader.conf for reference: > > http://193.124.210.26/{DARIA,loader.conf} In theory, acpi should handle this, but I've never been able to find where acpi either reads an acpi clock that gives the time or sets the time to the current time on resume after reading it from some other clock. Now I just found acpi_resync_clock() in dev/acpica/acpi.c. This if under an amd64 ifdef, so it can't work for i386, and its quality is much lower than that of pmtimer's already low quality (PS: actually it is equally low in a different way): @ static void @ acpi_resync_clock(struct acpi_softc *sc) @ { @ #ifdef __amd64__ @ if (!acpi_reset_clock) @ return; Since the code has not been tested (according to the comment on the sysctl), there is a sysctl to clear this variable to turn it off. The variable defaults to on. @ @ /* @ * Warm up timecounter again and reset system clock. @ */ @ (void)timecounter->tc_get_timecount(timecounter); @ (void)timecounter->tc_get_timecount(timecounter); @ inittodr(time_second + sc->acpi_sleep_delay); @ #endif @ } The parameter to the inittodr() is confusing and not much better than garbage. I think time_second is normally the time at which the system was suspended. sc->acpi_sleep_delay is just a short delay (default 1 second) and we add it to compensate for resume sleeping for that long. So the arg is usually much earlier than the current time. inittodr() only using it when reading the time from a hardware clock (always the RTC on x86?) fails; then inittodr() sets the time to that value. Here it might as well do nothing, since the time is already set to the old value (not quite, since setting the time also syncs with the hardware timecounter). Locking for inittodr() is broken (almost nonexistent), but that is not this code's problem. So the result of the above should be that the time is set to that of the RTC on x86. pmtimer is (PS: was) much more careful than this. On resume, it adjusts the time by the amount that the RTC time differed from the system time at the time of the suspend. This handles all drift between the clocks that occured before the suspend. There can be a further significantly large drift during long sleeps. Also, inittodr() is buggy, at least for the x86 RTC version -- it adds an error of up to 1 second by not syncing with the RTC seconds rollover. The last bug is fixed in my version (my inittodr() busy-waits for the rollover, but I have a better method in rtcintr() that reduces the error to < 10 usec plus the drift less the average drift. I only use the rtcintr() fixup after pseudo- suspensions by ddb)). @ static struct timeval suspend_time; @ static struct timeval diff_time; @ @ static int @ pmtimer_suspend(device_t dev) @ { @ @ microtime(&diff_time); @ inittodr(0); This inittodr(0) is a hack to read the RTC. Originally there was no API for this, but now CLOCK_GETTIME() is correct, at least if you can find the relevant clock. pmtimer can probablu assume the RTC, but acpi can't. This clobbers the current time, but we don't care since we are going to sleep. @ microtime(&suspend_time); This reads the current (clobbered) current time. This completes the hack for reading the RTC. @ timevalsub(&diff_time, &suspend_time); The fixup to add later. The RTC only has seconds granularity, but other clocks moght have more. microtime() gives us microseconds granularity but only seconds accuracy for the RTC due to the granularity in inittodr(). Locking is missing for all of this, and now it is not just inittodr()'s fault. The error of up to ~1 second may be increased by races, but not by another 1 second (unless the races in inittodr() gave a corrupt value), so we don't care. This last pretended to do locking in FreeBSD-4. splsoftclock() was used. This didn't actually work, since splhigh() was needed. When spl became null, the splsoftclock() was left as a reminder to fix the locking, but someone removed it without fixing the locking :-(. Intermediate versions with everything Giant locked had a better chance of working than even the ones mis-locked by splsoftclock(). @ return (0); @ } @ @ static int @ pmtimer_resume(device_t dev) @ { @ u_int second, minute, hour; @ struct timeval resume_time, tmp_time; @ @ /* modified for adjkerntz */ The comment is cryptic. I think it means that the fixup is much more necessary than might first appear, since adjkerntz can adjust the RTC by an hour or so for DST changes. The current time must not be changed in this case, but without a fixup that takes into account the RTC adjustment before the suspend, the error might be a full hour or so. This might depend on how smart inittodr() is. It will need to add an an even larger adjustment if the RTC is not on UTC. The DST adjustment should be folded into this, so perhaps the comment no longer applies. @ timer_restore(); /* restore the all timers */ @ inittodr(0); /* adjust time to RTC */ This gives the current RTC time, hopefully adjusted to UTC including DST. @ microtime(&resume_time); @ getmicrotime(&tmp_time); @ timevaladd(&tmp_time, &diff_time); Adjust, with the usual races. @ @ #ifdef FIXME @ /* XXX THIS DOESN'T WORK!!! */ @ time = tmp_time; @ #endif Bah, the adjustment isn't even applied. This rotted with timecounters even before FreeBSD-4. The current time used to be the simple variable 'time', but timecounters added lots of state to set. Timecounters also added the tc_setclock() API to set it. This should be used here. It is racy, but inittodr() has already just used it. @ @ #ifdef PMTIMER_FIXUP_CALLTODO @ /* Calculate the delta time suspended */ @ timevalsub(&resume_time, &suspend_time); @ /* Fixup the calltodo list with the delta time. */ @ adjust_timeout_calltodo(&resume_time); @ /* @ * We've already calculated resume_time to be the delta between @ * the suspend and the resume. @ */ @ second = resume_time.tv_sec; @ #else /* !PMTIMER_FIXUP_CALLTODO */ @ second = resume_time.tv_sec - suspend_time.tv_sec; @ #endif /* PMTIMER_FIXUP_CALLTODO */ The PMTIMER_FIXUP_CALLTODO garbage is purer. It stopped working before it was committed. @ hour = second / 3600; @ second %= 3600; @ minute = second / 60; @ second %= 60; @ log(LOG_NOTICE, "wakeup from sleeping state (slept %02d:%02d:%02d)\n", @ hour, minute, second); @ return (0); @ } So it seems that pmtimer has a lot of dead code and reduces to not much more than an inittodr(0) and usually acts like the inittodr(!0) in the acpi amd64 resume code. acpi amd64 resume also seems to be missing the verbose logging of the suspension time, but maybe acpi utilities give that. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130521172744.U1667>