Page MenuHomeFreeBSD

nuke sack_newdata
Needs ReviewPublic

Authored by rscheff_gmx.at on Thu, Jan 10, 3:17 PM.

Details

Reviewers
tuexen
Group Reviewers
transport
Summary

Combine SACK RecoveryPoint (sack_newdata) with Reno Recover (snd_recover) and remove sack_newdata.

sack_newdata was added during the original commit when SACK support was included in the BSD stack.

Both variables (snd_recover and sack_newdata) track the sequence number of snd_nxt when entering fast recovery, and are called similar in the respective RFCs. For some reason a new variable was added rather than using the existing one. While looking into improving the SACK scoreboard in preparation to implement RFC6675 and RFC6937 i found that more modern SACK code also uses sack_newdata inconsistently (e.g. only for pipe calculation).

A recent change disentangled the kernel TCPCB from access/reference by userspace programs, thus removing this variable (ABI) should be safe to do now.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 21918
Build 21162: arc lint + arc unit

Event Timeline

rscheff_gmx.at created this revision.Thu, Jan 10, 3:17 PM

Received a PM from tuexen. The Q was, why it is safe to nuke sack_newdata and completely replace it everywhere with snd_recover.

I've annotated tcp_input.c and found a few instances already, where code added after the original SACK patch (introducing the fork of the RFC variable "RecoveryPoint") already was using snd_recover for SACK-enabled flows, without bad stuff happening (obviously),but nevertheless inconsistently accessing the "wrong" variable which just happens to track the same thing.

Retaining sack_newdata seems to be pointless. The alternative to getting rid of sack_newdata, as it seems to me, would be to properly seggragate snd_recover to be used only for non SACK-enabled flows, and duplicating the relevant code with sack_newdata for SACK-enabled sessions, and use these two variables consistently depending on which flavor a particular session has.

sys/netinet/tcp_input.c
414

Recovery Point (RFC) is set in this wrapper irrespective if the session supports SACK or not.

1813

Move recovery point forward on full ACKs. Just in case?

2599

cc_cong_signal wrapper already sets snd_recover to snd_nxt. The special handling for SACK sessions below could omit setting snd_recover (which used to be sack_newdata) anew.

2607

redundant with cc_cong_signal

2608

could move up before the if clause (same on both branches)

2613

Could move this assignment up just before the SACK check, and save a small bit of code.

2701

Here is the point, where the split tracking of sack_newdata and snd_recover got mingled already. Fortunately, snd_recover gets set by the cc_cong_signal wrapper anyway (thus tracks sack_newdata for SACK-enabled sessions). If Reno and SACK had truly independent tracking of the RecoveryPoint variable, this would have failed a long time ago.

2834

another instance, where sack_newdata was already missed for SACK-enabled sessions

2838–2840

A prior example, where the sack_newdata was missed as the "proper" variable for RecoveryPoint, and snd_recover used instead.

sys/netinet/tcp_var.h
195

isn't cc_algo referenced rather often even during in-sequence processing, to warrant it to move to a front cacheline?

212

Fast Open is a compile-time option; #ifdef for the connected tcpcb variables?