Skip to content

Comments

Add initial portable time abstraction (WIP)#738

Merged
bmah888 merged 1 commit intoesnet:masterfrom
accelleran:iperf_time
Sep 28, 2018
Merged

Add initial portable time abstraction (WIP)#738
bmah888 merged 1 commit intoesnet:masterfrom
accelleran:iperf_time

Conversation

@ben-foxmoore
Copy link
Contributor

This is a WIP proposal for providing a portable replacement to gettimeofday. Discussion is welcomed!

The current usage of gettimeofday causes issues for us when performing tests shortly after restarting a system. In our setup, this occurs often as we restart the system before each test to ensure reliable results. We already maintain our own version of iperf for some subtle changes, but this change feels like it might be useful to upstream. (It's also a reasonable size change, so we'd prefer not maintain it with each new version of iperf.)

It uses clock_gettime on systems that have it available, and falls back to gettimeofday when it's not. These two options use different structures for storing time - clock_gettime uses timespec, and gettimeofday uses timeval. To provide abstraction to which one is available, a separate iperf_time struct is defined to store time.

timespec has nanosecond accuracy, while timeval only has microseconds. For the purposes of iperf, I don't think nanosecond accuracy is neccesary, so iperf_time only uses microseconds, throwing away any additional accuracy. Currently I have used the MONOTONIC clock, as I think we only need a consistent time interval measure.

The following functions are provided to work with iperf_time structs:

iperf_time_now       - gets the current "time", and returns it in an iperf_time
iperf_time_add_usecs - adds a number of usecs to the time stored in an iperf_time
iperf_time_compare   - compares two timestamps, and returns -1, 0, or 1
iperf_time_diff       - calculates the absolute difference between two timestamps, returns an integer to indicate the sign
iperf_time_in_usecs  - returns a uint64 representation of the provided time in microseconds
iperf_time_in_secs   - returns a double representation of the provided time in seconds

I'm not totally satisfied with the behaviour of iperf_time_diff. I may change it to return the same values as iperf_time_compare to be consistent. It was written the way it currently is so that timer.c can use the return code as a "timer is expired" flag. I'm also sure the implementation of iperf_time_diff could be greatly improved.

Unfortunately, this change has only been tested on environments that have clock_gettime - Linux 4.16.3 with glibc 2.27, and OSX 10.13.4. I don't have much experience with autoconf/automake, so this should be tested on a system without clock_gettime, and on a system which requires -lrt, before it is considered for merging.

Related to #210 and #253

@bmah888
Copy link
Contributor

bmah888 commented May 29, 2018

Thanks for submitting this change. I'm very interested in this, but clearly I'm going to need to study it.

I note that you've tested on two of the main development platforms for iperf3 (Linux and macOS), and it looks like the third supported platform (FreeBSD) also supports clock_gettime(2).

@ben-foxmoore
Copy link
Contributor Author

Of course, let me know if you have any questions.

I've since tested it on a system which requires librt - an old Linux box with glibc 2.12:

GNU C Library stable release version 2.12, by Roland McGrath et al.
Copyright (C) 2010 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE.
Compiled by GNU CC version 4.4.7 20120313 (Red Hat 4.4.7-16).
Compiled on a Linux 2.6.32 system on 2016-02-16.
Available extensions:
	The C stubs add-on version 2.1.2.
	crypt add-on version 2.1 by Michael Glad and others
	GNU Libidn by Simon Josefsson
	Native POSIX Threads Library by Ulrich Drepper et al
	BIND-8.2.3-T5B
	RT using linux kernel aio
libc ABIs: UNIQUE IFUNC

I still haven't found a system without clock_gettime to test against. All of our Macs have already updated to 10.12 unfortunately.

@bmah888
Copy link
Contributor

bmah888 commented Jun 15, 2018

I'm writing some (probably trivial) things down to prove I actually looked at this. :-) I don't need a diff for configure or src/Makefile.in (maybe src/iperf_config.h.in?) because those are auto-generated. I can deal with that however.

