quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.85k stars 394 forks source link

Increase GRO segments to 64 #1354

Closed alessandrod closed 2 years ago

alessandrod commented 2 years ago

Welp, this was a bit of a head scratcher.

While running perf_server and perf_client from two different hosts (and so not on loopback), I noticed that occasionally I was getting the following debug logs:

2022-05-10T02:53:58.233887Z DEBUG drive{id=0}: quinn_proto::connection: failed to authenticate packet

It turns out that these logs are due to truncated GRO segments.

After some kernel spelunking, I noticed this: https://github.com/torvalds/linux/blob/1e8a3f0d2a1ef544611a7ea4a7c1512c732e0e43/net/core/gro.c#L543. The GRO segment size (what we call stride), is set dynamically depending on the first datagram that starts a GRO list. Unless the receive buffer passed to recvmmsg() is an exact multiple of the segment size, segments can be truncated since recvmmsg() will happily fill the whole buffer it's given regardless of whether it's aligned to segment boundaries or not (this is a wart of the API, since in some cases it is ok for the last segment to be shorter than all the others).

The fix is to bump the value we return from gro::gro_segments() to the maximum allowed by the kernel (64, which incidentally is also the value we set for GSO). This way get_max_udp_payload_size() * gro::gro_segments() will always be strictly larger than the GRO lists the kernel produces, so recvmmsg() won't truncate.

Ralith commented 2 years ago

Good catch! Could you include this rationale in the comment so it's not lost?

alessandrod commented 2 years ago

Good catch! Could you include this rationale in the comment so it's not lost?

Done!