Skip to contentSkip to conversation

powerpc: Rework VDSO gettimeofday to prevent time going backwards

PATCH In review Paul Mackerras 1 participant 3 messages Jun 21, 2010

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/2 powerpc: Rework VDSO gettimeofday to prevent time going backwards

Currently it is possible for userspace to see the result of gettimeofday() going backwards by 1 microsecond, assuming that userspace is using the gettimeofday() in the VDSO. The VDSO gettimeofday() algorithm computes the time in "xsecs", which are units of 2^-20 seconds, or approximately 0.954 microseconds, using the algorithm

now = (timebase - tb_orig_stamp) * tb_to_xs + stamp_xsec

and then converts the time in xsecs to seconds and microseconds.

The kernel updates the tb_orig_stamp and stamp_xsec values every tick in update_vsyscall(). If the length of the tick is not an integer number of xsecs, then some precision is lost in converting the current time to xsecs. For example, with CONFIG_HZ=1000, the tick is 1ms long, which is 1048.576 xsecs. That means that stamp_xsec will advance by either 1048 or 1049 on each tick. With the right conditions, it is possible for userspace to get (timebase - tb_orig_stamp) * tb_to_xs being 1049 if the kernel is slightly late in updating the vdso_datapage, and then for stamp_xsec to advance by 1048 when the kernel does update it, and for userspace to then see (timebase - tb_orig_stamp) * tb_to_xs being zero due to integer truncation. The result is that time appears to go backwards by 1 microsecond.

To fix this we change the VDSO gettimeofday to use a new field in the VDSO datapage which stores the nanoseconds part of the time as a fractional number of seconds in a 0.32 binary fraction format. (Or put another way, as a 32-bit number in units of 0.23283 ns.) This is convenient because we can use the mulhwu instruction to convert it to either microseconds or nanoseconds.

Since it turns out that computing the time of day using this new field is simpler than either using stamp_xsec (as gettimeofday does) or stamp_xtime.tv_nsec (as clock_gettime does), this converts both gettimeofday and clock_gettime to use the new field. The existing __do_get_tspec function is converted to use the new field and take a parameter in r7 that indicates the desired resolution, 1,000,000 for microseconds or 1,000,000,000 for nanoseconds. The __do_get_xsec function is then unused and is deleted.

The new algorithm is

	now = ((timebase - tb_orig_stamp) << 12) * tb_to_xs
		+ (stamp_xtime_seconds << 32) + stamp_sec_fraction

with 'now' in units of 2^-32 seconds. That is then converted to seconds and either microseconds or nanoseconds with

	seconds = now >> 32
	partseconds = ((now & 0xffffffff) * resolution) >> 32

The 32-bit VDSO code also makes a further simplification: it ignores the bottom 32 bits of the tb_to_xs value, which is a 0.64 format binary fraction. Doing so gets rid of 4 multiply instructions. Assuming a timebase frequency of 1GHz or less and an update interval of no more than 10ms, the upper 32 bits of tb_to_xs will be at least 4503599, so the error from ignoring the low 32 bits will be at most 2.2ns, which is more than an order of magnitude less than the time taken to do gettimeofday or clock_gettime on our fastest processors, so there is no possibility of seeing inconsistent values due to this.

This also moves update_gtod() down next to its only caller, and makes update_vsyscall use the time passed in via the wall_time argument rather than accessing xtime directly. At present, wall_time always points to xtime, but that could change in future.

---
 arch/powerpc/include/asm/vdso_datapage.h  |    2 +
 arch/powerpc/kernel/asm-offsets.c         |    1 +
 arch/powerpc/kernel/time.c                |   61 ++++++----
 arch/powerpc/kernel/vdso32/gettimeofday.S |  184 +++++++----------------------
 arch/powerpc/kernel/vdso64/gettimeofday.S |   88 +++-----------
 5 files changed, 99 insertions(+), 237 deletions(-)
arch/powerpc/include/asm/vdso_datapage.h +2 -0
index 13c2c28..08679c5 100644
@@ -85,6 +85,7 @@ struct vdso_data {
 	__s32 wtom_clock_sec;			/* Wall to monotonic clock */
 	__s32 wtom_clock_nsec;
 	struct timespec stamp_xtime;	/* xtime as at tb_orig_stamp */
+	__u32 stamp_sec_fraction;	/* fractional seconds of stamp_xtime */
    	__u32 syscall_map_64[SYSCALL_MAP_SIZE]; /* map of syscalls  */
    	__u32 syscall_map_32[SYSCALL_MAP_SIZE]; /* map of syscalls */
 };
