From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759790AbbIEBjz (ORCPT ); Fri, 4 Sep 2015 21:39:55 -0400 Received: from smtprelay0184.hostedemail.com ([216.40.44.184]:44832 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1756270AbbIEBjo (ORCPT ); Fri, 4 Sep 2015 21:39:44 -0400 X-Session-Marker: 6A6F6540706572636865732E636F6D X-Spam-Summary: 2,0,0,,d41d8cd98f00b204,joe@perches.com,:::::::::::::::::::::::::::::::,RULES_HIT:2:41:355:379:541:800:960:973:982:988:989:1260:1277:1311:1313:1314:1345:1359:1373:1437:1515:1516:1518:1535:1593:1594:1605:1606:1730:1747:1777:1792:2194:2198:2199:2200:2393:2553:2559:2562:2690:2693:2731:2741:2828:2899:3138:3139:3140:3141:3142:3622:3653:3834:3865:3866:3867:3868:3870:3871:3872:3873:3874:4118:4250:4321:5007:6117:6119:6261:7514:7875:7903:7904:9036:10004:10848:11026:11232:11473:11657:11658:11914:12043:12048:12296:12438:12517:12519:12555:12663:12740:13255:13894:14096:14097:21080,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:fn,MSBL:0,DNSBL:none,Custom_rules:0:0:0 X-HE-Tag: shelf29_1a4123ec95842 X-Filterd-Recvd-Size: 7425 Message-ID: <1441417179.28194.35.camel@perches.com> Subject: defects for uses of abs(u64) (was: Re: Regression: can't apply frequency offsets above 1000ppm) From: Joe Perches To: John Stultz , Dmitry Torokhov , Neil Brown , Daniel Mack , Haojian Zhuang , Robert Jarzmik , Mark Brown , Jaegeuk Kim , Changman Lee , Chao Yu Cc: Miroslav Lichvar , Nuno =?ISO-8859-1?Q?Gon=E7alves?= , Thomas Gleixner , LKML , =?ISO-8859-1?Q?G=FCnter_K=F6llner?= , stable Date: Fri, 04 Sep 2015 18:39:39 -0700 In-Reply-To: References: <20150903112647.GD29274@localhost> Content-Type: text/plain; charset="ISO-8859-1" X-Mailer: Evolution 3.12.11-0ubuntu3 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 2015-09-04 at 18:00 -0700, John Stultz wrote: > On Fri, Sep 4, 2015 at 5:57 PM, John Stultz wrote: > > 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çalves 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 = abs(tick_error); > > > > Haven't chased down why yet, but apparently abs() isn't doing what one > > would think when passed a s64 value. > > 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. > */ > > Ouch. Here's a little cocci script that finds more of these in: lib/percpu_counter.c drivers/input/joystick/walkera0701.c drivers/md/raid5.c drivers/spi/spi-pxa2xx.c fs/f2fs/debug.c $ cat abs.cocci @@ u64 t; @@ * abs(t) @@ s64 t; @@ * abs(t) @@ long long t; @@ * abs(t) @@ unsigned long long t; @@ * abs(t) @@ uint64_t t; @@ * abs(t) @@ int64_t t; @@ * abs(t) $ diff -u -p ./lib/percpu_counter.c /tmp/nothing/lib/percpu_counter.c --- ./lib/percpu_counter.c +++ /tmp/nothing/lib/percpu_counter.c @@ -203,7 +203,6 @@ int __percpu_counter_compare(struct perc count = percpu_counter_read(fbc); /* Check to see if rough count will be sufficient for comparison */ - if (abs(count - rhs) > (batch * num_online_cpus())) { if (count > rhs) return 1; else diff -u -p ./drivers/input/joystick/walkera0701.c /tmp/nothing/drivers/input/joystick/walkera0701.c --- ./drivers/input/joystick/walkera0701.c +++ /tmp/nothing/drivers/input/joystick/walkera0701.c @@ -150,7 +150,6 @@ static void walkera0701_irq_handler(void if (w->counter == 24) { /* full frame */ walkera0701_parse_frame(w); w->counter = NO_SYNC; - if (abs(pulse_time - SYNC_PULSE) < RESERVE) /* new frame sync */ w->counter = 0; } else { if ((pulse_time > (ANALOG_MIN_PULSE - RESERVE) @@ -161,7 +160,6 @@ static void walkera0701_irq_handler(void } else w->counter = NO_SYNC; } - } else if (abs(pulse_time - SYNC_PULSE - BIN0_PULSE) < RESERVE + BIN1_PULSE - BIN0_PULSE) /* frame sync .. */ w->counter = 0; diff -u -p ./drivers/md/raid5.c /tmp/nothing/drivers/md/raid5.c --- ./drivers/md/raid5.c +++ /tmp/nothing/drivers/md/raid5.c @@ -6701,8 +6701,6 @@ static int run(struct mddev *mddev) * readonly mode so it can take control before * allowing any writes. So just check for that. */ - if (abs(min_offset_diff) >= mddev->chunk_sectors && - abs(min_offset_diff) >= mddev->new_chunk_sectors) /* not really in-place - so OK */; else if (mddev->ro == 0) { printk(KERN_ERR "md/raid:%s: in-place reshape " diff -u -p ./drivers/spi/spi-pxa2xx.c /tmp/nothing/drivers/spi/spi-pxa2xx.c --- ./drivers/spi/spi-pxa2xx.c +++ /tmp/nothing/drivers/spi/spi-pxa2xx.c @@ -786,7 +786,6 @@ static unsigned int quark_x1000_get_clk_ /* Get the remainder */ fssp = (u64)fref * m; do_div(fssp, 1 << 24); - r1 = abs(fssp - rate); /* Choose this one if it suits better */ if (r1 < r) { diff -u -p ./fs/f2fs/debug.c /tmp/nothing/fs/f2fs/debug.c --- ./fs/f2fs/debug.c +++ /tmp/nothing/fs/f2fs/debug.c @@ -109,7 +109,6 @@ static void update_sit_info(struct f2fs_ hblks_per_sec = blks_per_sec / 2; for (segno = 0; segno < MAIN_SEGS(sbi); segno += sbi->segs_per_sec) { vblocks = get_valid_blocks(sbi, segno, sbi->segs_per_sec); - dist = abs(vblocks - hblks_per_sec); bimodal += dist * dist; if (vblocks > 0 && vblocks < blks_per_sec) {