Re: [RFC] possible erronous use of tick_usec in do_gettimeofday

[George Anzinger <george@mvista.com>]

Joe Korty wrote:
> test10's version of do_gettimeofday is using tick_usec which is
> defined in terms of USER_HZ not HZ.
> 
> Against 2.6.0-test10-bk1.  Compiled, not tested, for comment only.

We still have the problem that we are doing this calculation in usecs while the 
wall clock uses nsecs.  This would be fine if there were an even number of usecs 
in tick_nsec, but in fact it is somewhat less than (USEC_PER_SEC / HZ).  This 
means that this correction (if we are behind by 7 or more ticks) will push the 
clock past current time.  Here are the numbers:

tick_nsec =999849 or 1ms less 151 ns.  So if we are behind 7 or more ticks we 
will report the time out 1 us too high.  (7 * 151 = 1057 or 1.057 usec).

Question is, do we care?  Will we ever be 7ms late in updating the wall clock? 
As I recall, the wall clock is updated in the interrupt handler for the tick so, 
to be this late, we would need to suffer a long interrupt hold off AND the tick 
recovery code would need to have done its thing.  But this whole time is covered 
by a write_seqlock on xtime_lock, so how can this even happen?  Seems like it is 
only possible when we are locked and we then throw the whole thing away.

A test I would like to see is to put this in the code AFTER the read unlock:

if (lost )
	printk("Lost is %d\n", lost);

(need to pull "	unsigned long lost;" out of the do{}while loop to do this)

In short, I think we are beating a dead issue.

-g
> 
> Joe
> 
> --- base/arch/i386/kernel/time.c	2003-11-23 20:31:55.000000000 -0500
> +++ new/arch/i386/kernel/time.c	2003-11-25 11:22:38.000000000 -0500
> @@ -94,7 +94,7 @@
>  {
>  	unsigned long seq;
>  	unsigned long usec, sec;
> -	unsigned long max_ntp_tick = tick_usec - tickadj;
> +	unsigned long max_ntp_tick;
>  
>  	do {
>  		unsigned long lost;
> @@ -110,13 +110,14 @@
>  		 * Better to lose some accuracy than have time go backwards..
>  		 */
>  		if (unlikely(time_adjust < 0)) {
> +			max_ntp_tick = (USEC_PER_SEC / HZ) - tickadj;
>  			usec = min(usec, max_ntp_tick);
>  
>  			if (lost)
>  				usec += lost * max_ntp_tick;
>  		}
>  		else if (unlikely(lost))
> -			usec += lost * tick_usec;
> +			usec += lost * (USEC_PER_SEC / HZ);
>  
>  		sec = xtime.tv_sec;
>  		usec += (xtime.tv_nsec / 1000);
> 
> 

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml