alpha_cubic SHOULD be 1 after cwnd >= prior_cwnd #146
Conversation
Fix a bug in the specification for changing alpha_cubic to 1, noted by Markku Kojo in an email to the tcpm list today: https://mailarchive.ietf.org/arch/msg/tcpm/H7GwAXdJlvXiNOKs4fr2XPmqM2k/ Namely, alpha_cubic SHOULD be set to 1 after cwnd >= prior_cwnd, not after cwnd >= W_max. This cwnd >= prior_cwnd threshold is used by my current Linux TCP CUBIC patch series draft and seems to work well. Quoting Markku's e-mail: """ 3 a) The rule for changing alpha to 1 when Wmax is reached in the Reno-friendly region is the correct thing to do during the normal steady state. However, it is incorrect action to take when in the fast convergence mode within the Reno-friendly region because it would act just *opposite* to what CUBIC should do when in the fast convergence mode; instead of slowing down the increase rate during congestion avoidance it actually accelerates because alpha becomes increased to 1 earlier than when not in the fast convergence mode. This seems an obvious mistake with the quite recent modifications to the rfc8312bis. """
…cubic=1 fix At the request of Lars, added a change log entry and added a section for the respective new revision.
The lint-checker was complaining about this.
At the request of Vidhi Goel, add prior_cwnd's definition in the "Variables of Interest" section.
draft-ietf-tcpm-rfc8312bis.md
Outdated
| @@ -408,6 +408,10 @@ Current congestion window in segments. | |||
| *ssthresh*: | |||
| Current slow start threshold in segments. | |||
|
|
|||
| *prior_cwnd*: | |||
| Size of *cwnd* in segments just before *cwnd* was reduced in the last | |||
| congestion event. | |||
There was a problem hiding this comment.
What if there was no last congestion event?
There was a problem hiding this comment.
Specifically I'm asking what if we entered CA due to HyStart, rather than due to a congestion event. The definition of W_max doesn't mention this case in this section, but in Section 4.10 (Slow Start) a definition is given. I think we need the same thing for prior_cwnd.
There was a problem hiding this comment.
prior_cwnd is used in "4.9.2. Spurious loss detected by acknowledgements". I think the definition is same as described here but now it is no longer optional?
There was a problem hiding this comment.
In section 4.9.2, prior_cwnd is defined as cwnd before the congestion event which is not enough as @maolson-msft said. We can add something at line 599. I provided a suggestion at line 599.
There was a problem hiding this comment.
All great comments, thanks. To try to address these I have added a commit to this pull request:
For the full set of changes in this pull request, see:
https://github.com/NTAP/rfc8312bis/pull/146/files
Please take a look. Thanks!
There was a problem hiding this comment.
Sorry, I just realized I think this pull request needs another tweak to be correct. Please take a look:
861e0ee
Thanks!
| Once *W<sub>est</sub>* has grown to reach the *cwnd* at the time of | ||
| the last congestion event, that is, *W<sub>est</sub>* >= *prior_cwnd*, | ||
| the sender SHOULD set {{{α}{}}}*<sub>cubic</sub>* to 1 to ensure that | ||
| it can achieve the same congestion window increment rate as Reno, | ||
| which uses AIMD(1, 0.5). |
There was a problem hiding this comment.
Suggestion for the text, needs some edits,
When congestion avoidance is first entered without any congestion
event (for example, when Hystart++ is used with CUBIC), as *prior_cwnd*
is undefined at that moment, *W<sub>max</sub>* can be used instead of
*prior_cwnd*. Look at Section 4.10 (reference) to see how *W<sub>max</sub>* is set
when CUBIC exits the first slow start without any congestion event.
There was a problem hiding this comment.
Great comment, thanks. To try to address this I have added a commit to this pull request:
For the full set of changes in this pull request, see:
https://github.com/NTAP/rfc8312bis/pull/146/files
Please take a look. Thanks!
Update definition of prior_cwnd to handle the case where we entered CA due to HyStart. Update the rule for setting prior_cwnd to indicate that it is required, since it is used for the W_est updates, and not just the optional undo mechanism.
Now that we're using prior_cwnd to decide the "shape" of the cwnd growth curve, we need to properly undo prior_cwnd, just like all the other variables that determine the shape of the cwnd growth curve. To avoid having a confusing name like prior_prior_cwnd, this commit switches to using an undo_ prefix rather than prior_ prefix to save the state needed to undo a previous cwnd reduction.
Fix a bug in the specification for changing alpha_cubic to 1, noted by
Markku Kojo in an email to the tcpm list today:
https://mailarchive.ietf.org/arch/msg/tcpm/H7GwAXdJlvXiNOKs4fr2XPmqM2k/
Namely, alpha_cubic SHOULD be set to 1 after cwnd >= prior_cwnd,
not after cwnd >= W_max.
This cwnd >= prior_cwnd threshold is used by my current Linux TCP
CUBIC patch series draft and seems to work well.
Quoting Markku's e-mail:
"""
3 a) The rule for changing alpha to 1 when Wmax is reached in the
Reno-friendly region is the correct thing to do during the normal
steady state. However, it is incorrect action to take when in the
fast convergence mode within the Reno-friendly region because it
would act just opposite to what CUBIC should do when in the fast
convergence mode; instead of slowing down the increase rate during
congestion avoidance it actually accelerates because alpha becomes
increased to 1 earlier than when not in the fast convergence mode.
This seems an obvious mistake with the quite recent modifications
to the rfc8312bis.
"""