Conversation
- Fixed bug in smooth average RTT calculation - Actually assign calculated RTO value (previously fixed to 1s) - Enforce min/max RTO according to RFC - Enforce Karn to only account for new ACK in RTO calculation
There was a problem hiding this comment.
Pull request overview
This PR extends wolfIP’s TCP behavior to better handle stalled connections by adding a zero-window persist/probe mechanism, while also introducing related TCP RTO/control-RTO updates and accompanying unit tests.
Changes:
- Add TCP persist timer logic and a 1-byte “zero window probe” send path when the peer advertises a zero receive window.
- Add/adjust TCP RTO tracking (SRTT/RTTVAR, Karn handling via
PKT_FLAG_WAS_RETRANS) and implement control-segment (SYN/FIN) retransmission timing with retry caps. - Expand unit tests to cover new RTO/control-RTO behaviors and basic persist/probe behavior; update README TCP feature list.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/wolfip.c |
Implements persist/probe logic, RFC6298-style RTO updates, Karn handling, and control-segment RTO retries. |
src/test/unit/unit.c |
Adds/updates unit tests for control-RTO, RTO math, Karn skip-on-retrans, and persist/probe basics. |
README.md |
Documents TCP RTO computation support in the feature matrix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (seg_len == 0) { | ||
| desc = fifo_next(&t->sock.tcp.txbuf, desc); | ||
| continue; | ||
| } | ||
| payload = (const uint8_t *)seg->ip.data + hdr_len; | ||
| probe_byte = payload[0]; | ||
| if (tcp_seq_leq(seg_seq, t->sock.tcp.snd_una) && | ||
| tcp_seq_lt(t->sock.tcp.snd_una, tcp_seq_inc(seg_seq, seg_len))) { | ||
| probe_seq = t->sock.tcp.snd_una; | ||
| break; | ||
| } | ||
| probe_seq = seg_seq; |
There was a problem hiding this comment.
In tcp_send_zero_wnd_probe(), probe_seq may be set to snd_una when snd_una falls within an existing queued segment, but probe_byte is always taken from payload[0] (the first byte of that segment). That can send incorrect data for the chosen sequence number (probe_seq != seg_seq), potentially corrupting the stream and/or eliciting incorrect ACK behavior. Compute the byte offset within the chosen segment (probe_seq - seg_seq, under the same in-range condition) and use that byte for probe->data[0].
| if (seg_len == 0) { | |
| desc = fifo_next(&t->sock.tcp.txbuf, desc); | |
| continue; | |
| } | |
| payload = (const uint8_t *)seg->ip.data + hdr_len; | |
| probe_byte = payload[0]; | |
| if (tcp_seq_leq(seg_seq, t->sock.tcp.snd_una) && | |
| tcp_seq_lt(t->sock.tcp.snd_una, tcp_seq_inc(seg_seq, seg_len))) { | |
| probe_seq = t->sock.tcp.snd_una; | |
| break; | |
| } | |
| probe_seq = seg_seq; | |
| uint32_t byte_offset; | |
| if (seg_len == 0) { | |
| desc = fifo_next(&t->sock.tcp.txbuf, desc); | |
| continue; | |
| } | |
| payload = (const uint8_t *)seg->ip.data + hdr_len; | |
| if (tcp_seq_leq(seg_seq, t->sock.tcp.snd_una) && | |
| tcp_seq_lt(t->sock.tcp.snd_una, tcp_seq_inc(seg_seq, seg_len))) { | |
| probe_seq = t->sock.tcp.snd_una; | |
| byte_offset = (uint32_t)(probe_seq - seg_seq); | |
| probe_byte = payload[byte_offset]; | |
| break; | |
| } | |
| probe_seq = seg_seq; | |
| probe_byte = payload[0]; |
| if (t->sock.tcp.state == TCP_LAST_ACK && tcp_seq_leq(fin_acked, ack)) { | ||
| tcp_ctrl_rto_stop(t); | ||
| t->sock.tcp.state = TCP_CLOSED; | ||
| close_socket(t); | ||
| return; | ||
| } |
There was a problem hiding this comment.
tcp_ack() now stops the control RTO timer and closes the socket when TCP_LAST_ACK observes the FIN being ACKed, but tcp_input() has a separate TCP_LAST_ACK branch that closes the socket without calling tcp_ack(). That means the new control RTO timer may remain scheduled in the timer heap after the connection is closed, consuming timer slots until expiry. Consider routing TCP_LAST_ACK ACK processing through tcp_ack() (or at least calling tcp_ctrl_rto_stop() in the TCP_LAST_ACK path before close_socket()).
| #define TCP_CTRL_RTO_MAXRTX 6U | ||
| #define TCP_RTO_MIN_MS 1000U | ||
| #define TCP_RTO_MAX_MS 60000U | ||
| #define TCP_RTO_G_MS 1U | ||
| #define TCP_PERSIST_MIN_MS 1000U | ||
| #define TCP_PERSIST_MAX_MS 60000U |
There was a problem hiding this comment.
PR title/description focus on “Zero window probe”, but this change set also introduces substantial RFC6298 RTO computation changes (srtt/rttvar, Karn handling flag), plus a new control-segment RTO mechanism for SYN/FIN. This looks like more than a single feature and makes review/traceability harder—please update the PR title/description to reflect the full scope or split the RTO/control-RTO work into a separate PR.
| tcp_hlen = (uint32_t)(tcp->hlen >> 2); | ||
| ck_assert_uint_eq(ip_len - (IP_HEADER_LEN + tcp_hlen), 1U); |
There was a problem hiding this comment.
The new persist/probe test validates that a 1-byte TCP segment is sent, but it doesn’t assert that the probe’s sequence number and payload byte match the queued TX data at that sequence (critical for correctness when snd_una is not at the start of the segment). Add assertions for tcp->seq (expected probe_seq) and tcp->data[0] (expected byte at that offset in the queued segment).
| tcp_hlen = (uint32_t)(tcp->hlen >> 2); | |
| ck_assert_uint_eq(ip_len - (IP_HEADER_LEN + tcp_hlen), 1U); | |
| tcp_hlen = (uint32_t)(tcp->hlen >> 2); | |
| /* Verify 1-byte TCP payload for persist probe. */ | |
| ck_assert_uint_eq(ip_len - (IP_HEADER_LEN + tcp_hlen), 1U); | |
| /* Verify probe sequence number and payload byte match queued TX data. */ | |
| { | |
| uint32_t probe_seq = ts->sock.tcp.snd_una; | |
| ck_assert_uint_eq(ee32(tcp->seq), probe_seq); | |
| ck_assert_uint_eq(tcp->data[0], 0x18); | |
| } |
1 participant
Based on previous PR #34