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

[scudo] Reflect the allowed values for M_DECAY_TIME on Android #89114

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

ChiaHungDuan
Copy link
Contributor

@ChiaHungDuan ChiaHungDuan commented Apr 17, 2024

No description provided.

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 17, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: None (ChiaHungDuan)

Changes

On Android, this interval constraint was used to avoid increasing the memory pressure by a long interval. Now the memory footprint is managed better and we always do a M_PURGE call before changing the interval. It's better to remove this legacy constraint.


Full diff: https://github.com/llvm/llvm-project/pull/89114.diff

1 Files Affected:

  • (modified) compiler-rt/lib/scudo/standalone/wrappers_c.inc (-8)
diff --git a/compiler-rt/lib/scudo/standalone/wrappers_c.inc b/compiler-rt/lib/scudo/standalone/wrappers_c.inc
index 21d5b7add51275..28374548e39076 100644
--- a/compiler-rt/lib/scudo/standalone/wrappers_c.inc
+++ b/compiler-rt/lib/scudo/standalone/wrappers_c.inc
@@ -251,14 +251,6 @@ INTERFACE WEAK int SCUDO_PREFIX(mallopt)(int param, int value) {
       // M_PURGE call so that we can minimize the impact of any unreleased pages
       // introduced by interval transition.
       SCUDO_ALLOCATOR.releaseToOS(scudo::ReleaseToOS::Force);
-
-      if (value == 0) {
-        // Will set the release values to their minimum values.
-        value = INT32_MIN;
-      } else {
-        // Will set the release values to their maximum values.
-        value = INT32_MAX;
-      }
     }
 
     SCUDO_ALLOCATOR.setOption(scudo::Option::ReleaseInterval,

@ChiaHungDuan
Copy link
Contributor Author

Based on the values allowed on Android, we may need to convert the values (-1, 0, 1) to ms, i.e., multiply 1000. But after reviewing few use cases of M_DECAY_TIME, I think we may still need to do some value mapping here. Will upload a new one.

@ChiaHungDuan ChiaHungDuan changed the title [scudo] Let allocator config limit the page releasing interval [scudo] Reflect the allowed values for M_DECAY_TIME on Android Apr 18, 2024
// Will set the release values to their maximum values.
// The values allowed on Android are {-1, 0, 1}. "1" means the longest
// interval.
if (value == 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this check be value > 0? That more closely matches what it did before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking instead of ">=", maybe we can assert the range of values? given that only three values are valid. (And the description also says 0 and 1 are only allowed numbers)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be fine with me. We sanitize the values when coming through Android anyway, so verifying a -1, 0, or 1 is probably a better way to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. CHECK added.

Copy link
Contributor

@cferris1000 cferris1000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@ChiaHungDuan ChiaHungDuan merged commit 6904e0e into llvm:main Apr 26, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants