On Fri, 2015-09-04 at 18:00 -0700, John Stultz wrote:
> On Fri, Sep 4, 2015 at 5:57 PM, John Stultz <john.stultz@xxxxxxxxxx> wrote:
> > On Thu, Sep 3, 2015 at 4:26 AM, Miroslav Lichvar <mlichvar@xxxxxxxxxx> 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 <nunojpg@xxxxxxxxx> 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) {
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html