From patchwork Mon Sep 14 23:24:23 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: John Stultz X-Patchwork-Id: 53604 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-la0-f69.google.com (mail-la0-f69.google.com [209.85.215.69]) by patches.linaro.org (Postfix) with ESMTPS id 034652056A for ; Mon, 14 Sep 2015 23:24:31 +0000 (UTC) Received: by lagj9 with SMTP id j9sf56709061lag.0 for ; Mon, 14 Sep 2015 16:24:29 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:mime-version:in-reply-to:references :date:message-id:subject:from:to:cc:content-type:sender:precedence :list-id:x-original-sender:x-original-authentication-results :mailing-list:list-post:list-help:list-archive:list-unsubscribe; bh=OkoitfYvy1ZRXJlJv8uNDGlBubuwar4svsxKeRWVWOI=; b=hzLLHmdouyRi1b8OT7bnpyP2DOXNMSZXEIkcWYbOwEk5Gz/HYiBuDZaaVOIIgzKAS0 /KL5BXEFdhhbcewykuTZXgd4C+yNnDTTeeNwAVhjrE/BHTGLfhlGyEeFjV7ihT12ayot 9D7lIvLmVL/C2s3YrmLxzzLNvcaG4IODWoNDkFBZyegXA98Qk73ctw9tE2TGWo7mh5HY mfrozCBoHtcRE1pscVM4qW5tc7T84FF/N1yWejoXZ8QDmXwFltHydGbtNWzIE5PYXqDL CYUbnfuY172jVwIDOa03voJeJ1+6Ux2HTIWZdu9cwk2T85MNw04klPKRaTI61Mt9hoDe xq2A== X-Gm-Message-State: ALoCoQkXs1uisV3P3Uh3GKj4JuX4QFUmVhJSNu5CcM2EF0f8E5ffMDI2+Q3g214Jg0pPeVDQ8+8q X-Received: by 10.180.81.165 with SMTP id b5mr103892wiy.1.1442273069538; Mon, 14 Sep 2015 16:24:29 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.25.130 with SMTP id c2ls360874lag.27.gmail; Mon, 14 Sep 2015 16:24:29 -0700 (PDT) X-Received: by 10.112.140.232 with SMTP id rj8mr16739473lbb.2.1442273069372; Mon, 14 Sep 2015 16:24:29 -0700 (PDT) Received: from mail-la0-f52.google.com (mail-la0-f52.google.com. [209.85.215.52]) by mx.google.com with ESMTPS id r5si11304142lbw.126.2015.09.14.16.24.29 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 14 Sep 2015 16:24:29 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.52 as permitted sender) client-ip=209.85.215.52; Received: by lagj9 with SMTP id j9so97801491lag.2 for ; Mon, 14 Sep 2015 16:24:29 -0700 (PDT) X-Received: by 10.152.5.170 with SMTP id t10mr2495177lat.112.1442273069183; Mon, 14 Sep 2015 16:24:29 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.59.35 with SMTP id w3csp1457473lbq; Mon, 14 Sep 2015 16:24:27 -0700 (PDT) X-Received: by 10.68.132.234 with SMTP id ox10mr37431926pbb.128.1442273067475; Mon, 14 Sep 2015 16:24:27 -0700 (PDT) Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id uw4si26747103pac.106.2015.09.14.16.24.26; Mon, 14 Sep 2015 16:24:27 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of stable-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751915AbbINXY0 (ORCPT + 1 other); Mon, 14 Sep 2015 19:24:26 -0400 Received: from mail-io0-f180.google.com ([209.85.223.180]:33877 "EHLO mail-io0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751772AbbINXYY (ORCPT ); Mon, 14 Sep 2015 19:24:24 -0400 Received: by iofb144 with SMTP id b144so184569890iof.1 for ; Mon, 14 Sep 2015 16:24:24 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.107.19.234 with SMTP id 103mr32004433iot.41.1442273064097; Mon, 14 Sep 2015 16:24:24 -0700 (PDT) Received: by 10.50.232.39 with HTTP; Mon, 14 Sep 2015 16:24:23 -0700 (PDT) In-Reply-To: <20150913083244.GA29934@gmail.com> References: <1441840051-20244-1-git-send-email-john.stultz@linaro.org> <20150913083244.GA29934@gmail.com> Date: Mon, 14 Sep 2015 16:24:23 -0700 Message-ID: Subject: Re: [PATCH 1/2] time: Fix timekeeping_freqadjust()'s incorrect use of abs() instead of abs64() From: John Stultz To: Ingo Molnar Cc: Linus Torvalds , Andrew Morton , Peter Zijlstra , Thomas Gleixner , LKML , =?UTF-8?Q?Nuno_Gon=C3=A7alves?= , Miroslav Lichvar , Prarit Bhargava , Richard Cochran , stable , Joe Perches Sender: stable-owner@vger.kernel.org Precedence: list List-ID: X-Mailing-List: stable@vger.kernel.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: john.stultz@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.52 as permitted sender) smtp.mailfrom=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , On Sun, Sep 13, 2015 at 1:32 AM, Ingo Molnar wrote: > > * John Stultz wrote: > >> The internal clocksteering done for fine-grained error correction >> uses a logarithmic approximation, so any time adjtimex() adjusts >> the clock steering, timekeeping_freqadjust() quickly approximates >> the correct clock frequency over a series of ticks. >> >> Unfortunately, the logic in timekeeping_freqadjust(), introduced >> in commit dc491596f639438 (Rework frequency adjustments to work >> better w/ nohz), used the abs() function with a s64 error value >> to calculate the size of the approximated adjustment to be made. >> >> Per include/linux/kernel.h: "abs() should not be used for 64-bit >> types (s64, u64, long long) - use abs64()". >> >> Thus on 32-bit platforms, this resulted in the clocksteering to >> take a quite dampended random walk trying to converge on the >> proper frequency, which caused the adjustments to be made much >> slower then intended (most easily observed when large adjustments >> are made). >> >> This patch fixes the issue by using abs64() instead. > >> /* Sort out the magnitude of the correction */ >> - tick_error = abs(tick_error); >> + tick_error = abs64(tick_error); > > Ugh, and we had this bug for almost two years! Well. I sat on the patch for quite awhile, so the author date isn't really representative. It was only included in mainline in 3.17, so its been in use for a little over a year. But yea, still. > Would it be possible to make abs() warn about 64-bit types during build time, > to prevent such mishaps? Yea. I was surprised this wasn't something the compiler would catch previously. So is BUILD_BUG_ON() the best option for this? Its catching a whole bunch of other related issues (frighteningly, more then Joe's cocci script). The down-side is BUILD_BUG_ON causes build errors, not warnings, and its output isn't super easy to parse on first view. Potential BUILD_BUG_ON patch attached. I'll also try to spin some patches to fix the issues this one catches. thanks -john diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 5582410..753be99 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -208,6 +209,9 @@ extern int _cond_resched(void); */ #define abs(x) ({ \ long ret; \ + BUILD_BUG_ON_MSG( \ + sizeof(typeof(x)) > sizeof(long), \ + "abs() should not be used for 64-bit types - use abs64()");\ if (sizeof(x) == sizeof(long)) { \ long __x = (x); \ ret = (__x < 0) ? -__x : __x; \