Skip to content
This repository has been archived by the owner on Aug 28, 2023. It is now read-only.

Rephrase text around algorithmic alternatives. #111

Merged
merged 12 commits into from
Oct 18, 2021
Merged

Rephrase text around algorithmic alternatives. #111

merged 12 commits into from
Oct 18, 2021

Conversation

larseggert
Copy link
Contributor

Fixes #90.

@goelvidhi goelvidhi linked an issue Sep 21, 2021 that may be closed by this pull request
Co-authored-by: Lars Eggert <lars@eggert.org>
draft-ietf-tcpm-rfc8312bis.md Outdated Show resolved Hide resolved
draft-ietf-tcpm-rfc8312bis.md Show resolved Hide resolved
Co-authored-by: Vidhi Goel <goel.vidhi07@gmail.com>
Copy link

@markkukojo markkukojo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

W.r.t. slow start:
This text is now better than the original.
However, is there any real reason to make CUBIC dependant of HyStart++?
Isn't HyStart++ similarly useful extension to the RFC 5681 slow start?
Moreover, it is now SHOULD, why is it not MUST? This begs for explaining when it might be justified to not implement HyStart++ with CUBIC.
IMO, If we want to recommend HyStart++, it is better done in HyStart++ spec.

And a nit: Reno TCP slow start algorithm does not make any sense to me. Reno is one alternative loss recovery algorithm to be applied after a Fast Retransmit. The one that first was implemented for BSD 4.3 Reno. The predecessor was Tahoe, which applied slow start after a Fast Retransmit. Later alternatives include NewReno and SACK-based loss recovery.

@sangtaeha
Copy link
Contributor

I don't think we have to mandate the use of a specific slow start algorithm even though Linux CUBIC uses Hystart. In fact, HyStart++ uses one of two indicators (delay spike) of Hystart (ack train length and delay spike). Can we just say any advanced slow start algorithms can be used for CUBIC?

@larseggert
Copy link
Contributor Author

There is only one on the standards track, and that is HyStart++, so I made that the SHOULD. Others area allowed experimentally.

@larseggert
Copy link
Contributor Author

Note: unless someone objects, I will merge this PR soon. If you do object, please make a concrete proposal for what would need to change.

Copy link

@huitema huitema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems ready.

Copy link
Contributor

@nealcardwell nealcardwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Copy link
Contributor

@sangtaeha sangtaeha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks!

@larseggert larseggert merged commit 3932f43 into main Oct 18, 2021
@larseggert larseggert deleted the downrefs branch October 18, 2021 10:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
8 participants