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

Basic TCP support. #34

Closed
wants to merge 3 commits into from
Closed

Conversation

nombwelling
Copy link
Contributor

This adds basic support for sending queries over TCP.

This is not based on https://github.com/Sinodun/dnsperf-tcp, but does reuse the -z option. It doesn't include any of the other changes there, such as potentially limiting the number of queries over a socket, changing the maximum number of open sockets, or using poll instead of select.

It is, however, far more efficient than the code in that fork, and built on top of the current dnsperf.

Additional functionality can be added later.

@jelu
Copy link
Member

jelu commented Jun 3, 2019

Thanks, have you done any benchmark on this branch compared to regular dnsperf and Sinodun's dnsperf-tcp?

I actually already have been working on TCP support which does it in a different way then this, and also supports adding TLS later on. Will look at both these branches when I get time to move this forward.

@nombwelling
Copy link
Contributor Author

I haven't done any rigorous benchmarking, but this branch doesn't do extra copying on send, and uses 1 syscall instead of 5 in the normal receive path.

Most of what's here would also be applicable to TLS, although it would be better to redo the sending code to not use writev, as there's no SSL_writev().

@nombwelling
Copy link
Contributor Author

I just pushed a commit to include the length in the buffer and use write() instead of writev(), which should be closer to what's needed for TLS.

@jelu
Copy link
Member

jelu commented Jun 4, 2019

Thanks! It's going to take a while to handle this, have other internal priorities and upcoming holiday. Also, why I ask about benchmarking, I want to test how the different code bases affect normal UDP QPS beside how it does on TCP.

@nombwelling
Copy link
Contributor Author

The changes I committed should have no noticeable effect on UDP. It's just a few "is this TCP" checks, and the replacement of ntohs() with an open-coded version (needed for potentially unaligned TCP packets).

@jelu
Copy link
Member

jelu commented Jul 9, 2019

Thanks but I took another approach on this (#37), it's hopefully easier to add TLS support later, actually increased UDP performance (~5%) and was ~50% faster on TCP then this implementation.

@jelu jelu closed this Jul 9, 2019
@nombwelling nombwelling mentioned this pull request Jul 19, 2019
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