[GIT PULL][PATCH 0/7] Reduce timekeeping lock hold time

[GIT PULL][PATCH 0/7] Reduce timekeeping lock hold time

[John Stultz]

Here is the second half of the timekeeping cleanups (the first
half are already in -tip) as well as some changes to reduce the
timekeeping lock hold times.

This work grew out of some of Eric Dumazet's and Thomas Gleixner's
suggestions, after they noticed the xtime_lock hold time could be
on the long side.

The basic idea is that we keep a shadow copy of the timekeeper
stucture, which can be updated while readers are accessing the
time. Then we only have to block readers as we switch to the
newly updated structure.

I've done some limited testing, but it would be good to get
this in -tip early, so we can be sure it gets enough testing
before 3.4 is released.

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Richard Cochran <richardcochran@gmail.com>

thanks
-john

Thomas,

These patches are available in the git repository at:
  git://git.linaro.org/people/jstultz/linux.git fortglx/3.4/time

John Stultz (7):
  time: Condense timekeeper.xtime into xtime_sec
  time: Rework timekeeping functions to take timekeeper ptr as argument
  time: Split timekeeper lock into separate reader/writer locks
  time: Update timekeeper structure using a local shadow
  time: Shadow cycle_last in timekeeper structure
  time: Reduce timekeeper read lock hold time
  time: Convert the timekeeper's wlock to a raw_spin_lock

 kernel/time/timekeeping.c |  414 ++++++++++++++++++++++++++-------------------
 1 files changed, 243 insertions(+), 171 deletions(-)

-- 
1.7.3.2.146.gca209

[PATCH 1/7] time: Condense timekeeper.xtime into xtime_sec

[John Stultz]

The timekeeper struct has a xtime_nsec, which keeps the
sub-nanosecond remainder.  This ends up being somewhat
duplicative of the timekeeper.xtime.tv_nsec value, and we
have to do extra work to keep them apart, copying the full
nsec portion out and back in over and over.

This patch simplifies some of the logic by taking the timekeeper
xtime value and splitting it into timekeeper.xtime_sec and
reuses the timekeeper.xtime_nsec for the sub-second portion
(stored in higher res shifted nanoseconds).

