Page MenuHomeFreeBSD

tcp: make inflight data (pipe) calculation consistent
AcceptedPublic

Authored by cc on Tue, Feb 18, 6:13 PM.

Details

Reviewers
rscheff
tuexen
rrs
glebius
Group Reviewers
transport

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 62502
Build 59386: arc lint + arc unit

Event Timeline

cc requested review of this revision.Tue, Feb 18, 6:13 PM
This revision is now accepted and ready to land.Tue, Feb 18, 6:47 PM

Question: why does this function returns signed value? And why some of the sackhint counters are signed?

There are several calls to tcp_compute_pipe() that did not check the V_tcp_do_newsack: 2 in tcp_output and 1 in tcp_input. Their behavior will change with revised SACK turned off. Commit message should explain why this is okay.

Question: why does this function returns signed value? And why some of the sackhint counters are signed?

There are several calls to tcp_compute_pipe() that did not check the V_tcp_do_newsack: 2 in tcp_output and 1 in tcp_input. Their behavior will change with revised SACK turned off. Commit message should explain why this is okay.

Way back to the original tcp_compute_pipe() in commit 12eeb81fc1ce, it was returning signed value, which I would think that fits into many places that require signed comparison or signed operation.
for example, in tcp_default_output()

...
	/*
	 * If we are doing retransmissions, then snd_nxt will
	 * not reflect the first unsent octet.  For ACK only
	 * packets, we do not want the sequence number of the
	 * retransmitted packet, we want the sequence number
	 * of the next unsent octet.  So, if there is no data
	 * (and no SYN or FIN), use snd_max instead of snd_nxt
	 * when filling in ti_seq.  But if we are in persist
	 * state, snd_max might reflect one byte beyond the
	 * right edge of the window, so use snd_nxt in that
	 * case, since we know we aren't doing a retransmission.
	 * (retransmit and persist are mutually exclusive...)
	 */
	if (!sack_rxmit) {
		if (len || (flags & (TH_SYN|TH_FIN)) ||
		    tcp_timer_active(tp, TT_PERSIST))
			th->th_seq = htonl(tp->snd_nxt);
		else
			th->th_seq = htonl(tp->snd_max);
	} else {
		th->th_seq = htonl(p->rxmit);
		p->rxmit += len;
		/*
		 * Lost Retransmission Detection
		 * trigger resending of a (then
		 * still existing) hole, when
		 * fack acks recoverypoint.
		 */
		if ((tp->t_flags & TF_LRD) && SEQ_GEQ(p->rxmit, p->end))
			p->rxmit = tp->snd_recover;
		tp->sackhint.sack_bytes_rexmit += len;                    <<<
	}
...

And given that struct sackhint is hard coded into tcpcb without a toggle, so I think it does no harm or no bother at all with revised SACK turned off. And also I think the toggle V_tcp_do_rfc6675_pipe or V_tcp_do_newsack should be included in tcp_compute_pipe() from the original commit to avoid such later discrepancy.

For these newly added two calls of tcp_compute_pipe() in tcp_output from commit 7dc78150c730, we may double confirm in meeting.

sys/netinet/cc/cc.c
405–410

Missing this part's update.