Possible clocksource wrapping issues w/ new vdso clock_gettime() code?

Possible clocksource wrapping issues w/ new vdso clock_gettime() code?

[john stultz]

Hey Andi,
	I've not been able to review the new vdso code very carefully yet, but
I noticed one thing right off: the offset calculation is not masked, so
its possible w/ counters less then 64bits wide to have wrapping issues.


It seems something like the following would be needed.

diff --git a/arch/x86_64/vdso/vclock_gettime.c b/arch/x86_64/vdso/vclock_gettime.c
index 17f6a00..9570f8e 100644
--- a/arch/x86_64/vdso/vclock_gettime.c
+++ b/arch/x86_64/vdso/vclock_gettime.c
@@ -36,8 +36,8 @@ static inline long vgetns(void)
 {
 	cycles_t (*vread)(void);
 	vread = gtod->clock.vread;
-	return ((vread() - gtod->clock.cycle_last) * gtod->clock.mult) >>
-		gtod->clock.shift;
+	return (((vread() - gtod->clock.cycle_last) & gtod->clock.mask) *
+		 gtod->clock.mult) >> gtod->clock.shift;
 }
 
 static noinline int do_realtime(struct timespec *ts)



Or am I just missing something? 

thanks
-john

Re: Possible clocksource wrapping issues w/ new vdso clock_gettime() code?

[john stultz]

On Mon, 2007-07-23 at 10:59 -0700, john stultz wrote:
> Hey Andi,
> 	I've not been able to review the new vdso code very carefully yet, but
> I noticed one thing right off: the offset calculation is not masked, so
> its possible w/ counters less then 64bits wide to have wrapping issues.

Here's another minor cleanup. Looks like wall_time_nsec is being set
twice.

diff --git a/arch/x86_64/kernel/vsyscall.c b/arch/x86_64/kernel/vsyscall.c
index 06c3494..fc62608 100644
--- a/arch/x86_64/kernel/vsyscall.c
+++ b/arch/x86_64/kernel/vsyscall.c
@@ -80,7 +80,6 @@ void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
 	vsyscall_gtod_data.wall_time_sec = wall_time->tv_sec;
 	vsyscall_gtod_data.wall_time_nsec = wall_time->tv_nsec;
 	vsyscall_gtod_data.sys_tz = sys_tz;
-	vsyscall_gtod_data.wall_time_nsec = wall_time->tv_nsec;
 	vsyscall_gtod_data.wall_to_monotonic = wall_to_monotonic;
 	write_sequnlock_irqrestore(&vsyscall_gtod_data.lock, flags);
 }


Also, is there a reason why with the new __vdso_gettimeofday
implementation, the old do_vgettimeofday has not been removed?

thanks
-john