Bug 203663 - [tcp] Incorrect dupack processing to detect loss
Summary: [tcp] Incorrect dupack processing to detect loss
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Hiren Panchasara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-09 19:32 UTC by Hiren Panchasara
Modified: 2016-02-25 04:41 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Hiren Panchasara freebsd_committer 2015-10-09 19:32:05 UTC
Quoting Randall:
"When we recognize a dup-ack we *will not* recognize it if for example if the rwnd changes even if new SACK information is reported in the sack blocks. This is due to the fact that in non-SACK you don't (on purpose) recognize ACK?s where the window changed (since you can?t really tell if its a plain window update or a dup-ack).. This means we occasionally miss out on stroking the dup-ack counter and getting out of recovery...."

Another report of the same/similar problem:
https://lists.freebsd.org/pipermail/freebsd-net/2015-September/043387.html

This bug leads to us detecting loss later than we should have. And that leads to suboptimal loss recovery as a whole.
Comment 1 Hiren Panchasara freebsd_committer 2015-11-19 23:05:45 UTC
https://reviews.freebsd.org/D4225
Comment 2 commit-hook freebsd_committer 2015-12-08 21:21:52 UTC
A commit references this bug:

Author: hiren
Date: Tue Dec  8 21:21:48 UTC 2015
New revision: 292003
URL: https://svnweb.freebsd.org/changeset/base/292003

Log:
  One of the ways to detect loss is to count duplicate acks coming back from the
  other end till it reaches predetermined threshold which is 3 for us right now.
  Once that happens, we trigger fast-retransmit to do loss recovery.

  Main problem with the current implementation is that we don't honor SACK
  information well to detect whether an incoming ack is a dupack or not. RFC6675
  has latest recommendations for that. According to it, dupack is a segment that
  arrives carrying a SACK block that identifies previously unknown information
  between snd_una and snd_max even if it carries new data, changes the advertised
  window, or moves the cumulative acknowledgment point.

  With the prevalence of Selective ACK (SACK) these days, improper handling can
  lead to delayed loss recovery.

  With the fix, new behavior looks like following:

  0) th_ack < snd_una --> ignore
  Old acks are ignored.
  1) th_ack == snd_una, !sack_changed --> ignore
  Acks with SACK enabled but without any new SACK info in them are ignored.
  2) th_ack == snd_una, window == old_window --> increment
  Increment on a good dupack.
  3) th_ack == snd_una, window != old_window, sack_changed --> increment
  When SACK enabled, it's okay to have advertized window changed if the ack has
  new SACK info.
  4) th_ack > snd_una --> reset to 0
  Reset to 0 when left edge moves.
  5) th_ack > snd_una, sack_changed --> increment
  Increment if left edge moves but there is new SACK info.

  Here, sack_changed is the indicator that incoming ack has previously unknown
  SACK info in it.

  Note: This fix is not fully compliant to RFC6675. That may require a few
  changes to current implementation in order to keep per-sackhole dupack counter
  and change to the way we mark/handle sack holes.

  PR:			203663
  Reviewed by:		jtl
  MFC after:		3 weeks
  Sponsored by:		Limelight Networks
  Differential Revision:	https://reviews.freebsd.org/D4225

Changes:
  head/sys/netinet/tcp_input.c
  head/sys/netinet/tcp_sack.c
  head/sys/netinet/tcp_var.h