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

exposes current path MTU discovered in connection stats #1967

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

behzadnouri
Copy link
Contributor

  • For debugging and performance analysis purposes we need to inspect path MTU discovered by the connection.
  • The commit exposes current path MTU discovered in connection stats.

@behzadnouri
Copy link
Contributor Author

Hey @djc , this is a pretty small trivial change.
I would appreciate if you can take a look.

@djc
Copy link
Member

djc commented Aug 14, 2024

Who's we, what's the use case, why aren't the currently exposed values good enough for you?

@djc
Copy link
Member

djc commented Aug 14, 2024

@behzadnouri
Copy link
Contributor Author

Who's we, what's the use case,

https://github.com/anza-xyz/agave

We need to understand and optimize around packet fragmentation and see if we can send larger packets without exacerbating packet fragmentation. That requires to gather some data about path MTU quinn is discovering and using between nodes in the cluster.

why aren't the currently exposed values good enough for you?

I don't think path MTU is exposed any where.

Are you aware of https://docs.rs/quinn-proto/latest/quinn_proto/struct.Connection.html#method.rtt?

I specifically need to know path MTU.

What is the concern about exposing path MTU?

@Ralith
Copy link
Collaborator

Ralith commented Aug 14, 2024

We need to understand and optimize around packet fragmentation and see if we can send larger packets without exacerbating packet fragmentation.

QUIC packets are never fragmented. That's why Quinn knows about the MTU internally in the first place: to maximize efficiency without sending packets that would exceed it.

https://docs.rs/quinn/latest/quinn/struct.Connection.html#method.max_datagram_size is a function of MTU, and may be of interest, but for streams you shouldn't need to think about this.

What is the concern about exposing path MTU?

Every new API incurs maintenance costs, so we need to ensure additions are strongly motivated.

@behzadnouri
Copy link
Contributor Author

We need to understand and optimize around packet fragmentation and see if we can send larger packets without exacerbating packet fragmentation.

QUIC packets are never fragmented.

Maybe this was misunderstood. I meant, for example, you can send any amount of data with this api:
https://docs.rs/quinn/0.11.3/quinn/struct.SendStream.html#method.write_all
but obviously that does not necessarily fit into the path MTU which then will be fragmented.

What is the concern about exposing path MTU?

Every new API incurs maintenance costs, so we need to ensure additions are strongly motivated.

That is fair and right.
But path MTU is indeed an actual thing in networking and we don't always have the luxury to abstract ourselves away from that.
I also assure you we do have strong need to understand path MTU across nodes in the cluster.

@Ralith
Copy link
Collaborator

Ralith commented Aug 14, 2024

but obviously that does not necessarily fit into the path MTU which then will be fragmented.

This is stream fragmentation, not packet fragmentation. Stream fragmentation does not incur the elevated loss risks that make packet fragmentation harmful, because the transport layer is aware of it.

I also assure you we do have strong need to understand path MTU across nodes in the cluster.

It does seem like a reasonable thing to monitor, since unexpected deviations would be a good signal of something wrong with your network.

quinn-proto/src/connection/stats.rs Outdated Show resolved Hide resolved
@djc
Copy link
Member

djc commented Aug 15, 2024

Would you mind squashing the extra commit?

@behzadnouri
Copy link
Contributor Author

Would you mind squashing the extra commit?

done.

@djc djc added this pull request to the merge queue Aug 15, 2024
@djc
Copy link
Member

djc commented Aug 15, 2024

Thanks!

Merged via the queue into quinn-rs:main with commit 5e0d96b Aug 15, 2024
8 checks passed
@behzadnouri behzadnouri deleted the path-mtu-stat branch August 15, 2024 18:24
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 this pull request may close these issues.

3 participants