I was a little concerned about an apparent API change in src/iperf_api.h (the signature of iperf_check_throttle() changed, although it's not really used anywhere except in src/iperf_api.c and not really intended for use by any external consumers. So that's probably OK.

So far I've tested a bit on macOS and not seen any regressions. I'll try on other platforms too. Not sure what I have easy access to that doesn't have clock_gettime.

Could you provide a few details about what kind of issues you ran into with the stock code? Not that I don't believe you, just want to learn what the problems are with gettimeofday (I can imagine some...like the clock getting stepped by NTP or something like that).

@bmah888 bmah888 added this to the 3.7 milestone Jun 26, 2018
@bmah888 bmah888 self-assigned this Jun 26, 2018
@bmah888
Copy link
Contributor

bmah888 commented Aug 9, 2018

@ben-foxmore: D'oh. I merged someone else's PR (#767) and this caused a merge conflict with your PR, as I was going to merge your PR. Do you feel up to trying to resolve this? If not I can give it a try. I'm not too worried about the auto-generated files because I'll re-run bootstrap.sh anyway after merging.

@bmah888
Copy link
Contributor

bmah888 commented Sep 17, 2018

@ben-foxmoore: I'm still interested in merging this. I just tried merging the PR against the current master and only ran into a couple of easily-fixed merge conflicts in src/Makefile.in and src/Makefile.am, which I think is the same as what I had in my prior comment on 9 August 2018. As I indicated above, I'm not worried about changes to autogenerated files, since for a change of this size/magnitude I'll regenerate all of the automake goop anyways.

Would you be able to answer the questions I had in my last comment here?

It looks like this code works correctly on the officially supported platforms, so absent the easy ability to test on anything else I feel like I could go ahead and merge this (resolving the conflicts as I go along). Thoughts?

EDIT: And I just now realized I misspelled your username in my last comment, so you probably didn't even read it yet. Sorry about that!

@bmah888 bmah888 merged commit cde81d7 into esnet:master Sep 28, 2018
bmah888 added a commit that referenced this pull request Sep 28, 2018
From author's notes (@ben-foxmore):

The current usage of gettimeofday causes issues for us when performing
tests shortly after restarting a system. In our setup, this occurs
often as we restart the system before each test to ensure reliable
results. We already maintain our own version of iperf for some subtle
changes, but this change feels like it might be useful to upstream.
(It's also a reasonable size change, so we'd prefer not maintain it
with each new version of iperf.)

It uses clock_gettime on systems that have it available, and falls
back to gettimeofday when it's not. These two options use different
structures for storing time - clock_gettime uses timespec, and
gettimeofday uses timeval. To provide abstraction to which one is
available, a separate iperf_time struct is defined to store time.

timespec has nanosecond accuracy, while timeval only has microseconds.
For the purposes of iperf, I don't think nanosecond accuracy is
neccesary, so iperf_time only uses microseconds, throwing away any
additional accuracy. Currently I have used the MONOTONIC clock, as I
think we only need a consistent time interval measure.
@ben-foxmoore
Copy link
Contributor Author

Shoot, sorry @bmah888 - I missed your last comment (Gmail filtered it out to some different tab or another). I see you've merged this over the weekend. Were there any problems, or anything you still want me to look at?

Looking back, I realise I've neglected this post a bit. As you predicted the main issues we were having was with gettimeofday being stepped quite aggressively against our internal NTP server.

@bmah888
Copy link
Contributor

bmah888 commented Oct 1, 2018

@ben-foxmoore : Nah I'm good, thanks. Sorry it took so long for me to actually merge this...I didn't quite give it the testing I wanted but I finally concluded I needed to merge this into master to get more eyes on it.

@ben-foxmoore
Copy link
Contributor Author

No problem. Tag me if you have any issues raised due to this change and I'll try to help out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants