Home » Mailing list archives » linux-kernel » 2011 » January » 4

Re: [PATCH 2/3] ntp: add ADJ_SETOFFSET mode bit

Previous thread: [PATCH 1/3] time: Introduce timekeeping_inject_offset by John Stultz on Monday, December 27, 2010 - 4:40 pm. (1 message)

Next thread: [PATCH 3/3] ntp: Change ADJ_SETOFFSET implementation to use timekeeping_inject_offset by John Stultz on Monday, December 27, 2010 - 4:40 pm. (1 message)
[view original message]
From: John Stultz
Subject: [PATCH 2/3] ntp: add ADJ_SETOFFSET mode bit
Date: Monday, December 27, 2010 - 4:40 pm

From: Richard Cochran <richardcochran@gmail.com>

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 <tglx@linutronix.de>
LKML-Reference: <880d82bb8120f73973db27e0c48e949014b1a106.1292512461.git.richard.cochran@omicron.at>
Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 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);
 	}
 ...
[ message continues ]
[view original message]
From: Kuwahara,T.
Subject: Re: [PATCH 2/3] ntp: add ADJ_SETOFFSET mode bit
Date: Tuesday, December 28, 2010 - 1:47 pm

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


[ message continues ]
[view original message]
From: John Stultz
Subject: Re: [PATCH 2/3] ntp: add ADJ_SETOFFSET mode bit
Date: Monday, January 3, 2011 - 1:44 pm

Again, this seems to be trying to add new functionality into a corner
case of a existing mode.

I don't like this because it makes testing and validating that the code
is correct for its uses difficult. Especially given that a number of
combinations of modes might be set at once. What happens to freq
adjustments done at the same time as an ADJ_OFFSET - STA_PLL?

Personally, I see the adjtimex call as too flexible, as I'd prefer to
have the modes be exclusive (adjusting one thing per call). As it is
now, the kernel doesn't throw out enough invalid to invalid-ish cases.
ie: MOD_NANO|MOD_MICRO: totally cool! We should fix these and make the
input checking more strict, but in my mind moving to mode-numbers rather
then mode-flags would be much nicer (and more extendable).

Richard: Maybe this is a good thing to think about for clock_adjtime? If
we are adding a new syscall, maybe we should make sure we clean up some
of the old syscalls issues? It does add a good bit of complexity, as the
idea of clock_adjtime being a multiplexing adjtimex was nice and simple.
We'd also have to review the mode usage to see if multi-mode adjustments
in a single call are all that common or not.

Kuwahara: Maybe could you maybe better explain your passion against
using a new mode-flag for this new functionality? You seem dead set
against it, but have not expressed your rational well.

thanks
-john

--


[ message continues ]
[view original message]
From: Richard Cochran
Subject: Re: [PATCH 2/3] ntp: add ADJ_SETOFFSET mode bit
Date: Tuesday, January 4, 2011 - 1:37 am

Where are the mode bits and their combinations documented, anyhow?

There are lot of papers on NTP, and I have not read them all, but I
don't recall having seen a precise and succinct decription of the
ntp_adjtime interface anywhere.

Thanks,
Richard
--


[ message continues ]
[view original message]
From: John Stultz
Subject: Re: [PATCH 2/3] ntp: add ADJ_SETOFFSET mode bit
Date: Tuesday, January 4, 2011 - 12:08 pm

You can find the original ntp_adjtime description here:
Page 22 of:
http://www.eecis.udel.edu/~mills/database/rfc/rfc1589.txt

This pre-dates the nanotime kernel paper, which added MOD_NANO.

thanks
-john



--


[ message continues ]
[view original message]
From: Richard Cochran
Subject: Re: [PATCH 2/3] ntp: add ADJ_SETOFFSET mode bit
Date: Tuesday, January 4, 2011 - 1:40 am

So, is the NTP source code the documentation of the kernel interface?

Thanks,
Richard
--


[ message continues ]
[view original message]
From: John Stultz
Subject: Re: [PATCH 2/3] ntp: add ADJ_SETOFFSET mode bit
Date: Tuesday, January 4, 2011 - 12:14 pm

Yea. adjtimex is a combination of ntp_adjtime and the older adjtime
interfaces. So its not identical to David Mill's design, but it is
compatible.  In fact, it wasn't until somewhat recently that it picked
up the ntpv4 changes and MOD/ADJ_NANO.

By the way, I'm not saying we should switch from using mode flags to
mode numbers for the new interface as I'm not sure if it would confuse
users moving to it (being very similar, but slightly different can be
worse then being totally different). But I figure it warrants some
consideration. We do still have 4 unused bits in the modes flags after
your patch, so this may be a premature worry.

thanks
-john




--


[ message continues ]
Previous thread: [PATCH 1/3] time: Introduce timekeeping_inject_offset by John Stultz on Monday, December 27, 2010 - 4:40 pm. (1 message)

Next thread: [PATCH 3/3] ntp: Change ADJ_SETOFFSET implementation to use timekeeping_inject_offset by John Stultz on Monday, December 27, 2010 - 4:40 pm. (1 message)