Date: Sun, 14 Jan 2018 20:09:49 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Warner Losh <imp@bsdimp.com> Cc: Bruce Evans <brde@optusnet.com.au>, Warner Losh <imp@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r327774 - in head: . sys/i386/bios Message-ID: <20180114174409.D1566@besplex.bde.org> In-Reply-To: <CANCZdfo_SiF5FMpcbRjX14FMXERPa0EbWex9z4SqrRny8b_2dQ@mail.gmail.com> References: <201801101725.w0AHP8Ca020379@repo.freebsd.org> <20180113225638.Q2336@besplex.bde.org> <CANCZdfo_SiF5FMpcbRjX14FMXERPa0EbWex9z4SqrRny8b_2dQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 13 Jan 2018, Warner Losh wrote:
> On Sat, Jan 13, 2018 at 6:17 AM, Bruce Evans <brde@optusnet.com.au> wrote:
>
>> On Wed, 10 Jan 2018, Warner Losh wrote:
>>
>> Log:
>>> inittodr(0) actually sets the time, so there's no need to call
>>> tc_setclock(). It's redundant. Tweak UPDATING based on code review of
>>> past releases.
>>
>> No, tc_setclock() is not redundant (except for bugs). initodr(0) sets
>> ...
>> Untested fixes:
>>
>> X Index: apm.c
>> X ===================================================================
>> X --- apm.c (revision 327911)
>> X +++ apm.c (working copy)
>> X @@ -1067,17 +1067,17 @@
>> X } while (apm_event != PMEV_NOEVENT);
>> X }
>> X X -static struct timeval suspend_time;
>> X -static struct timeval diff_time;
>> X +static struct timespec diff_time;
>> X +static struct timespec suspend_time;
>> X X static int
>> X apm_rtc_suspend(void *arg __unused)
>> X {
>> X X - microtime(&diff_time);
>> X - inittodr(0);
>> X - microtime(&suspend_time);
>> X - timevalsub(&diff_time, &suspend_time);
>> X + nanotime(&diff_time); /* not a difference yet */
>> X + inittodr(0); /* set tc to raw time seen by RTC
>> */
>> X + nanotime(&suspend_time); /* record this raw time */
>> X + timespecsub(&diff_time, &suspend_time); /* diff between tc and RTC
>> */
>> X return (0);
>> X }
>> X X @@ -1084,23 +1084,22 @@
>> X static int
>> X apm_rtc_resume(void *arg __unused)
>> X {
>> X + struct timespec resume_time, suspend_interval;
>> X u_int second, minute, hour;
>>
>> This already changes too much, so I didn't fix blind truncation of tv_sec
>> starting in 2038 or other type botches for second/minute/hour.
>
> I suspect this code will no longer be running in 2028, let alone 2038.
20 years is not far away.
(32-bit) u_int for seconds actually works (if time_t is >= 33 bits signed)
until 2106. If time_t is 32 bits signed and tv_sec is negative, then the
blind assignment to 'seconds' actually gives sign extension bugs.
Excessive fixups and sanity checking probably prevent negative time_t's
going near the RTC although they can still be set in software.
>> X - struct timeval resume_time, tmp_time;
>> X X - /* modified for adjkerntz */
>> X - timer_restore(); /* restore the all timers */
>> X - inittodr(0); /* adjust time to RTC */
>> X - microtime(&resume_time);
>> X - getmicrotime(&tmp_time);
>> X - timevaladd(&tmp_time, &diff_time);
>> X + timer_restore();
>> X + inittodr(0); /* set tc to new raw RTC time */
>> X + nanotime(&resume_time); /* record this raw time */
>> X + tc_setclock(&diff_time); /* restore old diff to tc */
>>
>> Fixing tc_setclock() is the easiest part
>
> tc_setclock() sets the absolute time. It's not a phase shift.
Oops.
> How is this then, it fixes the delta vs time setting in your version and
> tweaks variable names to remove banal comments....
>
> static struct timespec suspend_time; /* Saved RTC time at
> suspend */
This comment is too banal. The comment is too far to the right, and the
mail client highlights this by mangling the line by splitting it. The
comment seems to start in column 48. Columns 32 and 40 are normal.
Part of the banality is echoing 'suspend' and 'time' from the variable name
in the comment. The variable names aren't very good. Shorter names like
ts are better in some ways. 'time' in the variable name is no more
descriptive than 't'. 'suspend_time' might be either the timestamp of the
suspension event or the length of the suspension interval. The clock used
to make the timestamp is delicate. It isn't just the RTC, but the RTC
converted to a real time by adding a timezone offset and perhaps other
magic. (Note that in normal use, inittodr() is only used once at boot
time and them the timezone offset is 0. This is the only use where the
real time set by inittodr() is fully described as the RTC time.
> static struct timespec diff_interval; /* Delta between systime
> and RTC */
A better comment, but what is the systime? (it is actually the time
for the clock id CLOCK_REALTIME. I spelled this as tc (timecounter).
inittodr() really does init the full tc and we then specialize to the
real time using nanotime(). nanotime() obviously uses clock id
CLOCK_REALTIME so no commit is needed about what this clock id is,
but it is subtle that CLOCK_REALTIME is the correct (or least worst)
clock id to use.
>
> static int
> apm_rtc_suspend(void *arg __unused)
> {
>
> nanotime(&diff_interval);
'interval' here is worse than 'time'. It is not even redundant like it
would be for a difference between timestamps made using the same clock.
Here it is the difference between different clocks measure at a time
which is the same as possible. This difference is not of an interval.
> inittodr(0); /* systime = RTC */
> nanotime(&suspend_time);
> timespecsub(&diff_interval, &suspend_time);
>
> return (0);
> }
>
> static int
> apm_rtc_resume(void *arg __unused)
> {
> struct timespec resume_time, new_time, suspend_interval;
new_time is unsorted.
> u_int second, minute, hour;
Old unsorted declarations (fix later).
>
> timer_restore();
> inittodr(0); /* Set to updated RTC time */
> nanotime(&resume_time);
> new_time = resume_time;
Perhaps name it adj_resume_time.
> timespecadd(&new_time, &diff_interval);
> tc_setclock(&new_time); /* Pre-suspend system time + sleep
> time */
The comment is correct, but we haven't calculated the sleep time yet (and
we later name it suspend_interval), and the code actually calculates
(current_time + adjustment_for_difference_between_clocks_as_calculated_at_
suspend_time)
> ...
This can be further improved using CLOCK_GETTIME() instead of abusing
inittodr(). inittodr() has unwanted side effects including setting the
timecounter. CLOCK_GETTIME() really does return the RTC time.
Pseudo-code for suspend:
nanotime(&now);
CLOCK_GETTIME(myrtc, &rtc_now);
/* Try omitting utc_offset() adjustment -- see inittodr(). */
delta = now;
timespecsub(&delta, &rtc_now);
Pseudo-code for resume:
CLOCK_GETTIME(myrtc, &rtc_now);
now = rtc_now;
timespecadd(&now, &delta);
tc_setclock(&now);
Omitting utc_offset() gives large deltas near utc_offset() instead of near
0, but makes no difference to the result provided utc_offset() doesn't change
in between. More locking might be needed to prevent it changing in between.
There is some locking for CLOCK_GETTIME() but this doesn't cover sysctl
variables usied by utc_offset().
CLOCK_GETTIME() is over-engineered and hard to use. It is hard even
for i386/isa code to find its own RTC. Note that myrtc should be same
for suspend and resume. For i386 inittodr(), the RTC used to be just
the isa one, but inittodr() now loops over all available RTCs and there
is no guarantee that it always finds the same one.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180114174409.D1566>