iputils / iputils

The iputils package is set of small old utilities for Linux networking.
Other
640 stars 262 forks source link

Patch submission to fix ping interval. #524

Open greearb opened 8 months ago

greearb commented 8 months ago

The existing code calculated 'tokens' from current time. As system calls used up some small amount of code, the ping transmit interval tends to be about 2ms slower than expected (on my test system).

Instead of trying to keep track of a tokens counter, use a timestamp that is incremented each ping send request by 'interval'. This keeps the interval the same even with the system call latency.

Patch attached fixes it, could probably use some more testing. 0001-ping-Ensure-ping-is-sent-on-correct-interval.txt

pevik commented 8 months ago

Thanks for working on this. no answer yet for first reply would need to be fixed.

$ ./builddir/ping/ping -OD ::1
PING ::1 (::1) 56 data bytes
[1708670330.871771] no answer yet for icmp_seq=1
[1708670330.871817] 64 bytes from ::1: icmp_seq=1 ttl=64 time=0.041 ms
[1708670330.871832] 64 bytes from ::1: icmp_seq=2 ttl=64 time=0.013 ms
[1708670331.889429] 64 bytes from ::1: icmp_seq=3 ttl=64 time=0.058 ms
[1708670332.873644] 64 bytes from ::1: icmp_seq=4 ttl=64 time=0.068 ms

Have you tried a different interval than just the default 1s? It's slightly increasing.

$ ./builddir/ping/ping 8.8.8.8 -OD -i2
PING 8.8.8.8 (8.8.8.8) 56(84) bytes of data.
[1708670068.819166] no answer yet for icmp_seq=1
[1708670068.831540] 64 bytes from 8.8.8.8: icmp_seq=1 ttl=116 time=12.3 ms
[1708670068.832339] 64 bytes from 8.8.8.8: icmp_seq=2 ttl=116 time=13.1 ms
[1708670070.837011] 64 bytes from 8.8.8.8: icmp_seq=3 ttl=116 time=15.4 ms
[1708670072.838528] 64 bytes from 8.8.8.8: icmp_seq=4 ttl=116 time=16.3 ms
[1708670074.835015] 64 bytes from 8.8.8.8: icmp_seq=5 ttl=116 time=11.2 ms
...
[1708670174.842987] 64 bytes from 8.8.8.8: icmp_seq=55 ttl=116 time=11.1 ms
[1708670176.846084] 64 bytes from 8.8.8.8: icmp_seq=56 ttl=116 time=13.9 ms
[1708670178.843404] 64 bytes from 8.8.8.8: icmp_seq=57 ttl=116 time=11.9 ms
[1708670180.844339] 64 bytes from 8.8.8.8: icmp_seq=58 ttl=116 time=10.7 ms

nit (early return adds readability):

