: fix 32bits integer overflow in loops_per_jiffy calculation
PATCH 1/1 : fix 32bits integer overflow in loops_per_jiffy calculation
Hi,
The "low_part * mult" multiplication of the old function may overflow a 32bits integer...
This patch both fix the overflow issue (tested with frequencies up to 20Ghz), and make the result of the function lose less precision.
Please apply,
Signature · 5 lines
Yoann Vandoorselaere, http://www.prelude-ids.org "Programming is a race between programmers, who try and make more and more idiot-proof software, and universe, which produces more and more remarkable idiots. Until now, universe leads the race" -- R. Cook
kernel/cpufreq.c +10 -8
@@ -78,14 +78,16 @@ static unsigned int cpufreq_*/ static unsigned long scale (unsigned long old,u_int div,u_int mult) { -unsigned long low_part, high_part; - - high_part = old / div; - low_part = (old % div) /100 ; - high_part *= mult; - low_part = low_part * mult / div; - -return high_part + low_part *100 ; +unsigned long val, carry =0 ; + + mult /=100 ; + div /=100 ; + val = old / div * mult; + + carry = old % div; + carry = carry * mult / div; + +return val + carry; }
Quoted 22 lines · Yoann Vandoorselaere
Hi,
The "
low_part* mult" multiplication of the old function may overflow a 32bits integer...This patch both fix the overflow issue (tested with frequencies up to 20Ghz), and make the result of the function lose less precision.
Please apply,
------------------------------------------------------------------------
kernel/cpufreq.c +0
-2@@ -78,14 +78,16 @@ static unsigned int cpufreq_*/ static unsigned long scale (unsigned long old,u_int div,u_int mult) { -unsigned long low_part, high_part; -
kernel/cpufreq.c static unsigned int cpufreq_ · line 84
Quoted 10 lines
kernel/cpufreq.c +4
-6- high_part = old / div; - low_part = (old % div) /100 ; - high_part *= mult; - low_part = low_part * mult / div; - -return high_part + low_part *100 ; +unsigned long val, carry =0 ; + + mult /=100 ; + div /=100 ;
if(abs(div)<100) div=0;
kernel/cpufreq.c static unsigned int cpufreq_ · line 85
Quoted 1 line
kernel/cpufreq.c +1
-0+ val = old / div * mult;
Now happily divide by zero.
kernel/cpufreq.c static unsigned int cpufreq_ · line 87
Quoted 2 lines
kernel/cpufreq.c +2
-0+ + carry = old % div;
Again.
kernel/cpufreq.c static unsigned int cpufreq_ · line 88
Quoted 1 line
kernel/cpufreq.c +1
-0+ carry = carry * mult / div;
Again.
kernel/cpufreq.c static unsigned int cpufreq_ · line 92
Quoted 4 lines
kernel/cpufreq.c +2
-0+ +return val + carry; }
And I can't see how it can be more precise, you divide the numerator and
denominator of the fraction by 100 and then proceed forgetting everything about the rest. Basically this looses about 7 bits of precision.
Now altogether I believe that such a function pertains to a per arch optimized routine.
Hi Gabriel !
Quoted 6 lines
if(abs(div)<100) div=0;
Quoted 1 line
+ val = old / div * mult;
Now happily divide by zero.
Quoted 12 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ + carry = old % div;Again.
Quoted 1 line
+ carry = carry * mult / div;
Again.
Quoted 3 lines
kernel/cpufreq.c +2
-0+ +return val + carry; }
None of the above can happen in the domain of application of this function. It's used to scale up/down the loops_per_jiffy value when scaling the CPU frequency. Anyway, the above isn't worse than the original function. Ideally, we would want 64 bits arithmetics, but we decided long ago not to bring the libcc support routines for that in the kernel.
Quoted 4 lines
And I can't see how it can be more precise, you divide the numerator and denominator of the fraction by 100 and then proceed forgetting everything about the rest. Basically this looses about 7 bits of precision.
Which is mostly ok for what we need. I think Yoann didn't mean it's more precise that what it replace, but rather more precise than his original implementation that divided by 1000 ;) Anyway, it's not significantly worse than what we had and won't overflow as easily which is all we want for this routine now.
Quoted 2 lines
Now altogether I believe that such a function pertains to a per arch optimized routine.
Maybe... though in the context of cpufreq, it may not make that much sense.
Ben.