Messages in this thread | | | Date | Tue, 15 Sep 2015 07:20:49 +0200 | From | Ingo Molnar <> | Subject | Re: [RFC][PATCH 0/5] Fixes for abs() usage on 64bit values |
| |
* John Stultz <john.stultz@linaro.org> wrote:
> On Mon, Sep 14, 2015 at 6:49 PM, Tejun Heo <tj@kernel.org> wrote: > > Hello, > > > > On Mon, Sep 14, 2015 at 06:05:19PM -0700, John Stultz wrote: > >> As noted in include/linux/kernel.h: > >> "abs() should not be used for 64-bit types (s64, u64, long long) > >> - use abs64() for those." > >> > >> Unfortunately, there are quite a number of places where abs() > >> was used w/ 64bit values in the kernel, and the results are > >> then silently capped to 32-bit values on 32-bit systems. > > > > I don't get it. Why can't we just do the following? > > > > #define abs(x) \ > > ({ \ > > typeof(x) __x = (x); \ > > __x < 0 ? -__x : __x; \ > > }) > > > > Yea. The above make sense to me, but I suspect there's some very > subtle reason for the existing separated logic. > But I'd have to defer to akpm for hints on that.
On one hand there's a real cost from abs() bugs: the fact that abs() trims the high bits silently led to a (serious) timekeeping bug on 32-bit kernels, that was not found for almost 2 years:
2619d7e9c92d time: Fix timekeeping_freqadjust()'s incorrect use of abs() instead of abs64()
On the other hand, there's literally hundreds of abs() usages in the kernel - I think it would be a lot safer to just introduce a build time warning and migrate the few affected ones over to abs64() (i.e. what John has done), than to silently change semantics in an all-or-nothing fashion, even if arguably many (most?) of the 64-bit values passed to abs() are probably bugs.
This has another advantage: we'll see all the bugs that occured so far, and can judge their effect on a case by case basis. There's value in that kind of gradual approach as well.
Once we've gone through that fixing process (for 1-2 kernel releases) we could perhaps do the change and unify abs() and abs64(): users who really want 32-bit trimming in the future can do the cast explicitly.
Linus, any preferences?
Thanks,
Ingo
| |