+++ ping/ping.h
@@ -311,18 +311,19 @@ static inline void tvsub(struct timeval *out, struct timeval *in)

 static inline void tsincr(struct timespec *out, int msec)
 {
-       if (msec) {
-               long incr_msecs = msec % 1000;
-               long incr_secs = msec / 1000;
-               long incr_nsecs = incr_msecs * 1000000;
-
-               if (out->tv_nsec + incr_nsecs > 1000000000) {
-                       incr_secs++;
-                       incr_nsecs -= 1000000000;
-               }
-               out->tv_sec += incr_secs;
-               out->tv_nsec += incr_nsecs;
+       if (!msec)
+               return;
+
+       long incr_msecs = msec % 1000;
+       long incr_secs = msec / 1000;
+       long incr_nsecs = incr_msecs * 1000000;
+
+       if (out->tv_nsec + incr_nsecs > 1000000000) {
+               incr_secs++;
+               incr_nsecs -= 1000000000;
        }
+       out->tv_sec += incr_secs;
+       out->tv_nsec += incr_nsecs;
 }

 /*
greearb commented 8 months ago

Thanks for trying it out. What OS/kernel were you using to reproduce the increasing delay? We'll do some more testing and see if we can reproduce.

For the early return, your suggestion won't compile w/out warnings on at least some c compilers since variables need to be declared at the top of the code block.

pevik commented 8 months ago

Kernel 6.6, on ICMP datagram socket (sysctl net.ipv4.ping_group_range="1 1000").

yvs2014 commented 8 months ago

btw, with the above patch it takes 1sec less than before on 4 packets

% sudo LANG=C builddir/ping/ping -OD -c4 -n 1.1
PING 1.1 (1.0.0.1) 56(84) bytes of data.
[1708707647.909308] no answer yet for icmp_seq=1
[1708707647.912048] 64 bytes from 1.0.0.1: icmp_seq=1 ttl=60 time=2.63 ms
[1708707647.912081] 64 bytes from 1.0.0.1: icmp_seq=2 ttl=60 time=2.55 ms
[1708707648.913852] 64 bytes from 1.0.0.1: icmp_seq=3 ttl=60 time=2.55 ms
[1708707649.913605] 64 bytes from 1.0.0.1: icmp_seq=4 ttl=60 time=2.60 ms

--- 1.1 ping statistics ---
4 packets transmitted, 4 received, 0% packet loss, time 2002ms
rtt min/avg/max/mdev = 2.545/2.583/2.634/0.036 ms, pipe 2

comparing to std ping

% ping -OD -c4 -n 1.1
PING 1.1 (1.0.0.1) 56(84) bytes of data.
[1708707653.364185] 64 bytes from 1.0.0.1: icmp_seq=1 ttl=60 time=2.57 ms
[1708707654.365836] 64 bytes from 1.0.0.1: icmp_seq=2 ttl=60 time=2.53 ms
[1708707655.366765] 64 bytes from 1.0.0.1: icmp_seq=3 ttl=60 time=2.56 ms
[1708707656.368038] 64 bytes from 1.0.0.1: icmp_seq=4 ttl=60 time=2.69 ms

--- 1.1 ping statistics ---
4 packets transmitted, 4 received, 0% packet loss, time 3004ms
rtt min/avg/max/mdev = 2.532/2.588/2.689/0.059 ms

with smaller intervals it looks like not monotonic, for example can jump on 10msec every 10th packet

sudo LANG=C builddir/ping/ping -i0.001 -c100 -OD -n 1.2.3.4
PING 1.2.3.4 (1.2.3.4) 56(84) bytes of data.
[1708709502.502353] no answer yet for icmp_seq=1
[1708709502.502378] no answer yet for icmp_seq=2
[1708709502.512423] no answer yet for icmp_seq=3
[1708709502.512475] no answer yet for icmp_seq=4
[1708709502.512488] no answer yet for icmp_seq=5
[1708709502.512500] no answer yet for icmp_seq=6
[1708709502.512512] no answer yet for icmp_seq=7
[1708709502.512524] no answer yet for icmp_seq=8
[1708709502.512539] no answer yet for icmp_seq=9
[1708709502.512551] no answer yet for icmp_seq=10
[1708709502.512564] no answer yet for icmp_seq=11
[1708709502.512576] no answer yet for icmp_seq=12
[1708709502.522628] no answer yet for icmp_seq=13
[1708709502.522722] no answer yet for icmp_seq=14
[1708709502.522742] no answer yet for icmp_seq=15
[1708709502.522759] no answer yet for icmp_seq=16
[1708709502.522778] no answer yet for icmp_seq=17
[1708709502.522799] no answer yet for icmp_seq=18
[1708709502.522817] no answer yet for icmp_seq=19
[1708709502.522837] no answer yet for icmp_seq=20
[1708709502.522859] no answer yet for icmp_seq=21
[1708709502.522878] no answer yet for icmp_seq=22
[1708709502.532928] no answer yet for icmp_seq=23
[1708709502.532963] no answer yet for icmp_seq=24
[1708709502.532977] no answer yet for icmp_seq=25
[1708709502.532988] no answer yet for icmp_seq=26
[1708709502.533000] no answer yet for icmp_seq=27
[1708709502.533011] no answer yet for icmp_seq=28
[1708709502.533028] no answer yet for icmp_seq=29
[1708709502.533042] no answer yet for icmp_seq=30
[1708709502.533053] no answer yet for icmp_seq=31
[1708709502.533064] no answer yet for icmp_seq=32
[1708709502.543122] no answer yet for icmp_seq=33
[1708709502.543154] no answer yet for icmp_seq=34
[1708709502.543175] no answer yet for icmp_seq=35
[1708709502.543193] no answer yet for icmp_seq=36
[1708709502.543211] no answer yet for icmp_seq=37
[1708709502.543228] no answer yet for icmp_seq=38
[1708709502.543245] no answer yet for icmp_seq=39
[1708709502.543262] no answer yet for icmp_seq=40
[1708709502.543282] no answer yet for icmp_seq=41
[1708709502.543304] no answer yet for icmp_seq=42
[1708709502.543323] no answer yet for icmp_seq=43
[1708709502.553376] no answer yet for icmp_seq=44
[1708709502.553426] no answer yet for icmp_seq=45
[1708709502.553437] no answer yet for icmp_seq=46
[1708709502.553448] no answer yet for icmp_seq=47
[1708709502.553460] no answer yet for icmp_seq=48
[1708709502.553471] no answer yet for icmp_seq=49
[1708709502.553485] no answer yet for icmp_seq=50
[1708709502.553496] no answer yet for icmp_seq=51
[1708709502.553509] no answer yet for icmp_seq=52
[1708709502.553525] no answer yet for icmp_seq=53
[1708709502.563568] no answer yet for icmp_seq=54
[1708709502.563599] no answer yet for icmp_seq=55
[1708709502.563620] no answer yet for icmp_seq=56
[1708709502.563639] no answer yet for icmp_seq=57
[1708709502.563657] no answer yet for icmp_seq=58
[1708709502.563677] no answer yet for icmp_seq=59
[1708709502.563695] no answer yet for icmp_seq=60
[1708709502.563712] no answer yet for icmp_seq=61
[1708709502.563730] no answer yet for icmp_seq=62
[1708709502.563754] no answer yet for icmp_seq=63
[1708709502.573823] no answer yet for icmp_seq=64
[1708709502.573856] no answer yet for icmp_seq=65
[1708709502.573876] no answer yet for icmp_seq=66
[1708709502.573896] no answer yet for icmp_seq=67
[1708709502.573915] no answer yet for icmp_seq=68
[1708709502.573936] no answer yet for icmp_seq=69
[1708709502.573955] no answer yet for icmp_seq=70
[1708709502.573973] no answer yet for icmp_seq=71
[1708709502.573995] no answer yet for icmp_seq=72
[1708709502.574013] no answer yet for icmp_seq=73
[1708709502.584081] no answer yet for icmp_seq=74
[1708709502.584137] no answer yet for icmp_seq=75
[1708709502.584151] no answer yet for icmp_seq=76
[1708709502.584163] no answer yet for icmp_seq=77
[1708709502.584175] no answer yet for icmp_seq=78
[1708709502.584190] no answer yet for icmp_seq=79
[1708709502.584202] no answer yet for icmp_seq=80
[1708709502.584213] no answer yet for icmp_seq=81
[1708709502.584226] no answer yet for icmp_seq=82
[1708709502.584238] no answer yet for icmp_seq=83
[1708709502.594271] no answer yet for icmp_seq=84
[1708709502.594310] no answer yet for icmp_seq=85
[1708709502.594333] no answer yet for icmp_seq=86
[1708709502.594348] no answer yet for icmp_seq=87
[1708709502.594362] no answer yet for icmp_seq=88
[1708709502.594373] no answer yet for icmp_seq=89
[1708709502.594389] no answer yet for icmp_seq=90
[1708709502.594401] no answer yet for icmp_seq=91
[1708709502.594413] no answer yet for icmp_seq=92
[1708709502.594425] no answer yet for icmp_seq=93
[1708709502.594441] no answer yet for icmp_seq=94
[1708709502.604521] no answer yet for icmp_seq=95
[1708709502.604570] no answer yet for icmp_seq=96
[1708709502.604582] no answer yet for icmp_seq=97
[1708709502.604594] no answer yet for icmp_seq=98
[1708709502.604606] no answer yet for icmp_seq=99

--- 1.2.3.4 ping statistics ---
100 packets transmitted, 0 received, 100% packet loss, time 102ms
greearb commented 8 months ago

I think this fixes some of it:

diff --git a/ping/ping_common.c b/ping/ping_common.c
index 6abc202..e121372 100644
--- a/ping/ping_common.c
+++ b/ping/ping_common.c
@@ -315,7 +315,7 @@ int pinger(struct ping_rts *rts, ping_func_set_st *fset, socket_st *sock)
 {
        static int oom_count;
        int i;
-       long ms_until_xmit = 0;
+       long ms_until_xmit = rts->interval;

        /* Have we already sent enough? If we have, return an arbitrary positive value. */
        if (rts->exiting || (rts->npackets && rts->ntransmitted >= rts->npackets && !rts->deadline))
@@ -325,7 +325,7 @@ int pinger(struct ping_rts *rts, ping_func_set_st *fset, socket_st *sock)
        if (rts->cur_time.tv_sec == 0 && rts->cur_time.tv_nsec == 0) {
                clock_gettime(CLOCK_MONOTONIC_RAW, &rts->cur_time);
                rts->req_xmit_time = rts->cur_time;
-               rts->preload_left = rts->preload;
+               rts->preload_left = rts->preload - 1;
        } else {
                struct timespec tv;
pevik commented 5 months ago

I fixed the formatting and try to have look soon. But why don't you post a pull request or at least commit whole fix to your branch and post at least link to the branch? Posting patches like this make things hard to review.

You might be also interested in: https://github.com/iputils/iputils/pull/540 (or other https://github.com/iputils/iputils/labels/RTT pull requests).

pevik commented 3 months ago

@greearb gentle ping, how about creating a PR?