From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752191AbbINXY0 (ORCPT ); Mon, 14 Sep 2015 19:24:26 -0400 Received: from mail-io0-f175.google.com ([209.85.223.175]:32780 "EHLO mail-io0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751667AbbINXYY (ORCPT ); Mon, 14 Sep 2015 19:24:24 -0400 MIME-Version: 1.0 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 Content-Type: multipart/mixed; boundary=001a113f7a92a7f1ec051fbd5dd6 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --001a113f7a92a7f1ec051fbd5dd6 Content-Type: text/plain; charset=UTF-8 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 --001a113f7a92a7f1ec051fbd5dd6 Content-Type: text/x-patch; charset=US-ASCII; name="abs-build-bug.patch" Content-Disposition: attachment; filename="abs-build-bug.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_iekk1xl20 ZGlmZiAtLWdpdCBhL2luY2x1ZGUvbGludXgva2VybmVsLmggYi9pbmNsdWRlL2xpbnV4L2tlcm5l bC5oCmluZGV4IDU1ODI0MTAuLjc1M2JlOTkgMTAwNjQ0Ci0tLSBhL2luY2x1ZGUvbGludXgva2Vy bmVsLmgKKysrIGIvaW5jbHVkZS9saW51eC9rZXJuZWwuaApAQCAtOCw2ICs4LDcgQEAKICNpbmNs dWRlIDxsaW51eC90eXBlcy5oPgogI2luY2x1ZGUgPGxpbnV4L2NvbXBpbGVyLmg+CiAjaW5jbHVk ZSA8bGludXgvYml0b3BzLmg+CisjaW5jbHVkZSA8bGludXgvYnVnLmg+CiAjaW5jbHVkZSA8bGlu dXgvbG9nMi5oPgogI2luY2x1ZGUgPGxpbnV4L3R5cGVjaGVjay5oPgogI2luY2x1ZGUgPGxpbnV4 L3ByaW50ay5oPgpAQCAtMjA4LDYgKzIwOSw5IEBAIGV4dGVybiBpbnQgX2NvbmRfcmVzY2hlZCh2 b2lkKTsKICAqLwogI2RlZmluZSBhYnMoeCkgKHsJCQkJCQlcCiAJCWxvbmcgcmV0OwkJCQkJXAor CQlCVUlMRF9CVUdfT05fTVNHKAkJCQlcCisJCQlzaXplb2YodHlwZW9mKHgpKSA+IHNpemVvZihs b25nKSwJXAorCQkJImFicygpIHNob3VsZCBub3QgYmUgdXNlZCBmb3IgNjQtYml0IHR5cGVzIC0g dXNlIGFiczY0KCkiKTtcCiAJCWlmIChzaXplb2YoeCkgPT0gc2l6ZW9mKGxvbmcpKSB7CQlcCiAJ CQlsb25nIF9feCA9ICh4KTsJCQkJXAogCQkJcmV0ID0gKF9feCA8IDApID8gLV9feCA6IF9feDsJ CVwK --001a113f7a92a7f1ec051fbd5dd6--