This simplifies some of the accumulation logic. And will
allow for more accurate timekeeping once the vsyscall code
is updated to use the shifted nanosecond remainder.

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/timekeeping.c |  162 ++++++++++++++++++++++++++++-----------------
 1 files changed, 100 insertions(+), 62 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 403c2a0..325086a 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -39,8 +39,11 @@ struct timekeeper {
 	/* Raw nano seconds accumulated per NTP interval. */
 	u32	raw_interval;
 
-	/* Clock shifted nano seconds remainder not stored in xtime.tv_nsec. */
+	/* Current CLOCK_REALTIME time in seconds */
+	u64	xtime_sec;
+	/* Clock shifted nano seconds */
 	u64	xtime_nsec;
+
 	/* Difference between accumulated time and NTP time in ntp
 	 * shifted nano seconds. */
 	s64	ntp_error;
@@ -48,8 +51,6 @@ struct timekeeper {
 	 * ntp shifted nano seconds. */
 	int	ntp_error_shift;
 
-	/* The current time */
-	struct timespec xtime;
 	/*
 	 * wall_to_monotonic is what we need to add to xtime (or xtime corrected
 	 * for sub jiffie times) to get to monotonic time.  Monotonic is pegged
@@ -87,6 +88,38 @@ __cacheline_aligned_in_smp DEFINE_SEQLOCK(xtime_lock);
 int __read_mostly timekeeping_suspended;
 
 
+static inline void timekeeper_normalize_xtime(struct timekeeper *tk)
+{
+	while (tk->xtime_nsec >= (NSEC_PER_SEC << tk->shift)) {
+		tk->xtime_nsec -= NSEC_PER_SEC << tk->shift;
+		tk->xtime_sec++;
+	}
+}
+
+static struct timespec timekeeper_xtime(struct timekeeper *tk)
+{
+	struct timespec ts;
+
+	ts.tv_sec = tk->xtime_sec;
+	ts.tv_nsec = (long)(tk->xtime_nsec >> tk->shift);
+	return ts;
+}
+
+static void timekeeper_set_xtime(struct timekeeper *tk,
+					const struct timespec *ts)
+{
+	tk->xtime_sec = ts->tv_sec;
+	tk->xtime_nsec = ts->tv_nsec << tk->shift;
+}
+
+
+static void timekeeper_xtime_add(struct timekeeper *tk,
+					const struct timespec *ts)
+{
+	tk->xtime_sec += ts->tv_sec;
+	tk->xtime_nsec += ts->tv_nsec << tk->shift;
+}
+
 
 /**
  * timekeeper_setup_internals - Set up internals to use clocksource clock.
@@ -143,6 +176,7 @@ static inline s64 timekeeping_get_ns(void)
 {
 	cycle_t cycle_now, cycle_delta;
 	struct clocksource *clock;
+	s64 nsec;
 
 	/* read clocksource: */
 	clock = timekeeper.clock;
@@ -151,9 +185,8 @@ static inline s64 timekeeping_get_ns(void)
 	/* calculate the delta since the last update_wall_time: */
 	cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
 
-	/* return delta convert to nanoseconds using ntp adjusted mult. */
-	return clocksource_cyc2ns(cycle_delta, timekeeper.mult,
-				  timekeeper.shift);
+	nsec = cycle_delta * timekeeper.mult + timekeeper.xtime_nsec;
+	return nsec >> timekeeper.shift;
 }
 
 static inline s64 timekeeping_get_ns_raw(void)
@@ -175,11 +208,13 @@ static inline s64 timekeeping_get_ns_raw(void)
 /* must hold write on timekeeper.lock */
 static void timekeeping_update(bool clearntp)
 {
+	struct timespec xt;
 	if (clearntp) {
 		timekeeper.ntp_error = 0;
 		ntp_clear();
 	}
-	update_vsyscall(&timekeeper.xtime, &timekeeper.wall_to_monotonic,
+	xt = timekeeper_xtime(&timekeeper);
+	update_vsyscall(&xt, &timekeeper.wall_to_monotonic,
 			 timekeeper.clock, timekeeper.mult);
 }
 
@@ -189,7 +224,7 @@ void timekeeping_leap_insert(int leapsecond)
 	unsigned long flags;
 
 	write_seqlock_irqsave(&timekeeper.lock, flags);
-	timekeeper.xtime.tv_sec += leapsecond;
+	timekeeper.xtime_sec += leapsecond;
 	timekeeper.wall_to_monotonic.tv_sec -= leapsecond;
 	timekeeping_update(false);
 	write_sequnlock_irqrestore(&timekeeper.lock, flags);
@@ -214,13 +249,12 @@ static void timekeeping_forward_now(void)
 	cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
 	clock->cycle_last = cycle_now;
 
-	nsec = clocksource_cyc2ns(cycle_delta, timekeeper.mult,
-				  timekeeper.shift);
+	timekeeper.xtime_nsec += cycle_delta * timekeeper.mult;
 
 	/* If arch requires, add in gettimeoffset() */
-	nsec += arch_gettimeoffset();
+	timekeeper.xtime_nsec += arch_gettimeoffset() << timekeeper.shift;
 
-	timespec_add_ns(&timekeeper.xtime, nsec);
+	timekeeper_normalize_xtime(&timekeeper);
 
 	nsec = clocksource_cyc2ns(cycle_delta, clock->mult, clock->shift);
 	timespec_add_ns(&timekeeper.raw_time, nsec);
@@ -235,15 +269,15 @@ static void timekeeping_forward_now(void)
 void getnstimeofday(struct timespec *ts)
 {
 	unsigned long seq;
-	s64 nsecs;
+	s64 nsecs = 0;
 
 	WARN_ON(timekeeping_suspended);
 
 	do {
 		seq = read_seqbegin(&timekeeper.lock);
 
-		*ts = timekeeper.xtime;
-		nsecs = timekeeping_get_ns();
+		ts->tv_sec = timekeeper.xtime_sec;
+		ts->tv_nsec = timekeeping_get_ns();
 
 		/* If arch requires, add in gettimeoffset() */
 		nsecs += arch_gettimeoffset();
@@ -264,11 +298,10 @@ ktime_t ktime_get(void)
 
 	do {
 		seq = read_seqbegin(&timekeeper.lock);
-		secs = timekeeper.xtime.tv_sec +
+		secs = timekeeper.xtime_sec +
 				timekeeper.wall_to_monotonic.tv_sec;
-		nsecs = timekeeper.xtime.tv_nsec +
+		nsecs = timekeeping_get_ns() +
 				timekeeper.wall_to_monotonic.tv_nsec;
-		nsecs += timekeeping_get_ns();
 		/* If arch requires, add in gettimeoffset() */
 		nsecs += arch_gettimeoffset();
 
@@ -293,22 +326,21 @@ void ktime_get_ts(struct timespec *ts)
 {
 	struct timespec tomono;
 	unsigned int seq;
-	s64 nsecs;
 
 	WARN_ON(timekeeping_suspended);
 
 	do {
 		seq = read_seqbegin(&timekeeper.lock);
-		*ts = timekeeper.xtime;
+		ts->tv_sec = timekeeper.xtime_sec;
+		ts->tv_nsec = timekeeping_get_ns();
 		tomono = timekeeper.wall_to_monotonic;
-		nsecs = timekeeping_get_ns();
 		/* If arch requires, add in gettimeoffset() */
-		nsecs += arch_gettimeoffset();
+		ts->tv_nsec += arch_gettimeoffset();
 
 	} while (read_seqretry(&timekeeper.lock, seq));
 
 	set_normalized_timespec(ts, ts->tv_sec + tomono.tv_sec,
-				ts->tv_nsec + tomono.tv_nsec + nsecs);
+				ts->tv_nsec + tomono.tv_nsec);
 }
 EXPORT_SYMBOL_GPL(ktime_get_ts);
 
@@ -336,7 +368,8 @@ void getnstime_raw_and_real(struct timespec *ts_raw, struct timespec *ts_real)
 		seq = read_seqbegin(&timekeeper.lock);
 
 		*ts_raw = timekeeper.raw_time;
-		*ts_real = timekeeper.xtime;
+		ts_real->tv_sec = timekeeper.xtime_sec;
+		ts_real->tv_nsec = 0;
 
 		nsecs_raw = timekeeping_get_ns_raw();
 		nsecs_real = timekeeping_get_ns();
@@ -379,7 +412,7 @@ EXPORT_SYMBOL(do_gettimeofday);
  */
 int do_settimeofday(const struct timespec *tv)
 {
-	struct timespec ts_delta;
+	struct timespec ts_delta, xt;
 	unsigned long flags;
 
 	if ((unsigned long)tv->tv_nsec >= NSEC_PER_SEC)
@@ -389,12 +422,15 @@ int do_settimeofday(const struct timespec *tv)
 
 	timekeeping_forward_now();
 
-	ts_delta.tv_sec = tv->tv_sec - timekeeper.xtime.tv_sec;
-	ts_delta.tv_nsec = tv->tv_nsec - timekeeper.xtime.tv_nsec;
+	xt = timekeeper_xtime(&timekeeper);
+	ts_delta.tv_sec = tv->tv_sec - xt.tv_sec;
+	ts_delta.tv_nsec = tv->tv_nsec - xt.tv_nsec;
+
 	timekeeper.wall_to_monotonic =
 			timespec_sub(timekeeper.wall_to_monotonic, ts_delta);
 
-	timekeeper.xtime = *tv;
+	timekeeper_set_xtime(&timekeeper, tv);
+
 	timekeeping_update(true);
 
 	write_sequnlock_irqrestore(&timekeeper.lock, flags);
@@ -425,7 +461,8 @@ int timekeeping_inject_offset(struct timespec *ts)
 
 	timekeeping_forward_now();
 
-	timekeeper.xtime = timespec_add(timekeeper.xtime, *ts);
+
+	timekeeper_xtime_add(&timekeeper, ts);
 	timekeeper.wall_to_monotonic =
 				timespec_sub(timekeeper.wall_to_monotonic, *ts);
 
@@ -601,14 +638,12 @@ void __init timekeeping_init(void)
 		clock->enable(clock);
 	timekeeper_setup_internals(clock);
 
-	timekeeper.xtime.tv_sec = now.tv_sec;
-	timekeeper.xtime.tv_nsec = now.tv_nsec;
+	timekeeper_set_xtime(&timekeeper, &now);
 	timekeeper.raw_time.tv_sec = 0;
 	timekeeper.raw_time.tv_nsec = 0;
-	if (boot.tv_sec == 0 && boot.tv_nsec == 0) {
-		boot.tv_sec = timekeeper.xtime.tv_sec;
-		boot.tv_nsec = timekeeper.xtime.tv_nsec;
-	}
+	if (boot.tv_sec == 0 && boot.tv_nsec == 0)
+		boot = timekeeper_xtime(&timekeeper);
+
 	set_normalized_timespec(&timekeeper.wall_to_monotonic,
 				-boot.tv_sec, -boot.tv_nsec);
 	timekeeper.total_sleep_time.tv_sec = 0;
@@ -634,7 +669,7 @@ static void __timekeeping_inject_sleeptime(struct timespec *delta)
 		return;
 	}
 
-	timekeeper.xtime = timespec_add(timekeeper.xtime, *delta);
+	timekeeper_xtime_add(&timekeeper, delta);
 	timekeeper.wall_to_monotonic =
 			timespec_sub(timekeeper.wall_to_monotonic, *delta);
 	timekeeper.total_sleep_time = timespec_add(
@@ -731,7 +766,8 @@ static int timekeeping_suspend(void)
 	 * try to compensate so the difference in system time
 	 * and persistent_clock time stays close to constant.
 	 */
-	delta = timespec_sub(timekeeper.xtime, timekeeping_suspend_time);
+	delta = timespec_sub(timekeeper_xtime(&timekeeper),
+				timekeeping_suspend_time);
 	delta_delta = timespec_sub(delta, old_delta);
 	if (abs(delta_delta.tv_sec)  >= 2) {
 		/*
@@ -963,7 +999,7 @@ static cycle_t logarithmic_accumulation(cycle_t offset, int shift)
 	timekeeper.xtime_nsec += timekeeper.xtime_interval << shift;
 	while (timekeeper.xtime_nsec >= nsecps) {
 		timekeeper.xtime_nsec -= nsecps;
-		timekeeper.xtime.tv_sec++;
+		timekeeper.xtime_sec++;
 		second_overflow();
 	}
 
@@ -997,6 +1033,7 @@ static void update_wall_time(void)
 	cycle_t offset;
 	int shift = 0, maxshift;
 	unsigned long flags;
+	s64 remainder;
 
 	write_seqlock_irqsave(&timekeeper.lock, flags);
 
@@ -1011,8 +1048,6 @@ static void update_wall_time(void)
 #else
 	offset = (clock->read(clock) - clock->cycle_last) & clock->mask;
 #endif
-	timekeeper.xtime_nsec = (s64)timekeeper.xtime.tv_nsec <<
-						timekeeper.shift;
 
 	/*
 	 * With NO_HZ we may have to accumulate many cycle_intervals
@@ -1058,25 +1093,29 @@ static void update_wall_time(void)
 		timekeeper.ntp_error += neg << timekeeper.ntp_error_shift;
 	}
 
-
 	/*
-	 * Store full nanoseconds into xtime after rounding it up and
-	 * add the remainder to the error difference.
-	 */
-	timekeeper.xtime.tv_nsec = ((s64)timekeeper.xtime_nsec >>
-						timekeeper.shift) + 1;
-	timekeeper.xtime_nsec -= (s64)timekeeper.xtime.tv_nsec <<
-						timekeeper.shift;
-	timekeeper.ntp_error +=	timekeeper.xtime_nsec <<
-				timekeeper.ntp_error_shift;
+	* Store only full nanoseconds into xtime_nsec after rounding
+	* it up and add the remainder to the error difference.
+	* XXX - This is necessary to avoid small 1ns inconsistnecies caused
+	* by truncating the remainder in vsyscalls. However, it causes
+	* additional work to be done in timekeeping_adjust(). Once
+	* the vsyscall implementations are converted to use xtime_nsec
+	* (shifted nanoseconds), this can be killed.
+	*/
+	remainder = timekeeper.xtime_nsec & ((1<<timekeeper.shift)-1);
+	timekeeper.xtime_nsec -= remainder;
+	timekeeper.xtime_nsec += 1<<timekeeper.shift;
+	timekeeper.ntp_error += remainder <<
+					timekeeper.ntp_error_shift;
 
 	/*
 	 * Finally, make sure that after the rounding
 	 * xtime.tv_nsec isn't larger then NSEC_PER_SEC
 	 */
-	if (unlikely(timekeeper.xtime.tv_nsec >= NSEC_PER_SEC)) {
-		timekeeper.xtime.tv_nsec -= NSEC_PER_SEC;
-		timekeeper.xtime.tv_sec++;
+	if (unlikely(timekeeper.xtime_nsec >=
+			(NSEC_PER_SEC << timekeeper.shift))) {
+		timekeeper.xtime_nsec -= NSEC_PER_SEC << timekeeper.shift;
+		timekeeper.xtime_sec++;
 		second_overflow();
 	}
 
@@ -1125,21 +1164,20 @@ void get_monotonic_boottime(struct timespec *ts)
 {
 	struct timespec tomono, sleep;
 	unsigned int seq;
-	s64 nsecs;
 
 	WARN_ON(timekeeping_suspended);
 
 	do {
 		seq = read_seqbegin(&timekeeper.lock);
-		*ts = timekeeper.xtime;
+		ts->tv_sec = timekeeper.xtime_sec;
+		ts->tv_nsec = timekeeping_get_ns();
 		tomono = timekeeper.wall_to_monotonic;
 		sleep = timekeeper.total_sleep_time;
-		nsecs = timekeeping_get_ns();
 
 	} while (read_seqretry(&timekeeper.lock, seq));
 
 	set_normalized_timespec(ts, ts->tv_sec + tomono.tv_sec + sleep.tv_sec,
-			ts->tv_nsec + tomono.tv_nsec + sleep.tv_nsec + nsecs);
+			ts->tv_nsec + tomono.tv_nsec + sleep.tv_nsec);
 }
 EXPORT_SYMBOL_GPL(get_monotonic_boottime);
 
@@ -1172,13 +1210,13 @@ EXPORT_SYMBOL_GPL(monotonic_to_bootbased);
 
 unsigned long get_seconds(void)
 {
-	return timekeeper.xtime.tv_sec;
+	return timekeeper.xtime_sec;
 }
 EXPORT_SYMBOL(get_seconds);
 
 struct timespec __current_kernel_time(void)
 {
-	return timekeeper.xtime;
+	return timekeeper_xtime(&timekeeper);
 }
 
 struct timespec current_kernel_time(void)
@@ -1189,7 +1227,7 @@ struct timespec current_kernel_time(void)
 	do {
 		seq = read_seqbegin(&timekeeper.lock);
 
-		now = timekeeper.xtime;
+		now = timekeeper_xtime(&timekeeper);
 	} while (read_seqretry(&timekeeper.lock, seq));
 
 	return now;
@@ -1204,7 +1242,7 @@ struct timespec get_monotonic_coarse(void)
 	do {
 		seq = read_seqbegin(&timekeeper.lock);
 
-		now = timekeeper.xtime;
+		now = timekeeper_xtime(&timekeeper);
 		mono = timekeeper.wall_to_monotonic;
 	} while (read_seqretry(&timekeeper.lock, seq));
 
@@ -1239,7 +1277,7 @@ void get_xtime_and_monotonic_and_sleep_offset(struct timespec *xtim,
 
 	do {
 		seq = read_seqbegin(&timekeeper.lock);
-		*xtim = timekeeper.xtime;
+		*xtim = timekeeper_xtime(&timekeeper);
 		*wtom = timekeeper.wall_to_monotonic;
 		*sleep = timekeeper.total_sleep_time;
 	} while (read_seqretry(&timekeeper.lock, seq));
-- 
1.7.3.2.146.gca209

[PATCH 2/7] time: Rework timekeeping functions to take timekeeper ptr as argument

[John Stultz]

As part of cleaning up the timekeeping code, this patch converts
a number of internal functions to takea  timekeeper ptr as an
argument, so that the internal functions don't access the global
timekeeper structure directly. This allows for further optimizations
later.

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/timekeeping.c |  100 ++++++++++++++++++++++----------------------
 1 files changed, 50 insertions(+), 50 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 325086a..9e6b28e 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -206,16 +206,15 @@ static inline s64 timekeeping_get_ns_raw(void)
 }
 
 /* must hold write on timekeeper.lock */
-static void timekeeping_update(bool clearntp)
+static void timekeeping_update(struct timekeeper *tk, bool clearntp)
 {
 	struct timespec xt;
 	if (clearntp) {
-		timekeeper.ntp_error = 0;
+		tk->ntp_error = 0;
 		ntp_clear();
 	}
-	xt = timekeeper_xtime(&timekeeper);
-	update_vsyscall(&xt, &timekeeper.wall_to_monotonic,
-			 timekeeper.clock, timekeeper.mult);
+	xt = timekeeper_xtime(tk);
+	update_vsyscall(&xt, &tk->wall_to_monotonic, tk->clock, tk->mult);
 }
 
 
@@ -226,7 +225,7 @@ void timekeeping_leap_insert(int leapsecond)
 	write_seqlock_irqsave(&timekeeper.lock, flags);
 	timekeeper.xtime_sec += leapsecond;
 	timekeeper.wall_to_monotonic.tv_sec -= leapsecond;
-	timekeeping_update(false);
+	timekeeping_update(&timekeeper, false);
 	write_sequnlock_irqrestore(&timekeeper.lock, flags);
 
 }
@@ -431,7 +430,7 @@ int do_settimeofday(const struct timespec *tv)
 
 	timekeeper_set_xtime(&timekeeper, tv);
 
-	timekeeping_update(true);
+	timekeeping_update(&timekeeper, true);
 
 	write_sequnlock_irqrestore(&timekeeper.lock, flags);
 
@@ -466,7 +465,7 @@ int timekeeping_inject_offset(struct timespec *ts)
 	timekeeper.wall_to_monotonic =
 				timespec_sub(timekeeper.wall_to_monotonic, *ts);
 
-	timekeeping_update(true);
+	timekeeping_update(&timekeeper, true);
 
 	write_sequnlock_irqrestore(&timekeeper.lock, flags);
 
@@ -703,7 +702,7 @@ void timekeeping_inject_sleeptime(struct timespec *delta)
 
 	__timekeeping_inject_sleeptime(delta);
 
-	timekeeping_update(true);
+	timekeeping_update(&timekeeper, true);
 
 	write_sequnlock_irqrestore(&timekeeper.lock, flags);
 
@@ -806,7 +805,8 @@ device_initcall(timekeeping_init_ops);
  * If the error is already larger, we look ahead even further
  * to compensate for late or lost adjustments.
  */
-static __always_inline int timekeeping_bigadjust(s64 error, s64 *interval,
+static __always_inline int timekeeping_bigadjust(struct timekeeper *tk,
+						 s64 error, s64 *interval,
 						 s64 *offset)
 {
 	s64 tick_error, i;
@@ -822,7 +822,7 @@ static __always_inline int timekeeping_bigadjust(s64 error, s64 *interval,
 	 * here.  This is tuned so that an error of about 1 msec is adjusted
 	 * within about 1 sec (or 2^20 nsec in 2^SHIFT_HZ ticks).
 	 */
-	error2 = timekeeper.ntp_error >> (NTP_SCALE_SHIFT + 22 - 2 * SHIFT_HZ);
+	error2 = tk->ntp_error >> (NTP_SCALE_SHIFT + 22 - 2 * SHIFT_HZ);
 	error2 = abs(error2);
 	for (look_ahead = 0; error2 > 0; look_ahead++)
 		error2 >>= 2;
@@ -831,8 +831,8 @@ static __always_inline int timekeeping_bigadjust(s64 error, s64 *interval,
 	 * Now calculate the error in (1 << look_ahead) ticks, but first
 	 * remove the single look ahead already included in the error.
 	 */
-	tick_error = ntp_tick_length() >> (timekeeper.ntp_error_shift + 1);
-	tick_error -= timekeeper.xtime_interval >> 1;
+	tick_error = ntp_tick_length() >> (tk->ntp_error_shift + 1);
+	tick_error -= tk->xtime_interval >> 1;
 	error = ((error - tick_error) >> look_ahead) + tick_error;
 
 	/* Finally calculate the adjustment shift value.  */
@@ -857,9 +857,9 @@ static __always_inline int timekeeping_bigadjust(s64 error, s64 *interval,
  * this is optimized for the most common adjustments of -1,0,1,
  * for other values we can do a bit more work.
  */
-static void timekeeping_adjust(s64 offset)
+static void timekeeping_adjust(struct timekeeper *tk, s64 offset)
 {
-	s64 error, interval = timekeeper.cycle_interval;
+	s64 error, interval = tk->cycle_interval;
 	int adj;
 
 	/*
@@ -875,7 +875,7 @@ static void timekeeping_adjust(s64 offset)
 	 *
 	 * Note: It does not "save" on aggravation when reading the code.
 	 */
-	error = timekeeper.ntp_error >> (timekeeper.ntp_error_shift - 1);
+	error = tk->ntp_error >> (tk->ntp_error_shift - 1);
 	if (error > interval) {
 		/*
 		 * We now divide error by 4(via shift), which checks if
@@ -897,7 +897,8 @@ static void timekeeping_adjust(s64 offset)
 		if (likely(error <= interval))
 			adj = 1;
 		else
-			adj = timekeeping_bigadjust(error, &interval, &offset);
+			adj = timekeeping_bigadjust(tk, error, &interval,
+							&offset);
 	} else if (error < -interval) {
 		/* See comment above, this is just switched for the negative */
 		error >>= 2;
@@ -906,17 +907,17 @@ static void timekeeping_adjust(s64 offset)
 			interval = -interval;
 			offset = -offset;
 		} else
-			adj = timekeeping_bigadjust(error, &interval, &offset);
-	} else /* No adjustment needed */
+			adj = timekeeping_bigadjust(tk, error, &interval,
+							&offset);
+	} else
 		return;
 
-	WARN_ONCE(timekeeper.clock->maxadj &&
-			(timekeeper.mult + adj > timekeeper.clock->mult +
-						timekeeper.clock->maxadj),
+	WARN_ONCE(tk->clock->maxadj &&
+			(tk->mult + adj > tk->clock->mult + tk->clock->maxadj),
 			"Adjusting %s more then 11%% (%ld vs %ld)\n",
-			timekeeper.clock->name, (long)timekeeper.mult + adj,
-			(long)timekeeper.clock->mult +
-				timekeeper.clock->maxadj);
+			tk->clock->name, (long)tk->mult + adj,
+			(long)tk->clock->mult + tk->clock->maxadj);
+
 	/*
 	 * So the following can be confusing.
 	 *
@@ -966,11 +967,10 @@ static void timekeeping_adjust(s64 offset)
 	 *
 	 * XXX - TODO: Doc ntp_error calculation.
 	 */
-	timekeeper.mult += adj;
-	timekeeper.xtime_interval += interval;
-	timekeeper.xtime_nsec -= offset;
-	timekeeper.ntp_error -= (interval - offset) <<
-				timekeeper.ntp_error_shift;
+	tk->mult += adj;
+	tk->xtime_interval += interval;
+	tk->xtime_nsec -= offset;
+	tk->ntp_error -= (interval - offset) << tk->ntp_error_shift;
 }
 
 
@@ -983,41 +983,41 @@ static void timekeeping_adjust(s64 offset)
  *
  * Returns the unconsumed cycles.
  */
-static cycle_t logarithmic_accumulation(cycle_t offset, int shift)
+static cycle_t logarithmic_accumulation(struct timekeeper *tk, cycle_t offset,
+						int shift)
 {
-	u64 nsecps = (u64)NSEC_PER_SEC << timekeeper.shift;
+	u64 nsecps = (u64)NSEC_PER_SEC << tk->shift;
 	u64 raw_nsecs;
 
 	/* If the offset is smaller then a shifted interval, do nothing */
-	if (offset < timekeeper.cycle_interval<<shift)
+	if (offset < tk->cycle_interval<<shift)
 		return offset;
 
 	/* Accumulate one shifted interval */
-	offset -= timekeeper.cycle_interval << shift;
-	timekeeper.clock->cycle_last += timekeeper.cycle_interval << shift;
+	offset -= tk->cycle_interval << shift;
+	tk->clock->cycle_last += tk->cycle_interval << shift;
 
-	timekeeper.xtime_nsec += timekeeper.xtime_interval << shift;
-	while (timekeeper.xtime_nsec >= nsecps) {
-		timekeeper.xtime_nsec -= nsecps;
-		timekeeper.xtime_sec++;
+	tk->xtime_nsec += tk->xtime_interval << shift;
+	while (tk->xtime_nsec >= nsecps) {
+		tk->xtime_nsec -= nsecps;
+		tk->xtime_sec++;
 		second_overflow();
 	}
 
 	/* Accumulate raw time */
-	raw_nsecs = timekeeper.raw_interval << shift;
-	raw_nsecs += timekeeper.raw_time.tv_nsec;
+	raw_nsecs = tk->raw_interval << shift;
+	raw_nsecs += tk->raw_time.tv_nsec;
 	if (raw_nsecs >= NSEC_PER_SEC) {
 		u64 raw_secs = raw_nsecs;
 		raw_nsecs = do_div(raw_secs, NSEC_PER_SEC);
-		timekeeper.raw_time.tv_sec += raw_secs;
+		tk->raw_time.tv_sec += raw_secs;
 	}
-	timekeeper.raw_time.tv_nsec = raw_nsecs;
+	tk->raw_time.tv_nsec = raw_nsecs;
 
 	/* Accumulate error between NTP and clock interval */
-	timekeeper.ntp_error += ntp_tick_length() << shift;
-	timekeeper.ntp_error -=
-	    (timekeeper.xtime_interval + timekeeper.xtime_remainder) <<
-				(timekeeper.ntp_error_shift + shift);
+	tk->ntp_error += ntp_tick_length() << shift;
+	tk->ntp_error -= (tk->xtime_interval + tk->xtime_remainder) <<
+						(tk->ntp_error_shift + shift);
 
 	return offset;
 }
@@ -1063,13 +1063,13 @@ static void update_wall_time(void)
 	maxshift = (64 - (ilog2(ntp_tick_length())+1)) - 1;
 	shift = min(shift, maxshift);
 	while (offset >= timekeeper.cycle_interval) {
-		offset = logarithmic_accumulation(offset, shift);
+		offset = logarithmic_accumulation(&timekeeper, offset, shift);
 		if(offset < timekeeper.cycle_interval<<shift)
 			shift--;
 	}
 
 	/* correct the clock when NTP error is too big */
-	timekeeping_adjust(offset);
+	timekeeping_adjust(&timekeeper, offset);
 
 	/*
 	 * Since in the loop above, we accumulate any amount of time
@@ -1119,7 +1119,7 @@ static void update_wall_time(void)
 		second_overflow();
 	}
 
-	timekeeping_update(false);
+	timekeeping_update(&timekeeper, false);
 
 out:
 	write_sequnlock_irqrestore(&timekeeper.lock, flags);
-- 
1.7.3.2.146.gca209

[PATCH 3/7] time: Split timekeeper lock into separate reader/writer locks

[John Stultz]

In order to reduce the lock hold time, split the timekeeper lock
into a writer lock, which serializes updates to the timekeeper
structure, and a reader sequence counter, which ensures readers
see a consistent version of the timekeeper.

This will allow us to reduce the lock wait time for readers, by
doing updates on a shadow copy of the timekeeper.

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/timekeeping.c |  113 +++++++++++++++++++++++++++------------------
 1 files changed, 68 insertions(+), 45 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 9e6b28e..4962284 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -71,8 +71,9 @@ struct timekeeper {
 	/* The raw monotonic time for the CLOCK_MONOTONIC_RAW posix clock. */
 	struct timespec raw_time;
 
-	/* Seqlock for all timekeeper values */
-	seqlock_t lock;
+	/* locks for timekeeper structure */
+	seqcount_t rlock;   /* This seqcount serializes readers from updates */
+	spinlock_t wlock;	/* This spinlock serializes updaters */
 };
 
 static struct timekeeper timekeeper;
@@ -205,7 +206,7 @@ static inline s64 timekeeping_get_ns_raw(void)
 	return clocksource_cyc2ns(cycle_delta, clock->mult, clock->shift);
 }
 
-/* must hold write on timekeeper.lock */
+/* must hold write on timekeeper.wlock */
 static void timekeeping_update(struct timekeeper *tk, bool clearntp)
 {
 	struct timespec xt;
@@ -222,11 +223,13 @@ void timekeeping_leap_insert(int leapsecond)
 {
 	unsigned long flags;
 
-	write_seqlock_irqsave(&timekeeper.lock, flags);
+	spin_lock_irqsave(&timekeeper.wlock, flags);
+	write_seqcount_begin(&timekeeper.rlock);
 	timekeeper.xtime_sec += leapsecond;
 	timekeeper.wall_to_monotonic.tv_sec -= leapsecond;
 	timekeeping_update(&timekeeper, false);
-	write_sequnlock_irqrestore(&timekeeper.lock, flags);
+	write_seqcount_end(&timekeeper.rlock);
+	spin_unlock_irqrestore(&timekeeper.wlock, flags);
 
 }
 
@@ -273,7 +276,7 @@ void getnstimeofday(struct timespec *ts)
 	WARN_ON(timekeeping_suspended);
 
 	do {
-		seq = read_seqbegin(&timekeeper.lock);
+		seq = read_seqcount_begin(&timekeeper.rlock);
 
 		ts->tv_sec = timekeeper.xtime_sec;
 		ts->tv_nsec = timekeeping_get_ns();
@@ -281,7 +284,7 @@ void getnstimeofday(struct timespec *ts)
 		/* If arch requires, add in gettimeoffset() */
 		nsecs += arch_gettimeoffset();
 
-	} while (read_seqretry(&timekeeper.lock, seq));
+	} while (read_seqcount_retry(&timekeeper.rlock, seq));
 
 	timespec_add_ns(ts, nsecs);
 }
@@ -296,7 +299,7 @@ ktime_t ktime_get(void)
 	WARN_ON(timekeeping_suspended);
 
 	do {
-		seq = read_seqbegin(&timekeeper.lock);
+		seq = read_seqcount_begin(&timekeeper.rlock);
 		secs = timekeeper.xtime_sec +
 				timekeeper.wall_to_monotonic.tv_sec;
 		nsecs = timekeeping_get_ns() +
@@ -304,7 +307,7 @@ ktime_t ktime_get(void)
 		/* If arch requires, add in gettimeoffset() */
 		nsecs += arch_gettimeoffset();
 
-	} while (read_seqretry(&timekeeper.lock, seq));
+	} while (read_seqcount_retry(&timekeeper.rlock, seq));
 	/*
 	 * Use ktime_set/ktime_add_ns to create a proper ktime on
 	 * 32-bit architectures without CONFIG_KTIME_SCALAR.
@@ -329,14 +332,14 @@ void ktime_get_ts(struct timespec *ts)
 	WARN_ON(timekeeping_suspended);
 
 	do {
-		seq = read_seqbegin(&timekeeper.lock);
+		seq = read_seqcount_begin(&timekeeper.rlock);
 		ts->tv_sec = timekeeper.xtime_sec;
 		ts->tv_nsec = timekeeping_get_ns();
 		tomono = timekeeper.wall_to_monotonic;
 		/* If arch requires, add in gettimeoffset() */
 		ts->tv_nsec += arch_gettimeoffset();
 
-	} while (read_seqretry(&timekeeper.lock, seq));
+	} while (read_seqcount_retry(&timekeeper.rlock, seq));
 
 	set_normalized_timespec(ts, ts->tv_sec + tomono.tv_sec,
 				ts->tv_nsec + tomono.tv_nsec);
@@ -364,7 +367,7 @@ void getnstime_raw_and_real(struct timespec *ts_raw, struct timespec *ts_real)
 	do {
 		u32 arch_offset;
 
-		seq = read_seqbegin(&timekeeper.lock);
+		seq = read_seqcount_begin(&timekeeper.rlock);
 
 		*ts_raw = timekeeper.raw_time;
 		ts_real->tv_sec = timekeeper.xtime_sec;
@@ -378,7 +381,7 @@ void getnstime_raw_and_real(struct timespec *ts_raw, struct timespec *ts_real)
 		nsecs_raw += arch_offset;
 		nsecs_real += arch_offset;
 
-	} while (read_seqretry(&timekeeper.lock, seq));
+	} while (read_seqcount_retry(&timekeeper.rlock, seq));
 
 	timespec_add_ns(ts_raw, nsecs_raw);
 	timespec_add_ns(ts_real, nsecs_real);
@@ -417,7 +420,8 @@ int do_settimeofday(const struct timespec *tv)
 	if ((unsigned long)tv->tv_nsec >= NSEC_PER_SEC)
 		return -EINVAL;
 
-	write_seqlock_irqsave(&timekeeper.lock, flags);
+	spin_lock_irqsave(&timekeeper.wlock, flags);
+	write_seqcount_begin(&timekeeper.rlock);
 
 	timekeeping_forward_now();
 
@@ -432,7 +436,8 @@ int do_settimeofday(const struct timespec *tv)
 
 	timekeeping_update(&timekeeper, true);
 
-	write_sequnlock_irqrestore(&timekeeper.lock, flags);
+	write_seqcount_end(&timekeeper.rlock);
+	spin_unlock_irqrestore(&timekeeper.wlock, flags);
 
 	/* signal hrtimers about time change */
 	clock_was_set();
@@ -456,7 +461,8 @@ int timekeeping_inject_offset(struct timespec *ts)
 	if ((unsigned long)ts->tv_nsec >= NSEC_PER_SEC)
 		return -EINVAL;
 
-	write_seqlock_irqsave(&timekeeper.lock, flags);
+	spin_lock_irqsave(&timekeeper.wlock, flags);
+	write_seqcount_begin(&timekeeper.rlock);
 
 	timekeeping_forward_now();
 
@@ -467,7 +473,8 @@ int timekeeping_inject_offset(struct timespec *ts)
 
 	timekeeping_update(&timekeeper, true);
 
-	write_sequnlock_irqrestore(&timekeeper.lock, flags);
+	write_seqcount_end(&timekeeper.rlock);
+	spin_unlock_irqrestore(&timekeeper.wlock, flags);
 
 	/* signal hrtimers about time change */
 	clock_was_set();
@@ -539,11 +546,11 @@ void getrawmonotonic(struct timespec *ts)
 	s64 nsecs;
 
 	do {
-		seq = read_seqbegin(&timekeeper.lock);
+		seq = read_seqcount_begin(&timekeeper.rlock);
 		nsecs = timekeeping_get_ns_raw();
 		*ts = timekeeper.raw_time;
 
-	} while (read_seqretry(&timekeeper.lock, seq));
+	} while (read_seqcount_retry(&timekeeper.rlock, seq));
 
 	timespec_add_ns(ts, nsecs);
 }
