[PATCH 0/8] Move ntp state to be protected by timekeeping lock

[PATCH 0/8] Move ntp state to be protected by timekeeping lock

[John Stultz]

So this is sort of a frustrating patchset, as previously much work
had been done to split the NTP state out from being protected by the
xtime_lock of yore. But apparently the crystal ball was foggy back
then.

For some time now, Thomas and Eric have been pushing to use
shadow-updates on the timekeeping state. Basically he's split the
timekeeping seq lock into a  spinlock and a seqcounter. On updates,
one much take the spinlock and the seqcount write lock. And on the
read-side you just take the seqcount readlock.

The benefit with this appraoch is that classically timekeeping
updates can be somewhat calculation heavy and can take some time.
This new lock-splitting allows us  to just take the spinlock,
blocking only other updaters, and using  a shadow copy of the
timekeeping state calculate the update. Then we only take the
seqcounter write lock for a very short time in order to copy over
the pre-calculated shadow-time state. This allows for much reduced
write-side lock hold times on the timekeeping lock, which avoids
blocking timekeeping readers for the entire update.

In his realtime testing, this reduces his worst case latencies from
8us to 4us, which is a very attractive improvement.

The problem of course, is that the new asynchronous behavior the
shadow updates breaks some of the assumptions on how the NTP state
is used. Thus to correct this, we really need to serialize the ntp
state updates along with the timekeeping state. With the added side
benefit that of reducing lock acquisitions.

The downside is that the timekeeping state has been cleaned up to
live nicely in the timekeeper struct, which nicely bounded what the
timekeeping lock protected. Where as 99% of the NTP state was all
static to ntp.c, and was protected by the ntp.c static ntp_lock, so
it was all nicely encapsulated as well. 

This patchset makes the lock ownership lines less obvious, but I've
been sure to keep the ntp state static to ntp.c and instead provided
some accessors via ntp-internal.h that timekeping code can use to
make changes.  The only really ugly part is that do_adjtimex() has
to split some of the logic between timekeeping.c and ntp.c in order
to really get the locking done correctly.

I may try to rework the code in the future so that the timekeeper
holds the ntp state and passes it to the ntp.c functions to be
modified, but that is a much deeper rework then I'd like to do right
now, and causes fruther complexity to the shadow-state updates, since
we'd end up unnecessarily copying the ntp state back and forth every
time.

This applies on top of my fortglx/3.10/time queue here:
git://git.linaro.org/people/jstultz/linux.git fortglx/3.10/time

If you want to see this entire set (along with Thomas' shadow-update
work) it can be found here:
git://git.linaro.org/people/jstultz/linux.git dev/tglx-shadowtime
or 
http://git.linaro.org/gitweb?p=people/jstultz/linux.git;a=shortlog;h=refs/heads/dev/tglx-shadowtime


Let me know if you have any feedback or comments!

thanks
-john

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Prarit Bhargava <prarit@redhat.com>

John Stultz (8):
  ntp: Split out timex validation from do_adjtimex
  ntp: Move do_adjtimex() and hardpps() functions to timekeeping.c
  ntp: Move timex validation to timekeeping do_adjtimex call.
  ntp: Rework do_adjtimex to take timespec and tai arguments
  timekeeping: Move ADJ_SETOFFSET to top level do_adjtimex()
  timekeeping: Hold timekeepering locks in do_adjtimex and hardpps
  timekeeping: Simplify tai updating from do_adjtimex
  ntp: Remove ntp_lock, using the timekeeping locks to protect ntp
    state

 include/linux/timex.h      |    7 ----
 kernel/time/ntp.c          |   99 +++++++++++++-------------------------------
 kernel/time/ntp_internal.h |   12 ++++++
 kernel/time/timekeeping.c  |   66 ++++++++++++++++++++++++++++-
 4 files changed, 104 insertions(+), 80 deletions(-)
 create mode 100644 kernel/time/ntp_internal.h

-- 
1.7.10.4

[PATCH 1/8] ntp: Split out timex validation from do_adjtimex

[John Stultz]

