Skip to content
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

Relay protocol should have frame type for GSO packets #3035

Open
flub opened this issue Dec 12, 2024 · 2 comments
Open

Relay protocol should have frame type for GSO packets #3035

flub opened this issue Dec 12, 2024 · 2 comments
Labels
c-iroh c-iroh-relay feat New feature or request perf performance related issues
Milestone

Comments

@flub
Copy link
Contributor

flub commented Dec 12, 2024

Currently DerpCodec has the Frame::SendPacket frame, this contains a packet: Bytes which is actually 1-or-more datagrams, each prefixed with a big-endian 16-bit datagram length.

The problem is that the MagicSock (via AsyncUdpSocket) sends datagrams either as individual datagrams or as a GSO datagram. The latter has the semantics that there is a known "stride", each datagram is "stride" bytes long, the last datagram in the sequence could be smaller.

Currently these datagrams end up being split in the AsyncUdpSocket, which requires copying. They are then turned into the u16be-prefixed packets by PacketizeIter which will again involve copying. The PacketizeIter forces all datagrams sent via the relay to be copied, even those that are not GSO.

On the receiving path the u16be prefix also needs stripping of course.


This would all be much better if there was a Frame::SendGsoPacket or something which would include the stride. In this case the data could be sent without any copying and also on the receive path can be passed back to the AsyncUdpSocket unmodified using GRO. The only concern is that the max frame size should not be exceeded, but the max frame size of the MagicSock should be communicated correctly using the GSO/GRO socket options that already exist and then Quinn will not produce GSO packets larger than allowed.

Only the receive path could potentially end up with GRO size that is smaller than the sender's GSO size (because it must be the min between the UDP socket and relay max_frame_size for the MagicSock). In that case the receiver size will pay the cost of breaking up the packet. But overall this is probably worth it as this would be rather rare.

@flub flub added feat New feature or request perf performance related issues c-iroh-relay c-iroh labels Dec 12, 2024
@flub flub added this to the v1.0.0 milestone Dec 12, 2024
@flub
Copy link
Contributor Author

flub commented Jan 9, 2025

I suspect that this issue will disappear with multipath however: once the relay path is known to Quinn it will figure out a larger pMTU and GSO packets should almost never occur.

Almost though, because we will still have a single socket which means we can't disable GSO for this path and it could be that there is enough data to send that even with a larger MTU a GSO packet is generated.

@flub
Copy link
Contributor Author

flub commented Jan 9, 2025

Another note is that this isn't necessarily part of the relay protocol. Currently iroh-the-application defines its own packet format inside the SendPacket frame. This is actually very valid and it could keep doing this. So if we do this this would maybe be better as a nested protocol with frame types in it.

See also https://www.notion.so/number-zero/A-New-Relay-Protocol-1695df1306fb804f8a6ad204a5d510bd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-iroh c-iroh-relay feat New feature or request perf performance related issues
Projects
Status: No status
Development

No branches or pull requests

1 participant