@@ -559,11 +566,11 @@ int timekeeping_valid_for_hres(void)
 	int ret;
 
 	do {
-		seq = read_seqbegin(&timekeeper.lock);
+		seq = read_seqcount_begin(&timekeeper.rlock);
 
 		ret = timekeeper.clock->flags & CLOCK_SOURCE_VALID_FOR_HRES;
 
-	} while (read_seqretry(&timekeeper.lock, seq));
+	} while (read_seqcount_retry(&timekeeper.rlock, seq));
 
 	return ret;
 }
@@ -576,11 +583,11 @@ u64 timekeeping_max_deferment(void)
 	unsigned long seq;
 	u64 ret;
 	do {
-		seq = read_seqbegin(&timekeeper.lock);
+		seq = read_seqcount_begin(&timekeeper.rlock);
 
 		ret = timekeeper.clock->max_idle_ns;
 
-	} while (read_seqretry(&timekeeper.lock, seq));
+	} while (read_seqcount_retry(&timekeeper.rlock, seq));
 
 	return ret;
 }
@@ -627,11 +634,13 @@ void __init timekeeping_init(void)
 	read_persistent_clock(&now);
 	read_boot_clock(&boot);
 
-	seqlock_init(&timekeeper.lock);
-
+	seqcount_init(&timekeeper.rlock);
+	spin_lock_init(&timekeeper.wlock);
 	ntp_init();
 
