Skip to contentSkip to conversation

time/timekeeping: Simplify vsyscall locking

PATCH In review Thomas Gleixner 3 participants 8 messages Feb 28, 2012 – Mar 27, 2012 x86

lore
Share & tools
lore

Thomas Gleixner

author
#0 lore raw

cover time/timekeeping: Simplify vsyscall locking

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

author
#1 lore raw

PATCH 1/4 time: Remove bogus comments

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 <[email protected]>
---
 kernel/time.c |    6 ------
 1 file changed, 6 deletions(-)
kernel/time.c +0 -6
Index: 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;
 }
author
#2 lore raw

PATCH 2/4 x86: vdso: Remove bogus locking in update_vsyscall_tz()

In reply to Thomas Gleixner

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 <[email protected]>
---
 arch/x86/kernel/vsyscall_64.c |    5 -----
 1 file changed, 5 deletions(-)
arch/x86/kernel/vsyscall_64.c +0 -5
Index: 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,
author
#3 lore raw

PATCH 3/4 x86: vdso: Use seqcount instead of seqlock

In reply to Thomas Gleixner

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 <[email protected]>
---
 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(-)
arch/x86/include/asm/vgtod.h +1 -1
Index: 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;
arch/x86/kernel/vsyscall_64.c +3 -8
Index: 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,
arch/x86/vdso/vclock_gettime.c +8 -8
Index: 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.
author
#4 lore raw

PATCH 4/4 ia64: vsyscall: Use seqcount instead of seqlock

In reply to Thomas Gleixner

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 <[email protected]>
Cc Tony Luck <[email protected]>
---
 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(-)
arch/ia64/kernel/asm-offsets.c +2 -2
Index: 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,
arch/ia64/kernel/fsys.S +1 -1
Index: 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
arch/ia64/kernel/fsyscall_gtod_data.h +1 -1
Index: 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;
arch/ia64/kernel/time.c +3 -7
Index: 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);
 }
#5 lore raw

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

Quoted 5 lines

       /* 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

author
#6 lore raw

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

Quoted 10 lines · Tony Luck
Quoted 5 lines

       /* 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

Reviewed-by on the v1 series
#7 lore raw
In reply to Thomas Gleixner
Quoted 5 lines · 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.

FWIW, patches 2 and 3 are

Reviewed-by Andy Lutomirski <[email protected]>

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