[patch 0/4] time/timekeeping: Simplify vsyscall locking

[patch 0/4] time/timekeeping: Simplify vsyscall locking

[Thomas Gleixner]

vsyscall updates have a separate seqlock which nests into
xtime_lock. There is no point to grab another lock for nothing. All it
needs is to update the seqcount which is user space visible.

The following series cleans that up.

Thanks,

	tglx

[patch 1/4] time: Remove bogus comments

[Thomas Gleixner]

[-- Attachment #1: time-remove-bogus-comments.patch --]
[-- Type: text/plain, Size: 875 bytes --]

There is no global irq lock which makes a syscall magically SMP
safe. Remove the outdated comment concerning do_settimeofday() as
well.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/time.c |    6 ------
 1 file changed, 6 deletions(-)

Index: linux-3.2/kernel/time.c
===================================================================
--- linux-3.2.orig/kernel/time.c
+++ linux-3.2/kernel/time.c
@@ -163,7 +163,6 @@ int do_sys_settimeofday(const struct tim
 		return error;
 
 	if (tz) {
-		/* SMP safe, global irq locking makes it work. */
 		sys_tz = *tz;
 		update_vsyscall_tz();
 		if (firsttime) {
@@ -173,12 +172,7 @@ int do_sys_settimeofday(const struct tim
 		}
 	}
 	if (tv)
-	{
-		/* SMP safe, again the code in arch/foo/time.c should
-		 * globally block out interrupts when it runs.
-		 */
 		return do_settimeofday(tv);
-	}
 	return 0;
 }
 

[patch 2/4] x86: vdso: Remove bogus locking in update_vsyscall_tz()

[Thomas Gleixner]

[-- Attachment #1: x86-vdso-remove-bogus-locking-in-update_vsyscall_tz.patch --]
[-- Type: text/plain, Size: 946 bytes --]

Changing the sequence count in update_vsyscall_tz() is completely
pointless. 

The vdso code copies the data unprotected. There is no point to change
this as sys_tz is nowhere protected at all. See sys_gettimeofday().

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/vsyscall_64.c |    5 -----
 1 file changed, 5 deletions(-)

Index: linux-2.6/arch/x86/kernel/vsyscall_64.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/vsyscall_64.c
+++ linux-2.6/arch/x86/kernel/vsyscall_64.c
@@ -80,12 +80,7 @@ early_param("vsyscall", vsyscall_setup);
 
 void update_vsyscall_tz(void)
 {
-	unsigned long flags;
-
-	write_seqlock_irqsave(&vsyscall_gtod_data.lock, flags);
-	/* sys_tz has changed */
 	vsyscall_gtod_data.sys_tz = sys_tz;
-	write_sequnlock_irqrestore(&vsyscall_gtod_data.lock, flags);
 }
 
 void update_vsyscall(struct timespec *wall_time, struct timespec *wtm,

[patch 3/4] x86: vdso: Use seqcount instead of seqlock

[Thomas Gleixner]

[-- Attachment #1: x86-vdso-use-seqcount.patch --]
[-- Type: text/plain, Size: 4247 bytes --]

The update of the vdso data happens under xtime_lock, so adding a
nested lock is pointless. Just use a seqcount to sync the readers.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/vgtod.h   |    2 +-
 arch/x86/kernel/vsyscall_64.c  |   11 +++--------
 arch/x86/vdso/vclock_gettime.c |   16 ++++++++--------
 3 files changed, 12 insertions(+), 17 deletions(-)

Index: linux-2.6/arch/x86/include/asm/vgtod.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/vgtod.h
+++ linux-2.6/arch/x86/include/asm/vgtod.h
@@ -5,7 +5,7 @@
 #include <linux/clocksource.h>
 
 struct vsyscall_gtod_data {
-	seqlock_t	lock;
+	seqcount_t	seq;
 
 	/* open coded 'struct timespec' */
 	time_t		wall_time_sec;
Index: linux-2.6/arch/x86/kernel/vsyscall_64.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/vsyscall_64.c
+++ linux-2.6/arch/x86/kernel/vsyscall_64.c
@@ -52,10 +52,7 @@
 #include "vsyscall_trace.h"
 
 DEFINE_VVAR(int, vgetcpu_mode);
-DEFINE_VVAR(struct vsyscall_gtod_data, vsyscall_gtod_data) =
-{
-	.lock = __SEQLOCK_UNLOCKED(__vsyscall_gtod_data.lock),
-};
+DEFINE_VVAR(struct vsyscall_gtod_data, vsyscall_gtod_data);
 
 static enum { EMULATE, NATIVE, NONE } vsyscall_mode = EMULATE;
 
@@ -86,9 +83,7 @@ void update_vsyscall_tz(void)
 void update_vsyscall(struct timespec *wall_time, struct timespec *wtm,
 			struct clocksource *clock, u32 mult)
 {
-	unsigned long flags;
-
-	write_seqlock_irqsave(&vsyscall_gtod_data.lock, flags);
+	write_seqcount_begin(&vsyscall_gtod_data.seq);
 
 	/* copy vsyscall data */
 	vsyscall_gtod_data.clock.vclock_mode	= clock->archdata.vclock_mode;
@@ -101,7 +96,7 @@ void update_vsyscall(struct timespec *wa
 	vsyscall_gtod_data.wall_to_monotonic	= *wtm;
 	vsyscall_gtod_data.wall_time_coarse	= __current_kernel_time();
 
-	write_sequnlock_irqrestore(&vsyscall_gtod_data.lock, flags);
+	write_seqcount_end(&vsyscall_gtod_data.seq);
 }
 
 static void warn_bad_vsyscall(const char *level, struct pt_regs *regs,
Index: linux-2.6/arch/x86/vdso/vclock_gettime.c
===================================================================
--- linux-2.6.orig/arch/x86/vdso/vclock_gettime.c
+++ linux-2.6/arch/x86/vdso/vclock_gettime.c
@@ -86,11 +86,11 @@ notrace static noinline int do_realtime(
 {
 	unsigned long seq, ns;
 	do {
-		seq = read_seqbegin(&gtod->lock);
+		seq = read_seqcount_begin(&gtod->seq);
 		ts->tv_sec = gtod->wall_time_sec;
 		ts->tv_nsec = gtod->wall_time_nsec;
 		ns = vgetns();
-	} while (unlikely(read_seqretry(&gtod->lock, seq)));
+	} while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
 	timespec_add_ns(ts, ns);
 	return 0;
 }
@@ -99,12 +99,12 @@ notrace static noinline int do_monotonic
 {
 	unsigned long seq, ns, secs;
 	do {
-		seq = read_seqbegin(&gtod->lock);
+		seq = read_seqcount_begin(&gtod->seq);
 		secs = gtod->wall_time_sec;
 		ns = gtod->wall_time_nsec + vgetns();
 		secs += gtod->wall_to_monotonic.tv_sec;
 		ns += gtod->wall_to_monotonic.tv_nsec;
-	} while (unlikely(read_seqretry(&gtod->lock, seq)));
+	} while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
 
 	/* wall_time_nsec, vgetns(), and wall_to_monotonic.tv_nsec
 	 * are all guaranteed to be nonnegative.
@@ -123,10 +123,10 @@ notrace static noinline int do_realtime_
 {
 	unsigned long seq;
 	do {
-		seq = read_seqbegin(&gtod->lock);
+		seq = read_seqcount_begin(&gtod->seq);
 		ts->tv_sec = gtod->wall_time_coarse.tv_sec;
 		ts->tv_nsec = gtod->wall_time_coarse.tv_nsec;
-	} while (unlikely(read_seqretry(&gtod->lock, seq)));
+	} while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
 	return 0;
 }
 
@@ -134,12 +134,12 @@ notrace static noinline int do_monotonic
 {
 	unsigned long seq, ns, secs;
 	do {
-		seq = read_seqbegin(&gtod->lock);
+		seq = read_seqcount_begin(&gtod->seq);
 		secs = gtod->wall_time_coarse.tv_sec;
 		ns = gtod->wall_time_coarse.tv_nsec;
 		secs += gtod->wall_to_monotonic.tv_sec;
 		ns += gtod->wall_to_monotonic.tv_nsec;
-	} while (unlikely(read_seqretry(&gtod->lock, seq)));
+	} while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
 
 	/* wall_time_nsec and wall_to_monotonic.tv_nsec are
 	 * guaranteed to be between 0 and NSEC_PER_SEC.

[patch 4/4] ia64: vsyscall: Use seqcount instead of seqlock

[Thomas Gleixner]

[-- Attachment #1: ia64-vdso-use-seqcount.patch --]
[-- Type: text/plain, Size: 3071 bytes --]

The update of the vdso data happens under xtime_lock, so adding a
nested lock is pointless. Just use a seqcount to sync the readers.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
---
 arch/ia64/kernel/asm-offsets.c        |    4 ++--
 arch/ia64/kernel/fsys.S               |    2 +-
 arch/ia64/kernel/fsyscall_gtod_data.h |    2 +-
 arch/ia64/kernel/time.c               |   10 +++-------
 4 files changed, 7 insertions(+), 11 deletions(-)

Index: linux-2.6/arch/ia64/kernel/asm-offsets.c
===================================================================
--- linux-2.6.orig/arch/ia64/kernel/asm-offsets.c
+++ linux-2.6/arch/ia64/kernel/asm-offsets.c
@@ -269,8 +269,8 @@ void foo(void)
 	BLANK();
 
 	/* used by fsys_gettimeofday in arch/ia64/kernel/fsys.S */
-	DEFINE(IA64_GTOD_LOCK_OFFSET,
-		offsetof (struct fsyscall_gtod_data_t, lock));
+	DEFINE(IA64_GTOD_SEQ_OFFSET,
+		offsetof (struct fsyscall_gtod_data_t, seq);
 	DEFINE(IA64_GTOD_WALL_TIME_OFFSET,
 		offsetof (struct fsyscall_gtod_data_t, wall_time));
 	DEFINE(IA64_GTOD_MONO_TIME_OFFSET,
Index: linux-2.6/arch/ia64/kernel/fsys.S
===================================================================
--- linux-2.6.orig/arch/ia64/kernel/fsys.S
+++ linux-2.6/arch/ia64/kernel/fsys.S
@@ -174,7 +174,7 @@ ENTRY(fsys_set_tid_address)
 	FSYS_RETURN
 END(fsys_set_tid_address)
 
-#if IA64_GTOD_LOCK_OFFSET !=0
+#if IA64_GTOD_SEQ_OFFSET !=0
 #error fsys_gettimeofday incompatible with changes to struct fsyscall_gtod_data_t
 #endif
 #if IA64_ITC_JITTER_OFFSET !=0
Index: linux-2.6/arch/ia64/kernel/fsyscall_gtod_data.h
===================================================================
--- linux-2.6.orig/arch/ia64/kernel/fsyscall_gtod_data.h
+++ linux-2.6/arch/ia64/kernel/fsyscall_gtod_data.h
@@ -6,7 +6,7 @@
  */
 
 struct fsyscall_gtod_data_t {
-	seqlock_t	lock;
+	seqcount_t	seq;
 	struct timespec	wall_time;
 	struct timespec monotonic_time;
 	cycle_t		clk_mask;
Index: linux-2.6/arch/ia64/kernel/time.c
===================================================================
--- linux-2.6.orig/arch/ia64/kernel/time.c
+++ linux-2.6/arch/ia64/kernel/time.c
@@ -35,9 +35,7 @@
 
 static cycle_t itc_get_cycles(struct clocksource *cs);
 
-struct fsyscall_gtod_data_t fsyscall_gtod_data = {
-	.lock = __SEQLOCK_UNLOCKED(fsyscall_gtod_data.lock),
-};
+struct fsyscall_gtod_data_t fsyscall_gtod_data;
 
 struct itc_jitter_data_t itc_jitter_data;
 
@@ -460,9 +458,7 @@ void update_vsyscall_tz(void)
 void update_vsyscall(struct timespec *wall, struct timespec *wtm,
 			struct clocksource *c, u32 mult)
 {
-        unsigned long flags;
-
-        write_seqlock_irqsave(&fsyscall_gtod_data.lock, flags);
+	write_seqcount_begin(&fsyscall_gtod_data.seq);
 
         /* copy fsyscall clock data */
         fsyscall_gtod_data.clk_mask = c->mask;
@@ -485,6 +481,6 @@ void update_vsyscall(struct timespec *wa
 		fsyscall_gtod_data.monotonic_time.tv_sec++;
 	}
 
-        write_sequnlock_irqrestore(&fsyscall_gtod_data.lock, flags);
+	write_seqcount_end(&fsyscall_gtod_data.seq);
 }
 

Re: [patch 4/4] ia64: vsyscall: Use seqcount instead of seqlock

[Tony Luck]

>        /* used by fsys_gettimeofday in arch/ia64/kernel/fsys.S */
> -       DEFINE(IA64_GTOD_LOCK_OFFSET,
> -               offsetof (struct fsyscall_gtod_data_t, lock));
> +       DEFINE(IA64_GTOD_SEQ_OFFSET,
> +               offsetof (struct fsyscall_gtod_data_t, seq);

Thomas,

Could you spare one more ")" here please? The compiler likes
it if they match :-)

Thanks

-Tony

Re: [patch 4/4] ia64: vsyscall: Use seqcount instead of seqlock

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 471 bytes --]

On Mon, 26 Mar 2012, Tony Luck wrote:

> >        /* used by fsys_gettimeofday in arch/ia64/kernel/fsys.S */
> > -       DEFINE(IA64_GTOD_LOCK_OFFSET,
> > -               offsetof (struct fsyscall_gtod_data_t, lock));
> > +       DEFINE(IA64_GTOD_SEQ_OFFSET,
> > +               offsetof (struct fsyscall_gtod_data_t, seq);
> 
> Thomas,
> 
> Could you spare one more ")" here please? The compiler likes
> it if they match :-)

Duh, crap. /me rumages for an ia64 compiler

Re: [patch 0/4] time/timekeeping: Simplify vsyscall locking

[Andy Lutomirski]

On 02/28/2012 11:46 AM, Thomas Gleixner wrote:
> vsyscall updates have a separate seqlock which nests into
> xtime_lock. There is no point to grab another lock for nothing. All it
> needs is to update the seqcount which is user space visible.
> 
> The following series cleans that up.

FWIW, patches 2 and 3 are
Reviewed-by: Andy Lutomirski <luto@amacapital.net>

Can you let me know when these hit a git tree somewhere?  I have two
more vdso clock speedups I want to send for 3.4, and I'll rebase them on
top of these.

Thanks,
Andy