-	write_seqlock_irqsave(&timekeeper.lock, flags);
+	spin_lock_irqsave(&timekeeper.wlock, flags);
+	write_seqcount_begin(&timekeeper.rlock);
+
 	clock = clocksource_default_clock();
 	if (clock->enable)
 		clock->enable(clock);
@@ -647,7 +656,10 @@ void __init timekeeping_init(void)
 				-boot.tv_sec, -boot.tv_nsec);
 	timekeeper.total_sleep_time.tv_sec = 0;
 	timekeeper.total_sleep_time.tv_nsec = 0;
-	write_sequnlock_irqrestore(&timekeeper.lock, flags);
+
+	write_seqcount_end(&timekeeper.rlock);
+	spin_unlock_irqrestore(&timekeeper.wlock, flags);
+
 }
 
 /* time in seconds when suspend began */
@@ -696,7 +708,8 @@ void timekeeping_inject_sleeptime(struct timespec *delta)
 	if (!(ts.tv_sec == 0 && ts.tv_nsec == 0))
 		return;
 
-	write_seqlock_irqsave(&timekeeper.lock, flags);
+	spin_lock_irqsave(&timekeeper.wlock, flags);
+	write_seqcount_begin(&timekeeper.rlock);
 
 	timekeeping_forward_now();
 
