On 06/30/2016 05:37 PM, Nicolas Pitre wrote:

On Thu, 30 Jun 2016, Daniel Lezcano wrote:

[ ... ]

quoted

quoted

+	if (likely(nsec < DIV_APPROXIMATION_THRESHOLD)) {
+		u32 usec = nsec;
+
+		usec += usec >> 5;
+		usec = usec >> 10;
+
+		/* Can safely cast to int since usec is < INT_MAX */
+		return usec;
+	} else {
+		u64 usec = div_u64(nsec, 1000);
+
+		if (usec > INT_MAX)
+			usec = INT_MAX;
+
+		/* Can safely cast to int since usec is < INT_MAX */
+		return usec;
+	}
+}

What bothers me with this division is the benefit of adding an extra ultra
optimized division by 1000 in cpuidle.h while we have already ktime_divns
which is optimized in ktime.h.

It is "optimized" but still much heavier than what is presented above as
it provides maximum precision.

It all depends on how important the performance gain from the original
shift by 10 was in the first place.

Actually the original shift was there because it was convenient as a 
simple ~div1000 operation. But against all odds, the approximation 
introduced a regression on a very specific use case on PowerPC.

We are not in the hot path and I think we can live with a ktime_divns 
without problem. That would simplify the fix I believe.

Perhaps the div1000 routine could be moved in ktime.h to be used as a 
helper for ktime_divns when we divide by the 1000 constant and submitted 
in a separate patch as an optimization.

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog