[Linux-ia64] [PATCH] settimeofday() not synchronised with gettimeofday()

[Linux-ia64] [PATCH] settimeofday() not synchronised with gettimeofday()

[Eric Piel]

[-- Attachment #1: Type: text/plain, Size: 1442 bytes --]

Hello,

On the last 2.5 kernels the time we get with gettimeofday() is different
from the time we set with settimeofday().
I've written a small test case which should demonstrate that bug. It
simply sets the time to the current time and then reads a second time
the time. The difference between the two values should be only the
length of the syscalls.

Basically on a 2.4.19 I get this kind of result:

# ./a.out
requested:      1047481209s 671081ns
new:            1047481209s 671083ns
diff is  0.000002000sec

Fine.
But on a 2.5.64 I have something like that:
# ./a.out 
requested:      1047572128s 2564ns
new:            1047572127s 0239ns
diff is -1.232526000sec

gettimeofday() gave a time BEFORE the time we set!
First, I've corrected an obvious problem due to the change of the
returned value of gettimeoffset() from usec to nsec in settimeofday().
David, I think you can apply it, at least :-)

However, now, it still gives negative difference:
# ./a.out 
requested:      1047572128s 2564ns
new:            1047572128s 1588ns
diff is -0.000976000sec

That's better but there is still something...
Can anyone reproduce this bug? Any idea about what may cause this
shifted results?

I don't understand what does the line in settimeofday():
	nsec -= (jiffies - wall_jiffies ) * (1000000000 / HZ);
Maybe there is some error there. Removing it gives positive difference
but too big to look correct!

Any suggestion would be welcomed.
Eric

[-- Attachment #2: bad_settimeofday.c --]
[-- Type: text/plain, Size: 870 bytes --]

/* should detect a problem in settimeofday 
 * we set the time to the time we've just got
 * then we read the time, we should obtain a time just little bit after what we set
 * You need to be root to run this test
 */ 

#include <stdio.h>
#include <sys/time.h>
#include <signal.h>

#define USEC_PER_SEC	1000000
#define timerdiff(a,b) ((float)((a)->tv_sec - (b)->tv_sec) + \
                         (float)((a)->tv_usec - (b)->tv_usec)/USEC_PER_SEC)

main()
{
	struct timeval treq, tnew;
	float diff;

//	raise(SIGSTOP);
	/* do the test */
	gettimeofday(&treq, NULL);
	settimeofday(&treq, NULL);
	gettimeofday(&tnew, NULL);

	/* interpret the result */
	diff = timerdiff(&tnew, &treq);
	printf("requested:\t%lds %ldns\n"
               "new:\t\t%lds %ldns\n"	
               "diff is %12.9fsec\n",
		treq.tv_sec, treq.tv_usec,
		tnew.tv_sec, tnew.tv_usec,
		diff);



}

[-- Attachment #3: settimeofday-ia64-2.5.64-030313.patch --]
[-- Type: text/plain, Size: 1312 bytes --]

--- ../../../../linux-2.5.64-ia64.orig/arch/ia64/kernel/time.c	2003-03-07 13:27:04.000000000 +0100
+++ ./time.c	2003-03-13 16:40:06.000000000 +0100
@@ -60,7 +60,7 @@
 }
 
 /*
- * Return the number of micro-seconds that elapsed since the last update to jiffy.  The
+ * Return the number of nano-seconds that elapsed since the last update to jiffy.  The
  * xtime_lock must be at least read-locked when calling this routine.
  */
 static inline unsigned long
@@ -86,6 +86,9 @@
 void
 do_settimeofday (struct timeval *tv)
 {
+	unsigned long sec = tv->tv_sec;
+	unsigned long nsec = tv->tv_usec * 1000;
+
 	write_seqlock_irq(&xtime_lock);
 	{
 		/*
@@ -94,16 +97,16 @@
 		 * Discover what correction gettimeofday would have done, and then undo
 		 * it!
 		 */
-		tv->tv_usec -= gettimeoffset();
-		tv->tv_usec -= (jiffies - wall_jiffies) * (1000000 / HZ);
+		nsec -= gettimeoffset();
+		nsec -= (jiffies - wall_jiffies ) * (1000000000 / HZ);
 
-		while (tv->tv_usec < 0) {
-			tv->tv_usec += 1000000;
-			tv->tv_sec--;
+		while (nsec < 0) {
+			nsec += 1000000000;
+			sec--;
 		}
 
-		xtime.tv_sec = tv->tv_sec;
-		xtime.tv_nsec = 1000 * tv->tv_usec;
+		xtime.tv_sec = sec;
+		xtime.tv_nsec = nsec;
 		time_adjust = 0;		/* stop active adjtime() */
 		time_status |= STA_UNSYNC;
 		time_maxerror = NTP_PHASE_LIMIT;

Re: [Linux-ia64] [PATCH] settimeofday() not synchronised with

[Eric Piel]

[-- Attachment #1: Type: text/plain, Size: 1074 bytes --]

Eric Piel wrote:
> However, now, it still gives negative difference:
> # ./a.out
> requested:      1047572128s 2564ns
> new:            1047572128s 1588ns
> diff is -0.000976000sec
> 
> That's better but there is still something...
> Can anyone reproduce this bug? Any idea about what may cause this
> shifted results?
> 
> I don't understand what does the line in settimeofday():
>         nsec -= (jiffies - wall_jiffies ) * (1000000000 / HZ);

Finally I read the code to do the same thing for i386 (get and
settimeofday()). This explains the meaning of this line, in the i386
it's associated with the equivalent line in do_gettimeofday()! On ia64
everything is done inside of gettimeoffset(). Therefore I'm now
confident that suppressing this line is a Good Thing ;-) The patch doing
it wrt the bk tree is attached.

The test case confirms that it works:
requested:      1048681051s 194873ns
new:            1048681051s 194874ns
diff is  0.000001000sec

That's the same result than on a 2.4.19 . 
This also solved an error on the high resolution timers test suite.

	Eric

[-- Attachment #2: settimeofday2-ia64-2.5.64-030326.patch --]
[-- Type: text/plain, Size: 312 bytes --]

--- arch/ia64/kernel/time.c.back	2003-03-26 11:47:42.000000000 +0100
+++ arch/ia64/kernel/time.c	2003-03-26 11:48:05.000000000 +0100
@@ -98,7 +98,6 @@
 		 * it!
 		 */
 		nsec -= gettimeoffset();
-		nsec -= (jiffies - wall_jiffies ) * (1000000000 / HZ);
 
 		while (unlikely(nsec < 0)) {
 			nsec += 1000000000;

Re: [Linux-ia64] [PATCH] settimeofday() not synchronised with

[David Mosberger]

>>>>> On Wed, 26 Mar 2003 13:39:25 +0100, Eric Piel <Eric.Piel@Bull.Net> said:

  >> I don't understand what does the line in settimeofday(): nsec -  >> (jiffies - wall_jiffies ) * (1000000000 / HZ);

  Eric> Finally I read the code to do the same thing for i386 (get and
  Eric> settimeofday()). This explains the meaning of this line, in
  Eric> the i386 it's associated with the equivalent line in
  Eric> do_gettimeofday()! On ia64 everything is done inside of
  Eric> gettimeoffset(). Therefore I'm now confident that suppressing
  Eric> this line is a Good Thing ;-) The patch doing it wrt the bk
  Eric> tree is attached.

Good catch.  I applied this patch.

Thanks,

	--david