From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751955AbbIOFUz (ORCPT ); Tue, 15 Sep 2015 01:20:55 -0400 Received: from mail-wi0-f170.google.com ([209.85.212.170]:38223 "EHLO mail-wi0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751601AbbIOFUy (ORCPT ); Tue, 15 Sep 2015 01:20:54 -0400 Date: Tue, 15 Sep 2015 07:20:49 +0200 From: Ingo Molnar To: John Stultz , Linus Torvalds Cc: Tejun Heo , LKML , Andrew Morton , "Steven Rostedt (Red Hat)" , Peter Zijlstra , Masami Hiramatsu , Michal Nazarewicz , Prarit Bhargava , Richard Cochran , Thomas Gleixner , "Theodore Ts'o" , Andreas Dilger , Dave Chinner , Peter Zijlstra Subject: Re: [RFC][PATCH 0/5] Fixes for abs() usage on 64bit values Message-ID: <20150915052049.GA14215@gmail.com> References: <1442279124-7309-1-git-send-email-john.stultz@linaro.org> <20150915014936.GA25658@htj.duckdns.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * John Stultz wrote: > On Mon, Sep 14, 2015 at 6:49 PM, Tejun Heo 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