-
-
Notifications
You must be signed in to change notification settings - Fork 385
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
bench(udp): run GSO, GRO and recvmmsg permutations #2010
base: main
Are you sure you want to change the base?
Conversation
62dd45e
to
1406041
Compare
1406041
to
bc8825b
Compare
@@ -30,8 +30,13 @@ once_cell = { workspace = true } | |||
windows-sys = { workspace = true } | |||
|
|||
[dev-dependencies] | |||
criterion = "0.5" | |||
criterion = { version = "0.5", default-features = false, features = ["async_tokio"] } | |||
tokio = { workspace = true, features = ["rt", "rt-multi-thread", "net"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Linux blocking recvmmsg
calls will wait for all iovec
s to be filled.
A blocking recvmmsg() call blocks until vlen messages have been
received or until the timeout expires. A nonblocking call reads
as many messages as are available (up to the limit specified by
vlen) and returns immediately.
https://man7.org/linux/man-pages/man2/recvmmsg.2.html
This is e.g. problematic when combined with a GSO send with less segments than iovec
s in its recvmmsg
counterpart.
Thus this pull request changes the benchmark to use non-blocking instead of blocking recvmmsg
. To do so, it uses tokio
.
See also discussion in #1993 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest you keep a separate commit that makes the change from sync to async.
#[cfg(any(target_os = "linux", target_os = "windows"))] | ||
true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other platforms don't support GSO.
Needs to add apple platforms once #1993 merges.
const MAX_IP_UDP_HEADER_SIZE: usize = 48; | ||
const MAX_DATAGRAM_SIZE: usize = u16::MAX as usize - MAX_IP_UDP_HEADER_SIZE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linux allows for up to 64KiB per GSO and GRO call. That includes IP and UDP header.
// recv.readable() can lead to false positives. Try again. | ||
Err(e) if e.kind() == ErrorKind::WouldBlock => continue, | ||
e => e.unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UdpSocket::readable
is allowed to return false-positives.
The function may complete without the socket being readable. This is a false-positive and attempting a try_recv() will return with io::ErrorKind::WouldBlock.
https://docs.rs/tokio/latest/tokio/net/struct.UdpSocket.html#method.readable
- run: cargo test | ||
- run: cargo test --manifest-path fuzz/Cargo.toml | ||
if: ${{ matrix.rust }} == "stable" | ||
- run: cargo test --benches |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I did not simply extend the existing cargo test
call, but instead added an additional cargo test --benches
.
cargo test --benches
executes some but not all unit tests in addition to all benchmark tests. See also rust-lang/cargo#6454.
@larseggert this pull request should resolve all issues raised in #1993 (review). Can you take a look? Targeting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is probably a good change but would like to see more organization in terms of separate commits -- here's some early feedback.
@@ -30,8 +30,13 @@ once_cell = { workspace = true } | |||
windows-sys = { workspace = true } | |||
|
|||
[dev-dependencies] | |||
criterion = "0.5" | |||
criterion = { version = "0.5", default-features = false, features = ["async_tokio"] } | |||
tokio = { workspace = true, features = ["rt", "rt-multi-thread", "net"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest you keep a separate commit that makes the change from sync to async.
use std::cmp::min; | ||
use std::net::{Ipv4Addr, Ipv6Addr}; | ||
use std::{io::IoSliceMut, net::UdpSocket, slice}; | ||
use quinn_udp::{RecvMeta, Transmit, UdpSocketState, BATCH_SIZE}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: would be good to do StdExternalCrate
import blocks here.
use tokio::io::Interest; | ||
use tokio::runtime::Runtime; | ||
|
||
const MAX_IP_UDP_HEADER_SIZE: usize = 48; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: constants should go near the bottom.
io::{ErrorKind, IoSliceMut}, | ||
net::{Ipv4Addr, Ipv6Addr, UdpSocket}, | ||
}; | ||
use tokio::io::Interest; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: for consistency, there should be one import statement for all tokio
imports.
} | ||
}) | ||
}); | ||
} | ||
} | ||
|
||
fn new_socket(rt: &mut Runtime) -> (tokio::net::UdpSocket, UdpSocketState) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the extraction of this function in a separate commit.
recvmmsg
.cargo test --benches
, single execution in debug mode) to CI.