Repository navigation
fix(diskio): cap unpacking budget by available Linux memory - #5189
Kaileshwar16 wants to merge 4 commits into
Conversation
|
@anisse does this fixes the issue |
b255582 to
8a93a37
Compare
|
So, the joke is that I can't link rustup (tried disabling LTO, no dice). Let me find another host to build it and try. It might take a while. |
|
I think you can get it from CI?
(Not sure what arch you need.) |
|
I did some AI-assisted validation in an isolated x86_64 Linux VM with roughly 463 MiB available and no swap. The patched build installed nightly-2026-10-28 successfully, automatically used single-threaded unpacking, and peaked at about 188 MiB RSS. All 11 disk I/O tests passed, and the memory and thread overrides worked. The old build used threaded unpacking and stalled until the timeout, but I couldn’t confirm an OOM kill, so Im not claiming the original failure is fully reproduced. until confirmed in arm64 CI |
|
@djc in the current implementation tests call unpack_ram() directly, using test-only, thread-local memory readings to keep them deterministic and isolated. Is that testing approach okay with you? |
Personally I'd probably drop these tests rather than injecting stuff like this. IMO there is not a lot of logic to get wrong anyway? Not sure how other team members feel though. |
|
@djc lets drop the test for now |
There was a problem hiding this comment.
I have a few nits as well.
But before going into that, I'd love if @anisse could test the build provided by the CI such as the ones from https://github.com/rust-lang/rustup/actions/runs/41843200203?pr=5189 to make sure if this is indeed the right direction we are looking at.
Many thanks in advance :]
| #[cfg(target_os = "linux")] | ||
| fn linux_available_memory(path: &Path) -> Option<u64> { | ||
| let meminfo = std::fs::read_to_string(path).ok()?; | ||
| let available = meminfo | ||
| .lines() | ||
| .find_map(|line| line.strip_prefix("MemAvailable:"))?; | ||
| let mut fields = available.split_whitespace(); | ||
| let kib = fields.next()?.parse::<u64>().ok()?; | ||
| if fields.next()? != "kB" || fields.next().is_some() { | ||
| return None; | ||
| } | ||
| // MemAvailable includes reclaimable caches, unlike MemFree. Older kernels | ||
| // may omit it; in that case retain the effective-limits fallback. | ||
| kib.checked_mul(1024) | ||
| } |
There was a problem hiding this comment.
Not sure if that is the best way to do this because this info has already been fetched in the underlying sys_info::mem_info() call where .avail is the field we are looking for here. It looks like we either need to upstream a change in effective_limits or vendor our own module that uses sys_info directly.
Before all that I'd like to know if @rbtcollins has any relevant context to share here regarding the original rationale behind the decision of using MemTotal here. Was the option of looking at MemAvailable explicitly dismissed, and if so, why?
|
Full log of the test from this PR https://github.com/rust-lang/rustup/actions/runs/41843200203/job/126620096161?pr=5189#step:18:28 ; aarch64. It did not get sigkilled and seemed to detect the low memory situation pretty well. |
fixes #5173
changed the calculation on Linux to also check MemAvailable, which tells us how much memory the system estimates it can spare. We still respect the existing process limits and keep rustup’s 200 MiB allowance for other work.
With 508 MiB available, rustup now calculates a 308 MiB budget. That falls below the existing 512 MiB threshold, so it automatically uses the less memory-hungry, single-threaded unpacker.
added tests showing that the old calculation makes the wrong choice and the new calculation makes the correct one. also checked missing memory information, very small and large values, and user overrides.