Skip to content

XDE should listen to MSS suggestions made by senders for rack-external traffic #748

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

Closed
FelixMcFelix opened this issue May 7, 2025 · 0 comments · Fixed by #749
Closed

Comments

@FelixMcFelix
Copy link
Collaborator

The sender (illumos, viona) may have learned and deliberately set an MSS value on LSO traffic lower than the standard Ethernet MTU, accounting for tunnels and the like.

opte/xde/src/xde.rs

Lines 1779 to 1788 in c9c3d51

// Boost MSS to use full jumbo frames if we know our path
// can be served purely on internal links.
// Recall that SDU does not include L2 size, hence 'non_eth_payl'
let mut flags = offload_flags;
let inner_mtu = if mtu_unrestricted {
src_dev.underlay_capab.mtu - encap_len
} else {
u32::from(ETHERNET_MTU)
};
let mss = inner_mtu - non_eth_payl_bytes;

When we don't see mtu_unrestricted, we should be falling back to the value included in the dblk_t.

Part of unblocking PMTUD as in oxidecomputer/omicron#7998.

FelixMcFelix added a commit that referenced this issue May 7, 2025
First, we missed one branch in the overlay action for setting the
`mtu_unrestricted` property on VPC-local packets. This ended up getting
missed because the bench/xde-test runner set up the routing table
differently to omicron.

Second, when we open up some limited ICMP families for Nexus (intending
to support PMTUD), the illumos TCP stack will have its own (and better!)
idea about what MSS is actually assigned to a flow. We now reuse this
MSS when not explicitly boosting a flow into jumbo frame territory.

Closes #747.
Closes #748.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant