[RFC][PATCH] NTP shiftR cleanup

[RFC][PATCH] NTP shiftR cleanup

[john stultz]

All,
	Here is hopefully a fairly uncontroversial cleanup for the NTP code. It
uses min/max to simplify some conditionals and creates a macro shiftR
that avoids the numerous ugly conditionals in the NTP code that look
like:
	
	if(a < 0)
		b = -(-a >> shift);
	else
		b = a >> shift;

Replacing it with:

	b = shiftR(a, shift);

This should have zero effect on the logic, however it should probably
have a bit of testing just to be sure. I've compiled it for alpha, arm,
i386, ppc, ppc64, and s390 and its been running fine on my laptop all
day. It applies against Linus' tree from this morning.

Also Thanks to Ingo Oeser for catching a bug earlier in the shiftR
macro.

Please let me know if you have any comments or feedback.


thanks
-john


diff --git a/include/linux/timex.h b/include/linux/timex.h
--- a/include/linux/timex.h
+++ b/include/linux/timex.h
@@ -282,6 +282,11 @@ static inline int ntp_synced(void)
 	return !(time_status & STA_UNSYNC);
 }
 
+/* Required to safely shift negative values */
+#define shiftR(x, s) ({ __typeof__(x) __x = x;\
+		__typeof__(s) __s = s; \
+		(__x < 0) ? (-((-__x) >> (__s))) : ((__x) >> (__s));})
+
 
 #ifdef CONFIG_TIME_INTERPOLATION
 
diff --git a/kernel/time.c b/kernel/time.c
--- a/kernel/time.c
+++ b/kernel/time.c
@@ -338,30 +338,20 @@ int do_adjtimex(struct timex *txc)
 		        if (mtemp >= MINSEC) {
 			    ltemp = (time_offset / mtemp) << (SHIFT_USEC -
 							      SHIFT_UPDATE);
-			    if (ltemp < 0)
-			        time_freq -= -ltemp >> SHIFT_KH;
-			    else
-			        time_freq += ltemp >> SHIFT_KH;
+			    time_freq = shiftR(ltemp, SHIFT_KH);
 			} else /* calibration interval too short (p. 12) */
 				result = TIME_ERROR;
 		    } else {	/* PLL mode */
 		        if (mtemp < MAXSEC) {
 			    ltemp *= mtemp;
-			    if (ltemp < 0)
-			        time_freq -= -ltemp >> (time_constant +
-							time_constant +
-							SHIFT_KF - SHIFT_USEC);
-			    else
-			        time_freq += ltemp >> (time_constant +
+			    time_freq += shiftR(ltemp,(time_constant +
 						       time_constant +
-						       SHIFT_KF - SHIFT_USEC);
+						       SHIFT_KF - SHIFT_USEC));
 			} else /* calibration interval too long (p. 12) */
 				result = TIME_ERROR;
 		    }
