Messages in this thread |  | | Date | Thu, 22 Aug 2002 15:23:57 +0000 | From | Gabriel Paubert <> | Subject | Re: [PATCH]: fix 32bits integer overflow in loops_per_jiffy calculation |
| |
Benjamin Herrenschmidt wrote: >>Well, first on sane archs which have an easily accessible, fixed >>frequency time counter, loops_per_jiffy should never have existed :-) >> >>Second, putting this code there means that one day somebody will >>inevitably try to use it outside of its domain of operation (like it >>happened for div64 a few months ago when I pointed out that it would not >>work for divisors above 65535 or so). > > > Well... it's clearly located inside kernel/cpufreq.c, so there is > little risk, though it may be worth a big bold comment
Hmm, in my experience people hardly ever read detailed comments even when they are well-written. Perhaps if you called the function imprecise_scale or coarse_scale, it might ring a bell.
Besides that functions should do one thing and do that *well*[1]. Well, I'm usually not too dogmatic, but this function breaks the second rule beyond what I find acceptable.
>>In this case a generic scaling function, while not a standard libgcc/C >>library feature has potentially more applications than this simple >>cpufreq approximation. But I don't see very much the need for scaling a >>long (64 bit on 64 bit archs) value, 32 bit would be sufficient. > > > Well... if you can write one, go on then ;) In my case, I'm happy > with Yoann implementation for cpufreq right now. Though I agree that > could ultimately be moved to arch code.
Ok, I'll give it a try this week-end (PPC, i386 and all 64 bit should archs should be trivial).
Gabriel.
[1] Documentation/CodingStyle, which also claims that functions should be short and *sweet*. Well, I found the patch far too bitter ;-).
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |