From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:55844 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752046AbbIWHac (ORCPT ); Wed, 23 Sep 2015 03:30:32 -0400 From: Neil Brown To: Joe Perches , John Stultz , Dmitry Torokhov , Daniel Mack , Haojian Zhuang , Robert Jarzmik , Mark Brown , Jaegeuk Kim , Changman Lee , Chao Yu Date: Wed, 23 Sep 2015 17:21:34 +1000 Cc: Miroslav Lichvar , Nuno =?utf-8?Q?Gon=C3=A7alves?= , Thomas Gleixner , LKML , =?utf-8?Q?G=C3=BCnter_K=C3=B6llner?= , stable Subject: Re: defects for uses of abs(u64) (was: Re: Regression: can't apply frequency offsets above 1000ppm) In-Reply-To: <1441417179.28194.35.camel@perches.com> References: <20150903112647.GD29274@localhost> <1441417179.28194.35.camel@perches.com> Message-ID: <87y4fxwqgx.fsf@notabene.neil.brown.name> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: stable-owner@vger.kernel.org List-ID: --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Joe Perches writes: > On Fri, 2015-09-04 at 18:00 -0700, John Stultz wrote: >> On Fri, Sep 4, 2015 at 5:57 PM, John Stultz wro= te: >> > On Thu, Sep 3, 2015 at 4:26 AM, Miroslav Lichvar = wrote: >> >> On Wed, Sep 02, 2015 at 04:16:00PM -0700, John Stultz wrote: >> >>> On Tue, Sep 1, 2015 at 6:14 PM, Nuno Gon=C3=A7alves wrote: >> >>> > And just installing chrony from the feeds. With any kernel from 3.= 17 >> >>> > you'll have wrong estimates at chronyc sourcestats. >> >>> >> >>> Wrong estimates? Could you be more specific about what the failure >> >>> you're seeing is here? The >> >>> >> >>> I installed the image above, which comes with a 4.1.6 kernel, and >> >>> chrony seems to have gotten my BBB into ~1ms sync w/ servers over the >> >>> internet fairly quickly (at least according to chronyc tracking). >> >> >> >> To see the bug with chronyd the initial offset shouldn't be very close >> >> to zero, so it's forced to correct the offset by adjusting the >> >> frequency in a larger step. >> >> >> >> I'm attaching a simple C program that prints the frequency offset >> >> as measured between the REALTIME and MONOTONIC_RAW clocks when the >> >> adjtimex tick is set to 9000. It should show values close to -100000 >> >> ppm and I suspect on the BBB it will be much smaller. >> > >> > So I spent some time on this late last night and this afternoon. >> > >> > It was a little odd because things don't seem totally broken, but >> > something isn't quite right. >> > >> > Digging around it seems the iterative logrithmic approximation done in >> > timekeeping_freqadjust() wasn't working right. Instead of making >> > smaller order alternating positive and negative adjustments, it was >> > doing strange growing adjustments for the same value that wern't large >> > enough to actually correct things very quickly. This made it much >> > slower to adapt to specified frequency values. >> > >> > The odd bit, is it seems to come down to: >> > tick_error =3D abs(tick_error); >> > >> > Haven't chased down why yet, but apparently abs() isn't doing what one >> > would think when passed a s64 value. >>=20 >> Well.. chasing it down wasn't hard.. from include/linux/kernel.h: >> /* >> * abs() handles unsigned and signed longs, ints, shorts and chars. For= all >> * input types abs() returns a signed long. >> * abs() should not be used for 64-bit types (s64, u64, long long) - use= abs64() >> * for those. >> */ >>=20 >> Ouch. > > Here's a little cocci script that finds more of these in: Thanks. Maybe we should also: diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 5582410727cb..aa7d69afdcac 100644 =2D-- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -208,6 +208,7 @@ extern int _cond_resched(void); */ #define abs(x) ({ \ long ret; \ + BUILD_BUG_ON(sizeof(x) > sizeof(long)); \ if (sizeof(x) =3D=3D sizeof(long)) { \ long __x =3D (x); \ ret =3D (__x < 0) ? -__x : __x; \ so that people won't make the same mistake again. That finds bugs in driver/md/raid10.c drivers/gpu/drm/radeon/radeon_display.c kernel/time/clocksource.c kernel/time/timekeeping.c fs/ext4/mballoc.c =20 that your cocci scripted missed. All "abs(x - y)". As sector_t can be 32bit and can be 64bit, I wonder if abs_sector() would be a good idea ... probably not. Thoughts? NeilBrown --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJWAlL+AAoJEDnsnt1WYoG5/eEQAKdtyRPKryODnbUto7kJv+1m ir0Skkcx2w72m+tTp6Z58wE9+QQAyvwJG/Jr05vtHNalabr5mrr6O5qGs65c/j8e KmLfsCby2MenBmOOxL9/2YeZr4gBdOtF+WePtt5sJ/ctOQ1+ZBLfPuDW1XIGJjwu VwZoQAmvlsE+g2xjV8GIHFzye+ERJ6t11Nmq3TlT1YOdNeoKTGH44xEnMd9F6Asv Q+/kk6Jf7fAttoeFwIsgbXbXn64c8QKbLkrJ23T8PVz1MJxYO1bkbc0RLToMUgh1 GC+6d8LWDvK5uJnY6yDg5AdP1lcAwMOfVSEtTMRoVZEecOXIqFwadMaXlo3ilJYt 3TQW7NhLHFmNjrQ8EurCFCNIm/qtB4tOM3U9nTKbq7lgz+yNzZR+zw1pzVJce3MS ezzVGc52l5Rrvinch+1d1cktInzUjfwngaAejEONqb7x2MFnaItawgqbbWsKXiE2 frH6RIhPjVa2aA9IFNOdQI6x+wjsTlrBIUsdQldO18ndPTgDkfThxitnsPLHzcdQ aRB/jQIp0epmHHhrcmQSKmSMtrka+KwkUbRVaqD0AcB9tfYK2wMNnGd6iIwixAjc NUNBRtJROx4NcXAUtrcgAzlNxxFkh0QZkraFA9D7VbrA19fakkz+tU9Y/kty+hpS he81wrgbeE0Uo3LTHbff =yFM4 -----END PGP SIGNATURE----- --=-=-=--