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

Add netperf UDP stream test #2075

Conversation

dPhanekham
Copy link
Contributor

Add support for netperf UDP stream test.
Add flags to set size of stream tests
Also add same flag to container_netperf benchmark

@@ -27,6 +27,9 @@

FLAGS = flags.FLAGS

flags.DEFINE_integer('container_netperf_tcp_stream_send_size_in_bytes', 131072,
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment as to why this default value was selected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This value, 131072 bytes (128KB) was selected at the recommendation of @jonesrick.

"The 128K send size is deliberate - otherwise netperf will default to 16K based on Linux send socket buffer size initial values, which has enough overhead to preclude achieving "link rate" as it were. And the default receive size will be 87380 bytes for similar reasons on the receive side. That would be a good thing to change in the netperf command-lines in PKB. And it would match the default send size in iperf3. 64K may be sufficient but 128K would give additional cushion. The need for receive side (-M) isn't as clear-cut but it won't hurt."

Or, if preferred, I could give it a default of None and let netperf select the size unless this flag is set.

Copy link
Member

Choose a reason for hiding this comment

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

I think setting the default is important. I'd just like a comment in the code so that we know why we selected this default. Noting that the Linux default of 16K cannot achieve "link rate" and that the higher default value will let us would be sufficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to set -s?

Per https://hewlettpackard.github.io/netperf/doc/netperf.html#UDP_005fSTREAM
If the value of the -m option is larger than the local send socket buffer size (-s option) netperf will likely abort with an error message about how the send call failed:

 netperf -t UDP_STREAM -H 192.168.2.125
 UDP UNIDIRECTIONAL SEND TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.2.125 (192.168.2.125) port 0 AF_INET
 udp_send: data send error: Message too long

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment to clarify default size. Addressed -s flag in a comment below

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding the comment. Since comments are easier to parse if they contain punctuation and consist of complete sentences, perhaps edit it so that it reads more like the following:

We set the default to 128KB (131072 bytes) to override the Linux default

of 16K so that we can achieve the "link rate".

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Looks like the prefix of "#" changed my font size. I just meant to write a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed the comment

flags.DEFINE_integer('netperf_udp_stream_send_size_in_bytes', 1024,
'Send size to use for UDP_STREAM tests (netperf -m flag)')
flags.DEFINE_integer('netperf_tcp_stream_send_size_in_bytes', 131072,
'Send size to use for TCP_STREAM tests (netperf -m flag)')
Copy link
Member

Choose a reason for hiding this comment

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

Comment here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comment added

perfkitbenchmarker/linux_benchmarks/netperf_benchmark.py Outdated Show resolved Hide resolved
verbosity=verbosity)

