Messages in this thread | | | Subject | Re: [RFC][PATCH] new timeofday core subsystem (v. A2) | From | john stultz <> | Date | Tue, 01 Feb 2005 14:48:09 -0800 |
| |
On Tue, 2005-02-01 at 14:06 -0800, Tim Bird wrote: > Minor spelling fix, and a question. > > john stultz wrote: > > linux-2.6.11-rc2_timeofday-core_A2.patch > > ======================================== > > diff -Nru a/drivers/Makefile b/drivers/Makefile > > --- a/drivers/Makefile 2005-01-24 13:30:06 -08:00 > > +++ b/drivers/Makefile 2005-01-24 13:30:06 -08:00 > ... > > > + * all systems. It has the same course resolution as > should be "coarse"
Good catch, I'm a terrible speller.
> Do you replace get_cmos_time() - it doesn't look like it.
Nope, its still used on i386 and x86-64, however I had to create an arch independent abstraction for set/read_persistent_clock().
> You use it in your patch here... > > > diff -Nru a/arch/i386/kernel/time.c b/arch/i386/kernel/time.c > > --- a/arch/i386/kernel/time.c 2005-01-24 13:33:59 -08:00 > > +++ b/arch/i386/kernel/time.c 2005-01-24 13:33:59 -08:00 > ... > > > +/* arch specific timeofday hooks */ > > +nsec_t read_persistent_clock(void) > > +{ > > + return (nsec_t)get_cmos_time() * NSEC_PER_SEC; > > +} > > + > > I didn't scan for all uses of read_persistent_clock, but > in my experience get_cmos_time() has a latency of up to > 1 second on x86 because it synchronizes with the rollover > of the RTC seconds.
I believe you're right. Although we don't call read_persistent_clock() very frequently, nor do we call it in ways we don't already call get_cmos_time(). So I'm not sure exactly what the concern is.
> This comment in timeofday.c:timeofday_suspend_hook > worries me: > > > + /* First off, save suspend start time > > + * then quickly read the time source. > > + * These two calls hopefully occur quickly > > + * because the difference will accumulate as > > + * time drift on resume. > > + */ > > + suspend_start = read_persistent_clock(); > > Do you know if the sync problem is an issue here?
I don't believe so. The full context of the code is this:
/* First off, save suspend start time * then quickly read the time source. * These two calls hopefully occur quickly * because the difference will accumulate as * time drift on resume. */ suspend_start = read_persistent_clock(); now = read_timesource(timesource);
Since we call read_persistent_clock(), it should return right as the second changes, thus we will be marking the new second as closely as possible with the timesource value. If the order was reversed, I think it would be a concern.
I've only lightly tested the suspend code, but on my system I didn't see very much drift appear. Regardless, it should be better then what the current suspend/resume code does, which doesn't keep any sub-second resolution across suspend.
thanks so much for the code review! -john
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |