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

Initialization #28

Closed
mwelzl opened this issue Dec 1, 2020 · 7 comments · Fixed by #35
Closed

Initialization #28

mwelzl opened this issue Dec 1, 2020 · 7 comments · Fixed by #35
Assignees
Labels
editorial Editorial change relative to RFC8312 or earlier bis versions

Comments

@mwelzl
Copy link

mwelzl commented Dec 1, 2020

This is tiny and editorial:

Section 4.7 of RFC 8312 talks about how values are initialized after an RTO; it seems obvious that these values would also be used at the very beginning, but this is never explicitly said. It seems obvious that, at this point, W_last_max should be set to W_max, but how to initialize W_last_max is also not explicitly said (unless I missed it).

@larseggert larseggert added editorial Editorial change relative to RFC8312 or earlier bis versions help wanted Looking for someone resolve this issue labels Dec 1, 2020
@lisongxu
Copy link
Contributor

lisongxu commented Dec 1, 2020

Thanks, @mwelzl. W_last_max should be initialized to 0.

By the way, there is a confusing difference between RFC and Linux code: RFC has two variables, W_last_max and W_max, whereas Linux code has only W_last_max that corresponds to W_max in RFC. This is confusing, so I would recommend removing W_last_max from the RFC, as it is used only in Section 4.6.

How about changing paragraphs 2, 3, 4 of Section 4.6 as follows?

With fast convergence, when a congestion event occurs, we update W_max as follows before the window reduction described in Section 4.5.

      if (cwnd < W_max){                        // should we make room for others
          W_max = W_max*(1.0+beta_cubic)/2.0;   // further reduce W_max
      } else {
          W_max = cwnd                          // remember cwnd before reduction
      }

At a congestion event, if the current cwnd is less than W_max, this indicates that the saturation point experienced by this flow is getting reduced because of the change in available bandwidth. Then we allow this flow to release more bandwidth by reducing W_max further. This action effectively lengthens the time for this flow to increase its congestion window because the reduced W_max forces the flow to have the plateau earlier. This allows more time for the new flow to catch up to its congestion window size.

Then remove the following line from Section 4.5, because W_max is set in Section 4.6 now.

W_max = cwnd;                 // save window size before reduction

@mwelzl
Copy link
Author

mwelzl commented Dec 1, 2020

Oh yes, this is much better! Thanks!

@goelvidhi
Copy link
Contributor

Thanks, @mwelzl. W_last_max should be initialized to 0.

By the way, there is a confusing difference between RFC and Linux code: RFC has two variables, W_last_max and W_max, whereas Linux code has only W_last_max that corresponds to W_max in RFC. This is confusing, so I would recommend removing W_last_max from the RFC, as it is used only in Section 4.6.

How about changing paragraphs 2, 3, 4 of Section 4.6 as follows?

With fast convergence, when a congestion event occurs, we update W_max as follows before the window reduction described in Section 4.5.

      if (cwnd < W_max){                        // should we make room for others
          W_max = W_max*(1.0+beta_cubic)/2.0;   // further reduce W_max
      } else {
          W_max = cwnd                          // remember cwnd before reduction
      }

This looks good. One thing to note though, earlier we were saving W_last_max = W_max (before applying fast convergence) and now we won't have that state saved. Instead we would always compare with W_max after fast convergence is applied.

@larseggert
Copy link
Contributor

Anyone willing to create a PR for this one?

@lisongxu
Copy link
Contributor

lisongxu commented Dec 3, 2020

I can help with this one.

@larseggert larseggert removed the help wanted Looking for someone resolve this issue label Dec 3, 2020
@larseggert
Copy link
Contributor

@lisongxu could you prepare a PR? we'd like to be able to submit a -01 version to the IETF soon and ask for adoption.

@lisongxu
Copy link
Contributor

@larseggert done, thanks

@larseggert larseggert linked a pull request Dec 10, 2020 that will close this issue
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
editorial Editorial change relative to RFC8312 or earlier bis versions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants