[B.A.T.M.A.N.] [PATCHv2] batctl: Correct mdev calculation in ping subcommand

[B.A.T.M.A.N.] [PATCHv2] batctl: Correct mdev calculation in ping subcommand

[Daniel Seither]

Fixes the calculation of the mean absolute deviation (mdev)
of the roundtrip times. mdev was previously calculated by
computing the difference of maximum and minimum round trip times.

The calculation is now done using the same formula as in the iputils
ping program.

Signed-off-by: Daniel Seither <post@tiwoc.de>
---
Index: Makefile
===================================================================
--- Makefile	(revision 1659)
+++ Makefile	(working copy)
@@ -28,7 +28,7 @@
 CC = gcc
 CFLAGS += -pedantic -Wall -W -g3 -std=gnu99 -Os -fno-strict-aliasing
 EXTRA_CFLAGS = -DREVISION_VERSION=$(REVISION_VERSION)
-LDFLAGS +=
+LDFLAGS += -lm

 SBINDIR = $(INSTALL_PREFIX)/usr/sbin

Index: ping.c
===================================================================
--- ping.c	(revision 1659)
+++ ping.c	(working copy)
@@ -29,6 +29,7 @@
 #include <signal.h>
 #include <fcntl.h>
 #include <string.h>
+#include <math.h>

 #include "main.h"
 #include "ping.h"
@@ -76,7 +77,7 @@
 	unsigned int seq_counter = 0, packets_out = 0, packets_in = 0, packets_loss;
 	char *dst_string, *mac_string, *rr_string;
 	double time_delta;
-	float min = 0.0, max = 0.0, avg = 0.0;
+	float min = 0.0, max = 0.0, avg = 0.0, mdev = 0.0;
 	uint8_t last_rr_cur = 0, last_rr[BAT_RR_LEN][ETH_ALEN];
 	size_t packet_len;

@@ -260,6 +261,7 @@
 			if (time_delta > max)
 				max = time_delta;
 			avg += time_delta;
+			mdev += time_delta * time_delta;
 			packets_in++;
 			break;
 		case DESTINATION_UNREACHABLE:
@@ -289,11 +291,20 @@
 	else
 		packets_loss = ((packets_out - packets_in) * 100) / packets_out;

+	if (packets_in) {
+		avg /= packets_in;
+		mdev /= packets_in;
+		mdev = sqrt(mdev - avg * avg);
+	} else {
+		avg = 0.0;
+		mdev = 0.0;
+	}
+
 	printf("--- %s ping statistics ---\n", dst_string);
 	printf("%u packets transmitted, %u received, %u%% packet loss\n",
 		packets_out, packets_in, packets_loss);
 	printf("rtt min/avg/max/mdev = %.3f/%.3f/%.3f/%.3f ms\n",
-		min, (packets_in ? (avg / packets_in) : 0.000), max, (max - min));
+		min, avg, max, mdev);

 	ret = EXIT_SUCCESS;

Re: [B.A.T.M.A.N.] [PATCHv2] batctl: Correct mdev calculation in ping subcommand

[Sven Eckelmann]

[-- Attachment #1: Type: Text/Plain, Size: 461 bytes --]

Daniel Seither wrote:
> Fixes the calculation of the mean absolute deviation (mdev)
> of the roundtrip times. mdev was previously calculated by
> computing the difference of maximum and minimum round trip times.
> 
> The calculation is now done using the same formula as in the iputils
> ping program.
> 
> Signed-off-by: Daniel Seither <post@tiwoc.de>

Acked-by: Sven Eckelmann <sven.eckelmann@gmx.de>

Thanks a lot for your contributions,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCHv2] batctl: Correct mdev calculation in ping subcommand

[Sven Eckelmann]

