Subject: Re: some sack fixes
To: Jonathan Stone <jonathan@dsg.stanford.edu>
From: Kentaro A. Kurahone <kurahone@sigusr1.org>
List: tech-net
Date: 03/15/2005 21:45:21
--h31gzZEtNLTqOjlF
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Mar 15, 2005 at 01:12:22PM -0800, Jonathan Stone wrote:
>=20
> [... [discussion of SACK blocks for peers that do SACK but not
> rfc-1323 timestamps]]
>=20
> For now, I beleive NetBSD requires a conservative strategy: one
> where we have a SACK that is correct in all cases. I'd even give
> up DSACK to be sure that the SACK-only functionality is
> fully RFC-conformant and correct.

Agreed.  I'm not sure how many TCP stacks take advantage of D-SACK
(And for detecting spurious retransmits, RFC3522 feels like a better
solution[0]).  Removing D-SACK would simplify the code a bit.

> Sending one less SACK block than the absolute maximum, in the (rare?)
> case of a peer that will do SACK, but not timestamps, fits my definition
> of "conservative", as it eliminates some corner-case logic.
>=20
> On that note: just by quickly skimming the code, I can no longer
> quickly convince myself that tcp_update_sack_list() is fully
> conformant with RFC-2018 .  The original FreeBSDBSD code comments that:
>=20
>   first reported SACK block MUST be the most recent one [[most recently
>   received? -- jrs]], and the subsequent blocks SHOULD be in the order
>   in which they arrived at the receiver. These two conditions make the
>   implementation fully compliant with RFC-2018.
>=20
> Is the SHOULD implicitly guaranteed by the tailq, or by something
> else, or do we not guarantee it? Can we easily add kernel ASSERT()s to
> check the `MUST', and a DIAGNOSTIC check for the SHOULD?

The tailq should implicity guarantee this.
=20
> Also: it looks to me like FreeBSD sys/netinet/tcp_sack.c revision
> 1.8->1.9 (check for receiver sending a SACK block outside the window)
> would be useful. The limits (both per-connection and global) in
> FreeBSD's tcp_sack.c rev 1.9->1.10 sound like they're worth having.
> I also wonder about 1.7->1.8, although (see my first question!)
> maybe our tailq means you already eliminated those usages?

IIRC the reassembly code doen't get called for data received out of
window, so we should not need 1.7->1.8.

The other changes look like they are worth having.

> Hmm. Is FreeBSD's tcp_sack.c rev 1.6->1.7 the bugfix you alreadcy
> integrated before the NetBSD commit?

Yes, I pulled this in right before the commit.

--=20
Kentaro A. Kurahone
SIGUSR1 Research and Development

[0]: Except when the peer is WinXP I suppose.

--h31gzZEtNLTqOjlF
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.0 (FreeBSD)

iQIVAwUBQjdXb2fp+SLSG+tuAQKK4xAAhZ9jUAB0iOxNxqxyooLsV44LP1wrV5WS
fsu7N0R6h+BO2pmk51OrgQx9gfZYK/1cAU3bfOixgt30i7hHEPGzVt2Y8ggBxxud
yL79Qds+7TAJSyMoiz9kNkdVGB/+d6t0xUp3UBepmPbCxHpyT7ZuhIkDtjA3gdnf
0E0r33HMdPBbp0IkQaDqF13Zl6UMJ6MCTisJidRb9KUHmcuVmpRlxE7Hk1uL4U6s
VjIkA00/4vSy++PKZtx54vthTMFxH+lIZizGPcJQAWmmKo6nOlv7Q+I3gJBsyMEj
3ln8Roa7OlYxgoZMZLVHREjxH0jcf+NC4o9cp4okoOuwHiCCXRWJfvsGFrq+PvVu
6QjjXCXW3JLk7PjlnCatbU5oWx1V0CbH2bDIgqLiw5A+MxRXlvCBy2tWCy4r5Ui7
pnnaDPr69E6xWO0X5AV6Jll0l/8VEE3nqF4iEO5j7V1uCirSlyrW0tXXl1wPpSYQ
aO5Qm53neRUFxsjqBEXt84Adybm8FV0R/f0rJnEJj6HovpZBZSiPQsn54mCHwwRl
4Rwa16Dirx3K52dJgzwPitR4Pu5zVyiZcbP0jWUPMVGZrjhdBHuxWBto8Pokv4H4
enowbZ93iQi2r59JRcctLTXr79KXrpXp7iGayo7tsXkwXZLzHuqD6Bw9khAtlXMe
KXn63dmRJyQ=
=Pzo1
-----END PGP SIGNATURE-----

--h31gzZEtNLTqOjlF--