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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Aug 26, 2024
Merged

fix(iroh-net): Also check the last packet in MagicSock::poll_recv #2650

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
473df5e
fix(iroh-net): Don't drop the last packet in a transmit read
matheus23 Aug 20, 2024
2d9a5e7
refactor: Use `chunks_mut` in favor of custom iteration logic
matheus23 Aug 20, 2024
b9132e9
Update iroh-net/src/magicsock.rs
matheus23 Aug 26, 2024
3cdaf08
Update iroh-net/src/magicsock.rs
matheus23 Aug 26, 2024
318b3fa
Add `recv_gro_packets` metric
matheus23 Aug 26, 2024
ba30fa9
Merge branch 'main' into matheus23/more-packets
matheus23 Aug 26, 2024
f71a02f
rename `recv_gro_packets` to `recv_gro_datagrams`
matheus23 Aug 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions iroh-net/src/magicsock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,9 +699,21 @@ impl MagicSock {
for (meta, buf) in metas.iter_mut().zip(bufs.iter_mut()).take(msgs) {
let mut is_quic = false;
let mut quic_packets_count = 0;
if meta.len > meta.stride {
trace!(%meta.len, %meta.stride, "GRO packet received");
inc!(MagicsockMetrics, recv_gro_packets);
}

// find disco and stun packets and forward them to the actor
for packet in buf[..meta.len].chunks_mut(meta.stride) {
if packet.len() < meta.stride {
trace!(
len = %packet.len(),
%meta.stride,
"Last GRO datagram smaller than stride",
);
}

let packet_is_quic = if stun::is(packet) {
trace!(src = %meta.addr, len = %meta.stride, "UDP recv: stun packet");
let packet2 = Bytes::copy_from_slice(packet);
Expand Down
3 changes: 3 additions & 0 deletions iroh-net/src/magicsock/metrics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ pub struct Metrics {
pub recv_data_ipv6: Counter,
/// Number of QUIC datagrams received.
pub recv_datagrams: Counter,
/// Number of datagrams received using GRO
pub recv_gro_packets: Counter,

// Disco packets
pub send_disco_udp: Counter,
Expand Down Expand Up @@ -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"),
Copy link
Contributor

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.

Copy link
Contributor

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 :)

Copy link
Member Author

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"?


// Disco packets
send_disco_udp: Counter::new("disco_send_udp"),
Expand Down
Loading