-
Notifications
You must be signed in to change notification settings - Fork 213
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
fix(iroh-net): Also check the last packet in MagicSock::poll_recv
#2650
Conversation
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2650/docs/iroh/ Last updated: 2024-08-26T14:31:59Z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's at least add trace logging for this. maybe we should also do the metrics directly as well. I think two metrics:
- number of GRO datagrams
- number of datagrams a GRO datagram expands into
Co-authored-by: Floris Bruynooghe <[email protected]>
Co-authored-by: Floris Bruynooghe <[email protected]>
iroh-net/src/magicsock/metrics.rs
Outdated
@@ -90,6 +92,7 @@ impl Default for Metrics { | |||
recv_data_ipv4: Counter::new("recv_data_ipv4"), | |||
recv_data_ipv6: Counter::new("recv_data_ipv6"), | |||
recv_datagrams: Counter::new("recv_datagrams"), | |||
recv_gro_packets: Counter::new("recv_gro_packets"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to call this "datagram", so recv_gro_datagrams
.
I think this is in line with the RFC9000 terminology:
- A datagram is a single UDP, err... datagram sent. This is pretty universal UDP networking terminology.
- A packet is something that can be sent in a single datagram. Though some packets can be coalesced together into a datagram where needed (mostly the handshake).
- A frame is what is put in packets, again you can have multiple frames per packet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, our log messages confuse this terminology too. one day that will be cleaned up too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I went back and forth on that quite a bit.
Generally love any advice to consistent naming here. I've spent some time googling around, but people are arguing many ways.
You say "some packets can be coalesced together into a datagram", what kinds of packets are we talking about here?
This is super confusing to my ears because when I hear "packet" I think "IP packet". But I guess it can also mean "QUIC Packet"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename the metrics, otherwise looks good.
Description
These are the quinn docs for
RecvMeta::stride
:So,
meta.len
isn't necessarily evenly divided bymeta.stride
, and the last packet could be smaller than the stride.The previous code assumed so, however. I think that's a bug. Not bad enough that this caused issues, but still bad.
Breaking Changes
None
Notes & open questions
What are the exact effects of this code having been incorrect before?
I guess one piece is that the metrics for computing the # received bytes was way off.
Should we test this somehow specifically?
Change checklist
[ ] Documentation updates following the style guide, if relevant.[ ] Tests if relevant.