This repository has been archived by the owner on Aug 28, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
clarify the meaning of "application-limited" #139
clarify the meaning of "application-limited" #139
Changes from 3 commits
d308319
1c0f006
ad37705
72d174d
c1f8e58
9727140
82d96ba
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"currently sending less than cwnd"- is this too general? A pacer or CPU bottleneck could also cause this to happen. Although, maybe it would also make sense to limit cwnd growth in those cases (since the point is that increasing the cwnd will not increase the send rate)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pacing the send rate doesn't mean you are sending less than cwnd (excluding app-limited), it just means that you are sending the cwnd paced within a duration (equal to RTT) instead of sending it all at once. As the cwnd growth is based on received ACKs, and ACKs themselves are dependent on sent data, the cwnd sort of grows somewhat paced too (unless the ACKs are coalesced).
I agree that "currently sending less than cwnd" is too generic and maybe we can add text about pacing etc to clarify this. But the rest of the text about cwnd growth looks fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm talking about particular instants in time when I talk about pacing- we will eventually send the whole window, but at the time when we are making a decision about whether to increase the CWnd, we may have deferred sending some of those packets until later in the round. I'm not allowed to look at the linux code, but it's my rough understanding that normally it goes ahead and posts the full CWnd of packets with appropriate "due times" for actually sending each packet, but that's only one possible implementation. TCP itself could also defer sending with a timer (which I think I heard has also been implemented in linux).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be alright with leaving this text as Neal has written it, which is still vague but better than the existing text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got a little confused when I first read
instead of the sender's congestion window.
Should we rephrase it to,
... window) and thus sends less data than what is allowed by the sender's congestion window.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be the last open review comment. I will merge and publish a new rev once the PR shows no requested changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Vidhi and Lars. This morning's patch should reflect Vidhi's suggestion now:
https://github.com/NTAP/rfc8312bis/pull/139/files