Skip to content

MTU Path discovery - #3633

Open
Wunka wants to merge 50 commits into
PixelGuys:masterfrom
Wunka:mtu
Open

Wunka wants to merge 50 commits into
PixelGuys:masterfrom
Wunka:mtu

Conversation

@Wunka

@Wunka Wunka commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Closes #819

This has still one big problem:

  • Only client or server increase their mtu estimate. For example if client increases it, the server can't anymore for whatever reason. It seems like the probe is send, the other gets it and does a confirmation, but that confirmation is never received.
  • After fixing the above issue, debug information needs to be deleted and the setting for mtu also needs to be removed. Also the timers and in general constants need to be adjusted to right values. Currently they are made better for debugging.

Changes:

  • Connections now have a probingState. This + the connectionState about represent the State machine in RFC 8899. The only state missing is the Error state.
  • The slow channel now also acts as a ProbeChannel. That means it will query on the connections probingState if it should send a probe and if yes it queries which size it should have and gives the probingState the SequenzIndex of the probe. With this we can then identify when a confirmation is for the probe
  • Probes are for now only send when no other should be send over the slow channel, as this is easier to implement
  • Probes are done with the mtu estimate + 50. (here there is no number mentioned in the RFC)
  • On a double loss of packets in general, the mtu estimate is reseted again to the mtuMin. Here we can maybe in the future make better heuristics. But I don't think networking is such a bottleneck that we need that.
  • variables where in general created with the corresponding RFC 8899 name in mind.

@Wunka Wunka moved this to WIP/not ready for review in PRs to review Sep 23, 2026
@Wunka
Wunka marked this pull request as ready for review September 24, 2026 11:16
@Wunka Wunka moved this from WIP/not ready for review to Low Priority in PRs to review Sep 24, 2026
@Wunka

Wunka commented Sep 27, 2026

Copy link
Copy Markdown
Contributor Author

So, a problem we have:
We kinda want to pad the normal messages, but for that we would have to go very weird around our Sendbuffer. As we don't need probes to be always received, so resending / confirmation is something we don't that much care about. That's why in this pr I have gone the way of doing a seperate channel for probe messages, which don't have a send / receive buffer. The other side just sends a confirmation packet that it was received and nothing more. This sadly means we can't pad messages, but instead have to send special probe messages which only contain nonsense data.

@IntegratedQuantum

Copy link
Copy Markdown
Member

This sadly means we can't pad messages, but instead have to send special probe messages which only contain nonsense data.

At the end of the data it probably doesn't matter. Both techniques have performance impacts, and as long as the algorithm guarantees that only a small fraction of the throughput is probes, it shouldn't matter.

@Wunka
Wunka marked this pull request as draft October 1, 2026 21:31
@Wunka Wunka moved this from Low Priority to WIP/not ready for review in PRs to review Oct 1, 2026
@Wunka
Wunka marked this pull request as ready for review October 25, 2026 13:33
@Wunka Wunka moved this from WIP/not ready for review to Low Priority in PRs to review Oct 25, 2026

@IntegratedQuantum IntegratedQuantum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't reviewed the actual MTU discovery code yet, I'll check it another day

Comment thread src/gui/windows/debug_network_advanced.zig Outdated
Comment thread src/network.zig
try windowsError(c.WSAGetLastError());
return error.UNKNOWN;
}
const setting: c_int = 1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of numbers, coud you maybe use c.TRUE?

Comment thread src/network.zig Outdated
Comment thread src/network.zig Outdated
Comment thread src/network.zig Outdated
lossyChannel: Channel, // TODO: Actually allow it to be lossy
secureChannel: SecureChannel,
slowChannel: Channel,
slowChannel: ProbeChannel,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the call, the slow channel is independent, and instead you should just add a separate check at the callsite of slowChannel.sendNextPacketAndGetSize

Then you also don't need to introduce this wrapper channel, instead you can just add another helping function to the MtuHandler struct (which is where this functionality belongs anyways).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. I was not yet done with doing everything from the call. That's why this is still there

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I just saw it on the PR board and thought it was ready

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, now it's done.

Comment thread src/network.zig Outdated
Wunka and others added 2 commits October 25, 2026 20:35
Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>
Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>
@IntegratedQuantum IntegratedQuantum moved this from Low Priority to In review in PRs to review Oct 25, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

We need Path MTU discovery

2 participants