@@ -704,7 +717,8 @@ void timekeeping_inject_sleeptime(struct timespec *delta)
 
 	timekeeping_update(&timekeeper, true);
 
-	write_sequnlock_irqrestore(&timekeeper.lock, flags);
+	write_seqcount_end(&timekeeper.rlock);
+	spin_unlock_irqrestore(&timekeeper.wlock, flags);
 
 	/* signal hrtimers about time change */
 	clock_was_set();
@@ -727,7 +741,8 @@ static void timekeeping_resume(void)
 
 	clocksource_resume();
 
-	write_seqlock_irqsave(&timekeeper.lock, flags);
+	spin_lock_irqsave(&timekeeper.wlock, flags);
+	write_seqcount_begin(&timekeeper.rlock);
 
 	if (timespec_compare(&ts, &timekeeping_suspend_time) > 0) {
 		ts = timespec_sub(ts, timekeeping_suspend_time);
@@ -737,7 +752,9 @@ static void timekeeping_resume(void)
 	timekeeper.clock->cycle_last = timekeeper.clock->read(timekeeper.clock);
 	timekeeper.ntp_error = 0;
 	timekeeping_suspended = 0;
-	write_sequnlock_irqrestore(&timekeeper.lock, flags);
+
+	write_seqcount_end(&timekeeper.rlock);
+	spin_unlock_irqrestore(&timekeeper.wlock, flags);
 
 	touch_softlockup_watchdog();
 
@@ -755,7 +772,9 @@ static int timekeeping_suspend(void)
 
 	read_persistent_clock(&timekeeping_suspend_time);
 
-	write_seqlock_irqsave(&timekeeper.lock, flags);
+	spin_lock_irqsave(&timekeeper.wlock, flags);
+	write_seqcount_begin(&timekeeper.rlock);
+
 	timekeeping_forward_now();
 	timekeeping_suspended = 1;
 
@@ -779,7 +798,9 @@ static int timekeeping_suspend(void)
 		timekeeping_suspend_time =
 			timespec_add(timekeeping_suspend_time, delta_delta);
 	}
-	write_sequnlock_irqrestore(&timekeeper.lock, flags);
+
+	write_seqcount_end(&timekeeper.rlock);
+	spin_unlock_irqrestore(&timekeeper.wlock, flags);
 
 	clockevents_notify(CLOCK_EVT_NOTIFY_SUSPEND, NULL);
 	clocksource_suspend();
@@ -1035,7 +1056,8 @@ static void update_wall_time(void)
 	unsigned long flags;
 	s64 remainder;
 
-	write_seqlock_irqsave(&timekeeper.lock, flags);
+	spin_lock_irqsave(&timekeeper.wlock, flags);
+	write_seqcount_begin(&timekeeper.rlock);
 
 	/* Make sure we're fully resumed: */
 	if (unlikely(timekeeping_suspended))
@@ -1122,7 +1144,8 @@ static void update_wall_time(void)
 	timekeeping_update(&timekeeper, false);
 
 out:
-	write_sequnlock_irqrestore(&timekeeper.lock, flags);
+	write_seqcount_end(&timekeeper.rlock);
+	spin_unlock_irqrestore(&timekeeper.wlock, flags);
 
 }
 
@@ -1168,13 +1191,13 @@ void get_monotonic_boottime(struct timespec *ts)
 	WARN_ON(timekeeping_suspended);
 
 	do {
-		seq = read_seqbegin(&timekeeper.lock);
+		seq = read_seqcount_begin(&timekeeper.rlock);
 		ts->tv_sec = timekeeper.xtime_sec;
 		ts->tv_nsec = timekeeping_get_ns();
 		tomono = timekeeper.wall_to_monotonic;
 		sleep = timekeeper.total_sleep_time;
 
-	} while (read_seqretry(&timekeeper.lock, seq));
+	} while (read_seqcount_retry(&timekeeper.rlock, seq));
 
 	set_normalized_timespec(ts, ts->tv_sec + tomono.tv_sec + sleep.tv_sec,
 			ts->tv_nsec + tomono.tv_nsec + sleep.tv_nsec);
@@ -1225,10 +1248,10 @@ struct timespec current_kernel_time(void)
 	unsigned long seq;
 
 	do {
-		seq = read_seqbegin(&timekeeper.lock);
+		seq = read_seqcount_begin(&timekeeper.rlock);
 
 		now = timekeeper_xtime(&timekeeper);
-	} while (read_seqretry(&timekeeper.lock, seq));
+	} while (read_seqcount_retry(&timekeeper.rlock, seq));
 
 	return now;
 }
@@ -1240,11 +1263,11 @@ struct timespec get_monotonic_coarse(void)
 	unsigned long seq;
 
 	do {
-		seq = read_seqbegin(&timekeeper.lock);
+		seq = read_seqcount_begin(&timekeeper.rlock);
 
 		now = timekeeper_xtime(&timekeeper);
 		mono = timekeeper.wall_to_monotonic;
-	} while (read_seqretry(&timekeeper.lock, seq));
+	} while (read_seqcount_retry(&timekeeper.rlock, seq));
 
 	set_normalized_timespec(&now, now.tv_sec + mono.tv_sec,
 				now.tv_nsec + mono.tv_nsec);
@@ -1276,11 +1299,11 @@ void get_xtime_and_monotonic_and_sleep_offset(struct timespec *xtim,
 	unsigned long seq;
 
 	do {
-		seq = read_seqbegin(&timekeeper.lock);
+		seq = read_seqcount_begin(&timekeeper.rlock);
 		*xtim = timekeeper_xtime(&timekeeper);
 		*wtom = timekeeper.wall_to_monotonic;
 		*sleep = timekeeper.total_sleep_time;
-	} while (read_seqretry(&timekeeper.lock, seq));
+	} while (read_seqcount_retry(&timekeeper.rlock, seq));
 }
 
 /**
@@ -1292,9 +1315,9 @@ ktime_t ktime_get_monotonic_offset(void)
 	struct timespec wtom;
 
 	do {
-		seq = read_seqbegin(&timekeeper.lock);
+		seq = read_seqcount_begin(&timekeeper.rlock);
 		wtom = timekeeper.wall_to_monotonic;
-	} while (read_seqretry(&timekeeper.lock, seq));
+	} while (read_seqcount_retry(&timekeeper.rlock, seq));
 
 	return timespec_to_ktime(wtom);
 }
-- 
1.7.3.2.146.gca209

[PATCH 4/7] time: Update timekeeper structure using a local shadow

[John Stultz]

Uses a local shadow structure to update the timekeeper. This
will allow for reduced timekeeper.rlock hold time.

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/timekeeping.c |   42 ++++++++++++++++++++++--------------------
 1 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 4962284..6c36d19 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1051,6 +1051,7 @@ static cycle_t logarithmic_accumulation(struct timekeeper *tk, cycle_t offset,
 static void update_wall_time(void)
 {
 	struct clocksource *clock;
+	struct timekeeper tk;
 	cycle_t offset;
 	int shift = 0, maxshift;
 	unsigned long flags;
@@ -1063,10 +1064,11 @@ static void update_wall_time(void)
 	if (unlikely(timekeeping_suspended))
 		goto out;
 
-	clock = timekeeper.clock;
+	tk = timekeeper;
+	clock = tk.clock;
 
 #ifdef CONFIG_ARCH_USES_GETTIMEOFFSET
-	offset = timekeeper.cycle_interval;
+	offset = tk.cycle_interval;
 #else
 	offset = (clock->read(clock) - clock->cycle_last) & clock->mask;
 #endif
@@ -1079,19 +1081,19 @@ static void update_wall_time(void)
 	 * chunk in one go, and then try to consume the next smaller
 	 * doubled multiple.
 	 */
-	shift = ilog2(offset) - ilog2(timekeeper.cycle_interval);
+	shift = ilog2(offset) - ilog2(tk.cycle_interval);
 	shift = max(0, shift);
 	/* Bound shift to one less then what overflows tick_length */
 	maxshift = (64 - (ilog2(ntp_tick_length())+1)) - 1;
 	shift = min(shift, maxshift);