if benchmark_name.upper() == 'UDP_STREAM':
netperf_cmd += (' -R 1 -m {send_size} '.format(
Copy link
Member

Choose a reason for hiding this comment

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

Is this a change in response size as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have also added the -M flag to set the remote receive size as well. I do not believe it changes that response size, as UDP_STREAM and TCP_STREAM test throughput unidirectionally.

@@ -27,6 +27,9 @@

FLAGS = flags.FLAGS

flags.DEFINE_integer('container_netperf_tcp_stream_send_size_in_bytes', 131072,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to set -s?

Per https://hewlettpackard.github.io/netperf/doc/netperf.html#UDP_005fSTREAM
If the value of the -m option is larger than the local send socket buffer size (-s option) netperf will likely abort with an error message about how the send call failed:

 netperf -t UDP_STREAM -H 192.168.2.125
 UDP UNIDIRECTIONAL SEND TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.2.125 (192.168.2.125) port 0 AF_INET
 udp_send: data send error: Message too long

@@ -356,7 +360,7 @@ def RunNetperf(vm, benchmark_name, server_ip, num_streams):
enable_latency_histograms = FLAGS.netperf_enable_histograms or num_streams > 1
# Throughput benchmarks don't have latency histograms
enable_latency_histograms = enable_latency_histograms and \
benchmark_name != 'TCP_STREAM'
(benchmark_name != 'TCP_STREAM' and benchmark_name != 'UDP_STREAM')
Copy link
Contributor

Choose a reason for hiding this comment

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

More readable as
(benchmark_name not in ['TCP_STREAM' , 'UDP_STREAM'])

Also if you could fix the \ continue as well, that'd be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when testing UDP_STREAM, i found the default send socket buffer size to be 212,992 bytes, which is larger than the max possible UDP message size of 65,507 bytes, so it should not be a problem and we don't need the -s flag. I can add in an upper limit of 65507 bytes, because it will cause an error if you try to use a larger size than that.

Copy link
Contributor

Choose a reason for hiding this comment

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

What platforms have you tested this on? My reading is that there may be some where -s is required (no idea which) but I don't see a downside to specifying both -s and -S

Copy link
Contributor Author

@dPhanekham dPhanekham Jan 10, 2020

Choose a reason for hiding this comment

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

I've tested on ubuntu18, debian9, and centos8. So far I haven't needed to specify the -s/-S flags for things to work. But I can certainly add them in as pkb flags and give them default values

@jonesrick
Copy link

jonesrick commented Jan 9, 2020 via email

@jonesrick
Copy link

jonesrick commented Jan 9, 2020 via email

@jonesrick
Copy link

jonesrick commented Jan 9, 2020 via email

@jonesrick
Copy link

jonesrick commented Jan 9, 2020 via email

perfkitbenchmarker/linux_benchmarks/netperf_benchmark.py Outdated Show resolved Hide resolved
@@ -356,7 +360,7 @@ def RunNetperf(vm, benchmark_name, server_ip, num_streams):
enable_latency_histograms = FLAGS.netperf_enable_histograms or num_streams > 1
# Throughput benchmarks don't have latency histograms
enable_latency_histograms = enable_latency_histograms and \
benchmark_name != 'TCP_STREAM'
(benchmark_name != 'TCP_STREAM' and benchmark_name != 'UDP_STREAM')
Copy link
Contributor

Choose a reason for hiding this comment

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

What platforms have you tested this on? My reading is that there may be some where -s is required (no idea which) but I don't see a downside to specifying both -s and -S

@dlott
Copy link
Contributor

dlott commented Jan 9, 2020

If you move the additions in CHANGES.next.md up a line or two, they'll won't conflict as people add things.

[
"MIGRATED UDP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 104.198.67.251 () port 20001 AF_INET : +/-2.500% @ 99% conf.",
"Throughput,Throughput Units,50th Percentile Latency Microseconds,90th Percentile Latency Microseconds,99th Percentile Latency Microseconds,Stddev Latency Microseconds,Minimum Latency Microseconds,Maximum Latency Microseconds,Confidence Iterations Run,Throughput Confidence Width (%),Local Transport Retransmissions,Remote Transport Retransmissions",
"1102.42,10^6bits/s,3,3,11,46.14,1,15144,1,-1.000,-1,-1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this only a send throughput? Should there also be a receive throughput?

https://hewlettpackard.github.io/netperf/doc/netperf.html#UDP_005fSTREAM

The biggest of these implications is the data which is sent might not be received by the remote. For this reason, the output of a UDP_STREAM test shows both the sending and receiving throughput. On some platforms, it may be possible for the sending throughput to be reported as a value greater than the maximum rate of the link. This is common when the CPU(s) are faster than the network and there is no intra-stack flow-control.

Is do you have a pkb.log that shows the raw output of a run?

@jonesrick
Copy link

jonesrick commented Jan 9, 2020 via email

@jonesrick
Copy link

jonesrick commented Jan 9, 2020 via email

@jonesrick
Copy link

jonesrick commented Jan 10, 2020 via email

Copy link
Contributor

@dlott dlott 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.

@bvliu bvliu merged commit 07441b9 into GoogleCloudPlatform:master Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants