[PATCH] update clocksource raw_time in timekeeping_suspend

[PATCH] update clocksource raw_time in timekeeping_suspend

[ye janboe]

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.

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);

Re: [PATCH] update clocksource raw_time in timekeeping_suspend

[Yong Zhang]

On Wed, Sep 9, 2009 at 3:35 PM, ye janboe<janboe.ye@gmail.com> wrote:
> after resume from suspend, raw_time is not updated in
> timekeeping_suspend. CLOCK_MONOTONIC_RAW could not get the real hw
> time.

It seems that this is not a bug. The design of CLOCK_MONOTONIC_RAW doesn't
say it will reflect the real hw time. It's just monotonic time which
is unpoisoned by
ntp.

Cheers,
Yong

> This patch fix this issue.
>
> 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/
>

Re: [PATCH] update clocksource raw_time in timekeeping_suspend

[ye janboe]

Thanks, Yong Zhang

But I do not think so. From the comments in commit
2d42244ae71d6c7b0884b5664cf2eda30fb2ae68, MONOTONIC_RAW want to give
users access to purely hardware based time.


2009/9/9 Yong Zhang <yong.zhang0@gmail.com>:
> On Wed, Sep 9, 2009 at 3:35 PM, ye janboe<janboe.ye@gmail.com> wrote:
>> after resume from suspend, raw_time is not updated in
>> timekeeping_suspend. CLOCK_MONOTONIC_RAW could not get the real hw
>> time.
>
> It seems that this is not a bug. The design of CLOCK_MONOTONIC_RAW doesn't
> say it will reflect the real hw time. It's just monotonic time which
> is unpoisoned by
> ntp.
>
> Cheers,
> Yong
>
>> This patch fix this issue.
>>
>> 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/
>>
>

Re: [PATCH] update clocksource raw_time in timekeeping_suspend

[john stultz]

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);

Re: [PATCH] update clocksource raw_time in timekeeping_suspend

[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);
>
>

Re: [PATCH] update clocksource raw_time in timekeeping_suspend

[Yong Zhang]

On Thu, Sep 10, 2009 at 4:07 PM, ye janboe <janboe.ye@gmail.com> wrote:
> 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.
>

What I get from the code is that CLOCK_MONOTONIC doesn't consider
sleep_length either. Do I miss something?

IMO, CLOCK_MONOTONIC_RAW should be coordinate with
CLOCK_MONOTONIC. The difference between them is
whether it's modified by NTP or not.

Correct me if I'm wrong.

-Yong

> 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/
>

Re: [PATCH] update clocksource raw_time in timekeeping_suspend

[john stultz]

On Thu, 2009-09-10 at 17:31 +0800, Yong Zhang wrote:
> On Thu, Sep 10, 2009 at 4:07 PM, ye janboe <janboe.ye@gmail.com> wrote:
> > 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.
> >
> 
> What I get from the code is that CLOCK_MONOTONIC doesn't consider
> sleep_length either. Do I miss something?

You're quite right. I forgot we drop sleep_length from
wall_to_monotonic, so it should not increase while we're suspended.

Janboe: So I think things are fine as they stand. No patch necessary.

But please let me know if I'm yet again forgetting something or you're
finding CLOCK_MONOTONIC_RAW to be insufficient in some way.

thanks
-john

Re: [PATCH] update clocksource raw_time in timekeeping_suspend

[ye janboe]

So CLOCK_MONOTONIC_RAW is not suitable for getting real hw time which
includes sleep time.

Is there other timer suitable for this?

Thanks

2009/9/11 john stultz <johnstul@us.ibm.com>:
> On Thu, 2009-09-10 at 17:31 +0800, Yong Zhang wrote:
>> On Thu, Sep 10, 2009 at 4:07 PM, ye janboe <janboe.ye@gmail.com> wrote:
>> > 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.
>> >
>>
>> What I get from the code is that CLOCK_MONOTONIC doesn't consider
>> sleep_length either. Do I miss something?
>
> You're quite right. I forgot we drop sleep_length from
> wall_to_monotonic, so it should not increase while we're suspended.
>
> Janboe: So I think things are fine as they stand. No patch necessary.
>
> But please let me know if I'm yet again forgetting something or you're
> finding CLOCK_MONOTONIC_RAW to be insufficient in some way.
>
> thanks
> -john
>
>

Re: [PATCH] update clocksource raw_time in timekeeping_suspend

[john stultz]

On Fri, 2009-09-11 at 11:35 +0800, ye janboe wrote:
> So CLOCK_MONOTONIC_RAW is not suitable for getting real hw time which
> includes sleep time.

Hmm.. This is sort of an odd request. 

The clocksource hardware counters stop in during suspend, so any "real
hw time that includes sleep" is likely to be combined from different
sources (with slightly different drifts), so I'm not sure why
CLOCK_MONOTONIC_RAW would be preferred over CLOCK_MONOTONIC?


> Is there other timer suitable for this?

Maybe the CMOS / RTC?  Would that suffice?

Could you maybe provide more details about why you need this? Maybe
better understanding your needs would help point to a solution.

thanks
-john