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

Flow Control Tuning #3216

Closed
gorryfair opened this issue Nov 11, 2019 · 4 comments
Closed

Flow Control Tuning #3216

gorryfair opened this issue Nov 11, 2019 · 4 comments
Labels
-transport design has-consensus

Comments

@gorryfair
Copy link
Contributor

@gorryfair gorryfair commented Nov 11, 2019

/ A receiver can use an autotuning mechanism to tune the frequency and
amount of advertised additional credit based on a round-trip time
estimate and the rate at which the receiving application consumes
data, similar to common TCP implementations.
/

  • I think this has potential for strange pathologies where flow control interacts with ACKs within the transport. I now have at least some data where we have seen problems, from one policy that updated flow control too infrequently. Achieving similar behaviour to TCP could require more than currently stated.
@mnot mnot added this to Triage in Late Stage Processing Nov 12, 2019
@mnot mnot changed the title Transport: Flow Control Tuning Flow Control Tuning Nov 12, 2019
@martinthomson
Copy link
Member

@martinthomson martinthomson commented Nov 28, 2019

After briefly discussing this with Gorry, I think that we concluded that advice about the frequency of flow control updates would help.

Where flow control limits broadly correspond to the congestion window size, those updates would need to be sent about as often as acknowledgments in order to ensure that flow control didn't stall unnecessarily. That would be the upper limit; larger flow control limits need proportionally fewer updates. If flow control limits are very large, updates could be far less frequent.

I don't think that we have an algorithm here, but it seems like this advice would go a long way to avoiding the worst problems.

There's a question then about where this advice goes. I am inclined to keep this in the transport doc as long as our response only extends to advice in the abstract.

@gorryfair
Copy link
Contributor Author

@gorryfair gorryfair commented Nov 29, 2019

Thanks for starting this issue. I will speak to my team and see if we can come up with some more context and plots that show the key issues and send a link to what we have.

@ianswett
Copy link
Contributor

@ianswett ianswett commented Dec 2, 2019

I think we should keep this in transport.

I think we can provide more specific guidance than we do now, including guidance to bundle flow control updates with ACK frames, but not to bundle them with every ACK frame(otherwise you end up ACK-ing ACKs, which we know is bad). I'm biased towards marking this editorial, but we can wait for a PR to see.

Our implementation does(approximately):

  1. Send an update every time 1/2 the window has been consumed
  2. Double the size of the window every time two updates are sent within an RTT.

Particularly early in the connection, ideally the flow control window is 2x the peer's current congestion control window, because the CWND doubles every RTT in slow start.

@mnot mnot added the design label Jan 17, 2020
@mnot mnot moved this from Triage to Design Issues in Late Stage Processing Jan 17, 2020
@larseggert
Copy link
Member

@larseggert larseggert commented Feb 5, 2020

Discussed in ZRH. Proposed resolution is to close with no action.

@larseggert larseggert added the proposal-ready label Feb 5, 2020
@project-bot project-bot bot moved this from Design Issues to Consensus Emerging in Late Stage Processing Feb 5, 2020
@LPardue LPardue moved this from Consensus Emerging to Consensus Call issued in Late Stage Processing Feb 19, 2020
@LPardue LPardue removed the proposal-ready label Feb 19, 2020
@LPardue LPardue added the call-issued label Feb 26, 2020
@LPardue LPardue added has-consensus and removed call-issued labels Mar 4, 2020
@project-bot project-bot bot moved this from Consensus Call issued to Consensus Declared in Late Stage Processing Mar 4, 2020
Late Stage Processing automation moved this from Consensus Declared to Text Incorporated Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design has-consensus
Projects
Late Stage Processing
  
Issue Handled
Development

No branches or pull requests

6 participants