Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Congestion window reduction should be based on size of inflight #3943

Closed
janaiyengar opened this issue Jul 23, 2020 · 9 comments
Closed

Congestion window reduction should be based on size of inflight #3943

janaiyengar opened this issue Jul 23, 2020 · 9 comments

Comments

@janaiyengar
Copy link
Contributor

The recovery draft says that on congestion, NewReno halves the congestion window. The corresponding pseudo-code is as follows:

congestion_window *= kLossReductionFactor

This isn't quite correct however, since when application limited, the amount of data in flight can be lesser than that allowed. The reduction should be on what was actually in flight, rather than what the sender was allowed to send.

For reference, RFC 5681, which defines NewReno for TCP, says this on Page 7 (Eq. 4):

 ssthresh = max (FlightSize / 2, 2*SMSS)

(and cwnd is later set to this value).

@junhochoi
Copy link
Contributor

@janaiyengar I can make a PR if you're ok

@ianswett
Copy link
Contributor

I would suggest not making this change. This is a case where I believe implementations typically use CWND and not flightsize, and we should make the same choice.

Feel free to correct me if I'm wrong about what implementations do.

@janaiyengar
Copy link
Contributor Author

@ianswett : you are right. I had thought that this is what Linux did but I was wrong. After chatting with Yuchung a bit, it's become clear to me that this is ok.

So, the reason to consider doing this change is because you might have a situation where the cwnd is 100 packets, but you have sent 10; reducing the cwnd from 100 to 50 is not meaningful, because the actual network load was 10 packets.

On the other hand, if you sent 100 packets, and the loss happened at packet 95, the flightsize now is 4, causing the cwnd to become 2. That does not make sense either.

If there is in fact congestion in the application limited case with a large cwnd (the first case above), there will be losses in subsequent rounds, causing the cwnd to eventually come down within a few RTTs, by halving each RTT. Reducing the cwnd to a very small number on the other hand (the second case above) means that the connection has to potentially go through many RTTs to increase, since the increase is a linear 1 per RTT.

There is a way to do this correctly: to track what was in flight when a given packet was sent, so that on loss, you use the appropriate flightsize. But that is quite a departure from the simple Reno algorithm.

On balance, I now think we should keep what we have. I know this is a departure from RFC 5681, but I think that's ok, and we have a convincing argument as to why it is.

I'll leave this issue open for a bit for others to comment, but I'm happy with closing this issue with no action.

@janaiyengar
Copy link
Contributor Author

I should note that both the Reno and Cubic implementations in Linux use cwnd/2.

@kazuho
Copy link
Member

kazuho commented Jul 24, 2020

+1 to retaining status quo, based on the view that there is a "bug" in what RFC 5681 does (the 100 packet case that @janaiyengar points out), in addition to what linux has been doing.

@junhochoi
Copy link
Contributor

junhochoi commented Jul 24, 2020

seems like freebsd newreno also does cwnd/2.

On the other hand, if you sent 100 packets, and the loss happened at packet 95, the flightsize now is 4, causing the cwnd to become 2. That does not make sense either.

Just thinking how this is possible? assuming cwnd=100, I think knowing packet 95 is lost without receiving ack of 0-95 is not realistic, so flightsize will be much higher than 4.

@junhochoi
Copy link
Contributor

Also, RFC5681 3.1 has the following text, indicating we need to consider receiver window too?

Implementation Note: An easy mistake to make is to simply use cwnd,
rather than FlightSize, which in some implementations may
incidentally increase well beyond rwnd.

@janaiyengar
Copy link
Contributor Author

I don't think that is an issue as such. The point of the note was, I suspect, to say that the cwnd might be even larger than the rwnd. That's not a problem for us ... since we don't really have the concept of an rwnd. Ours is a changing credit, and in TCP terms, it reflects a dynamic rwnd value.

@janaiyengar
Copy link
Contributor Author

Ok, I'm closing this issue. Please reopen if anyone thinks this discussion needs to continue.

Late Stage Processing automation moved this from Triage to Issue Handled Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Late Stage Processing
  
Issue Handled
Development

No branches or pull requests

5 participants