Page MenuHomeFreeBSD

ping: Avoid reporting NaNs
ClosedPublic

Authored by jlduran_gmail.com on Oct 6 2023, 6:13 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 26, 6:12 PM
Unknown Object (File)
Fri, Apr 26, 6:31 AM
Unknown Object (File)
Fri, Apr 26, 5:58 AM
Unknown Object (File)
Fri, Apr 26, 5:57 AM
Unknown Object (File)
Fri, Apr 26, 1:59 AM
Unknown Object (File)
Thu, Apr 25, 10:44 PM
Unknown Object (File)
Sun, Apr 21, 12:15 AM
Unknown Object (File)
Fri, Apr 19, 3:23 AM

Details

Summary
Avoid calculating the square root of negative zero, which can easily
happen on certain architectures when computing the population standard
deviation with a sample size of one, e.g.,
0.01 - (0.1 * 0.1) = -0.000000.

Avoid returning a NaN by capping the minimum possible variance value to
zero (positive).

In the future, maybe skip reporting statistics at all for a single
sample.

Reported by:    Jenkins
Test Plan

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

If we were computing with real numbers, then it should be impossible for vari to ever be negative. And in fact, all of the failing tests use -t 1, which means that n will be 1, so vari should always be exactly zero. So the fact that you're seeing these errors must mean that we're hitting some kind of floating point round-off error. Your patch will work, but it begs the question: why print min/avg/max/stddev when there is only one datapoint? I think that we should only print those things when n > 1. In fact, the standard deviation isn't really meaningful when n == 2, but it probably wouldn't hurt to print it, if that makes the code more readable.

If we were computing with real numbers, then it should be impossible for vari to ever be negative. And in fact, all of the failing tests use -t 1, which means that n will be 1, so vari should always be exactly zero. So the fact that you're seeing these errors must mean that we're hitting some kind of floating point round-off error.

Precisely! There must be something that is only present on aarch64 causing this error. This proposed fix just sweeps it under the carpet.

Your patch will work, but it begs the question: why print min/avg/max/stddev when there is only one datapoint? I think that we should only print those things when n > 1. In fact, the standard deviation isn't really meaningful when n == 2, but it probably wouldn't hurt to print it, if that makes the code more readable.

I'll jot this suggestion on my list of things, thank you!

Approved, but please note in the commit message that this condition can only occur due to FPU rounding error.

This revision is now accepted and ready to land.Oct 6 2023, 8:38 PM

I'll investigate a bit further. I am able to reproduce it on my mac:

#include <stdio.h>
#include <math.h>

int
main(void) {
	double tsumsq;
	double num = 1;
	double avg = 0.147;

	tsumsq = avg * avg;

	printf("0 = %f\n", tsumsq / num - avg * avg);
}
0 = -0.000000

As you well pointed out, here is the fix from NetBSD:

https://github.com/NetBSD/src/commit/79b9ef47d29a719583422b3394753e47a7157443

I'll submit it with the unification of ping/ping6's statistics section (D39126).

jlduran_gmail.com edited the summary of this revision. (Show Details)
jlduran_gmail.com edited the test plan for this revision. (Show Details)

Updated commit message.

This revision is now accepted and ready to land.Oct 7 2023, 10:37 AM

I'll submit it with the unification of ping/ping6's statistics section (D39126).

Are some of your older ping patches ready to commit? Please point me at them if so.

I'll submit it with the unification of ping/ping6's statistics section (D39126).

Are some of your older ping patches ready to commit? Please point me at them if so.

I have created: https://github.com/freebsd/freebsd-src/pull/863

Still missing the update to only display stats when n > 1. I'll submit it afterwards. Thank you!

This revision was automatically updated to reflect the committed changes.

I would like to kindly request an MFC to stable/14 of at least this fix.
Reported by: Jenkins
https://ci.freebsd.org/view/Test/job/FreeBSD-stable-14-riscv64-test/lastCompletedBuild/testReport/sbin.ping/ping_test/ping6_c1_s8_t1/
(pasting RISC-V, as aarch64 images are failing)

I would like to kindly request an MFC to stable/14 of at least this fix.
Reported by: Jenkins
https://ci.freebsd.org/view/Test/job/FreeBSD-stable-14-riscv64-test/lastCompletedBuild/testReport/sbin.ping/ping_test/ping6_c1_s8_t1/
(pasting RISC-V, as aarch64 images are failing)

Done, thanks for the reminder.

I would like to kindly request an MFC to stable/14 of at least this fix.
Reported by: Jenkins
https://ci.freebsd.org/view/Test/job/FreeBSD-stable-14-riscv64-test/lastCompletedBuild/testReport/sbin.ping/ping_test/ping6_c1_s8_t1/
(pasting RISC-V, as aarch64 images are failing)

Done, thanks for the reminder.

Thank you!

Sorry to bother again, would it be possible to MFC just this commit (4d348e83b738347f6aaf2b110459a01c5402d04e) to stable/13, the other commits in this series cannot be applied.
Reported by: Jenkins
https://ci.freebsd.org/view/Test/job/FreeBSD-stable-13-riscv64-test/936/testReport/sbin.ping/

Sorry to bother again, would it be possible to MFC just this commit (4d348e83b738347f6aaf2b110459a01c5402d04e) to stable/13, the other commits in this series cannot be applied.
Reported by: Jenkins
https://ci.freebsd.org/view/Test/job/FreeBSD-stable-13-riscv64-test/936/testReport/sbin.ping/

No problem, done.