svn commit: r247088 - head/sys/i386/isa

[Bruce Evans]

On Thu, 21 Feb 2013, Warner Losh wrote:

> Log:
>  Locking for todr got pushed down into inittodr and the client
>  libraries it calls (although some might not be doing it right). We are
>  serialized right now by giant as well. This means the splsoftclock are
>  now an anachronism that has no benefit, even marking where locking
>  needs to happen. Remove them.

No, it (a working version of it) was needed to prevent delays from
being preempted.

> Modified:
>  head/sys/i386/isa/pmtimer.c
>
> Modified: head/sys/i386/isa/pmtimer.c
> ==============================================================================
> --- head/sys/i386/isa/pmtimer.c	Thu Feb 21 06:38:49 2013	(r247087)
> +++ head/sys/i386/isa/pmtimer.c	Thu Feb 21 07:16:40 2013	(r247088)
> @@ -82,26 +82,21 @@ static struct timeval diff_time;
> static int
> pmtimer_suspend(device_t dev)
> {
> -	int	pl;
>
> -	pl = splsoftclock();
> 	microtime(&diff_time);
> 	inittodr(0);
> 	microtime(&suspend_time);
> 	timevalsub(&diff_time, &suspend_time);
> -	splx(pl);
> 	return (0);
> }

For this to work, it is also necessary for inittodr() to be accurate.
With inittodr() in -current being off by up to 1 second (even if the
RTC hardare time is perfectly accurate), it doesn't matter if there is
a small additional error from being preempted here.

settime(2) has similar races (even worse, since it asks for an absolute
time).  If either the user or kernel parts of settime() are preempted,
then the final time will be in the past by the amount of the preemption
plus the syscall time (if the time was perfectly accurate at the start
of settime()).

>
> static int
> pmtimer_resume(device_t dev)
> {
> -	int pl;
> 	u_int second, minute, hour;
> 	struct timeval resume_time, tmp_time;
>
> 	/* modified for adjkerntz */
> -	pl = splsoftclock();
> 	timer_restore();		/* restore the all timers */
> 	inittodr(0);			/* adjust time to RTC */
> 	microtime(&resume_time);

I remember a bit better how this is supposed to work.  It doesn't need
a very accurate RTC hardware time.  Suspend records the difference
between the hardware time and the software time.  Resume reads the
hardware time and adds the difference to get the new software time.
So the only unavoidable error is the RTC hardware time drift during
the suspsension period.  This should be small if the suspension period
is only a few hours.

I use similar methods to fix up the software time after sitting in
ddb for a few seconds or minutes.  The RTC drifts a few usec/sec
and the fixup adds a few more usec.

Here the fix isn't as simple as putting everything in a critical
section.  inittodr() would be in the section, but a working version
of it can't be locked like that since it needs to wait.  In my version,
inittodr() is not used for this.  The RTC is programmed to interrupt
on every seconds boundary, and a difference corresponding to the above
is calculated on avery 64th of these interrupts.  Since the RTC just
rolled over, it can be read without waiting for it to be non-busy.
Or we could just count the interrupts and add 64 to the previous
RTC time in seconds.

> @@ -118,16 +113,13 @@ pmtimer_resume(device_t dev)
> 	timevalsub(&resume_time, &suspend_time);
> 	/* Fixup the calltodo list with the delta time. */
> 	adjust_timeout_calltodo(&resume_time);
> -#endif /* PMTIMER_FIXUP_CALLTODOK */
> -	splx(pl);
> -#ifndef PMTIMER_FIXUP_CALLTODO
> -	second = resume_time.tv_sec - suspend_time.tv_sec;
> -#else /* PMTIMER_FIXUP_CALLTODO */

The PMTIMER_FIXUP_CALLTODO is bogus.  It never even compiled in any
committed version.  However, I think something like it is still needed
timeouts in monotonic time are very wrong for long timeouts.  After a
resume after a suspension period of hours or days, lots of timeouts
should have expired and the problem is to avoid creating a thundering
herd of them by timing them out all at once.

> 	/*
> 	 * 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 */
> 	hour = second / 3600;
> 	second %= 3600;

Bruce