CVPN-2346 Implement GSO offload on lightway-server#413
Conversation
|
Code coverage summary for 807f7d1: ✅ Region coverage 68% passes |
2b45738 to
ed9e475
Compare
b5849fe to
c62619f
Compare
| } | ||
|
|
||
| impl PluginList { | ||
| #[cfg(target_os = "linux")] |
There was a problem hiding this comment.
This need not be a linux specific method. Looks generic
There was a problem hiding this comment.
Failed lint because the function is unused on other platforms, needed to add back #[allow(dead_code)].
| // Expose the full slab to `recv_gso` as `&mut [u8]`. | ||
| // SAFETY: every byte of the slab was zero-initialized at | ||
| // construction; subsequent iters only ever shrunk `len` or | ||
| // overwrote bytes. We never hand out uninitialized memory. |
There was a problem hiding this comment.
This does not sound safe, as pkt is mutable we could also create new BytesMut and replace it.
So now if you reserve, it might be unintialized.
I think what you want is https://docs.rs/bytes/latest/bytes/struct.BytesMut.html#method.spare_capacity_mut which gives pointer to spare buffer which you can sent to recv_gso
There was a problem hiding this comment.
Because spare_capacity_mut(&mut self) -> &mut [MaybeUninit<u8>], I think we ultimately still need a unsafe cast on the &mut [MaybeUninit<u8>] because tun_rs::AsyncDevice::recv() only takes &mut [u8] in the end. I will think about it.
Interestingly there's also std::io::Read::read_buf in the nightly std which takes MaybeUninit<u8> (which tun-rs could use) but it is never stabilized.
Edit: I changed recv signatures to accept MaybeUninit and do one unsafe cast in the end, I am hoping to drop it once tun-rs accepts MaybeUninit. Please take a look at e995ffa 👀
4642134 to
39cf489
Compare
kp-samuel-tam
left a comment
There was a problem hiding this comment.
Some updates and comments to help with reviews.
| // SAFETY: `tun_rs::AsyncDevice::recv` takes `&mut [u8]` and forwards | ||
| // to `libc::read(2)`. The kernel only writes — it never dereferences | ||
| // userspace memory for reading — so handing it our uninitialized slab | ||
| // is sound at the syscall boundary. The unsoundness lives in *Rust*: | ||
| // constructing a `&mut [u8]` over uninitialized bytes is UB per strict | ||
| // aliasing rules, even if no one reads them. This cast is the only | ||
| // place we paper over that gap. Delete it (and revert this signature) | ||
| // once `tun-rs` exposes a `MaybeUninit`-aware recv. | ||
| #[allow(unsafe_code)] | ||
| let raw = | ||
| unsafe { std::slice::from_raw_parts_mut(buf.as_mut_ptr().cast::<u8>(), buf.len()) }; |
There was a problem hiding this comment.
This is new to cast MaybeUninit away for tun.recv() which doesn't support MaybeUninit yet.
|
|
||
| /// Raw read from Tun, returning the full virtio frame (header + payload). | ||
| #[cfg(target_os = "linux")] | ||
| pub async fn recv_gso(&self, buf: &mut [std::mem::MaybeUninit<u8>]) -> IOCallbackResult<usize> { |
There was a problem hiding this comment.
A lot of read/recv signatures are changed from &mut [u8] to &mut [MaybeUninit<u8>].
| // IFF_VNET_HDR requires a zeroed `virtio_net_hdr` prefix | ||
| // on every write (NEEDS_CSUM=0, GSO_NONE). | ||
| let hdr_len = tun_rs::VIRTIO_NET_HDR_LEN; | ||
| let mut prefixed = bytes::BytesMut::zeroed(hdr_len); | ||
| prefixed.extend_from_slice(&buf[..]); | ||
| tun.try_send(&prefixed[..]) | ||
| .map(|n| n.saturating_sub(hdr_len)) |
There was a problem hiding this comment.
This can be optimized to be writev but I'd defer it for now.
| /// When `Some`, the wolfssl IO callback `send()` appends raw | ||
| /// encrypted segments here instead of sending to the socket. | ||
| /// After all segments are collected, `udp_send_gso` wraps each | ||
| /// with `wire::Header`, runs plugins, and sends via `send_gso`. | ||
| pub(crate) gso_buf: Option<BytesMut>, |
There was a problem hiding this comment.
This is not new but this is a buffer to "intercept" wolfssl doing send immediately after the packet is encrypted. Now when this is Some(_) it will store packets here and return early; for us to do GSO instead of shooting individual packets out right away.
Edit: A future plan is to do streaming/in-place encryption on the original superpacket, so this would not be needed... but deferred
| @@ -48,32 +49,45 @@ impl std::fmt::Display for BindMode { | |||
|
|
|||
| fn send_to_socket( | |||
There was a problem hiding this comment.
Not new, but did additional refactoring on fn send_to_socket, now .with_control() is unconditional but kernel handles a cmsg_len = 0 cmsg just fine.
| send_to_socket(&self.sock, buf, &peer_addr.1, self.reply_pktinfo) | ||
| send_to_socket( | ||
| &self.sock, | ||
| &[IoSlice::new(buf)], |
There was a problem hiding this comment.
Not new, but this is changed buf -> &[IoSlice::new(buf)] because send_to_socket signature changed for iovec aka scatter/gather IO.
| } | ||
| } | ||
|
|
||
| async fn inside_io_loop_default( |
There was a problem hiding this comment.
This is pulled directly from the inside_io_loop tokio::spawn code and unchanged.
| } | ||
|
|
||
| #[cfg(target_os = "linux")] | ||
| async fn inside_io_loop_gso( |
There was a problem hiding this comment.
This is the GSO replacement for original inside_io_loop to handle the vnet_hdr and other GSO stuff.
| # headers per segment can exceed what sendmsg(UDP_SEGMENT) accepts, causing the | ||
| # kernel to return EMSGSIZE ("Message too long") on the outside socket. | ||
| ip link set dev "${tunname}" gso_max_size 60300 | ||
| ip link set dev "${tunname}" up |
There was a problem hiding this comment.
This is new to cap gso_max_size to not blow up sendmsg(UDP_SEGMENT) later. We also need this in deployment scripts because it's not in the lightway code anymore.
| /// `out` holds exactly the one segment's wire bytes. | ||
| /// | ||
| /// `out.capacity()` must be ≥ one segment's maximum wire length. | ||
| pub(crate) fn build_segment( |
There was a problem hiding this comment.
I added some new tests below for this function - this is run to construct a complete, checksum-computed packet for wolfssl to encrypt. Could be written in IO scatter/gather style and avoid the copies if we use the wolfssl streaming/in-place encrypt APIs.
|
I tried to give it a go in my dev environments. And then I tried to test in 2 VMs, where server panic'd with the following after starting the iperf3 test. I set the tun gso properly similar to earthly script. Debugging whether it is my setup issue. |
0475f8d to
057ded1
Compare
|
Thank you all for the reviews 🙌 I have updated the PR but mistakenly rebased on a wrong branch, so now even if I fixed it, the force-push To make the review easier, please see https://github.com/expressvpn/lightway/compare/39cf489..057ded1 for changes since May 28. You can verify the two commit hashes and history with curl -s "https://api.github.com/repos/expressvpn/lightway/issues/413/events?per_page=100" | jq '[.[] | select(.event == "head_ref_force_pushed") | {created_at, actor: .actor.login, commit_id}]'And for the crash that @kp-mariappan-ramasamy raised, that is on me. That was not a setup issue but a subtle bug in the lightway/lightway-core/src/gso.rs Lines 195 to 200 in cd00ef8 It's subtle but well-documented that kernel breaks the virtio-net spec on
|
|
|
||
| // No tcp_clamp_mss — GSO packets are never SYN | ||
|
|
||
| // Plugins see the whole superpacket as one packet |
There was a problem hiding this comment.
There can be some plugins to reject/dropwithreply individual packet, ex dns requests. This will break that.
| // SAFETY: `recv_gso` wrote `len` bytes into the spare capacity | ||
| // starting at slab offset 0 (the window was just compacted back | ||
| // there). `len ≤ pkt.capacity()` because the kernel wrote into | ||
| // a slice of exactly that length. | ||
| #[allow(unsafe_code)] | ||
| unsafe { | ||
| pkt.set_len(len); | ||
| } | ||
|
|
||
| if len <= VIRTIO_NET_HDR_LEN { | ||
| tracing::warn!("TUN read too short ({len} <= {VIRTIO_NET_HDR_LEN})"); | ||
| pkt.clear(); | ||
| continue; | ||
| } | ||
|
|
||
| let hdr = match VirtioNetHdr::from_bytes(&pkt[..VIRTIO_NET_HDR_LEN]) { | ||
| Ok(hdr) => *hdr, | ||
| Err(err) => { | ||
| tracing::warn!("Failed to decode virtio header: {err}"); | ||
| pkt.clear(); | ||
| continue; | ||
| } | ||
| }; | ||
| // Strip the virtio header — `pkt` is now the IP payload. | ||
| pkt.advance(VIRTIO_NET_HDR_LEN); |
There was a problem hiding this comment.
I think these lines can be moved inside recv_gso and make the api return hdr and len along with bytesMut
async fn recv_gso(&self, buf: &mut BytesMut) -> IOCallbackResult<(usize, VirtioNetHdr)>;
Better to keep unsafe code inside the library abstractions instead of from main library code
| if let Some(gso_buf) = self.gso_buf.as_mut() { | ||
| // GSO: buffer raw encrypted data; udp_send_gso | ||
| // will wrap each segment with wire::Header later. | ||
| if gso_buf.is_empty() { | ||
| self.gso_size = buf.len(); | ||
| } | ||
| gso_buf.extend_from_slice(buf); | ||
| IOCallbackResult::Ok(buf.len()) |
There was a problem hiding this comment.
We can move this part inside self.udp_send() so expresslane handling in connection.rs::send_expresslane_data() does not need to handle this separately.
|
There are 2 other issues i found:
|
75e8942 to
a8a3690
Compare
a8a3690 to
c6f7619
Compare
| // Read directly into the spare capacity. BytesMut's | ||
| // spare_capacity_mut returns &mut [MaybeUninit<u8>] so there's | ||
| // no zero-init pass on the hot path. | ||
| let n = match self.0.recv_gso(buf.spare_capacity_mut()).await { | ||
| IOCallbackResult::Ok(n) => n, | ||
| IOCallbackResult::WouldBlock => return IOCallbackResult::WouldBlock, | ||
| IOCallbackResult::Err(e) => return IOCallbackResult::Err(e), | ||
| }; | ||
|
|
||
| if n <= VIRTIO_NET_HDR_LEN { | ||
| tracing::warn!(n, "tun recv_gso: read shorter than virtio header"); | ||
| metrics::tun_recv_gso_short_read(); | ||
| // Discard the partial read (buf is untouched — no set_len) | ||
| // and return WouldBlock so the caller's recv loop retries | ||
| // immediately instead of treating this as a hard error. | ||
| return IOCallbackResult::WouldBlock; | ||
| } | ||
|
|
||
| // SAFETY: AppUtilsTun::recv_gso wrote exactly `n` bytes into | ||
| // the spare slab; n <= buf.capacity() because the kernel wrote | ||
| // into a slice of that length. | ||
| #[allow(unsafe_code)] | ||
| unsafe { | ||
| buf.set_len(n); | ||
| } | ||
|
|
||
| // SAFETY for VirtioNetHdr::from_bytes: BytesMut is heap-backed | ||
| // and 8-byte aligned; `n > VIRTIO_NET_HDR_LEN` was just checked. | ||
| let hdr = match VirtioNetHdr::from_bytes(&buf[..VIRTIO_NET_HDR_LEN]) { | ||
| Ok(h) => *h, | ||
| Err(e) => { | ||
| tracing::warn!(?e, "tun recv_gso: virtio header decode failed"); | ||
| buf.clear(); | ||
| return IOCallbackResult::WouldBlock; | ||
| } | ||
| }; | ||
| buf.advance(VIRTIO_NET_HDR_LEN); | ||
|
|
||
| // Note: payload bytes (post-virtio-strip), not raw kernel | ||
| // bytes — see metrics::tun_to_client doc. | ||
| metrics::tun_to_client(buf.len()); | ||
| IOCallbackResult::Ok((buf.len(), hdr)) |
There was a problem hiding this comment.
This is a low level processing. It should be moved inside app_utils::tun::recv_gso
c6f7619 to
fd1ffcf
Compare
|
@kp-samuel-tam Did you test the local namespace based env. Please let me know if you test the following cases:
|
| if n <= VIRTIO_NET_HDR_LEN { | ||
| tracing::warn!(n, "tun recv_gso: read shorter than virtio header"); | ||
| crate::metrics::tun_recv_gso_short_read(); | ||
| // Discard the partial read (buf is untouched — no set_len) | ||
| // and return WouldBlock so the caller's recv loop retries | ||
| // immediately instead of treating this as a hard error. | ||
| return IOCallbackResult::WouldBlock; | ||
| } | ||
|
|
||
| // SAFETY: TunDirect::recv_gso wrote exactly `n` bytes into the spare | ||
| // slab; n <= buf.capacity() because the kernel wrote into a slice of | ||
| // that length. | ||
| #[allow(unsafe_code)] | ||
| unsafe { | ||
| buf.set_len(n); | ||
| } | ||
|
|
||
| // SAFETY for VirtioNetHdr::from_bytes: BytesMut is heap-backed | ||
| // and 8-byte aligned; `n > VIRTIO_NET_HDR_LEN` was just checked. | ||
| let hdr = match lightway_core::VirtioNetHdr::from_bytes(&buf[..VIRTIO_NET_HDR_LEN]) { | ||
| Ok(h) => *h, | ||
| Err(e) => { | ||
| tracing::warn!(?e, "tun recv_gso: virtio header decode failed"); | ||
| buf.clear(); | ||
| return IOCallbackResult::WouldBlock; | ||
| } | ||
| }; | ||
| buf.advance(VIRTIO_NET_HDR_LEN); |
There was a problem hiding this comment.
Can you move this implementation even further down to TunDirect ?
|
@kp-mariappan-ramasamy I bumped tun-rs 2.5.1 -> 2.8.5 in this branch, which should resolve the issue properly now. Tested the offload off → on → off cycle in both real wire and local dev env. |
c6208d3 to
dfeafe3
Compare
kp-mariappan-ramasamy
left a comment
There was a problem hiding this comment.
Thanks for the PR. LGTM
Add the `gso` module to lightway-core: VirtioNetHdr type, virtio-net
GSO/checksum constants, and the per-segment fixup primitives that
split a GSO superpacket into wire-format segments (IP ID, TCP seq,
checksums).
calc_hdr_len and build_segment return Result<_, Gso{Hdr,Seg}Error>
with metric_reason() labels. VirtioNetHdr has an is_tcp() method.
Five reason-labeled counters land in metrics.rs.
build_segment is in safe Rust via pnet_packet's mutable packet
types (Ipv4 / Ipv6 / Tcp / Udp); only unsafe left in the module is
VirtioNetHdr::from_bytes.
Add `send_gso` method for sending a concatenated wire buffer via kernel GSO (UDP_SEGMENT). Stub implementations in client TCP/UDP, server TCP, and test harnesses satisfy the trait contract.
Add a `GsoBuffer` to TlsIOAdapter: a per-connection coalescing buffer + typed batch state (Passthrough / Pending / Coalescing). The wolfssl send() callback coalesces raw encrypted segments via GsoBuffer::put() when a batch is open; udp_send_gso wraps each with wire::Header and ships the whole frame as one sendmsg via the vectored send_gso callback. The state machine encodes "first segment fixes gso_size" with NonZeroUsize in the Coalescing variant — Some(0)-after-first-segment becomes representationally impossible. Reservations are bounded by MAX_GSO_FRAME_BYTES (= MAX_GSO_SEGS * MAX_OUTSIDE_MTU) on the first open(); subsequent batches reuse the allocation. Connections that never coalesce pay no heap. Zero-copy fast path when no outside plugins are configured: scatter- gather via iovec with a shared header buffer and borrowed slices of the coalescing buffer. The plugin path builds each segment as its own BytesMut and enforces the uniform-stride requirement of UDP_SEGMENT.
Add inside_data_received_gso and send_to_outside_gso to Connection.
They process a GSO superpacket as a single packet through plugins /
encoder, then split into per-segment encrypted frames coalesced into
the connection's GsoBuffer for batch send via UDP_SEGMENT.
send_to_outside_gso drops aggregates with gso_size == 0, gso_segs >
MAX_GSO_SEGS (UDP_MAX_SEGMENTS kernel cap), or per-segment
(hdr_len + gso_size) > mtu, and increments gso_dropped_zero_gso_size,
gso_dropped_iov_overflow, gso_dropped_oversized_segment,
gso_dropped_invalid_hdr_len{reason}, or gso_send_failed accordingly.
All dropping paths surface as InvalidPacketError::InvalidGsoPacket.
GsoBuffer::reset() on every exit path ensures coalescing state
never persists into the next packet.
Add an `offload` field to TunConfig that turns on IFF_VNET_HDR on the TUN device. Add `recv_gso` for raw reads that include the virtio_net_hdr prefix; `try_send` prepends a zeroed virtio header when offload is on.
Extend send_to_socket with an optional gso_size and a UDP_SEGMENT cmsg for kernel-level segmentation. Wire up the real send_gso on the server's outside UdpSocket.
Add `enable_tun_offload` config option and wire it through ServerConfig to main. Extract the default inside IO loop into its own function and add `inside_io_loop_gso`, which reads virtio-framed superpackets from TUN and dispatches GSO vs single-packet paths. Spawn picks one loop based on the cfg flag in a single if/else. gso_max_size capping lives in tests/server/docker-entrypoint.sh, not in the server.
Lets the GSO recv loop use BytesMut::spare_capacity_mut() directly, dropping the one-time 65 KB zero-init and one of the two call-site unsafe blocks. The cast back to &mut [u8] now lives only at the syscall boundary in TunDirect::recv_gso, with a comment pointing at the tun-rs upstream gap to track for cleanup.
New Earthfile target `run-udp-tun-offload-test` runs the standard UDP e2e against a server started with `--enable-tun-offload`, exercising the TUN IFF_VNET_HDR + UDP_SEGMENT + GRO path end-to-end. Wired into `run-all-tests` so CI picks it up alongside the other UDP variants.
tun-rs 2.8.5 issues TUNSETOFFLOAD(0) in the offload=false attach path (tun-rs PR #141), clearing the device-wide TUN_F_* mask left behind on a persistent TUN by a prior offload=true attach.
dfeafe3 to
d2c3070
Compare
Description
Implement GSO on server side on DTLS and Expresslane, specifically on bulk server->client traffic. This consistently halves the total syscalls used during bulk transfers, and also improves aggregated server throughput by 2x for multiple clients doing transfers.
When
--enable-tun-offloadis set, the server reads TSO superpackets from the TUN withIFF_VNET_HDR, segments them in userspace, and emits each superpacket as a singlesendmsg(UDP_SEGMENT)instead of N per-segment syscalls. On a single-flow iperf3 reverse test the kernel UDP send path collapses near-completely:udp_sendmsg0.71% → ~0.05%,sock_alloc_send_pskb2.61% → ~0.13%,mlx5e_xmit1.88% → ~0%.Trade-off: kernel work is replaced with userspace work (per-segment IP/TCP/UDP checksum recomputation, segment assembly). Kernel-side wins are clear and measurable; userspace cost is now the dominant factor.
Pacing: each
sendmsg(UDP_SEGMENT)produces a NIC burst of up to N segments. This can exceed receiver socket buffer depth and increase tail drops at peak rates. We will need to revisit better TX pacing under congested links.Future work will focus on compatibility with TUN backends like io_uring, GRO on the server side, and full GSO/GRO on client side, where single-flow workloads should see the biggest visible speedup not in this PR.
Motivation and Context
See ticket CVPN-2346.
How Has This Been Tested?
Types of changes
Checklist:
main