Page MenuHomeFreeBSD

nuke sack_newdata
Needs ReviewPublic

Authored by on Jan 10 2019, 3:17 PM.


Group Reviewers

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 OK
No Unit Test Coverage
Build Status
Buildable 23921
Build 22841: arc lint + arc unit

Event Timeline created this revision.Jan 10 2019, 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.


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


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


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.


redundant with cc_cong_signal


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


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


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.


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


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


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


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

Here is one Patch, where the "wrong" variable name holding recovery has been used for SACK in the past: rS132417

Removal of the variable in a cacheline > 4 is not that critical (to retain the alignment of tcpcb at that place still.)

jtl added subscribers: rrs, jtl.Apr 25 2019, 2:19 PM
jtl added inline comments.

You need to bump this if you change the binary format (which this does).


@rrs should comment on this, since he did the cacheline usage analysis.

  • bump tcp_log_buf version marked an inline comment as done.Apr 26 2019, 8:41 AM