BBR: avoid busy loop #446
Merged
BBR: avoid busy loop #446
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There's no actual reason for this, since the relevant select has a default branch and therefore will never block. This isn't the select at issue in the profile Louis showed me, but it seems worth fixing since I've noticed it.
...in the case where the connection is idle. Previously, if this branch was taken then the select would always terminate immediately; we really need it to block unless there's actually something to send. This is probably a big part of the high cpu use we're seeing.
zenhack
commented
Feb 9, 2023
| @@ -84,9 +79,7 @@ func (l *Limiter) run(ctx context.Context) { | |||
| // might be in the far future, and there is no ack on its way | |||
| // to save us from our ignorance. Fortunately we always want | |||
| // to send if there's nothing on the wire, so just do it. | |||
| // Note that we don't use sendReqs here, since we don't want | |||
| // to skip over the logic that checks for app limited flows: | |||
There was a problem hiding this comment.
I have no idea what my reasoning was with this comment, but it makes no sense as a justification, since either way the probability of choosing other branches of the select is non-zero.
I want to stare at this a bit more; I wonder if there's some bookkeeping that I knew we needed to be doing here, but apparently failed at actually ensuring we did? I'll have to have a look when we revisit fixing the estimation problems.
There was a problem hiding this comment.
Yeah, this seems like a thread worth pulling on. Want to open an issue to track it, or are you confident you'll remember?
lthibault
approved these changes
Feb 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
...in the case where the connection is idle. Previously, if this branch
was taken then the select would always terminate immediately; we really
need it to block unless there's actually something to send.
This is probably a big part of the high cpu use we're seeing. Also: the requirement that the pipe be empty is probably why we are not seeing high CPU usage in tests.
Stacked on top of #445; review & merge that first.