I was looking to queue Richard's ADJ_SETOFFSET patch to see
if it could be merged into -tip for 2.6.38, but on second
glance I noticed the ugly local_irq_disable bits as well
as the fact that adding the offset uses a gettime/add/settime
pattern which will also add a small error as the action isn't
atomic.
So I implemented a timekeeping function to add a fixed offset:
timekeeping_inject_offset(), and reworked Richard's code to
make use of it.
Richard: Any objection here? Mind testing this with the rest
of your patch queue?
Thomas: Any comments? Does this seem reasonable for 2.6.38?
Should I fold my cleanups into Richard's patch?
thanks
-john
CC: Richard Cochran <[email protected]>
CC: Thomas Gleixner <[email protected]>
John Stultz (2):
time: Introduce timekeeping_inject_offset
ntp: Change ADJ_SETOFFSET implementation to use
timekeeping_inject_offset
Richard Cochran (1):
ntp: add ADJ_SETOFFSET mode bit
include/linux/time.h | 1 +
include/linux/timex.h | 3 ++-
kernel/time/ntp.c | 13 +++++++++++++
kernel/time/timekeeping.c | 36 ++++++++++++++++++++++++++++++++++++
4 files changed, 52 insertions(+), 1 deletions(-)
--
1.7.3.2.146.gca209
This adds a kernel-internal timekeeping interface to add or subtract
a fixed amount from CLOCK_REALTIME. This makes it so kernel users or
interfaces trying to do so do not have to read the time, then add an
offset and then call settimeofday(), which adds some extra error in
comparision to just simply adding the offset in the kernel timekeeping
core.
CC: Thomas Gleixner <[email protected]>
CC: Richard Cochran <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
include/linux/time.h | 1 +
kernel/time/timekeeping.c | 36 ++++++++++++++++++++++++++++++++++++
2 files changed, 37 insertions(+), 0 deletions(-)
diff --git a/include/linux/time.h b/include/linux/time.h
index 9f15ac7..b402134 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -166,6 +166,7 @@ extern int timekeeping_valid_for_hres(void);
extern u64 timekeeping_max_deferment(void);
extern void update_wall_time(void);
extern void timekeeping_leap_insert(int leapsecond);
+extern int timekeeping_inject_offset(struct timespec *ts);
struct tms;
extern void do_sys_times(struct tms *);
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 49010d8..77e79b3 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -340,6 +340,42 @@ int do_settimeofday(struct timespec *tv)
EXPORT_SYMBOL(do_settimeofday);
+
+/**
+ * timekeeping_inject_offset - Adds or subtracts from the current time.
+ * @tv: pointer to the timespec variable containing the offset
+ *
+ * Adds or subtracts an offset value from the current time.
+ */
+int timekeeping_inject_offset(struct timespec *ts)
+{
+ unsigned long flags;
+
+ if ((unsigned long)ts->tv_nsec >= NSEC_PER_SEC)
+ return -EINVAL;
+
+ write_seqlock_irqsave(&xtime_lock, flags);
+
+ timekeeping_forward_now();
+
+ xtime = timespec_add(xtime, *ts);
+ wall_to_monotonic = timespec_sub(wall_to_monotonic, *ts);
+
+ timekeeper.ntp_error = 0;
+ ntp_clear();
+
+ update_vsyscall(&xtime, &wall_to_monotonic, timekeeper.clock,
+ timekeeper.mult);
+
+ write_sequnlock_irqrestore(&xtime_lock, flags);
+
+ /* signal hrtimers about time change */
+ clock_was_set();
+
+ return 0;
+}
+EXPORT_SYMBOL(timekeeping_inject_offset);
+
/**
* change_clocksource - Swaps clocksources if a new one is available
*
--
1.7.3.2.146.gca209
From: Richard Cochran <[email protected]>
This patch adds a new mode bit into the timex structure. When set, the bit
instructs the kernel to add the given time value to the current time.
CC: Thomas Gleixner <[email protected]>
LKML-Reference: <880d82bb8120f73973db27e0c48e949014b1a106.1292512461.git.richard.cochran@omicron.at>
Signed-off-by: Richard Cochran <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
include/linux/timex.h | 3 ++-
kernel/time/ntp.c | 26 ++++++++++++++++++++++++++
2 files changed, 28 insertions(+), 1 deletions(-)
diff --git a/include/linux/timex.h b/include/linux/timex.h
index 32d852f..82d4b24 100644
--- a/include/linux/timex.h
+++ b/include/linux/timex.h
@@ -73,7 +73,7 @@ struct timex {
long tolerance; /* clock frequency tolerance (ppm)
* (read only)
*/
- struct timeval time; /* (read only) */
+ struct timeval time; /* (read only, except for ADJ_SETOFFSET) */
long tick; /* (modified) usecs between clock ticks */
long ppsfreq; /* pps frequency (scaled ppm) (ro) */
@@ -101,6 +101,7 @@ struct timex {
#define ADJ_ESTERROR 0x0008 /* estimated time error */
#define ADJ_STATUS 0x0010 /* clock status */
#define ADJ_TIMECONST 0x0020 /* pll time constant */
+#define ADJ_SETOFFSET 0x0040 /* add 'time' to current time */
#define ADJ_TAI 0x0080 /* set TAI offset */
#define ADJ_MICRO 0x1000 /* select microsecond resolution */
#define ADJ_NANO 0x2000 /* select nanosecond resolution */
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index d232189..e9e3915 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -454,6 +454,7 @@ static inline void process_adjtimex_modes(struct timex *txc, struct timespec *ts
int do_adjtimex(struct timex *txc)
{
struct timespec ts;
+ ktime_t delta, kt;
int result;
/* Validate the data before disabling interrupts */
@@ -482,8 +483,33 @@ int do_adjtimex(struct timex *txc)
hrtimer_cancel(&leap_timer);
}
+ if (txc->modes & ADJ_SETOFFSET) {
+ /* Validate the delta value. */
+ if (txc->time.tv_sec && txc->time.tv_usec < 0)
+ return -EINVAL;
+
+ if (txc->modes & ADJ_NANO) {
+ struct timespec tmp;
+ tmp.tv_sec = txc->time.tv_sec;
+ tmp.tv_nsec = txc->time.tv_usec;
+ delta = timespec_to_ktime(tmp);
+ } else
+ delta = timeval_to_ktime(txc->time);
+
+ /* Adding the delta should be an "atomic" operation. */
+ local_irq_disable();
+ }
+
getnstimeofday(&ts);
+ if (txc->modes & ADJ_SETOFFSET) {
+ kt = timespec_to_ktime(ts);
+ kt = ktime_add(kt, delta);
+ ts = ktime_to_timespec(kt);
+ do_settimeofday(&ts);
+ local_irq_enable();
+ }
+
write_seqlock_irq(&xtime_lock);
if (txc->modes & ADJ_ADJTIME) {
--
1.7.3.2.146.gca209
Cleans up the ADJ_SETOFFSET implementation to use
timekeeping_inject_offset which greatly simplifies the code.
CC: Richard Cochran <[email protected]>
CC: Thomas Gleixner <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
kernel/time/ntp.c | 25 ++++++-------------------
1 files changed, 6 insertions(+), 19 deletions(-)
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index e9e3915..18d7a0f 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -454,7 +454,6 @@ static inline void process_adjtimex_modes(struct timex *txc, struct timespec *ts
int do_adjtimex(struct timex *txc)
{
struct timespec ts;
- ktime_t delta, kt;
int result;
/* Validate the data before disabling interrupts */
@@ -484,32 +483,20 @@ int do_adjtimex(struct timex *txc)
}
if (txc->modes & ADJ_SETOFFSET) {
+ struct timespec delta;
/* Validate the delta value. */
if (txc->time.tv_sec && txc->time.tv_usec < 0)
return -EINVAL;
- if (txc->modes & ADJ_NANO) {
- struct timespec tmp;
- tmp.tv_sec = txc->time.tv_sec;
- tmp.tv_nsec = txc->time.tv_usec;
- delta = timespec_to_ktime(tmp);
- } else
- delta = timeval_to_ktime(txc->time);
-
- /* Adding the delta should be an "atomic" operation. */
- local_irq_disable();
+ delta.tv_sec = txc->time.tv_sec;
+ delta.tv_nsec = txc->time.tv_usec;
+ if (!(txc->modes & ADJ_NANO))
+ delta.tv_nsec *= 1000;
+ timekeeping_inject_offset(&delta);
}
getnstimeofday(&ts);
- if (txc->modes & ADJ_SETOFFSET) {
- kt = timespec_to_ktime(ts);
- kt = ktime_add(kt, delta);
- ts = ktime_to_timespec(kt);
- do_settimeofday(&ts);
- local_irq_enable();
- }
-
write_seqlock_irq(&xtime_lock);
if (txc->modes & ADJ_ADJTIME) {
--
1.7.3.2.146.gca209
On Mon, Dec 27, 2010 at 03:40:41PM -0800, John Stultz wrote:
> I was looking to queue Richard's ADJ_SETOFFSET patch to see
> if it could be merged into -tip for 2.6.38, but on second
> glance I noticed the ugly local_irq_disable bits as well
> as the fact that adding the offset uses a gettime/add/settime
> pattern which will also add a small error as the action isn't
> atomic.
>
> So I implemented a timekeeping function to add a fixed offset:
> timekeeping_inject_offset(), and reworked Richard's code to
> make use of it.
Okay, so you added an optimized version of do_settimeofday() for
jumping the clock. It certainly helps the code in do_adjtimex(), but
it also (nearly) duplicates do_settimeofday(). I guess you will not
mind having to maintain both code paths...
> Richard: Any objection here? Mind testing this with the rest
> of your patch queue?
Well, you have uncovered a problem.
The code I offered was broken to begin with, but I think your patch is
also troubled. The timespec is awkwardly split into seconds and
nanoseconds, and I think that arithmetic using timespecs is not well
defined. Or perhaps only I am confused by it all.
The problem seems to be, how can a timespec have a sign?
The exisiting code seems to assume that a timespec can only have the
sign in one field. In other words, if tv_sec is non-zero, then tv_nsec
must be non-negative. (I added a check for this into my patch).
I took a second look at ktime_add() vs. timespec_add() and discovered
a problems both. Consider the following test code:
static void kt_add(struct timespec now, struct timespec adj)
{
ktime_t delta, kt;
struct timespec ts;
delta = timespec_to_ktime(adj);
kt = timespec_to_ktime(now);
kt = ktime_add(kt, delta);
ts = ktime_to_timespec(kt);
pr_err("kt add: {%ld,%ld} + {%ld,%ld} = {%ld,%ld}\n",
now.tv_sec, now.tv_nsec,
adj.tv_sec, adj.tv_nsec,
ts.tv_sec, ts.tv_nsec);
}
static void ts_add(struct timespec now, struct timespec adj)
{
struct timespec ts;
ts = timespec_add(now, adj);
pr_err("ts add: {%ld,%ld} + {%ld,%ld} = {%ld,%ld}\n",
now.tv_sec, now.tv_nsec,
adj.tv_sec, adj.tv_nsec,
ts.tv_sec, ts.tv_nsec);
}
There are (at least) four cases to consider:
1. adj > 1.0
kt add: {2,0} + {1,100} = {3,100}
ts add: {2,0} + {1,100} = {3,100}
2. adj < -1.0
kt add: {2,0} + {-1,100} = {1,100}
ts add: {2,0} + {-1,100} = {1,100}
3. 0 < adj < 1.0
kt add: {2,0} + {0,100} = {2,100}
ts add: {2,0} + {0,100} = {2,100}
4. -1.0 < adj < 0
kt add: {2,0} + {0,-100} = {6,294967196}
ts add: {2,0} + {0,-100} = {1,999999900}
Case 2 fails for both functions.
Case 4 fails when using ktime_add().
So it seems that I have now way to correctly encode a negative offset
less than -1.0 into a timespec. Perhaps we could specify new rules for
timespecs.
1. Time Values:
If tv_sec is non-zero, then tv_nsec must be non-negative.
2. Time Deltas:
The sign of tv_sec and tv_nsec must be the same.
In any case, I would like you help in clarifying all of this...
Thanks,
Richard
On Tue, Dec 28, 2010 at 12:20:00PM +0100, Richard Cochran wrote:
> The exisiting code seems to assume that a timespec can only have the
> sign in one field. In other words, if tv_sec is non-zero, then tv_nsec
> must be non-negative. (I added a check for this into my patch).
Looking at ktime.h we find:
* Be especially aware that negative values are represented in a way
* that the tv.sec field is negative and the tv.nsec field is greater
* or equal to zero but less than nanoseconds per second. This is the
* same representation which is used by timespecs.
*
* tv.sec < 0 and 0 >= tv.nsec < NSEC_PER_SEC
So it seems that the time values (or time intervals) in the range
-1 < x < 0 are not even possible, from the point of view of ktime.
> So it seems that I have now way to correctly encode a negative offset
I meant, "no way"---------^
> less than -1.0 into a timespec. Perhaps we could specify new rules for
> timespecs.
> 1. Time Values:
> If tv_sec is non-zero, then tv_nsec must be non-negative.
>
> 2. Time Deltas:
> The sign of tv_sec and tv_nsec must be the same.
For me, the confusion gets worse when you consider the timespec values
delivered by the system for dates before the 1970 epoch. In that case,
the kernel always gives the tv_nsec value as a positive number. The
value makes sense when interpreted as a sum.
Reading consecutive clock values every 0.1 seconds produces, for
example:
{sec, nsec} sum
-------------------------
{-2, +600000000} -1.4
{-2, +700000000} -1.3
{-2, +800000000} -1.2
{-2, +900000000} -1.1
{-1, 000000000} -1.0
{-1, +100000000} -0.9
{-1, +200000000} -0.8
{-1, +300000000} -0.7
If we say that the time value or interval in a timespec is always the
sum of the fields, still it would seem that the ktime code is broken
according to that assumption.
Perhaps someone can clarify?
Thanks,
Richard
On Tue, Dec 28, 2010 at 01:53:32PM +0100, Richard Cochran wrote:
> {sec, nsec} sum
> -------------------------
> {-2, +600000000} -1.4
> {-2, +700000000} -1.3
> {-2, +800000000} -1.2
> {-2, +900000000} -1.1
> {-1, 000000000} -1.0
> {-1, +100000000} -0.9
> {-1, +200000000} -0.8
> {-1, +300000000} -0.7
>
> If we say that the time value or interval in a timespec is always the
> sum of the fields, still it would seem that the ktime code is broken
> according to that assumption.
Okay, now I think I get it.
The value of a timespec is the sum of its fields. The tv_nsec field
should always be non-negative.
So, if a user space program wants to jump the clock back by one-tenth
second, it must pass a timespec with value {-1,900000000}.
Right?
Thanks,
Richard
On Tue, Dec 28, 2010 at 8:40 AM, John Stultz <[email protected]> wrote:
> From: Richard Cochran <[email protected]>
>
> This patch adds a new mode bit into the timex structure. When set, the
> bit instructs the kernel to add the given time value to the current time.
I came up with this simple solution: "Just use ADJ_OFFSET as usual,
but don't forget to disable the kernel PLL."
Here's my (untested) patch.
---
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index c631168..d492887 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -119,14 +119,21 @@
return div_s64(offset64 << (NTP_SCALE_SHIFT - SHIFT_FLL), secs);
}
-static void ntp_update_offset(long offset)
+static void ntp_update_offset(long offset, struct timespec *ts)
{
s64 freq_adj;
s64 offset64;
long secs;
- if (!(time_status & STA_PLL))
+ if (!(time_status & STA_PLL)) {
+ offset64 = (s64)offset;
+ if (!(time_status & STA_NANO))
+ offset64 *= NSEC_PER_USEC;
+
+ set_normalized_timespec(ts, ts->tv_sec, offset64 + ts->tv_nsec);
+
return;
+ }
if (!(time_status & STA_NANO))
offset *= NSEC_PER_USEC;
@@ -430,7 +437,7 @@
time_tai = txc->constant;
if (txc->modes & ADJ_OFFSET)
- ntp_update_offset(txc->offset);
+ ntp_update_offset(txc->offset, ts);
if (txc->modes & ADJ_TICK)
tick_usec = txc->tick;
@@ -526,6 +533,9 @@
write_sequnlock_irq(&xtime_lock);
+ if ((txc->modes & ADJ_OFFSET) && !(time_status & STA_PLL))
+ do_settimeofday(&ts);
+
txc->time.tv_sec = ts.tv_sec;
txc->time.tv_usec = ts.tv_nsec;
if (!(time_status & STA_NANO))
--