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

MIN_RTT management #2923

Closed
gorryfair opened this issue Jul 22, 2019 · 8 comments
Closed

MIN_RTT management #2923

gorryfair opened this issue Jul 22, 2019 · 8 comments
Labels
-recovery design has-consensus

Comments

@gorryfair
Copy link
Contributor

@gorryfair gorryfair commented Jul 22, 2019

So, I understand that min_RTT is maintained per path as the minimum RTT.

I foresee paths that persist for long periods and where the set of network devices forming the can path change, and hence the min_RTT can be "ridiculously" small concerned to the actual value had it been measured recently. I don't much like that min_RTT is never raised.

The solution could be simple. Re-initialise min_RTT from the RTT sample periodically. How often to reset is not critical, but needs to be reasonably often - I suggest every 600 secs. (I'd be OK with other numbers, this is the default path state timeout in RFC1981).

@gorryfair
Copy link
Contributor Author

@gorryfair gorryfair commented Jul 23, 2019

Related to #2908, but this is more than editorial.

@mnot mnot added the design label Jul 24, 2019
@ianswett ianswett self-assigned this Oct 15, 2019
@mnot mnot added this to Design Issues in Late Stage Processing Nov 25, 2019
@janaiyengar
Copy link
Contributor

@janaiyengar janaiyengar commented Jan 13, 2020

@gorryfair : Having thought about this some more, I don't think we should be doing anything here.

We need to define the constraints around min_rtt based on its use in the recovery document. As it stands, this document uses min_rtt for rejecting RTT samples that are too small. The cost of having a min_rtt that is too low is that the endpoint might allow for the SRTT to be smaller when the peer is incorrectly sending ack-delays that are too large.

We knew this problem when we chose to use ack-delays in the SRTT computation and we decided to not solve it then. I don't think we need to solve it now.

@larseggert larseggert changed the title MIn_RTT management MIN_RTT management Feb 4, 2020
@ianswett ianswett removed their assignment Feb 4, 2020
@ianswett
Copy link
Contributor

@ianswett ianswett commented Feb 4, 2020

After more discussion, I think Jana's right and we should close this with no action. The min_rtt in QUIC recovery is used for a very specific purpose(preventing 'silly' SRTTs due to ack_delay) and not meant for other purposes.

@gorryfair
Copy link
Contributor Author

@gorryfair gorryfair commented Feb 4, 2020

I don't agree that the method is robust to path changes that result in a larger RTT. I can read the latest draft and see if the comment still applies.

@huitema
Copy link
Contributor

@huitema huitema commented Feb 5, 2020

The corresponding mechanism in BBR uses a 10 seconds interval. Quite different from the 600 seconds mentioned above.

@larseggert
Copy link
Member

@larseggert larseggert commented Feb 5, 2020

Discussed in ZRH. Proposed resolution is to close with no action. Possible new Editorial PR to explain that min_rtt is only used to provide a safety feature and having a potentially stale value is therefore not terrible.

@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
@gorryfair
Copy link
Contributor Author

@gorryfair gorryfair commented Feb 6, 2020

OK - I missed the part of the meeting discussing that: I concur that if QUIC never uses this variable for other things, then the current specific use of min_rtt in the latest rev does note the problem above.

@gorryfair gorryfair closed this Feb 6, 2020
Late Stage Processing automation moved this from Consensus Emerging to Text Incorporated Feb 6, 2020
@martinthomson
Copy link
Member

@martinthomson martinthomson commented Feb 6, 2020

I'm going to re-open this so that we can let the process run, but thanks for acknowledging this Gorry.

@martinthomson martinthomson reopened this Feb 6, 2020
Late Stage Processing automation moved this from Text Incorporated to Triage Feb 6, 2020
@LPardue LPardue moved this from Triage to Consensus Emerging in Late Stage Processing Feb 16, 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
-recovery design has-consensus
Projects
Late Stage Processing
  
Issue Handled
Development

No branches or pull requests

9 participants