Page MenuHomeFreeBSD

Avoid cwnd update for SYN sequence space
Needs ReviewPublic

Authored by rscheff_gmx.at on Jan 28 2019, 11:11 AM.

Details

Reviewers
tuexen
lstewart
Group Reviewers
transport
Summary

During testing i found an issue (filed as Bug https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=235256 ) where the Initial Window would be 11 SMSS for server-side sessions. This is due to the macro BYTES_THIS_ACK including the sequence space of the SYN bit, and regular ACK processing will commence right after cc_conn_init has initialized the initial window. When Appropriate Byte Counting (default) is enabled, this adds 1 to cwnd, but without ABC, an entire SMSS is added.

Found that the most effective way to deal with this appears to be to adjust snd_una right after the connection initialization, to include the one byte in sequence space occupied by the SYN bit.

This does not change the regular ACK processing, while making BYTES_THIS_ACK work properly (skipping cc_ack_received processing for empty SYN-ACK and final 3WHS ACK).

Also addressing an oversight, where RFC5681 mentions that cwnd is only to be updated for ACKs which acknowledge any new data. When doing ABC, this had been done as an empty ACK (keepalive, window update) will increase cwnd only by 0 bytes. But in the non ABC case, keep alives and window updates would still inflate cwnd.

adjusting initwnd when not doing ABC, as SYN-bit takes sequence space

messing with snd_una not good

before ack processing, adjust snd_una to include SYN sequence space

no SEQ_EQ

Test Plan

The following packetdrill scripts check the initial window with and without ABC, for client side and server side sessions. Note that with this patch, as the initial SYN bit is removed from the sequence space, cwnd may be reduced by 1 in other packetdrill tests, who were written against the old behavior.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 22553
Build 21688: arc lint + arc unit

Event Timeline

rscheff_gmx.at edited the summary of this revision. (Show Details)Jan 28 2019, 11:21 AM
rscheff_gmx.at edited the test plan for this revision. (Show Details)
rscheff_gmx.at added reviewers: tuexen, lstewart.
rscheff_gmx.at set the repository for this revision to rS FreeBSD src repository.
  • adding RACK to the SYN bit sequence space fix

The validation scripts for RACK, which was actually similarly affected.

One thing to note, though:

I found that RACK will perform the "RTO" with the next new data segment (eg. 11th) when no ACKs are received. I haven't read the draft closely enough to know if this is the expected behavior (since cwnd does not reflect that the congestion window is then actually 11 segments instead of 10:

window at accept: cwnd: 10000 ssthresh: 1073725440
initial window: cwnd: 10000 ssthresh: 1073725440
(segments 1-10 sent)
(timeout ~2sec)
(segment 11 sent)
RTO window: cwnd: 10000 ssthresh: 1073725440

RACK already increments snd_una to deal with FASTOPEN. Consolidate the snd_una++ in the code then?

sys/netinet/tcp_stacks/rack.c
5240

Already have the adjustment of snd_una up here.

5462–5463

snd_una is already adjusted for the FASTOPEN special case

  • consolidate snd_una adjustment for SYN bit
  • set snd_wnd in the generic case in rack
rscheff_gmx.at added inline comments.Jan 31 2019, 2:41 PM
sys/netinet/tcp_stacks/rack.c
5468

snd_una is already advanced for the syn-bit in the fast open case. move this out and do it in all cases when allowable.

tuexen requested changes to this revision.Feb 17 2019, 11:05 AM

Hi Richard,

can you please:

  1. Move your suggested changes in cc/cc_newreno.c to a different review, since they fix a different bug.
  2. Rebase you patch of tcp_stacks/rack.c to an up to date version. The changes related to tp->snd_wnd are already committed in r343661.

I think then this patch looks good.

This revision now requires changes to proceed.Feb 17 2019, 11:05 AM
  • Merge branch 'master' into fix-not-rfc3465
  • - remove non-abc fix for cong-avoidance cwnd increase on empty acks