Split out the timex validation done in do_adjtimex into a separate
function. This will help simplify logic in following patches.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/ntp.c |   39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 59e2749..457d2ba 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -622,17 +622,13 @@ static inline void process_adjtimex_modes(struct timex *txc,
 		ntp_update_frequency();
 }
 
-/*
- * adjtimex mainly allows reading (and writing, if superuser) of
- * kernel time-keeping variables. used by xntpd.
+
+
+/**
+ * ntp_validate_timex - Ensures the timex is ok for use in do_adjtimex
  */
-int do_adjtimex(struct timex *txc)
+int ntp_validate_timex(struct timex *txc)
 {
-	struct timespec ts;
-	u32 time_tai, orig_tai;
-	int result;
-
-	/* Validate the data before disabling interrupts */
 	if (txc->modes & ADJ_ADJTIME) {
 		/* singleshot must not be used with any other mode bits */
 		if (!(txc->modes & ADJ_OFFSET_SINGLESHOT))
@@ -644,7 +640,6 @@ int do_adjtimex(struct timex *txc)
 		/* In order to modify anything, you gotta be super-user! */
 		 if (txc->modes && !capable(CAP_SYS_TIME))
 			return -EPERM;
-
 		/*
 		 * if the quartz is off by more than 10% then
 		 * something is VERY wrong!
@@ -655,12 +650,32 @@ int do_adjtimex(struct timex *txc)
 			return -EINVAL;
 	}
 
+	if ((txc->modes & ADJ_SETOFFSET) && (!capable(CAP_SYS_TIME)))
+		return -EPERM;
+
+	return 0;
+}
+
+
+/*
+ * adjtimex mainly allows reading (and writing, if superuser) of
+ * kernel time-keeping variables. used by xntpd.
+ */
+int do_adjtimex(struct timex *txc)
+{
+	struct timespec ts;
+	u32 time_tai, orig_tai;
+	int result;
+
+	/* Validate the data before disabling interrupts */
+	result = ntp_validate_timex(txc);
+	if (result)
+		return result;
+
 	if (txc->modes & ADJ_SETOFFSET) {
 		struct timespec delta;
 		delta.tv_sec  = txc->time.tv_sec;
 		delta.tv_nsec = txc->time.tv_usec;
-		if (!capable(CAP_SYS_TIME))
-			return -EPERM;
 		if (!(txc->modes & ADJ_NANO))
 			delta.tv_nsec *= 1000;
 		result = timekeeping_inject_offset(&delta);
-- 
1.7.10.4

[PATCH 2/8] ntp: Move do_adjtimex() and hardpps() functions to timekeeping.c

[John Stultz]

In preparation for changing the ntp locking rules, move
do_adjtimex and hardpps accessor functions to timekeeping.c,
but keep the code logic in ntp.c.

This patch also introduces a ntp_internal.h file so timekeeping
specific interfaces of ntp.c can be more limitedly shared with
timekeeping.c.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 include/linux/timex.h      |    7 -------
 kernel/time/ntp.c          |    9 ++++-----
 kernel/time/ntp_internal.h |   11 +++++++++++
 kernel/time/timekeeping.c  |   21 +++++++++++++++++++++
 4 files changed, 36 insertions(+), 12 deletions(-)
 create mode 100644 kernel/time/ntp_internal.h

diff --git a/include/linux/timex.h b/include/linux/timex.h
index 5ec87c6..b3726e6 100644
--- a/include/linux/timex.h
+++ b/include/linux/timex.h
@@ -125,9 +125,6 @@
 extern unsigned long tick_usec;		/* USER_HZ period (usec) */
 extern unsigned long tick_nsec;		/* SHIFTED_HZ period (nsec) */
 
-extern void ntp_init(void);
-extern void ntp_clear(void);
-
 /* Required to safely shift negative values */
 #define shift_right(x, s) ({	\
 	__typeof__(x) __x = (x);	\
@@ -140,10 +137,6 @@ extern void ntp_clear(void);
 #define NTP_INTERVAL_FREQ  (HZ)
 #define NTP_INTERVAL_LENGTH (NSEC_PER_SEC/NTP_INTERVAL_FREQ)
 
-/* Returns how long ticks are at present, in ns / 2^NTP_SCALE_SHIFT. */
-extern u64 ntp_tick_length(void);
-
-extern int second_overflow(unsigned long secs);
 extern int do_adjtimex(struct timex *);
 extern void hardpps(const struct timespec *, const struct timespec *);
 
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 457d2ba..8b10706 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -18,6 +18,7 @@
 #include <linux/rtc.h>
 
 #include "tick-internal.h"
+#include "ntp_internal.h"
 
 /*
  * NTP timekeeping variables:
@@ -661,7 +662,7 @@ int ntp_validate_timex(struct timex *txc)
  * adjtimex mainly allows reading (and writing, if superuser) of
  * kernel time-keeping variables. used by xntpd.
  */
-int do_adjtimex(struct timex *txc)
+int __do_adjtimex(struct timex *txc)
 {
 	struct timespec ts;
 	u32 time_tai, orig_tai;
@@ -911,7 +912,7 @@ static void hardpps_update_phase(long error)
 }
 
 /*
- * hardpps() - discipline CPU clock oscillator to external PPS signal
+ * __hardpps() - discipline CPU clock oscillator to external PPS signal
  *
  * This routine is called at each PPS signal arrival in order to
  * discipline the CPU clock oscillator to the PPS signal. It takes two
@@ -922,7 +923,7 @@ static void hardpps_update_phase(long error)
  * This code is based on David Mills's reference nanokernel
  * implementation. It was mostly rewritten but keeps the same idea.
  */
-void hardpps(const struct timespec *phase_ts, const struct timespec *raw_ts)
+void __hardpps(const struct timespec *phase_ts, const struct timespec *raw_ts)
 {
 	struct pps_normtime pts_norm, freq_norm;
 	unsigned long flags;
@@ -976,8 +977,6 @@ void hardpps(const struct timespec *phase_ts, const struct timespec *raw_ts)
 
 	raw_spin_unlock_irqrestore(&ntp_lock, flags);
 }
-EXPORT_SYMBOL(hardpps);
-
 #endif	/* CONFIG_NTP_PPS */
 
 static int __init ntp_tick_adj_setup(char *str)
diff --git a/kernel/time/ntp_internal.h b/kernel/time/ntp_internal.h
new file mode 100644
index 0000000..fdee80c
--- /dev/null
+++ b/kernel/time/ntp_internal.h
@@ -0,0 +1,11 @@
+#ifndef _LINUX_NTP_INTERNAL_H
+#define _LINUX_NTP_INTERNAL_H
+
+extern void ntp_init(void);
+extern void ntp_clear(void);
+/* Returns how long ticks are at present, in ns / 2^NTP_SCALE_SHIFT. */
+extern u64 ntp_tick_length(void);
+extern int second_overflow(unsigned long secs);
+extern int __do_adjtimex(struct timex *);
+extern void __hardpps(const struct timespec *, const struct timespec *);
+#endif /* _LINUX_NTP_INTERNAL_H */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index c5feb7a..a138ec2 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -24,6 +24,7 @@
 #include <linux/pvclock_gtod.h>
 
 #include "tick-internal.h"
+#include "ntp_internal.h"
 
 static struct timekeeper timekeeper;
 static DEFINE_RAW_SPINLOCK(timekeeper_lock);
@@ -1613,6 +1614,26 @@ ktime_t ktime_get_monotonic_offset(void)
 EXPORT_SYMBOL_GPL(ktime_get_monotonic_offset);
 
 /**
+ * do_adjtimex() - Accessor function to NTP __do_adjtimex function
+ */
+int do_adjtimex(struct timex *txc)
+{
+	return __do_adjtimex(txc);
+}
+
+
+#ifdef CONFIG_NTP_PPS
+/**
+ * hardpps() - Accessor function to NTP __hardpps function
+ */
+void hardpps(const struct timespec *phase_ts, const struct timespec *raw_ts)
+{
+	__hardpps(phase_ts, raw_ts);
+}
+EXPORT_SYMBOL(hardpps);
+#endif
+
+/**
  * xtime_update() - advances the timekeeping infrastructure
  * @ticks:	number of ticks, that have elapsed since the last call.
  *
-- 
1.7.10.4

[PATCH 3/8] ntp: Move timex validation to timekeeping do_adjtimex call.

[John Stultz]

Move logic that does not need the ntp state to be done
in the timekeeping do_adjtimex() call.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/ntp.c          |    5 -----
 kernel/time/ntp_internal.h |    1 +
 kernel/time/timekeeping.c  |    7 +++++++
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 8b10706..2dc60c6 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -668,11 +668,6 @@ int __do_adjtimex(struct timex *txc)
 	u32 time_tai, orig_tai;
 	int result;
 
-	/* Validate the data before disabling interrupts */
-	result = ntp_validate_timex(txc);
-	if (result)
-		return result;
-
 	if (txc->modes & ADJ_SETOFFSET) {
 		struct timespec delta;
 		delta.tv_sec  = txc->time.tv_sec;
diff --git a/kernel/time/ntp_internal.h b/kernel/time/ntp_internal.h
index fdee80c..a2a3976 100644
--- a/kernel/time/ntp_internal.h
+++ b/kernel/time/ntp_internal.h
@@ -6,6 +6,7 @@ extern void ntp_clear(void);
 /* Returns how long ticks are at present, in ns / 2^NTP_SCALE_SHIFT. */
 extern u64 ntp_tick_length(void);
 extern int second_overflow(unsigned long secs);
+extern int ntp_validate_timex(struct timex *);
 extern int __do_adjtimex(struct timex *);
 extern void __hardpps(const struct timespec *, const struct timespec *);
 #endif /* _LINUX_NTP_INTERNAL_H */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index a138ec2..f6c8a72 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1618,6 +1618,13 @@ EXPORT_SYMBOL_GPL(ktime_get_monotonic_offset);
  */
 int do_adjtimex(struct timex *txc)
 {
+	int ret;
+
+	/* Validate the data before disabling interrupts */
+	ret = ntp_validate_timex(txc);
+	if (ret)
+		return ret;
+
 	return __do_adjtimex(txc);
 }
 
-- 
1.7.10.4

[PATCH 4/8] ntp: Rework do_adjtimex to take timespec and tai arguments

[John Stultz]

In order to change the locking rules, we need to provide
the timespec and tai values rather then having the ntp
logic acquire these values itself.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/ntp.c          |   18 +++++-------------
 kernel/time/ntp_internal.h |    2 +-
 kernel/time/timekeeping.c  |   13 +++++++++++--
 3 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 2dc60c6..d17e13c 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -662,10 +662,8 @@ int ntp_validate_timex(struct timex *txc)
  * adjtimex mainly allows reading (and writing, if superuser) of
  * kernel time-keeping variables. used by xntpd.
  */
-int __do_adjtimex(struct timex *txc)
+int __do_adjtimex(struct timex *txc, struct timespec *ts, s32 *time_tai)
 {
-	struct timespec ts;
-	u32 time_tai, orig_tai;
 	int result;
 
 	if (txc->modes & ADJ_SETOFFSET) {
@@ -679,9 +677,6 @@ int __do_adjtimex(struct timex *txc)
 			return result;
 	}
 
-	getnstimeofday(&ts);
-	orig_tai = time_tai = timekeeping_get_tai_offset();
-
 	raw_spin_lock_irq(&ntp_lock);
 
 	if (txc->modes & ADJ_ADJTIME) {
@@ -697,7 +692,7 @@ int __do_adjtimex(struct timex *txc)
 
 		/* If there are input parameters, then process them: */
 		if (txc->modes)
-			process_adjtimex_modes(txc, &ts, &time_tai);
+			process_adjtimex_modes(txc, ts, time_tai);
 
 		txc->offset = shift_right(time_offset * NTP_INTERVAL_FREQ,
 				  NTP_SCALE_SHIFT);
@@ -719,18 +714,15 @@ int __do_adjtimex(struct timex *txc)
 	txc->precision	   = 1;
 	txc->tolerance	   = MAXFREQ_SCALED / PPM_SCALE;
 	txc->tick	   = tick_usec;
-	txc->tai	   = time_tai;
+	txc->tai	   = *time_tai;
 
 	/* fill PPS status fields */
 	pps_fill_timex(txc);
 
 	raw_spin_unlock_irq(&ntp_lock);
 
-	if (time_tai != orig_tai)
-		timekeeping_set_tai_offset(time_tai);
-
-	txc->time.tv_sec = ts.tv_sec;
-	txc->time.tv_usec = ts.tv_nsec;
+	txc->time.tv_sec = ts->tv_sec;
+	txc->time.tv_usec = ts->tv_nsec;
 	if (!(time_status & STA_NANO))
 		txc->time.tv_usec /= NSEC_PER_USEC;
 
diff --git a/kernel/time/ntp_internal.h b/kernel/time/ntp_internal.h
index a2a3976..1950cb4 100644
--- a/kernel/time/ntp_internal.h
+++ b/kernel/time/ntp_internal.h
@@ -7,6 +7,6 @@ extern void ntp_clear(void);
 extern u64 ntp_tick_length(void);
 extern int second_overflow(unsigned long secs);
 extern int ntp_validate_timex(struct timex *);
-extern int __do_adjtimex(struct timex *);
+extern int __do_adjtimex(struct timex *, struct timespec *, s32 *);
 extern void __hardpps(const struct timespec *, const struct timespec *);
 #endif /* _LINUX_NTP_INTERNAL_H */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index f6c8a72..5f7a233 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1618,6 +1618,8 @@ EXPORT_SYMBOL_GPL(ktime_get_monotonic_offset);
  */
 int do_adjtimex(struct timex *txc)
 {
+	struct timespec ts;
+	s32 tai, orig_tai;
 	int ret;
 
 	/* Validate the data before disabling interrupts */
@@ -1625,9 +1627,16 @@ int do_adjtimex(struct timex *txc)
 	if (ret)
 		return ret;
 
-	return __do_adjtimex(txc);
-}
+	getnstimeofday(&ts);
+	orig_tai = tai = timekeeping_get_tai_offset();
+
+	ret = __do_adjtimex(txc, &ts, &tai);
 
+	if (tai != orig_tai)
+		timekeeping_set_tai_offset(tai);
+
+	return ret;
+}
 
 #ifdef CONFIG_NTP_PPS
 /**
-- 
1.7.10.4

[PATCH 5/8] timekeeping: Move ADJ_SETOFFSET to top level do_adjtimex()

[John Stultz]

Since ADJ_SETOFFSET adjusts the timekeeping state, process
it as part of the top level do_adjtimex() function in
timekeeping.c.

This avoids deadlocks that could occur once we change the
ntp locking rules.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/ntp.c         |   11 -----------
 kernel/time/timekeeping.c |   11 +++++++++++
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index d17e13c..a331ebc 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -666,17 +666,6 @@ int __do_adjtimex(struct timex *txc, struct timespec *ts, s32 *time_tai)
 {
 	int result;
 
-	if (txc->modes & ADJ_SETOFFSET) {
-		struct timespec delta;
-		delta.tv_sec  = txc->time.tv_sec;
-		delta.tv_nsec = txc->time.tv_usec;
-		if (!(txc->modes & ADJ_NANO))
-			delta.tv_nsec *= 1000;
-		result = timekeeping_inject_offset(&delta);
-		if (result)
-			return result;
-	}
-
 	raw_spin_lock_irq(&ntp_lock);
 
 	if (txc->modes & ADJ_ADJTIME) {
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 5f7a233..e44915c 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1627,6 +1627,17 @@ int do_adjtimex(struct timex *txc)
 	if (ret)
 		return ret;
 
+	if (txc->modes & ADJ_SETOFFSET) {
+		struct timespec delta;
+		delta.tv_sec  = txc->time.tv_sec;
+		delta.tv_nsec = txc->time.tv_usec;
+		if (!(txc->modes & ADJ_NANO))
+			delta.tv_nsec *= 1000;
+		ret = timekeeping_inject_offset(&delta);
+		if (ret)
+			return ret;
+	}
+
 	getnstimeofday(&ts);
 	orig_tai = tai = timekeeping_get_tai_offset();
 
-- 
1.7.10.4

[PATCH 6/8] timekeeping: Hold timekeepering locks in do_adjtimex and hardpps

[John Stultz]

In moving the NTP state to be protected by the timekeeping locks,
be sure to acquire the timekeeping locks prior to calling
ntp functions.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/timekeeping.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index e44915c..d10bd73 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -787,10 +787,10 @@ void __init timekeeping_init(void)
 		boot.tv_nsec = 0;
 	}
 
-	ntp_init();
-
 	raw_spin_lock_irqsave(&timekeeper_lock, flags);
 	write_seqcount_begin(&timekeeper_seq);
+	ntp_init();
+
 	clock = clocksource_default_clock();
 	if (clock->enable)
 		clock->enable(clock);
@@ -1618,6 +1618,7 @@ EXPORT_SYMBOL_GPL(ktime_get_monotonic_offset);
  */
 int do_adjtimex(struct timex *txc)
 {
+	unsigned long flags;
 	struct timespec ts;
 	s32 tai, orig_tai;
 	int ret;
@@ -1641,8 +1642,14 @@ int do_adjtimex(struct timex *txc)
 	getnstimeofday(&ts);
 	orig_tai = tai = timekeeping_get_tai_offset();
 
+	raw_spin_lock_irqsave(&timekeeper_lock, flags);
+	write_seqcount_begin(&timekeeper_seq);
+
 	ret = __do_adjtimex(txc, &ts, &tai);
 
+	write_seqcount_end(&timekeeper_seq);
+	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
+
 	if (tai != orig_tai)
 		timekeeping_set_tai_offset(tai);
 
@@ -1655,7 +1662,15 @@ int do_adjtimex(struct timex *txc)
  */
 void hardpps(const struct timespec *phase_ts, const struct timespec *raw_ts)
 {
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&timekeeper_lock, flags);
+	write_seqcount_begin(&timekeeper_seq);
+
 	__hardpps(phase_ts, raw_ts);
+
+	write_seqcount_end(&timekeeper_seq);
+	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
 }
 EXPORT_SYMBOL(hardpps);
 #endif
-- 
1.7.10.4

[PATCH 7/8] timekeeping: Simplify tai updating from do_adjtimex

[John Stultz]

Since we are taking the timekeeping locks, just go ahead
and update any tai change directly, rather then dropping
the lock and calling a function that will just take it again.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/timekeeping.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index d10bd73..f93f60c 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1618,9 +1618,10 @@ EXPORT_SYMBOL_GPL(ktime_get_monotonic_offset);
  */
 int do_adjtimex(struct timex *txc)
 {
+	struct timekeeper *tk = &timekeeper;
 	unsigned long flags;
 	struct timespec ts;
-	s32 tai, orig_tai;
+	s32 tai;
 	int ret;
 
 	/* Validate the data before disabling interrupts */
@@ -1640,19 +1641,17 @@ int do_adjtimex(struct timex *txc)
 	}
 
 	getnstimeofday(&ts);
-	orig_tai = tai = timekeeping_get_tai_offset();
 
 	raw_spin_lock_irqsave(&timekeeper_lock, flags);
 	write_seqcount_begin(&timekeeper_seq);
 
+	tai = tk->tai_offset;
 	ret = __do_adjtimex(txc, &ts, &tai);
 
+	__timekeeping_set_tai_offset(tk, tai);
 	write_seqcount_end(&timekeeper_seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
 
-	if (tai != orig_tai)
-		timekeeping_set_tai_offset(tai);
-
 	return ret;
 }
 
-- 
1.7.10.4

[PATCH 8/8] ntp: Remove ntp_lock, using the timekeeping locks to protect ntp state

[John Stultz]

In order to properly handle the NTP state in future changes to the
timekeeping lock management, this patch moves the management of
all of the ntp state under the timekeeping locks.

This allows us to remove the ntp_lock.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/ntp.c |   41 ++++-------------------------------------
 1 file changed, 4 insertions(+), 37 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index a331ebc..12ff13a 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -22,10 +22,10 @@
 
 /*
  * NTP timekeeping variables:
+ *
+ * Note: All of the NTP state is protected by the timekeeping locks.
  */
 
-DEFINE_RAW_SPINLOCK(ntp_lock);
-
 
 /* USER_HZ period (usecs): */
 unsigned long			tick_usec = TICK_USEC;
@@ -132,8 +132,6 @@ static inline void pps_reset_freq_interval(void)
 
 /**
  * pps_clear - Clears the PPS state variables
- *
- * Must be called while holding a write on the ntp_lock
  */
 static inline void pps_clear(void)
 {
@@ -148,8 +146,6 @@ static inline void pps_clear(void)
 /* Decrease pps_valid to indicate that another second has passed since
  * the last PPS signal. When it reaches 0, indicate that PPS signal is
  * missing.
- *
- * Must be called while holding a write on the ntp_lock
  */
 static inline void pps_dec_valid(void)
 {
@@ -344,10 +340,6 @@ static void ntp_update_offset(long offset)
  */
 void ntp_clear(void)
 {
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&ntp_lock, flags);
-
 	time_adjust	= 0;		/* stop active adjtime() */
 	time_status	|= STA_UNSYNC;
 	time_maxerror	= NTP_PHASE_LIMIT;
@@ -360,20 +352,12 @@ void ntp_clear(void)
 
 	/* Clear PPS state variables */
 	pps_clear();
-	raw_spin_unlock_irqrestore(&ntp_lock, flags);
-
 }
 
 
 u64 ntp_tick_length(void)
 {
-	unsigned long flags;
-	s64 ret;
-
-	raw_spin_lock_irqsave(&ntp_lock, flags);
-	ret = tick_length;
-	raw_spin_unlock_irqrestore(&ntp_lock, flags);
-	return ret;
+	return tick_length;
 }
 
 
@@ -391,9 +375,6 @@ int second_overflow(unsigned long secs)
 {
 	s64 delta;
 	int leap = 0;
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&ntp_lock, flags);
 
 	/*
 	 * Leap second processing. If in leap-insert state at the end of the
@@ -475,8 +456,6 @@ int second_overflow(unsigned long secs)
 	time_adjust = 0;
 
 out:
-	raw_spin_unlock_irqrestore(&ntp_lock, flags);
-
 	return leap;
 }
 
@@ -571,10 +550,7 @@ static inline void process_adj_status(struct timex *txc, struct timespec *ts)
 	time_status |= txc->status & ~STA_RONLY;
 }
 
-/*
- * Called with ntp_lock held, so we can access and modify
- * all the global NTP state:
- */
+
 static inline void process_adjtimex_modes(struct timex *txc,
 						struct timespec *ts,
 						s32 *time_tai)
@@ -666,8 +642,6 @@ int __do_adjtimex(struct timex *txc, struct timespec *ts, s32 *time_tai)
 {
 	int result;
 
-	raw_spin_lock_irq(&ntp_lock);
-
 	if (txc->modes & ADJ_ADJTIME) {
 		long save_adjust = time_adjust;
 
@@ -708,8 +682,6 @@ int __do_adjtimex(struct timex *txc, struct timespec *ts, s32 *time_tai)
 	/* fill PPS status fields */
 	pps_fill_timex(txc);
 
-	raw_spin_unlock_irq(&ntp_lock);
-
 	txc->time.tv_sec = ts->tv_sec;
 	txc->time.tv_usec = ts->tv_nsec;
 	if (!(time_status & STA_NANO))
@@ -906,8 +878,6 @@ void __hardpps(const struct timespec *phase_ts, const struct timespec *raw_ts)
 
 	pts_norm = pps_normalize_ts(*phase_ts);
 
-	raw_spin_lock_irqsave(&ntp_lock, flags);
-
 	/* clear the error bits, they will be set again if needed */
 	time_status &= ~(STA_PPSJITTER | STA_PPSWANDER | STA_PPSERROR);
 
@@ -919,7 +889,6 @@ void __hardpps(const struct timespec *phase_ts, const struct timespec *raw_ts)
 	 * just start the frequency interval */
 	if (unlikely(pps_fbase.tv_sec == 0)) {
 		pps_fbase = *raw_ts;
-		raw_spin_unlock_irqrestore(&ntp_lock, flags);
 		return;
 	}
 
@@ -934,7 +903,6 @@ void __hardpps(const struct timespec *phase_ts, const struct timespec *raw_ts)
 		time_status |= STA_PPSJITTER;
 		/* restart the frequency calibration interval */
 		pps_fbase = *raw_ts;
-		raw_spin_unlock_irqrestore(&ntp_lock, flags);
 		pr_err("hardpps: PPSJITTER: bad pulse\n");
 		return;
 	}
@@ -951,7 +919,6 @@ void __hardpps(const struct timespec *phase_ts, const struct timespec *raw_ts)
 
 	hardpps_update_phase(pts_norm.nsec);
 
-	raw_spin_unlock_irqrestore(&ntp_lock, flags);
 }
 #endif	/* CONFIG_NTP_PPS */
 
-- 
1.7.10.4

Re: [PATCH 0/8] Move ntp state to be protected by timekeeping lock

[Richard Cochran]

On Mon, Mar 25, 2013 at 01:08:10PM -0700, John Stultz wrote:

> This patchset makes the lock ownership lines less obvious, but I've
> been sure to keep the ntp state static to ntp.c and instead provided
> some accessors via ntp-internal.h that timekeping code can use to
> make changes.  The only really ugly part is that do_adjtimex() has
> to split some of the logic between timekeeping.c and ntp.c in order
> to really get the locking done correctly.

I didn't find this too ugly or troublesome. The reshuffling you have
here looks straightforward to me.

Thanks,
Richard

Re: [PATCH 0/8] Move ntp state to be protected by timekeeping lock

[Prarit Bhargava]

On 03/25/2013 04:08 PM, John Stultz wrote:

> 
> The problem of course, is that the new asynchronous behavior the
> shadow updates breaks some of the assumptions on how the NTP state
> is used. Thus to correct this, we really need to serialize the ntp
> state updates along with the timekeeping state. With the added side
> benefit that of reducing lock acquisitions.
> 
> The downside is that the timekeeping state has been cleaned up to
> live nicely in the timekeeper struct, which nicely bounded what the
> timekeeping lock protected. Where as 99% of the NTP state was all
> static to ntp.c, and was protected by the ntp.c static ntp_lock, so
> it was all nicely encapsulated as well. 

> 
> This patchset makes the lock ownership lines less obvious, but I've
> been sure to keep the ntp state static to ntp.c and instead provided
> some accessors via ntp-internal.h that timekeping code can use to
> make changes.  The only really ugly part is that do_adjtimex() has
> to split some of the logic between timekeeping.c and ntp.c in order
> to really get the locking done correctly.


John, I have no technical objection to the patch ... but after reviewing the
changes I think you've significantly changed the way the locking works in the
NTP code, and IMO, some note should be made in the code about the timekeeper
lock and its impact to ntp.  It's not trivial reading this code and I think the
dropping of the ntp lock will confuse the casual viewer.

IMO of course.

> 
> I may try to rework the code in the future so that the timekeeper
> holds the ntp state and passes it to the ntp.c functions to be
> modified, but that is a much deeper rework then I'd like to do right
> now, and causes fruther complexity to the shadow-state updates, since
> we'd end up unnecessarily copying the ntp state back and forth every
> time.
> 
> This applies on top of my fortglx/3.10/time queue here:
> git://git.linaro.org/people/jstultz/linux.git fortglx/3.10/time
> 
> If you want to see this entire set (along with Thomas' shadow-update
> work) it can be found here:
> git://git.linaro.org/people/jstultz/linux.git dev/tglx-shadowtime
> or 
> http://git.linaro.org/gitweb?p=people/jstultz/linux.git;a=shortlog;h=refs/heads/dev/tglx-shadowtime
> 

Beyond the above comment, a quick test shows that ntp does work AFAICT (at least
on F18 + your git tree.  I'll try and do a heavier test next week.

So for now ...

Acked-by: Prarit Bhargava <prarit@redhat.com>

P.