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

On page 13 #102

Closed
larseggert opened this issue Aug 31, 2021 · 5 comments · Fixed by #122
Closed

On page 13 #102

larseggert opened this issue Aug 31, 2021 · 5 comments · Fixed by #122
Assignees

Comments

@larseggert
Copy link
Contributor

Markku Kojo said:

On page 13:

" the sender MAY employ a Fast
Recovery algorithm to gradually adjust the congestion window to its
new reduced ssthresh value."

I assume this is aiming at saying that something similar to PRR MAY be used to reduce cwnd. This, however, is somewhat vaguely said and using fasr recovery is misleading. We need to remember also that it might not be trivial to have it right. So, dunno whether it would be useful to drop this.

@goelvidhi
Copy link
Contributor

goelvidhi commented Sep 1, 2021

@markkukojo, Not necessarily. RFC3782 describes an updated Fast Recovery for new Reno which can be used for CUBIC as well. RFC 6675 provides better loss recovery using SACK scoreboard. Or as you said, one can use PRR for better adjustments to congestion window. We should refrain from specifying what an implementation should use.

@goelvidhi goelvidhi self-assigned this Sep 1, 2021
@markkukojo
Copy link

Mayby I was not clear in my comment. Sure RFC 3782 and RFC 6675 (and RFC 5681) Fast Recovery algorithms are applicable with CUBIC, but none of these Fast Recovery algorithms "gradually adjust the congestion window to its new reduced ssthresh value." As the name "Fast Recovery" indicates, these algorithms include loss recovery in addition to the intertwined congestion control actions. And, when ECN is used the ECN congestion response to ECE involves no Fast Recovery actions. So, it is unclear what this statement means and what an implementer could possibly do different from the actions listed in Sec 4.6?

More importantly. The draft says in Sec. 4.2:
"It does not make any changes to the TCP Fast Recovery and Fast
Retransmit algorithms [RFC6582][RFC6675]."

But it does modify RFC6582, RFC 6675, and RFC 5681 Fast Retransmit&Fast Recovery by applying beta_cubic to cwnd, not FlightSize. This is incorrect and would bring back an old design&implementation bug that was corrected more than two decades ago when RFC 2581 was published (and subsequently corrected also for RFC 6582 and RFC 6675). Please see the implementation note attached to Equation (4) of RFC 5681: "An easy mistake to make is to simply use cwnd, rather than FlightSize, which in some implementations may incidentally increase well beyond rwnd."

Using cwnd in the equation will also result in incorrect outcome in some other cases as well, for example, if the sender is application limited.

Please open a new issue to address this latter problem.

@goelvidhi
Copy link
Contributor

goelvidhi commented Sep 21, 2021

For the first part, we can fix it by separating the losses from ECE,
Something like,

On detecting a packet loss, the sender MAY employ a Fast Recovery algorithm to update
the congestion window to its new reduced ssthresh value at the end of loss recovery.

@markkukojo Is this change enough for your first comment?

I have filed a separate issue #114 for using FlightSize instead of cwnd.

@markkukojo
Copy link

For the first part, we can fix it by separating the losses from ECE,
Something like,

On detecting a packet loss, the sender MAY employ a Fast Recovery algorithm to update
the congestion window to its new reduced ssthresh value at the end of loss recovery.

@markkukojo Is this change enough for your first comment?

No, I don't think it is. It does not give any algorithm nor advise how to do it, and particularly, how to do it correctly. So, an implementer has no idea based on this text what to do as there is no Fast Recovery algorithm in the RFC series that would give a correct algorithm nor is there any reference to such an algorithm.

I'm also still a bit unsure whether my assumption was correct in that this text would like to point to something like PRR (or rate-halving assuming that rate-halving could be adapted to a decrease factor other than 0.5)? Could you confirm, so we know that we are talking about the same thing. If it does, I think this document simply need not to say anything because the use of such an rate reduction algorithm during loss recovery is totally orthogonal to CUBIC and an implementer of CUBIC may select to use such an algorithm likewise an implementer of RFC 6675 or RFC 6582. E.g., PRR already has a placeholder for an MD factor other than 0.5, so it is all applicable with CUBIC.

So, maybe the simplest solution is just to delete the sentence? Or, am I possibly missing something?

I have filed a separate issue #114 for using FlightSize instead of cwnd.

Good, thanks.

@goelvidhi
Copy link
Contributor

goelvidhi commented Sep 24, 2021

@markkukojo I remember that we used to point to PRR as an option - either we removed it or I am remembering incorrectly. Either way, we intend to tell the implementor that you don't have to set cwnd = cwnd * Beta right away when you see loss and you can use PRR or 6675 etc to do fast recovery and then at the end of recovery, set cwnd = ssthresh (cwnd * Beta).

We can provide a reference algorithm as a recommendation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants