Prev: x86-32: Fix crashes with CPU hotplug on AMD machines
Next: pps: add parallel port PPS signal generator
From: john stultz on 4 Aug 2010 19:10
On Thu, 2010-08-05 at 01:06 +0400, Alexander Gordeev wrote:
> MONOTONIC_RAW clock timestamps are ideally suited for frequency
> calculation and also fit well into the original NTP hardpps design. Now
> phase and frequency can be adjusted separately: the former based on
> REALTIME clock and the latter based on MONOTONIC_RAW clock.
> A new function getnstime_raw_and_real is added to timekeeping subsystem
> to capture both timestamps at the same time and atomically.
>
> Signed-off-by: Alexander Gordeev <lasaine(a)lvk.cs.msu.su>
> ---
> include/linux/pps_kernel.h | 3 ++-
> include/linux/time.h | 2 ++
> kernel/time/timekeeping.c | 34 ++++++++++++++++++++++++++++++++++
> 3 files changed, 38 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
> index 5af0498..39fc712 100644
> --- a/include/linux/pps_kernel.h
> +++ b/include/linux/pps_kernel.h
> @@ -48,6 +48,7 @@ struct pps_source_info {
> };
>
> struct pps_event_time {
> + struct timespec ts_raw;
> struct timespec ts_real;
> };
>
> @@ -111,7 +112,7 @@ static inline void timespec_to_pps_ktime(struct pps_ktime *kt,
>
> static inline void pps_get_ts(struct pps_event_time *ts)
> {
> - getnstimeofday(&ts->ts_real);
> + getnstime_raw_and_real(&ts->ts_raw, &ts->ts_real);
> }
>
> #endif /* LINUX_PPS_KERNEL_H */
> diff --git a/include/linux/time.h b/include/linux/time.h
> index ea3559f..1da8a7b 100644
> --- a/include/linux/time.h
> +++ b/include/linux/time.h
> @@ -143,6 +143,8 @@ extern unsigned int alarm_setitimer(unsigned int seconds);
> extern int do_getitimer(int which, struct itimerval *value);
> extern void getnstimeofday(struct timespec *tv);
> extern void getrawmonotonic(struct timespec *ts);
> +extern void getnstime_raw_and_real(struct timespec *ts_raw,
> + struct timespec *ts_real);
> extern void getboottime(struct timespec *ts);
> extern void monotonic_to_bootbased(struct timespec *ts);
>
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index caf8d4d..ef37b14 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -286,6 +286,40 @@ void ktime_get_ts(struct timespec *ts)
> EXPORT_SYMBOL_GPL(ktime_get_ts);
>
> /**
> + * getnstime_raw_and_real - Returns both the time of day an raw
> + * monotonic time in a timespec format
> + * @ts_mono_raw: pointer to the timespec to be set to raw
> + * monotonic time
> + * @ts_real: pointer to the timespec to be set to the time
> + * of day
> + */
> +void getnstime_raw_and_real(struct timespec *ts_raw, struct timespec *ts_real)
> +{
> + unsigned long seq;
> + s64 nsecs_raw, nsecs_real;
> +
> + WARN_ON(timekeeping_suspended);
> +
> + do {
> + seq = read_seqbegin(&xtime_lock);
> +
> + *ts_raw = raw_time;
> + *ts_real = xtime;
> +
> + nsecs_raw = timekeeping_get_ns_raw();
> + nsecs_real = timekeeping_get_ns();
> +
> + /* If arch requires, add in gettimeoffset() */
> + nsecs_real += arch_gettimeoffset();

Hmm. So this brings up an interesting point. If the system is an one
which uses arch_gettimeoffset() (which means it doesn't have
clocksources), I'm not sure if this function would return what you'd
expect.

The issue is that jiffies is the basic clocksource that is in use on
these systems, but the arch_gettimeoffset() function returns an
inter-tick time value (usually by reading the interrupt generating
decrementer or whatnot). So in this case, you'd get a
CLOCK_MONOTONIC_RAW time that is based off the raw jiffies value at the
last tick, and CLOCK_REALTIME time value at the exact time of the call.

That offset between the two would then vary depending on when in the
inter-tick period the call was made.


The two solutions would be:
1) Add the arch_gettimeoffset() value to nsecs_raw as well. Should be
safe, as any freq variance from the raw freq will be limited to 1 tick
length and won't accumulate.

2) Don't add arch_gettimeoffset to either value, providing the raw and
real time values at the last tick.

I'd probably go with #1.

Otherwise, the code looks good.

thanks
-john


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 4 Aug 2010 19:40
On Thu, 5 Aug 2010 01:06:50 +0400
Alexander Gordeev <lasaine(a)lvk.cs.msu.su> wrote:

> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -286,6 +286,40 @@ void ktime_get_ts(struct timespec *ts)
> EXPORT_SYMBOL_GPL(ktime_get_ts);
>
> /**
> + * getnstime_raw_and_real - Returns both the time of day an raw
> + * monotonic time in a timespec format
> + * @ts_mono_raw: pointer to the timespec to be set to raw
> + * monotonic time
> + * @ts_real: pointer to the timespec to be set to the time
> + * of day
> + */
> +void getnstime_raw_and_real(struct timespec *ts_raw, struct timespec *ts_real)
> +{
> + unsigned long seq;
> + s64 nsecs_raw, nsecs_real;
> +
> + WARN_ON(timekeeping_suspended);

I suspect that if this warning ever triggers, it'll trigger at some
high frequency making a complete mess all over the floor.

WARN_ON_ONCE, perhaps? Or just remove it?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Alexander Gordeev on 5 Aug 2010 08:20
В Wed, 04 Aug 2010 16:03:57 -0700
john stultz <johnstul(a)us.ibm.com> пишет:

> On Thu, 2010-08-05 at 01:06 +0400, Alexander Gordeev wrote:
> > MONOTONIC_RAW clock timestamps are ideally suited for frequency
> > calculation and also fit well into the original NTP hardpps design. Now
> > phase and frequency can be adjusted separately: the former based on
> > REALTIME clock and the latter based on MONOTONIC_RAW clock.
> > A new function getnstime_raw_and_real is added to timekeeping subsystem
> > to capture both timestamps at the same time and atomically.
> >
> > Signed-off-by: Alexander Gordeev <lasaine(a)lvk.cs.msu.su>
> > ---
> > include/linux/pps_kernel.h | 3 ++-
> > include/linux/time.h | 2 ++
> > kernel/time/timekeeping.c | 34 ++++++++++++++++++++++++++++++++++
> > 3 files changed, 38 insertions(+), 1 deletions(-)
> >
> > diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
> > index 5af0498..39fc712 100644
> > --- a/include/linux/pps_kernel.h
> > +++ b/include/linux/pps_kernel.h
> > @@ -48,6 +48,7 @@ struct pps_source_info {
> > };
> >
> > struct pps_event_time {
> > + struct timespec ts_raw;
> > struct timespec ts_real;
> > };
> >
> > @@ -111,7 +112,7 @@ static inline void timespec_to_pps_ktime(struct pps_ktime *kt,
> >
> > static inline void pps_get_ts(struct pps_event_time *ts)
> > {
> > - getnstimeofday(&ts->ts_real);
> > + getnstime_raw_and_real(&ts->ts_raw, &ts->ts_real);
> > }
> >
> > #endif /* LINUX_PPS_KERNEL_H */
> > diff --git a/include/linux/time.h b/include/linux/time.h
> > index ea3559f..1da8a7b 100644
> > --- a/include/linux/time.h
> > +++ b/include/linux/time.h
> > @@ -143,6 +143,8 @@ extern unsigned int alarm_setitimer(unsigned int seconds);
> > extern int do_getitimer(int which, struct itimerval *value);
> > extern void getnstimeofday(struct timespec *tv);
> > extern void getrawmonotonic(struct timespec *ts);
> > +extern void getnstime_raw_and_real(struct timespec *ts_raw,
> > + struct timespec *ts_real);
> > extern void getboottime(struct timespec *ts);
> > extern void monotonic_to_bootbased(struct timespec *ts);
> >
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index caf8d4d..ef37b14 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -286,6 +286,40 @@ void ktime_get_ts(struct timespec *ts)
> > EXPORT_SYMBOL_GPL(ktime_get_ts);
> >
> > /**
> > + * getnstime_raw_and_real - Returns both the time of day an raw
> > + * monotonic time in a timespec format
> > + * @ts_mono_raw: pointer to the timespec to be set to raw
> > + * monotonic time
> > + * @ts_real: pointer to the timespec to be set to the time
> > + * of day
> > + */
> > +void getnstime_raw_and_real(struct timespec *ts_raw, struct timespec *ts_real)
> > +{
> > + unsigned long seq;
> > + s64 nsecs_raw, nsecs_real;
> > +
> > + WARN_ON(timekeeping_suspended);
> > +
> > + do {
> > + seq = read_seqbegin(&xtime_lock);
> > +
> > + *ts_raw = raw_time;
> > + *ts_real = xtime;
> > +
> > + nsecs_raw = timekeeping_get_ns_raw();
> > + nsecs_real = timekeeping_get_ns();
> > +
> > + /* If arch requires, add in gettimeoffset() */
> > + nsecs_real += arch_gettimeoffset();
>
> Hmm. So this brings up an interesting point. If the system is an one
> which uses arch_gettimeoffset() (which means it doesn't have
> clocksources), I'm not sure if this function would return what you'd
> expect.
>
> The issue is that jiffies is the basic clocksource that is in use on
> these systems, but the arch_gettimeoffset() function returns an
> inter-tick time value (usually by reading the interrupt generating
> decrementer or whatnot). So in this case, you'd get a
> CLOCK_MONOTONIC_RAW time that is based off the raw jiffies value at the
> last tick, and CLOCK_REALTIME time value at the exact time of the call.
>
> That offset between the two would then vary depending on when in the
> inter-tick period the call was made.
>
>
> The two solutions would be:
> 1) Add the arch_gettimeoffset() value to nsecs_raw as well. Should be
> safe, as any freq variance from the raw freq will be limited to 1 tick
> length and won't accumulate.
>
> 2) Don't add arch_gettimeoffset to either value, providing the raw and
> real time values at the last tick.
>
> I'd probably go with #1.
>
> Otherwise, the code looks good.

Ok, thanks for the point! I'll use #1 then.

--
Alexander
From: Alexander Gordeev on 5 Aug 2010 08:30
В Wed, 4 Aug 2010 16:29:07 -0700
Andrew Morton <akpm(a)linux-foundation.org> пишет:

> On Thu, 5 Aug 2010 01:06:50 +0400
> Alexander Gordeev <lasaine(a)lvk.cs.msu.su> wrote:
>
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -286,6 +286,40 @@ void ktime_get_ts(struct timespec *ts)
> > EXPORT_SYMBOL_GPL(ktime_get_ts);
> >
> > /**
> > + * getnstime_raw_and_real - Returns both the time of day an raw
> > + * monotonic time in a timespec format
> > + * @ts_mono_raw: pointer to the timespec to be set to raw
> > + * monotonic time
> > + * @ts_real: pointer to the timespec to be set to the time
> > + * of day
> > + */
> > +void getnstime_raw_and_real(struct timespec *ts_raw, struct timespec *ts_real)
> > +{
> > + unsigned long seq;
> > + s64 nsecs_raw, nsecs_real;
> > +
> > + WARN_ON(timekeeping_suspended);
>
> I suspect that if this warning ever triggers, it'll trigger at some
> high frequency making a complete mess all over the floor.
>
> WARN_ON_ONCE, perhaps? Or just remove it?

Well, getnstime_raw_and_real() is actually just a merge of
getnstimeofday() and getrawmonotonic(). The warning came from
getnstimeofday(). Usually this code should be called once a second but
the frequency can be higher. IMHO other functions like getnstimeofday()
and ktime_get_ts() would be a bigger problem anyway because they have
the same checks.

So I'm ok with either choice.
Hmm, will use WARN_ON_ONCE then if nobody objects.

--
Alexander
 | 
Pages: 1
Prev: x86-32: Fix crashes with CPU hotplug on AMD machines
Next: pps: add parallel port PPS signal generator