-	while (offset >= timekeeper.cycle_interval) {
-		offset = logarithmic_accumulation(&timekeeper, offset, shift);
-		if(offset < timekeeper.cycle_interval<<shift)
+	while (offset >= tk.cycle_interval) {
+		offset = logarithmic_accumulation(&tk, offset, shift);
+		if (offset < tk.cycle_interval<<shift)
 			shift--;
 	}
 
 	/* correct the clock when NTP error is too big */
-	timekeeping_adjust(&timekeeper, offset);
+	timekeeping_adjust(&tk, offset);
 
 	/*
 	 * Since in the loop above, we accumulate any amount of time
@@ -1109,10 +1111,10 @@ static void update_wall_time(void)
 	 * We'll correct this error next time through this function, when
 	 * xtime_nsec is not as small.
 	 */
-	if (unlikely((s64)timekeeper.xtime_nsec < 0)) {
-		s64 neg = -(s64)timekeeper.xtime_nsec;
-		timekeeper.xtime_nsec = 0;
-		timekeeper.ntp_error += neg << timekeeper.ntp_error_shift;
+	if (unlikely((s64)tk.xtime_nsec < 0)) {
+		s64 neg = -(s64)tk.xtime_nsec;
+		tk.xtime_nsec = 0;
+		tk.ntp_error += neg << tk.ntp_error_shift;
 	}
 
 	/*
@@ -1124,23 +1126,23 @@ static void update_wall_time(void)
 	* the vsyscall implementations are converted to use xtime_nsec
 	* (shifted nanoseconds), this can be killed.
 	*/
-	remainder = timekeeper.xtime_nsec & ((1<<timekeeper.shift)-1);
-	timekeeper.xtime_nsec -= remainder;
-	timekeeper.xtime_nsec += 1<<timekeeper.shift;
-	timekeeper.ntp_error += remainder <<
-					timekeeper.ntp_error_shift;
+	remainder = tk.xtime_nsec & ((1<<tk.shift)-1);
+	tk.xtime_nsec -= remainder;
+	tk.xtime_nsec += 1<<tk.shift;
+	tk.ntp_error += remainder << tk.ntp_error_shift;
 
 	/*
 	 * Finally, make sure that after the rounding
 	 * xtime.tv_nsec isn't larger then NSEC_PER_SEC
 	 */
-	if (unlikely(timekeeper.xtime_nsec >=
-			(NSEC_PER_SEC << timekeeper.shift))) {
-		timekeeper.xtime_nsec -= NSEC_PER_SEC << timekeeper.shift;
-		timekeeper.xtime_sec++;
+	if (unlikely(tk.xtime_nsec >= (NSEC_PER_SEC << tk.shift))) {
+		tk.xtime_nsec -= NSEC_PER_SEC << tk.shift;
+		tk.xtime_sec++;
 		second_overflow();
 	}
 
+
+	timekeeper = tk;
 	timekeeping_update(&timekeeper, false);
 
 out:
-- 
1.7.3.2.146.gca209

[PATCH 5/7] time: Shadow cycle_last in timekeeper structure

[John Stultz]

The clocksource cycle_last value is problematic for working on
shadow copies of the timekeeper, because the clocksource is global.

Since its mostly used only for timekeeping, move cycle_last into
the timekeeper. Unfortunately there are some uses for cycle_last
outside of timekeeping (such as tsc_read, which makes sure we haven't
skipped to a core that the TSC is behind the last read), so we
keep the clocksource cycle_last updated as well.

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/timekeeping.c |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 6c36d19..ebfb037 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -29,7 +29,8 @@ struct timekeeper {
 	u32	mult;
 	/* The shift value of the current clocksource. */
 	int	shift;
-
+	/* cycle value at last accumulation point */
+	cycle_t cycle_last;
 	/* Number of clock cycles in one NTP interval. */
 	cycle_t cycle_interval;
 	/* Number of clock shifted nano seconds in one NTP interval. */
@@ -138,7 +139,8 @@ static void timekeeper_setup_internals(struct clocksource *clock)
 	u64 tmp, ntpinterval;
 
 	timekeeper.clock = clock;
-	clock->cycle_last = clock->read(clock);
+	timekeeper.cycle_last = clock->read(clock);
+	clock->cycle_last = timekeeper.cycle_last;
 
 	/* Do the ns -> cycle conversion first, using original mult */
 	tmp = NTP_INTERVAL_LENGTH;
@@ -184,7 +186,7 @@ static inline s64 timekeeping_get_ns(void)
 	cycle_now = clock->read(clock);
 
 	/* calculate the delta since the last update_wall_time: */
-	cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
+	cycle_delta = (cycle_now - timekeeper.cycle_last) & clock->mask;
 
 	nsec = cycle_delta * timekeeper.mult + timekeeper.xtime_nsec;
 	return nsec >> timekeeper.shift;
@@ -200,7 +202,7 @@ static inline s64 timekeeping_get_ns_raw(void)
 	cycle_now = clock->read(clock);
 
 	/* calculate the delta since the last update_wall_time: */
-	cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
+	cycle_delta = (cycle_now - timekeeper.cycle_last) & clock->mask;
 
 	/* return delta convert to nanoseconds. */
 	return clocksource_cyc2ns(cycle_delta, clock->mult, clock->shift);
@@ -248,8 +250,9 @@ static void timekeeping_forward_now(void)
 
 	clock = timekeeper.clock;
 	cycle_now = clock->read(clock);
-	cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
-	clock->cycle_last = cycle_now;
+	cycle_delta = (cycle_now - timekeeper.cycle_last) & clock->mask;
+	timekeeper.cycle_last = cycle_now;
+	timekeeper.clock->cycle_last = cycle_now;
 
 	timekeeper.xtime_nsec += cycle_delta * timekeeper.mult;
 
@@ -749,7 +752,8 @@ static void timekeeping_resume(void)
 		__timekeeping_inject_sleeptime(&ts);
 	}
 	/* re-base the last cycle value */
-	timekeeper.clock->cycle_last = timekeeper.clock->read(timekeeper.clock);
+	timekeeper.cycle_last = timekeeper.clock->read(timekeeper.clock);
+	timekeeper.clock->cycle_last = timekeeper.cycle_last;
 	timekeeper.ntp_error = 0;
 	timekeeping_suspended = 0;
 
@@ -1016,7 +1020,7 @@ static cycle_t logarithmic_accumulation(struct timekeeper *tk, cycle_t offset,
 
 	/* Accumulate one shifted interval */
 	offset -= tk->cycle_interval << shift;
-	tk->clock->cycle_last += tk->cycle_interval << shift;
+	tk->cycle_last += tk->cycle_interval << shift;
 
 	tk->xtime_nsec += tk->xtime_interval << shift;
 	while (tk->xtime_nsec >= nsecps) {
@@ -1070,7 +1074,7 @@ static void update_wall_time(void)
 #ifdef CONFIG_ARCH_USES_GETTIMEOFFSET
 	offset = tk.cycle_interval;
 #else
-	offset = (clock->read(clock) - clock->cycle_last) & clock->mask;
+	offset = (clock->read(clock) - tk.cycle_last) & clock->mask;
 #endif
 
 	/*
@@ -1143,6 +1147,7 @@ static void update_wall_time(void)
 
 
 	timekeeper = tk;
+	timekeeper.clock->cycle_last = timekeeper.cycle_last;
 	timekeeping_update(&timekeeper, false);
 
 out:
-- 
1.7.3.2.146.gca209

[PATCH 6/7] time: Reduce timekeeper read lock hold time

[John Stultz]

Now that timekeeper updates are done with a shadow copy,
we can reduce the readlock hold time to only the update.

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/timekeeping.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index ebfb037..5a444b8 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1062,7 +1062,6 @@ static void update_wall_time(void)
 	s64 remainder;
 
 	spin_lock_irqsave(&timekeeper.wlock, flags);
-	write_seqcount_begin(&timekeeper.rlock);
 
 	/* Make sure we're fully resumed: */
 	if (unlikely(timekeeping_suspended))
@@ -1145,13 +1144,18 @@ static void update_wall_time(void)
 		second_overflow();
 	}
 
-
+	write_seqcount_begin(&timekeeper.rlock);
+	/*
+	 * We have to store rlock back to tk, otherwise we'll
+	 * corrupt the timeekeerp.rlock when we copy over timekeeper
+	 */
+	tk.rlock = timekeeper.rlock;
 	timekeeper = tk;
 	timekeeper.clock->cycle_last = timekeeper.cycle_last;
 	timekeeping_update(&timekeeper, false);
+	write_seqcount_end(&timekeeper.rlock);
 
 out:
-	write_seqcount_end(&timekeeper.rlock);
 	spin_unlock_irqrestore(&timekeeper.wlock, flags);
 
 }
-- 
1.7.3.2.146.gca209

[PATCH 7/7] time: Convert the timekeeper's wlock to a raw_spin_lock

[John Stultz]

Convert the wlock to raw spin lock.

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/timekeeping.c |   36 ++++++++++++++++++------------------
 1 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 5a444b8..0a4decb 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -74,7 +74,7 @@ struct timekeeper {
 
 	/* locks for timekeeper structure */
 	seqcount_t rlock;   /* This seqcount serializes readers from updates */
-	spinlock_t wlock;	/* This spinlock serializes updaters */
+	raw_spinlock_t wlock;	/* This spinlock serializes updaters */
 };
 
 static struct timekeeper timekeeper;
@@ -225,13 +225,13 @@ void timekeeping_leap_insert(int leapsecond)
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&timekeeper.wlock, flags);
+	raw_spin_lock_irqsave(&timekeeper.wlock, flags);
 	write_seqcount_begin(&timekeeper.rlock);
 	timekeeper.xtime_sec += leapsecond;
 	timekeeper.wall_to_monotonic.tv_sec -= leapsecond;
 	timekeeping_update(&timekeeper, false);
 	write_seqcount_end(&timekeeper.rlock);
-	spin_unlock_irqrestore(&timekeeper.wlock, flags);
+	raw_spin_unlock_irqrestore(&timekeeper.wlock, flags);
 
 }
 
@@ -423,7 +423,7 @@ int do_settimeofday(const struct timespec *tv)
 	if ((unsigned long)tv->tv_nsec >= NSEC_PER_SEC)
 		return -EINVAL;
 
-	spin_lock_irqsave(&timekeeper.wlock, flags);
+	raw_spin_lock_irqsave(&timekeeper.wlock, flags);
 	write_seqcount_begin(&timekeeper.rlock);
 
 	timekeeping_forward_now();
@@ -440,7 +440,7 @@ int do_settimeofday(const struct timespec *tv)
 	timekeeping_update(&timekeeper, true);
 
 	write_seqcount_end(&timekeeper.rlock);
-	spin_unlock_irqrestore(&timekeeper.wlock, flags);
+	raw_spin_unlock_irqrestore(&timekeeper.wlock, flags);
 
 	/* signal hrtimers about time change */
 	clock_was_set();
@@ -464,7 +464,7 @@ int timekeeping_inject_offset(struct timespec *ts)
 	if ((unsigned long)ts->tv_nsec >= NSEC_PER_SEC)
 		return -EINVAL;
 
-	spin_lock_irqsave(&timekeeper.wlock, flags);
+	raw_spin_lock_irqsave(&timekeeper.wlock, flags);
 	write_seqcount_begin(&timekeeper.rlock);
 
 	timekeeping_forward_now();
@@ -477,7 +477,7 @@ int timekeeping_inject_offset(struct timespec *ts)
 	timekeeping_update(&timekeeper, true);
 
 	write_seqcount_end(&timekeeper.rlock);
-	spin_unlock_irqrestore(&timekeeper.wlock, flags);
+	raw_spin_unlock_irqrestore(&timekeeper.wlock, flags);
 
 	/* signal hrtimers about time change */
 	clock_was_set();
@@ -638,10 +638,10 @@ void __init timekeeping_init(void)
 	read_boot_clock(&boot);
 
 	seqcount_init(&timekeeper.rlock);
-	spin_lock_init(&timekeeper.wlock);
+	raw_spin_lock_init(&timekeeper.wlock);
 	ntp_init();
 
-	spin_lock_irqsave(&timekeeper.wlock, flags);
+	raw_spin_lock_irqsave(&timekeeper.wlock, flags);
 	write_seqcount_begin(&timekeeper.rlock);
 
 	clock = clocksource_default_clock();
@@ -661,7 +661,7 @@ void __init timekeeping_init(void)
 	timekeeper.total_sleep_time.tv_nsec = 0;
 
 	write_seqcount_end(&timekeeper.rlock);
-	spin_unlock_irqrestore(&timekeeper.wlock, flags);
+	raw_spin_unlock_irqrestore(&timekeeper.wlock, flags);
 
 }
 
@@ -711,7 +711,7 @@ void timekeeping_inject_sleeptime(struct timespec *delta)
 	if (!(ts.tv_sec == 0 && ts.tv_nsec == 0))
 		return;
 
-	spin_lock_irqsave(&timekeeper.wlock, flags);
+	raw_spin_lock_irqsave(&timekeeper.wlock, flags);
 	write_seqcount_begin(&timekeeper.rlock);
 
 	timekeeping_forward_now();
@@ -721,7 +721,7 @@ void timekeeping_inject_sleeptime(struct timespec *delta)
 	timekeeping_update(&timekeeper, true);
 
 	write_seqcount_end(&timekeeper.rlock);
-	spin_unlock_irqrestore(&timekeeper.wlock, flags);
+	raw_spin_unlock_irqrestore(&timekeeper.wlock, flags);
 
 	/* signal hrtimers about time change */
 	clock_was_set();
@@ -744,7 +744,7 @@ static void timekeeping_resume(void)
 
 	clocksource_resume();
 
