Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No select() when reading stream data #1787

Conversation

davidBar-On
Copy link
Contributor

  • Version of iperf3 (or development branch, such as master or
    3.1-STABLE) to which this pull request applies:
    master

  • Issues fixed (if any): High UDP packet loss, compare with nuttcp #1707

  • Brief description of code changes (suitable for use as a commit message):

Suggested solution to make iperf3 UDP performance comparable to nuttcp (and probably other tools). The change is to not use select() when reading stream data. This by replacing in these case the calls to Nread() to calls to Nread_no_select() (new function).

The assumption is that the select() in Nread() was added to make sure that the main thread will not get stuck (when reading control/JSON data), and that it is not needed for the data streams. E.g., the server will not get stuck even if one of its data streams threads is stuck, as the that thread will be canceled at the end of the test.

Although the reported issue was only about UDP, the change was done also for TCP, SCTP, as it seems reasonable to do. If that may cause issues, the change to these protocols may be removed.

@bmah888 bmah888 self-assigned this Oct 18, 2024
@bmah888
Copy link
Contributor

bmah888 commented Oct 18, 2024

Thanks for the PR! This change is something I've been thinking about since we went to multi-threading, so I'm glad you were able to make this work with positive results. Looking forward to testing this.

@davidBar-On davidBar-On force-pushed the issue-1707-UDP-higher-packet-loss-compared-to-nuttcp branch from d0c7624 to c038abe Compare October 19, 2024 06:58
@bmah888
Copy link
Contributor

bmah888 commented Oct 23, 2024

I did some testing with our dev hosts (two older servers connected by a 100GE path, one router hop within the same rack) and saw much less packet loss and greater "goodput" with single-stream UDP performance. (I briefly tested some other scenarios as well, but this addressed the main reason for this code change.)

I was trying to see if there's an easy way to factor out the common code between Nread() and Nread_no_select() but haven't found one yet. (This is not a blocker for merging this change.)

Hoping to get this merged in the next few days after a little more testing.

@davidBar-On
Copy link
Contributor Author

... easy way to factor out the common code between Nread() and Nread_no_select() ...

The way I considered was to add a parameter to Nread() for whether to do the select(), but I didn't do that as it would require to change all the calls to Nread().

Copy link
Contributor

@bmah888 bmah888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good...testing showed a significant improvement on our test hosts (reflected in less packet loss and higher goodput for UDP tests).

@bmah888 bmah888 merged commit 38b4dde into esnet:master Oct 25, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants