Repository navigation
leave page cache out of the memory guard's overshoot check - #3476
Tim Hockin (thockin) merged 3 commits into
Conversation
a snapshot write's dirty pages can stay charged to the actor's cgroup after its memory.high comes back, which could kill the paused VM before teardown Signed-off-by: Benjamin Elder <[email protected]>
Benjamin Elder (BenTheElder)
added
kind/bug
memory.stat is flushed lazily and can trail memory.current, so subtracting its page cache from memory.current could count fresh page cache as used Signed-off-by: Benjamin Elder <[email protected]>
GKE burn-in: rc.1 vs rc.1 + #3485 + #3476Tested this PR together with #3485 on top of Result: with both PRs applied, the #3353 suspend/resume regression is gone on microVM, and the gVisor suspend tail shrinks. microVM Setup
microVM
gVisor
Samples per build: microVM ~215 → ~260, gVisor ~270 → ~325 restores and checkpoints. The patched window was 23 s longer; the rest of the increase comes from faster cycles. Regression checks (patched): 0 locust failures (baseline 0). 0 ERROR-level lines in atelet or worker logs. 0 worker or ate-system restarts. No OOMKilled or Evicted events. No actors stuck in RESUMING/SUSPENDING. No new warning types: the only WARN in either run is the same pre-existing Caveats
Commits and image digestsCommits
Cherry-picks (patched ← original): Only Control plane (identical in both runs):
Workers
The patched worker binaries embed Benchmark images (not published; built from each build's commit): glutton |
Correction: the micro-VM checkpoint tail is not back to pre-break yetMy comment above used only the first ~4.5 minutes of the 6-user step, and that overstated the micro-VM result. Over a longer window (same patched build, same clusters, still 6 users), the micro-VM checkpoint tail grows with time. gVisor stays fixed. Patched, first 22 minutes of the 6-user step (exact percentiles, ms; 0 failures)
micro-VM
Compared with the original burn-in: Round 1 =
Where the slow micro-VM checkpoints spend their time (the worker's own "Checkpoint timing breakdown"):
So the remaining tail is in the guest-memory |
| if current < limit-limit/16 { | ||
| w.samples = w.samples[:0] | ||
| return false, current, nil | ||
| } |
There was a problem hiding this comment.
🤖 nit 🟢 – Both the overshoot and stall checks below now use used (memoryUsed), so current < limit-limit/16 is purely a fast path on the 250ms tick. Please add a short comment noting the invariant this relies on: used is a subset of current, so when memory.current is below limit - limit/16, used is too and we can skip reading memory.stat, memory.pressure, and memory.events.local.
There was a problem hiding this comment.
I almost factored these out but I didn't want to cause conflicts with #3485
could just pull it in. Addressed in follow-up commit with comments for now
| // microVM guest's, kernel and sock. All from one memory.stat read, which the | ||
| // kernel only flushes past a threshold of updates, so it can trail | ||
| // memory.current, and mixing the two could count fresh page cache as used. | ||
| // https://github.com/torvalds/linux/blob/v7.0/mm/memcontrol.c#L561-L564 |
There was a problem hiding this comment.
Perhaps state the kernel accounting logic here so it is clear why these four keys are not an arbitrary subset: memory.stat partitions charged memory into anon, file, kernel, and sock. file is filesystem page cache PLUS shmem. Excluding filesystem page cache (file) and including shared memory (shmem) is what we want: anon + shmem + kernel + sock.
There was a problem hiding this comment.
done
There was a problem hiding this comment.
though again, I'd rather structure it, and I'm considering breaking it into a shared util and just merging this into the bigger PR
|
Heads-up: a second GKE burn-in confirms Grant McCloskey (@MushuEE)'s findings here, including a microVM node-disk regression (deleted checkpoint files held open by Note: our run was main @ 34cce1e + #3485 only, since this PR's memory-guard change doesn't apply on that base, so it doesn't isolate this PR. Drafted and posted by an AI assistant on behalf of Anna Pendleton (@annapendleton). |
…es, drop stale test lines Signed-off-by: Benjamin Elder <[email protected]>
…uard's overshoot check (#3513) Cherry-pick of #3476 onto `release-1.0` (`git cherry-pick -x` of the squash commit), it applied cleanly with no changes. The memory guard's overshoot check leaves reclaimable page cache out, so it no longer kills a paused microVM and reports the actor crashed when a snapshot write leaves dirty page cache behind. This fixes a bug in #3429, which is in 1.0. AI use: AI-assisted. Clean pick. - [x] Tests pass - [x] Docs updated Signed-off-by: Benjamin Elder <[email protected]>
Follow-up to #3429, from Anish Ramasekar (@aramase)'s review: #3429 (comment)
When the pod can lend a microVM snapshot write all the memory it needs, the actor's cgroup has no
memory.highduring the write, so it can end with a lot of dirty page cache. Puttingmemory.highback reclaims what it can, but gives up on pages still dirty or under writeback:https://github.com/torvalds/linux/blob/v7.0/mm/memcontrol.c#L4409-L4429
The memory guard resumes as soon as the write returns, before teardown, and its overshoot check counted that page cache. So it could kill the paused VM and report the actor crashed for exceeding its memory limit.
The overshoot check now leaves reclaimable page cache out too, and both checks take what's left from one
memory.statread. The kernel only flushesmemory.statpast a threshold of updates, so it can trailmemory.current, and subtracting one from the other could count fresh page cache as used:https://github.com/torvalds/linux/blob/v7.0/mm/memcontrol.c#L561-L564
AI use: AI-assisted, I reviewed and tested it.