feat(connlib): reduce packet drops #4168
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Terraform Cloud Plan Output |
Performance Test ResultsTCP
UDP
|
|
Still benchmarking this but looks like an improvement. |
Benchmarks
|
There was a problem hiding this comment.
I asked some questions since I didn't understand all of it.
It sounds like we did not propagate readiness from tokio's UdpSocket up to higher layers of the stack, now we do that, and we also queue packets. I don't quite understand the queueing. I know in other places we said we don't need to queue since it's UDP / IP.
| /// Returns `Ready` if the socket is able to accept more data. | ||
| pub fn poll_flush(&mut self, cx: &mut Context<'_>) -> Poll<io::Result<()>> { |
There was a problem hiding this comment.
I see this Sockets struct contains both IPv4 and IPv6 sockets. So poll_flush will only be Ready when both sockets are flushed and both can take more data?
Could this result in e.g. IPv6 traffic waiting for the IPv4 socket to flush even if the IPv6 socket is ready? Or maybe that's unlikely if everything is going through 1 physical interface, anyway?
Do we have any perf tests that cover simultaneous tx+rx on IPv4+IPv6?
There was a problem hiding this comment.
Could this result in e.g. IPv6 traffic waiting for the IPv4 socket to flush even if the IPv6 socket is ready?
Technically yes. Both of them have to be ready because prior to reading a packet from the device, I don't know whether I'll need to send it via IPv4 or IPv6.
There was a problem hiding this comment.
Do we have any perf tests that cover simultaneous tx+rx on IPv4+IPv6?
No. We would need two gateways, one connected via IPv4 and one via IPv6 and IPv6 doesn't work properly in docker so we don't have any tests for that.
| fn send(&mut self, transmit: quinn_udp::Transmit) { | ||
| tracing::trace!(target: "wire", to = "network", src = ?transmit.src_ip, dst = %transmit.destination, num_bytes = %transmit.contents.len()); | ||
|
|
||
| self.buffered_transmits.push(transmit); |
There was a problem hiding this comment.
| self.buffered_transmits.push(transmit); | |
| if self.buffered_transmits.length() > 1_000_000_000 { | |
| panic!("Too many buffered outgoing packets"); | |
| } | |
| self.buffered_transmits.push(transmit); |
Couldn't hurt, right?
There was a problem hiding this comment.
Because we enforce further send-readiness before continuing, this buffer cannot grow unbounded.
I saw this in the original post but I can't see where it's enforced. Maybe it's in another file?
There was a problem hiding this comment.
The enforcement of send-readiness is the poll_send_ready at the end of flush and the ready! within Io::poll.
There was a problem hiding this comment.
I can add an assertion for the size, yeah.
| self.socket.poll_send_ready(cx) | ||
| fn poll_flush(&mut self, cx: &mut Context<'_>) -> Poll<io::Result<()>> { | ||
| loop { | ||
| match self.socket.try_io(Interest::WRITABLE, || { |
There was a problem hiding this comment.
What does try_io do that's different from checking if it's writable and then writing? It's not like it avoids a TOCTOU error, right?
There was a problem hiding this comment.
It internally clears the readiness flag of the socket. As far as I understand, that is what poll_send_ready returns.
I.e. we need to clear that flag otherwise it is stale and we never register a waker for when it is ready.
| self.state | ||
| .send((&self.socket).into(), &self.buffered_transmits) |
There was a problem hiding this comment.
If the sending is done in poll_flush, that means nothing is actually written to the network unless something is always calling poll_flush, right?
It took me a while to figure out, normally flushing is optional on sockets. But here if I never flushed, it would queue forever
There was a problem hiding this comment.
Yes, we flush on every iteration of Io::poll though which is pretty frequent.
Essentially, we flush if nothing else can make progress without reading more packets from the device.
|
@thomaseizinger Little to no difference on macOS (maybe even a performance hit): |
There was a problem hiding this comment.
No major change on macOS, but will approve anyway. It's kind of hard to test in debug and not trivial to make a release build without using the CI pipeline to do so.
jamil@Airbook-Mac:~/Developer/firezone/firezone (fix/buffer-packets$=) % iperf3 -c 10.0.32.101 -t 30
Connecting to host 10.0.32.101, port 5201
[ 5] local 100.71.243.67 port 49203 connected to 10.0.32.101 port 5201
[ ID] Interval Transfer Bitrate
[ 5] 0.00-1.01 sec 4.88 MBytes 40.7 Mbits/sec
[ 5] 1.01-2.00 sec 6.62 MBytes 55.6 Mbits/sec
[ 5] 2.00-3.00 sec 7.50 MBytes 63.1 Mbits/sec
[ 5] 3.00-4.00 sec 7.62 MBytes 63.7 Mbits/sec
[ 5] 4.00-5.00 sec 7.88 MBytes 66.3 Mbits/sec
[ 5] 5.00-6.01 sec 5.12 MBytes 42.8 Mbits/sec
[ 5] 6.01-7.00 sec 2.62 MBytes 22.1 Mbits/sec
[ 5] 7.00-8.00 sec 4.75 MBytes 39.8 Mbits/sec
[ 5] 8.00-9.00 sec 5.12 MBytes 43.0 Mbits/sec
[ 5] 9.00-10.01 sec 5.25 MBytes 44.0 Mbits/sec
[ 5] 10.01-11.00 sec 5.25 MBytes 44.2 Mbits/sec
[ 5] 11.00-12.00 sec 5.25 MBytes 44.0 Mbits/sec
[ 5] 12.00-13.00 sec 2.75 MBytes 23.1 Mbits/sec
[ 5] 13.00-14.00 sec 3.50 MBytes 29.3 Mbits/sec
[ 5] 14.00-15.00 sec 3.75 MBytes 31.5 Mbits/sec
[ 5] 15.00-16.01 sec 4.25 MBytes 35.6 Mbits/sec
[ 5] 16.01-17.00 sec 4.25 MBytes 35.7 Mbits/sec
[ 5] 17.00-18.00 sec 4.88 MBytes 40.9 Mbits/sec
[ 5] 18.00-19.00 sec 5.25 MBytes 44.0 Mbits/sec
[ 5] 19.00-20.00 sec 5.38 MBytes 45.1 Mbits/sec
[ 5] 20.00-21.00 sec 5.38 MBytes 45.1 Mbits/sec
[ 5] 21.00-22.00 sec 5.25 MBytes 44.0 Mbits/sec
[ 5] 22.00-23.00 sec 5.25 MBytes 44.0 Mbits/sec
[ 5] 23.00-24.00 sec 4.25 MBytes 35.8 Mbits/sec
[ 5] 24.00-25.00 sec 3.88 MBytes 32.4 Mbits/sec
[ 5] 25.00-26.00 sec 4.88 MBytes 41.0 Mbits/sec
[ 5] 26.00-27.00 sec 5.12 MBytes 43.0 Mbits/sec
[ 5] 27.00-28.01 sec 5.38 MBytes 45.0 Mbits/sec
[ 5] 28.01-29.00 sec 5.12 MBytes 43.0 Mbits/sec
[ 5] 29.00-30.00 sec 5.62 MBytes 47.2 Mbits/sec
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval Transfer Bitrate
[ 5] 0.00-30.00 sec 152 MBytes 42.5 Mbits/sec sender
[ 5] 0.00-30.08 sec 152 MBytes 42.3 Mbits/sec receiver
Okay, can you try once it built one for |
We only queue the one that failed to send because of a full buffer. The problem with relying on the retransmission stack of the kernel is that it hurts performance because you need to wait for timeouts. It is essentially packetloss on the wire. |
@jamilbk How difficult would it be to run that with a release build? I don't want to merge something that regresses performance. |
Coming right up |
@conectado Managed to test it and it is a 10x improvement in upload speed for him (only upload matters until we deploy this to the gateways). Curious to see your results from a US-based line! |
|
Yeah, unfortunately we don't seem to be able to leverage the same benefit from the Mach kernel: Statistically about the same. 
This is |
Previously, we used
SocketState::sendwithout wrapping it inUdpSocket::try_io. This meant that tokio had no chance of clearing the readiness flag on the socket when we actually failed to send a packet, resulting in many log messages like this:This PR refactors how we send UDP packets and when we read IP packet from the device. Instead of just polling for send-readiness, we flush all buffered packets and then check for send-readiness. That will only succeed if we managed to send all buffered packets and the socket still has space for more packets.
Typically, this buffer only has 1-2 packets. That is because we currently only ever read a single packet from the device. See #4139 for how this might change. It may have more packets when our
Allocations emit some (like multiple channel bindings in a row). Because we enforce further send-readiness before continuing, this buffer cannot grow unbounded.Resolves: #3931.