Add a fix for the freq_ctr overshoot calculation overflowing with high limits #12
Conversation
This file contains hidden or 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
…h limits Bandwidth limits in haproxy are almost always stored or passed around as uints internally. The one exception to that is freq_ctr_overshoot_period, which calculates how far over the limit a freq_ctr has gone (producing the minimum amount of time that the user should wait before submitting more events). This result is returned currently as a signed integer. This is not inherently problematic, but it's a little strange (all other quantities are unsigned in those calculations, and they're all closely related) and also the conversion to a signed quantity happens implicitly right at the end, after a possible overflow of the unsigned intermediate value. The end result of this for us is that if you set a very high limit when using the freq_ctr-based bandwidth limiting, then in some cases you will be instructed to wait for a long time, causing transfers to stall.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Bandwidth limits in haproxy are almost always stored or passed around as uints internally. The one exception to that is freq_ctr_overshoot_period, which calculates how far over the limit a freq_ctr has gone (producing the minimum amount of time that the user should wait before submitting more events). This result is returned currently as a signed integer. This is not inherently problematic, but it's a little strange (all other quantities are unsigned in those calculations, and they're all closely related) and also the conversion to a signed quantity happens implicitly right at the end, after a possible overflow of the unsigned intermediate value. The end result of this for us is that if you set a very high limit when using the freq_ctr-based bandwidth limiting, then in some cases you will be instructed to wait for a long time, causing transfers to stall.
Note that this does not affect us currently because the new bandwidth limiting code has been unhooked for now until we can get the time to properly investigate the stale connection issues and get it back in.
I've also submitted this patch to HAProxy itself so we'll see what they say. If they accept it then much like our earlier bugfixes this patch will go away in a future HAProxy upgrade.
Type of Change