[-- Attachment #1: Type: Text/Plain, Size: 927 bytes --]

Sven Eckelmann wrote:
> Daniel Seither wrote:
> > Fixes the calculation of the mean absolute deviation (mdev)
> > of the roundtrip times. mdev was previously calculated by
> > computing the difference of maximum and minimum round trip times.
> > 
> > The calculation is now done using the same formula as in the iputils
> > ping program.
> > 
> > Signed-off-by: Daniel Seither <post@tiwoc.de>
> 
> Acked-by: Sven Eckelmann <sven.eckelmann@gmx.de>

Ok, have a small suggestion. Please check that mdev - ... is positive. 
Otherwise we would have something like (mdev - avg * avg == -0.000....., 
sqrt(...) == NaN):

PING 02:00:00:00:00:02 (02:00:00:00:00:02) 19(47) bytes of data
19 bytes from 02:00:00:00:00:02 icmp_seq=1 ttl=50 time=0.10 ms
--- 02:00:00:00:00:02 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss
rtt min/avg/max/mdev = 0.101/0.101/0.101/nan ms

Best regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCHv2] batctl: Correct mdev calculation in ping subcommand

[Daniel Seither]

Am 20.05.2010 19:00, schrieb Sven Eckelmann:
> Sven Eckelmann wrote:
> Ok, have a small suggestion. Please check that mdev - ... is positive. 
> Otherwise we would have something like (mdev - avg * avg == -0.000....., 
> sqrt(...) == NaN):
> 
> PING 02:00:00:00:00:02 (02:00:00:00:00:02) 19(47) bytes of data
> 19 bytes from 02:00:00:00:00:02 icmp_seq=1 ttl=50 time=0.10 ms
> --- 02:00:00:00:00:02 ping statistics ---
> 1 packets transmitted, 1 received, 0% packet loss
> rtt min/avg/max/mdev = 0.101/0.101/0.101/nan ms

There seems to be a problem with the floats... The value of mdev - avg^2
cannot become negative if computed with infinite precision. In this
case, it seems to be slightly less than zero because of rounding errors.
In iputils' ping, all calculations are done using integers which
prevents this kind of problems.

I think there are two solutions for the problem you discovered:

1) check whether the difference is <= 0 (easy)

2) rewrite the time measurement to use integers (more code to change,
but calculations will be exact)

Please check whether replacing the line calling sqrt by the following
code fixes the problem for you (first solution):

		mdev = mdev - avg * avg;
		if (mdev > 0.0)
			mdev = sqrt(mdev);
		else
			mdev = 0.0;

Or should we aim for the second solution?

- Daniel

Re: [B.A.T.M.A.N.] [PATCHv2] batctl: Correct mdev calculation in ping subcommand

[Sven Eckelmann]

[-- Attachment #1: Type: Text/Plain, Size: 1760 bytes --]

Daniel Seither wrote:
> Am 20.05.2010 19:00, schrieb Sven Eckelmann:
> > Sven Eckelmann wrote:
> > Ok, have a small suggestion. Please check that mdev - ... is positive.
> > Otherwise we would have something like (mdev - avg * avg == -0.000.....,
> > sqrt(...) == NaN):
> > 
> > PING 02:00:00:00:00:02 (02:00:00:00:00:02) 19(47) bytes of data
> > 19 bytes from 02:00:00:00:00:02 icmp_seq=1 ttl=50 time=0.10 ms
> > --- 02:00:00:00:00:02 ping statistics ---
> > 1 packets transmitted, 1 received, 0% packet loss
> > rtt min/avg/max/mdev = 0.101/0.101/0.101/nan ms
> 
> There seems to be a problem with the floats... The value of mdev - avg^2
> cannot become negative if computed with infinite precision. In this
> case, it seems to be slightly less than zero because of rounding errors.
> In iputils' ping, all calculations are done using integers which
> prevents this kind of problems.

Correct. Because of that I recommended to check before calling sqrt.

> I think there are two solutions for the problem you discovered:
> 
> 1) check whether the difference is <= 0 (easy)
> 
> 2) rewrite the time measurement to use integers (more code to change,
> but calculations will be exact)
> 
> Please check whether replacing the line calling sqrt by the following
> code fixes the problem for you (first solution):
> 
> 		mdev = mdev - avg * avg;
> 		if (mdev > 0.0)
> 			mdev = sqrt(mdev);
> 		else
> 			mdev = 0.0;

Already done that - that's why I recommended it :)

> Or should we aim for the second solution?

I think the first solution is good enough for now. Please send the patch again 
with that fix and it is ok for now. If you want you can rewrite the code later 
to use only integers.

Best regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]