-	spin_lock_irqsave(&timekeeper.wlock, flags);
+	raw_spin_lock_irqsave(&timekeeper.wlock, flags);
 	write_seqcount_begin(&timekeeper.rlock);
 
 	if (timespec_compare(&ts, &timekeeping_suspend_time) > 0) {
@@ -758,7 +758,7 @@ static void timekeeping_resume(void)
 	timekeeping_suspended = 0;
 
 	write_seqcount_end(&timekeeper.rlock);
-	spin_unlock_irqrestore(&timekeeper.wlock, flags);
+	raw_spin_unlock_irqrestore(&timekeeper.wlock, flags);
 
 	touch_softlockup_watchdog();
 
@@ -776,7 +776,7 @@ static int timekeeping_suspend(void)
 
 	read_persistent_clock(&timekeeping_suspend_time);
 
-	spin_lock_irqsave(&timekeeper.wlock, flags);
+	raw_spin_lock_irqsave(&timekeeper.wlock, flags);
 	write_seqcount_begin(&timekeeper.rlock);
 
 	timekeeping_forward_now();
@@ -804,7 +804,7 @@ static int timekeeping_suspend(void)
 	}
 
 	write_seqcount_end(&timekeeper.rlock);
-	spin_unlock_irqrestore(&timekeeper.wlock, flags);
+	raw_spin_unlock_irqrestore(&timekeeper.wlock, flags);
 
 	clockevents_notify(CLOCK_EVT_NOTIFY_SUSPEND, NULL);
 	clocksource_suspend();
@@ -1061,7 +1061,7 @@ static void update_wall_time(void)
 	unsigned long flags;
 	s64 remainder;
 
-	spin_lock_irqsave(&timekeeper.wlock, flags);
+	raw_spin_lock_irqsave(&timekeeper.wlock, flags);
 
 	/* Make sure we're fully resumed: */
 	if (unlikely(timekeeping_suspended))
@@ -1156,7 +1156,7 @@ static void update_wall_time(void)
 	write_seqcount_end(&timekeeper.rlock);
 
 out:
-	spin_unlock_irqrestore(&timekeeper.wlock, flags);
+	raw_spin_unlock_irqrestore(&timekeeper.wlock, flags);
 
 }
 
-- 
1.7.3.2.146.gca209

Re: [PATCH 1/7] time: Condense timekeeper.xtime into xtime_sec

[Ingo Molnar]

* John Stultz <john.stultz@linaro.org> wrote:

> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -39,8 +39,11 @@ struct timekeeper {
>  	/* Raw nano seconds accumulated per NTP interval. */
>  	u32	raw_interval;
>  
> -	/* Clock shifted nano seconds remainder not stored in xtime.tv_nsec. */
> +	/* Current CLOCK_REALTIME time in seconds */
> +	u64	xtime_sec;
> +	/* Clock shifted nano seconds */
>  	u64	xtime_nsec;
> +
>  	/* Difference between accumulated time and NTP time in ntp
>  	 * shifted nano seconds. */
>  	s64	ntp_error;
> @@ -48,8 +51,6 @@ struct timekeeper {
>  	 * ntp shifted nano seconds. */
>  	int	ntp_error_shift;
>  
> -	/* The current time */
> -	struct timespec xtime;

Please use consistent vertical spacing for this structure.

> +static struct timespec timekeeper_xtime(struct timekeeper *tk)
> +{
> +	struct timespec ts;
> +
> +	ts.tv_sec = tk->xtime_sec;
> +	ts.tv_nsec = (long)(tk->xtime_nsec >> tk->shift);
> +	return ts;
> +}

btw., is tk->shift intentionally a signed int? If not then it 
would be better to make it u32, like tk->mult, to make sure the 
compiler never does complex signed arithmetics - and to clean up 
'struct timekeeper'.

> +
> +static void timekeeper_set_xtime(struct timekeeper *tk,
> +					const struct timespec *ts)

Pointless (because ugly) line break.

> +{
> +	tk->xtime_sec = ts->tv_sec;
> +	tk->xtime_nsec = ts->tv_nsec << tk->shift;
> +}
> +
> +
> +static void timekeeper_xtime_add(struct timekeeper *tk,
> +					const struct timespec *ts)

Pointless (because ugly) line break.

Thanks,

	Ingo

Re: [PATCH 3/7] time: Split timekeeper lock into separate reader/writer locks

[Ingo Molnar]

* John Stultz <john.stultz@linaro.org> wrote:

> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -71,8 +71,9 @@ struct timekeeper {
>  	/* The raw monotonic time for the CLOCK_MONOTONIC_RAW posix clock. */
>  	struct timespec raw_time;
>  
> -	/* Seqlock for all timekeeper values */
> -	seqlock_t lock;
> +	/* locks for timekeeper structure */
> +	seqcount_t rlock;   /* This seqcount serializes readers from updates */
> +	spinlock_t wlock;	/* This spinlock serializes updaters */

Vertical spacing broken again - not just of the fields but of 
the comments as well.

> @@ -576,11 +583,11 @@ u64 timekeeping_max_deferment(void)
>  	unsigned long seq;
>  	u64 ret;
>  	do {

sigh.

Thanks,

	Ingo

Re: [PATCH 4/7] time: Update timekeeper structure using a local shadow

[Ingo Molnar]

* John Stultz <john.stultz@linaro.org> wrote:

>  static void update_wall_time(void)
>  {
>  	struct clocksource *clock;
> +	struct timekeeper tk;
>  	cycle_t offset;
>  	int shift = 0, maxshift;
>  	unsigned long flags;
> @@ -1063,10 +1064,11 @@ static void update_wall_time(void)
>  	if (unlikely(timekeeping_suspended))
>  		goto out;
>  
> -	clock = timekeeper.clock;
> +	tk = timekeeper;

'tk' is now an on-stack copy of a very non-trivial (read: large) 
structure - including locks, amongst other things. That's a 
no-no.

> +	timekeeper = tk;

You just broke lockdep here.

It's also ugly code: global data structures should almost always 
be updated in situ, instead of updating a local copy and then 
copying it back blindly ...

Thanks,

	Ingo

Re: [PATCH 5/7] time: Shadow cycle_last in timekeeper structure

[Ingo Molnar]

* John Stultz <john.stultz@linaro.org> wrote:

> @@ -184,7 +186,7 @@ static inline s64 timekeeping_get_ns(void)
>  	cycle_now = clock->read(clock);
>  
>  	/* calculate the delta since the last update_wall_time: */
> -	cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> +	cycle_delta = (cycle_now - timekeeper.cycle_last) & clock->mask;

Just a general code design observation: the way how the global 
'timekeeper' structure is accessed is rather ugly.

The standard method is to pass it in to timekeeping_get_ns() 
(and other methods) as a parameter, with the highest level 
taking the address via &timekeeping.

That will also make the code shorter and more obvious: the 
familar tk->cycle_last pattern instead of the current mixed uses 
of timekeeping.cycle_last and tk->cycle_last.

There's many similar patterns throughout this code, they need to 
be fixed as well.

Thanks,

	Ingo

Re: [PATCH 1/7] time: Condense timekeeper.xtime into xtime_sec

[John Stultz]

On Tue, 2012-02-28 at 09:06 +0100, Ingo Molnar wrote:
> * John Stultz <john.stultz@linaro.org> wrote:
> > +
> > +static void timekeeper_set_xtime(struct timekeeper *tk,
> > +					const struct timespec *ts)
> 
> Pointless (because ugly) line break.
> 
> > +{
> > +	tk->xtime_sec = ts->tv_sec;
> > +	tk->xtime_nsec = ts->tv_nsec << tk->shift;
> > +}
> > +
> > +
> > +static void timekeeper_xtime_add(struct timekeeper *tk,
> > +					const struct timespec *ts)
> 
> Pointless (because ugly) line break.

Ack on your other comments, but I'm not sure I'm following you here.
What would you rather see in this case?

(I know the 80col limit was discussed recently, but it didn't sound like
the consensus was for extending it.)

thanks
-john

Re: [PATCH 1/7] time: Condense timekeeper.xtime into xtime_sec

[Ingo Molnar]

* John Stultz <john.stultz@linaro.org> wrote:

> On Tue, 2012-02-28 at 09:06 +0100, Ingo Molnar wrote:
> > * John Stultz <john.stultz@linaro.org> wrote:
> > > +
> > > +static void timekeeper_set_xtime(struct timekeeper *tk,
> > > +					const struct timespec *ts)
> > 
> > Pointless (because ugly) line break.
> > 
> > > +{
> > > +	tk->xtime_sec = ts->tv_sec;
> > > +	tk->xtime_nsec = ts->tv_nsec << tk->shift;
> > > +}
> > > +
> > > +
> > > +static void timekeeper_xtime_add(struct timekeeper *tk,
> > > +					const struct timespec *ts)
> > 
> > Pointless (because ugly) line break.
> 
> Ack on your other comments, but I'm not sure I'm following you here.
> What would you rather see in this case?

Well, either not break the line really (it's just trivially 
above col80: it's 83 cols), or do a *sensible* line break - one 
of:

static void timekeeper_xtime_add(struct timekeeper *tk,
				 const struct timespec *ts)

(here the arguments align vertically)

Or:

static void
timekeeper_xtime_add(struct timekeeper *tk, const struct timespec *ts)

(here he return type is on a separate line)

Or, my favorite, just make the name shorter, because it's too 
long:

static void tk_xtime_add(struct timekeeper *tk, const struct timespec *ts)

Obviously this applies to other functions as well - the tk_*() 
method pattern should be applied consistently. Anyone reading 
time code will know that 'tk' is about the global timekeeper 
structure - no need to use the verbose name in methods.

Thanks,

	Ingo