-		    if (time_freq > time_tolerance)
-		        time_freq = time_tolerance;
-		    else if (time_freq < -time_tolerance)
-		        time_freq = -time_tolerance;
+		    time_freq = min(time_freq, time_tolerance);
+		    time_freq = max(time_freq, -time_tolerance);
 		} /* STA_PLL || STA_PPSTIME */
 	    } /* txc->modes & ADJ_OFFSET */
 	    if (txc->modes & ADJ_TICK) {
@@ -384,10 +374,7 @@ leave:	if ((time_status & (STA_UNSYNC|ST
 	if ((txc->modes & ADJ_OFFSET_SINGLESHOT) == ADJ_OFFSET_SINGLESHOT)
 	    txc->offset	   = save_adjust;
 	else {
-	    if (time_offset < 0)
-		txc->offset = -(-time_offset >> SHIFT_UPDATE);
-	    else
-		txc->offset = time_offset >> SHIFT_UPDATE;
+	    txc->offset = shiftR(time_offset, SHIFT_UPDATE);
 	}
 	txc->freq	   = time_freq + pps_freq;
 	txc->maxerror	   = time_maxerror;
diff --git a/kernel/timer.c b/kernel/timer.c
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -703,23 +703,13 @@ static void second_overflow(void)
      * the adjustment over not more than the number of
      * seconds between updates.
      */
-    if (time_offset < 0) {
-	ltemp = -time_offset;
-	if (!(time_status & STA_FLL))
-	    ltemp >>= SHIFT_KG + time_constant;
-	if (ltemp > (MAXPHASE / MINSEC) << SHIFT_UPDATE)
-	    ltemp = (MAXPHASE / MINSEC) << SHIFT_UPDATE;
-	time_offset += ltemp;
-	time_adj = -ltemp << (SHIFT_SCALE - SHIFT_HZ - SHIFT_UPDATE);
-    } else {
 	ltemp = time_offset;
 	if (!(time_status & STA_FLL))
-	    ltemp >>= SHIFT_KG + time_constant;
+		ltemp = shiftR(ltemp, SHIFT_KG + time_constant);
 	if (ltemp > (MAXPHASE / MINSEC) << SHIFT_UPDATE)
-	    ltemp = (MAXPHASE / MINSEC) << SHIFT_UPDATE;
+		ltemp = (MAXPHASE / MINSEC) << SHIFT_UPDATE;
 	time_offset -= ltemp;
 	time_adj = ltemp << (SHIFT_SCALE - SHIFT_HZ - SHIFT_UPDATE);
-    }
 
     /*
      * Compute the frequency estimate and additional phase
@@ -736,30 +726,19 @@ static void second_overflow(void)
 			 STA_PPSWANDER | STA_PPSERROR);
     }
     ltemp = time_freq + pps_freq;
-    if (ltemp < 0)
-	time_adj -= -ltemp >>
-	    (SHIFT_USEC + SHIFT_HZ - SHIFT_SCALE);
-    else
-	time_adj += ltemp >>
-	    (SHIFT_USEC + SHIFT_HZ - SHIFT_SCALE);
+    time_adj += shiftR(ltemp,(SHIFT_USEC + SHIFT_HZ - SHIFT_SCALE));
 
 #if HZ == 100
     /* Compensate for (HZ==100) != (1 << SHIFT_HZ).
      * Add 25% and 3.125% to get 128.125; => only 0.125% error (p. 14)
      */
-    if (time_adj < 0)
-	time_adj -= (-time_adj >> 2) + (-time_adj >> 5);
-    else
-	time_adj += (time_adj >> 2) + (time_adj >> 5);
+    time_adj += shiftR(time_adj, 2) + shiftR(time_adj, 5);
 #endif
 #if HZ == 1000
     /* Compensate for (HZ==1000) != (1 << SHIFT_HZ).
      * Add 1.5625% and 0.78125% to get 1023.4375; => only 0.05% error (p. 14)
      */
-    if (time_adj < 0)
-	time_adj -= (-time_adj >> 6) + (-time_adj >> 7);
-    else
-	time_adj += (time_adj >> 6) + (time_adj >> 7);
+    time_adj += shiftR(time_adj, 6) + shiftR(time_adj, 7);
 #endif
 }
 
@@ -778,10 +757,8 @@ static void update_wall_time_one_tick(vo
 	     * Limit the amount of the step to be in the range
 	     * -tickadj .. +tickadj
 	     */
-	     if (time_adjust > tickadj)
-		time_adjust_step = tickadj;
-	     else if (time_adjust < -tickadj)
-		time_adjust_step = -tickadj;
+	     time_adjust_step = min(time_adjust_step, (long)tickadj);
+	     time_adjust_step = max(time_adjust_step, (long)-tickadj);
 
 	    /* Reduce by this step the amount of time left  */
 	    time_adjust -= time_adjust_step;
@@ -792,13 +769,8 @@ static void update_wall_time_one_tick(vo
 	 * advance the tick more.
 	 */
 	time_phase += time_adj;
-	if (time_phase <= -FINENSEC) {
-		long ltemp = -time_phase >> (SHIFT_SCALE - 10);
-		time_phase += ltemp << (SHIFT_SCALE - 10);
-		delta_nsec -= ltemp;
-	}
-	else if (time_phase >= FINENSEC) {
-		long ltemp = time_phase >> (SHIFT_SCALE - 10);
+	if (abs(time_phase) >= FINENSEC) {
+		long ltemp = shiftR(time_phase, (SHIFT_SCALE - 10));
 		time_phase -= ltemp << (SHIFT_SCALE - 10);
 		delta_nsec += ltemp;
 	}

Re: [RFC][PATCH] NTP shiftR cleanup

[YOSHIFUJI Hideaki / 吉藤英明]

In article <1126314217.3455.10.camel@cog.beaverton.ibm.com> (at Fri, 09 Sep 2005 18:03:37 -0700), john stultz <johnstul@us.ibm.com> says:

> diff --git a/kernel/time.c b/kernel/time.c
> --- a/kernel/time.c
> +++ b/kernel/time.c
> @@ -338,30 +338,20 @@ int do_adjtimex(struct timex *txc)
>  		        if (mtemp >= MINSEC) {
>  			    ltemp = (time_offset / mtemp) << (SHIFT_USEC -
>  							      SHIFT_UPDATE);
> -			    if (ltemp < 0)
> -			        time_freq -= -ltemp >> SHIFT_KH;
> -			    else
> -			        time_freq += ltemp >> SHIFT_KH;
> +			    time_freq = shiftR(ltemp, SHIFT_KH);
>  			} else /* calibration interval too short (p. 12) */
>  				result = TIME_ERROR;

Maybe, "time_freq += shiftR(ltemp, SHIFT_KH);"?

--yoshfuji

Re: [RFC][PATCH] NTP shiftR cleanup

[Roman Zippel]

Hi,

On Fri, 9 Sep 2005, john stultz wrote:

> +/* Required to safely shift negative values */
> +#define shiftR(x, s) ({ __typeof__(x) __x = x;\
> +		__typeof__(s) __s = s; \
> +		(__x < 0) ? (-((-__x) >> (__s))) : ((__x) >> (__s));})
> +

Some parenthesis are missing and some are redundant. Formatting it so it 
looks more like normal function makes it more readable:

#define shiftR(x, s) ({				\
	__typeof__(x) __x = (x);		\
	__typeof__(s) __s = (s);		\
	__x < 0 ? -(-__x >> __s) : __x >> __s;	\
})

> @@ -792,13 +769,8 @@ static void update_wall_time_one_tick(vo
>  	 * advance the tick more.
>  	 */
>  	time_phase += time_adj;
> -	if (time_phase <= -FINENSEC) {
> -		long ltemp = -time_phase >> (SHIFT_SCALE - 10);
> -		time_phase += ltemp << (SHIFT_SCALE - 10);
> -		delta_nsec -= ltemp;
> -	}
> -	else if (time_phase >= FINENSEC) {
> -		long ltemp = time_phase >> (SHIFT_SCALE - 10);
> +	if (abs(time_phase) >= FINENSEC) {
> +		long ltemp = shiftR(time_phase, (SHIFT_SCALE - 10));
>  		time_phase -= ltemp << (SHIFT_SCALE - 10);
>  		delta_nsec += ltemp;
>  	}

It would be interesting to check, whether gcc produces the same code here.

bye, Roman