Messages in this thread | | | Date | Thu, 10 Sep 2009 16:07:10 +0800 | Subject | Re: [PATCH] update clocksource raw_time in timekeeping_suspend | From | ye janboe <> |
| |
hi, John
Thanks for your comments.
After sent this patch, I realize that this patch exposes the hardware detail ugly in common code.
In embed system, user space apps need to have a method to get the right time which will not be impacted by NTP and suspend.
Yes, you are right. I want to add sleep_length to the raw time and user space apps could get the right time after suspend.
Is this right semantics of CLOCK_MONOTONIC_RAW?
Janboe 2009/9/10 john stultz <johnstul@us.ibm.com>: > On Wed, 2009-09-09 at 15:35 +0800, ye janboe wrote: >> after resume from suspend, raw_time is not updated in >> timekeeping_suspend. CLOCK_MONOTONIC_RAW could not get the real hw >> time. >> This patch fix this issue. > > Hmm.. I'll admit suspend probably was less considered with > CLOCK_MONOTONIC_RAW, so the semantics aren't well established. > > However, I do think we want CLOCK_MONOTONIC_RAW to at-least closely map > to CLOCK_MONOTONIC (but *not* be NTP adjusted). I think that is what > folks would most likely expect. > > However, that isn't what this patch seems to do. > > Over suspend, I believe all hardware counters reset, so this patch would > seem to try to subtract the value back. > > This sort of makes sense for something like the TSC, which never wraps, > so the raw_time would be set back to a tranlation of the actual TSC > counter, but for other clocksources like the ACPI PM, it would only > subtract at most 5 seconds. So this leaks hardware specific detail in an > ugly way. > > Instead I suspect the most intuitive change would be to add in the > sleep_length to the raw time. This keeps CLOCK_MONOTONIC_RAW behaving > similarly to CLOCK_MONOTONIC, which I believe makes it more useful for > folks using CLOCK_MONOTONIC_RAW for things like tuning time > synchronization. > > But let me know more why you chose this implementation and maybe that > will show some better insight in to how you expect it to behave. > > thanks > -john > > > >> Signed-off-by: janboe <janboe.ye@gmail.com> >> --- >> kernel/time/timekeeping.c | 6 ++++++ >> 1 files changed, 6 insertions(+), 0 deletions(-) >> >> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c >> index e8c77d9..8420b85 100644 >> --- a/kernel/time/timekeeping.c >> +++ b/kernel/time/timekeeping.c >> @@ -331,6 +331,8 @@ static unsigned long timekeeping_suspend_time; >> static int timekeeping_resume(struct sys_device *dev) >> { >> unsigned long flags; >> + s64 nsec; >> + cycle_t last_cycle, cycle_delta; >> unsigned long now = read_persistent_clock(); >> >> clocksource_resume(); >> @@ -346,8 +348,12 @@ static int timekeeping_resume(struct sys_device *dev) >> } >> update_xtime_cache(0); >> /* re-base the last cycle value */ >> + last_cycle = clock->cycle_last; >> clock->cycle_last = 0; >> clock->cycle_last = clocksource_read(clock); >> + cycle_delta = clock->cycle_last - last_cycle; >> + nsec = cyc2ns(clock, cycle_delta); >> + timespec_add_ns(&clock->raw_time, nsec); >> clock->error = 0; >> timekeeping_suspended = 0; >> write_sequnlock_irqrestore(&xtime_lock, flags); > > -- 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/
| |