Handle invalid active defrag fragmentation thresholds#15513
Handle invalid active defrag fragmentation thresholds#15513vitahlin wants to merge 17 commits intoredis:unstableredis/redis:unstablefrom vitahlin:limit-defrag-thresholdvitahlin/redis:limit-defrag-thresholdCopy head branch name to clipboard
Conversation
| static int isValidActiveDefragThresholdLower(long long val, const char **err) { | ||
| if (val >= server.active_defrag_threshold_upper) { | ||
| *err = "active-defrag-threshold-lower must be less than active-defrag-threshold-upper"; | ||
| return 0; | ||
| } | ||
| return 1; | ||
| } | ||
|
|
||
| static int isValidActiveDefragThresholdUpper(long long val, const char **err) { | ||
| if (val <= server.active_defrag_threshold_lower) { | ||
| *err = "active-defrag-threshold-upper must be greater than active-defrag-threshold-lower"; | ||
| return 0; | ||
| } | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
It'll be a breaking change.
Users who use the same threshold are not able to start now.
Maybe we can just add a warning here, then check if they are equal in computeDefragCycles().
If they are equal, we always use active_defrag_cycle_min.
@ShooterIT WDYT?
int cpu_pct;
if (server.active_defrag_threshold_upper <= server.active_defrag_threshold_lower) {
cpu_pct = server.active_defrag_cycle_min;
} else {
cpu_pct = INTERPOLATE(frag_pct,
server.active_defrag_threshold_lower,
server.active_defrag_threshold_upper,
server.active_defrag_cycle_min,
server.active_defrag_cycle_max);
}
There was a problem hiding this comment.
I feel like we should allow the same threshold.
There was a problem hiding this comment.
maybe i misunderstood, i was thinking it is not illegal if the lower value is equal to the upper value.
There was a problem hiding this comment.
@vitahlin what happen if active_defrag_threshold_upper < active_defrag_threshold_lower?
There was a problem hiding this comment.
When upper <= lower, reaching the lower threshold also means reaching the upper threshold, so using active_defrag_cycle_max is more consistent with the configuration semantics. It adds no extra computation cost, although an invalid threshold configuration may switch directly to the maximum defrag effort. I updated the code and added a comment.
There was a problem hiding this comment.
if so, we don't need isValidActiveDefragThresholdLower, right?
There was a problem hiding this comment.
isValidActiveDefragThresholdLower() only logs a warning . Maybe we can keep it to warn users and guide them toward a valid config?
There was a problem hiding this comment.
it seems the most important thing of this PR is to fix calculating cpu_pct, right?
I am not sure if printing a log is useful, as @sundb said, maybe we can just add some comments in redis.conf
This reverts commit b294d35.
Co-authored-by: debing.sun <debing.sun@redis.com>
Co-authored-by: debing.sun <debing.sun@redis.com>
|
CI failure is unrelated to this commit
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit e140680. Configure here.
| [s total_active_defrag_time] ne 0 | ||
| } else { | ||
| fail "defrag not started." | ||
| } |
There was a problem hiding this comment.
Test fails on jemalloc builds without DEBUG_DEFRAG_FORCE
Medium Severity
On standard jemalloc CI builds (without DEBUG_DEFRAG=force), catch {r config set activedefrag yes} succeeds, so the if guard at line 69 evaluates to true and the test enters wait_for_condition. However, getAllocatorFragmentation() returns real (near-zero) fragmentation for a fresh empty server, so computeDefragCycles() returns early at the frag_pct < active_defrag_threshold_lower check and defrag never starts. The wait_for_condition then times out and the test fails with "defrag not started." The catch only handles non-jemalloc builds, not jemalloc-without-forced-fragmentation builds.
Reviewed by Cursor Bugbot for commit e140680. Configure here.
|
Fully daily test with command : |
Issue
computeDefragCycles()uses the difference betweenactive-defrag-threshold-upperandactive-defrag-threshold-loweras a divisor.Equal thresholds cause a division-by-zero UBSAN error.
Change
Allow equal or reversed thresholds for backward compatibility and log a warning.
When
active_defrag_threshold_upper <= active_defrag_threshold_lower,computeDefragCycles()usesactive_defrag_cycle_max. This avoids division by zero without rejecting existing configurations.Test
make SANITIZER=undefined DEBUG_DEFRAG=force ./runtest --single unit/memefficiency --only "Active defrag handles equal fragmentation thresholds"Before fix:
After fix:
Note
Low Risk
Small, localized change to defrag CPU calculation with a regression test; no auth, persistence, or replication impact.
Overview
Fixes undefined behavior in
computeDefragCycles()whenactive-defrag-threshold-upperis not greater thanactive-defrag-threshold-lower. The adaptive CPU percentage used to come fromINTERPOLATE(), which divides by(upper - lower)and UBSAN fails when those values are equal.When
upper <= lower, the code now skips interpolation and sets defrag effort toactive_defrag_cycle_max, so equal or reversed threshold pairs remain accepted without crashing.redis.confcomments note that the lower threshold should stay below the upper bound.Adds
unit/memefficiencycoverage that enables active defrag with equal thresholds (optionally underDEBUG_DEFRAG=force) and asserts the server stays up and defrag runs.Reviewed by Cursor Bugbot for commit 4ca7681. Bugbot is set up for automated code reviews on this repo. Configure here.