@@ -105,6 +106,7 @@ struct vdso_data {
 	__s32 wtom_clock_sec;			/* Wall to monotonic clock */
 	__s32 wtom_clock_nsec;
 	struct timespec stamp_xtime;	/* xtime as at tb_orig_stamp */
+	__u32 stamp_sec_fraction;	/* fractional seconds of stamp_xtime */
    	__u32 syscall_map_32[SYSCALL_MAP_SIZE]; /* map of syscalls */
 	__u32 dcache_block_size;	/* L1 d-cache block size     */
 	__u32 icache_block_size;	/* L1 i-cache block size     */
arch/powerpc/kernel/asm-offsets.c +1 -0
index 496cc5b..acbbac6 100644
@@ -342,6 +342,7 @@ int main(void)
 	DEFINE(WTOM_CLOCK_SEC, offsetof(struct vdso_data, wtom_clock_sec));
 	DEFINE(WTOM_CLOCK_NSEC, offsetof(struct vdso_data, wtom_clock_nsec));
 	DEFINE(STAMP_XTIME, offsetof(struct vdso_data, stamp_xtime));
+	DEFINE(STAMP_SEC_FRAC, offsetof(struct vdso_data, stamp_sec_fraction));
 	DEFINE(CFG_ICACHE_BLOCKSZ, offsetof(struct vdso_data, icache_block_size));
 	DEFINE(CFG_DCACHE_BLOCKSZ, offsetof(struct vdso_data, dcache_block_size));
 	DEFINE(CFG_ICACHE_LOGBLOCKSZ, offsetof(struct vdso_data, icache_log_block_size));
arch/powerpc/kernel/time.c +34 -27

84 lines not on this page · raw message

arch/powerpc/kernel/vdso32/gettimeofday.S +42 -142

247 lines not on this page · raw message

arch/powerpc/kernel/vdso64/gettimeofday.S +20 -68

128 lines not on this page · raw message

Signature · 1 line
1.7.1
#1 lore raw

PATCH 2/2 powerpc: Clean up obsolete code relating to decrementer and timebase

Since the decrementer and timekeeping code was moved over to using the generic clockevents and timekeeping infrastructure, several variables and functions have been obsolete and effectively unused. This deletes them.

In particular, wakeup_decrementer() is no longer needed since the generic code reprograms the decrementer as part of the process of resuming the timekeeping code, which happens during sysdev resume. Thus the wakeup_decrementer calls in the suspend_enter methods for 52xx platforms have been removed. The call in the powermac cpu frequency change code has been replaced by set_dec(1), which will cause a timer interrupt as soon as interrupts are enabled, and the generic code will then reprogram the decrementer with the correct value.

This also simplifies the generic_suspend_en/disable_irqs functions and makes them static since they are not referenced outside time.c. The preempt_enable/disable calls are removed because the generic code has disabled all but the boot cpu at the point where these functions are called, so we can't be moved to another cpu.

---
 arch/powerpc/include/asm/machdep.h           |    3 -
 arch/powerpc/include/asm/time.h              |    7 --
 arch/powerpc/kernel/smp.c                    |    2 -
 arch/powerpc/kernel/time.c                   |  136 +-------------------------
 arch/powerpc/platforms/52xx/lite5200_pm.c    |    3 -
 arch/powerpc/platforms/52xx/mpc52xx_pm.c     |    3 -
 arch/powerpc/platforms/powermac/cpufreq_32.c |    8 +-
 7 files changed, 9 insertions(+), 153 deletions(-)
arch/powerpc/include/asm/machdep.h +0 -3
index 9f0fc9e..7716e68 100644
@@ -366,8 +366,5 @@ static inline void log_error(char *buf, unsigned int err_type, int fatal)
 #define machine_late_initcall(mach,fn)		__define_machine_initcall(mach,"7",fn,7)
 #define machine_late_initcall_sync(mach,fn)	__define_machine_initcall(mach,"7s",fn,7s)
 
-void generic_suspend_disable_irqs(void);
-void generic_suspend_enable_irqs(void);
-
 #endif /* __KERNEL__ */
 #endif /* _ASM_POWERPC_MACHDEP_H */
arch/powerpc/include/asm/time.h +0 -7
index 27ccb76..dc779df 100644
@@ -28,16 +28,12 @@
 extern unsigned long tb_ticks_per_jiffy;
 extern unsigned long tb_ticks_per_usec;
 extern unsigned long tb_ticks_per_sec;
-extern u64 tb_to_xs;
-extern unsigned      tb_to_us;
 
 struct rtc_time;
 extern void to_tm(int tim, struct rtc_time * tm);
 extern void GregorianDay(struct rtc_time *tm);
-extern time_t last_rtc_update;
 
 extern void generic_calibrate_decr(void);
-extern void wakeup_decrementer(void);
 extern void snapshot_timebase(void);
 
 extern void set_dec_cpu6(unsigned int val);
@@ -204,9 +200,6 @@ static inline unsigned long tb_ticks_since(unsigned long tstamp)
 extern u64 mulhdu(u64, u64);
 #endif
 
-extern void smp_space_timers(unsigned int);
-
-extern unsigned mulhwu_scale_factor(unsigned, unsigned);
 extern void div128_by_32(u64 dividend_high, u64 dividend_low,
 			 unsigned divisor, struct div_result *dr);
 
arch/powerpc/kernel/smp.c +0 -2
index 5c196d1..8764daa 100644
@@ -288,8 +288,6 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 			max_cpus = NR_CPUS;
 	else
 		max_cpus = 1;
- 
-	smp_space_timers(max_cpus);
 
 	for_each_possible_cpu(cpu)
 		if (cpu != boot_cpuid)
arch/powerpc/kernel/time.c +3 -133

230 lines not on this page · raw message

arch/powerpc/platforms/52xx/lite5200_pm.c +0 -3
index b5c753d..80234e5 100644
@@ -216,9 +216,6 @@ static int lite5200_pm_enter(suspend_state_t state)
 
 	lite5200_restore_regs();
 
-	/* restart jiffies */
-	wakeup_decrementer();
-
 	iounmap(mbar);
 	return 0;
 }
