fix(connlib): maintain packet order across GSO batches#8920
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the batching logic used for sending UDP datagrams via GSO. The primary changes include updating the Ecn enum to support ordering, removing segment_size from the batching key in favor of grouping by connection (src, dst, and ECN), and updating the enqueue and datagram draining logic with corresponding tests to preserve packet ordering.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| rust/ip-packet/src/lib.rs | Updated the Ecn enum derivations to include Hash, PartialOrd, and Ord for key use. |
| rust/connlib/tunnel/src/io/gso_queue.rs | Refactored the GsoQueue to group batches by connection and use a VecDeque for order. |
| rust/connlib/tunnel/src/io.rs | Removed the call to handle_timeout from send_network to reflect batching changes. |
Comments suppressed due to low confidence (1)
rust/connlib/tunnel/src/io/gso_queue.rs:60
- [nitpick] Consider renaming the shadowed 'batch_size' variable (for example, to 'current_batch_size') to avoid confusion with the tuple's batch size value.
let batch_size = *batch_size;
| // A batch is considered "ongoing" if so far we have only pushed packets of the same length. | ||
| let batch_is_ongoing = buffer.len() % batch_size == 0; | ||
|
|
||
| if batch_is_ongoing && payload_len <= batch_size { |
There was a problem hiding this comment.
The condition allows packets with a smaller length than the established batch size to be appended to the ongoing batch, which contradicts the intent to start a new batch when the packet length differs. Consider changing the condition to 'if batch_is_ongoing && payload_len == batch_size' so that only packets matching the batch size continue the current batch.
| if batch_is_ongoing && payload_len <= batch_size { | |
| if batch_is_ongoing && payload_len == batch_size { |
There was a problem hiding this comment.
This is intended, we allow for one packet that isn't of the current batch size.
The upcoming order preserving GSO queue will not need this.
thomaseizinger
force-pushed
the
fix/gso-batches
branch
from
826b9b6 to
e602527
Compare
|
If this ends up fixing things on Apple (i.e. #8899), I'll send a follow-up PR with some changelog entries. This primarily affects the Gateway actually because it was re-ordering packets as it sent them to the Clients. |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
When a platform's network driver does not support GSO, `quinn-udp` detects that and disables segmentation offloading: > 04-30 11:32:49.161 19612 19836 I connlib : quinn_udp::imp: `libc::sendmsg` failed with I/O error (os error 5); halting segmentation offload What this means is that it sets an internal field that sets the GSO batch-size to 1 (instead of the default 32). We then use this batch-size to compute, how we are meant to chunk up the already batched datagrams. As a consequence of #8920, we are now also using a "feature" of GSO where the last datagram in a GSO batch is allowed to be less than the segment size. The combination of these two features now makes it possible that we are passing a datagram to the kernel where the `segment_size` is greater than the actual length. Android's Linux kernel doesn't seem to like that an barfs when being passed such a datagram with an IO error 5. The long-term fix is to sanitise this within `quinn-udp` but in the short-term, we can do this ourselves as part of the loop where we segment the datagrams. --------- Signed-off-by: Thomas Eizinger <thomas@eizinger.io> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Despite our efforts in #8912, the current implementation still does not do enough to maintain packet ordering across GSO batches.
At present, we very aggressively batch packets of the same length together. This however is too eager when we consider packet flows such as the following:
As we can see here, the remote sends us packet batches of varying lengths:
1228 represents a "full" TCP packet so any packet following a full-packet SHOULD be grouped together into a GSO batch.
Currently, we are batching all the 1228 packets together and we ignore the fact that there were actually smaller sized packets inbetween those that belong together.
To mitigate this, we refactor the
GsoQueueto remove thesegment_sizefrom the binning key of our map and instead only group batches by their source, destination and ECN information. Within such a connection, we then create an ordered list of batches. A new batch is started if the length differs or we have previously pushed a packet that isn't of the length of the batch, therefore signalling the end of the batch.The result here looks very promising (this is loading
blog.firezone.devvia thelynxbrowser from within the headless-client docker container, so going through a Gateway running this PR):Related: #8899