arch/powerpc/platforms/52xx/mpc52xx_pm.c +0 -3
index a55b0b6..30cc48d 100644
@@ -162,9 +162,6 @@ int mpc52xx_pm_enter(suspend_state_t state)
 	/* restore SRAM */
 	memcpy(sram, saved_sram, sram_size);
 
-	/* restart jiffies */
-	wakeup_decrementer();
-
 	/* reenable interrupts in PIC */
 	out_be32(&intr->main_mask, intr_main_mask);
 
arch/powerpc/platforms/powermac/cpufreq_32.c +6 -2
index 1e9eba1..415ca6d 100644
@@ -310,8 +310,12 @@ static int pmu_set_cpu_speed(int low_speed)
 	/* Restore low level PMU operations */
 	pmu_unlock();
 
-	/* Restore decrementer */
-	wakeup_decrementer();
+	/*
+	 * Restore decrementer; we'll take a decrementer interrupt
+	 * as soon as interrupts are re-enabled and the generic
+	 * clockevents code will reprogram it with the right value.
+	 */
+	set_dec(1);
 
 	/* Restore interrupts */
  	mpic_cpu_set_priority(pic_prio);
Signature · 1 line
1.7.1
#2 lore raw
In reply to Paul Mackerras

Here's the test program I used to verify that the VDSO gettimeofday and clock_gettime are giving correct results.

Paul.

/*
* Copyright 2010 Paul Mackerras <[email protected]>, IBM. *
* This program is free software; it may be used and redistributed
* under the terms of the GNU Public Licence, either version 2, or
* (at your option) any later version.
*/
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include <sys/time.h>
#include <sys/unistd.h>
#include <linux/auxvec.h>
#include <asm/unistd.h>

#define TVTIME(tv) ((tv).tv_sec * 1000000000ull + (tv).tv_usec * 1000ull)
#define TSTIME(ts) ((ts).tv_sec * 1000000000ull + (ts).tv_nsec)

main(int ac, char **av)

{
	struct timeval tv1, tv2;
	struct timespec ts1, ts2, s1, s2;
	int count;

	for (count = 0; count < 10000000; ++count) {
		gettimeofday(&tv1, NULL);
		clock_gettime(CLOCK_REALTIME, &ts1);
		syscall(__NR_clock_gettime, CLOCK_REALTIME, &s1);
		gettimeofday(&tv2, NULL);
		clock_gettime(CLOCK_REALTIME, &ts2);
		syscall(__NR_clock_gettime, CLOCK_REALTIME, &s2);
		if (TVTIME(tv1) > TSTIME(ts1) ||
		    TSTIME(ts1) > TSTIME(s1) ||
		    TSTIME(s1) - s1.tv_nsec % 1000 > TVTIME(tv2) ||
		    TVTIME(tv2) > TSTIME(ts2) ||
		    TSTIME(ts2) > TSTIME(s2)) {
			printf("ERROR:  %lld %lld %lld\n\t%lld %lld %lld\n",
			       TVTIME(tv1), TSTIME(ts1), TSTIME(s1),
			       TVTIME(tv2), TSTIME(ts2), TSTIME(s2));
			exit(1);
		}
	}
	exit(0);
}