Skip to content

The inventory keeps its gaps once, by length, and inserts and removes in place - #1256

Merged
Japabu merged 3 commits into
mainfrom
memperf-inventory
Nov 11, 2026
Merged

Japabu merged 3 commits into
mainfrom
memperf-inventory

Conversation

@Japabu

@Japabu Japabu commented Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

The 2026-11-11 stage probes put about 70% of the T14's mem.map_unmap_4k (2,070 ns against Linux's 1,609) in the inventory: 735 ns to insert the page and 720 to remove it (evidence). The inventory kept its free ranges twice, by start in a map and by (length, start) in a set, and rewrote both on every change, though the entries already say where every gap is. An insert then joined neighbours by walking every entry around the range, and a removal listed the starts it would take in one Vec before collecting the entries in another.

Change

  • The free ranges are kept once, by (length, start), the order placement needs. An insert or removal finds the gaps it changes from the neighbouring entries.
  • An insert joins the neighbours it continues from the two lookups it makes anyway.
  • A removal takes its entries one at a time, with one allocation (the Vec it answers) where it made two.

The contract is unchanged: same placements, same joins, same answers.

Checks

  • Differential against the structure it replaces (host, scratch harness, evidence): both inventories driven through the same 6,000,000 random places, inserts, removals, updates and queries over spaces of 64 to 4,096 pages, with objects and copies whose offsets make neighbours continue each other. Every answer, and the whole state with 24 placements after each change, were equal. Two seeded defects, a stale gap left on removal and a skipped join with the following range, were each caught.
  • Host timing split, same harness: one page placed, inserted and removed in spaces of 16, 64 and 512 ranges, min/median of 300 interleaved batches. The insert path took 116-176 ns before and 50-82 after; the removal 148-177 before and 105-145 after; allocations per pair went from 2 to 1. The host was under load (average about 90), so read these as relative.
  • Unit tests: the mixed-load test now checks every placement's exact answer (smallest gap, lowest of equals, at alignments 1 to 16 pages) against a page map, and a new case joins a range to both neighbours.
  • Host checks at 55fafd6, each exit 0 (logs):
    • cargo test -p kernel --lib --features sched-check: 454 passed.
    • cargo clippy -p kernel --lib --tests --features sched-check on the host.
    • cargo clippy -p kernel on x86_64-unknown-none and on aarch64-unknown-none-softfloat.
    • Every clippy run had the adopted lints and -D warnings.
  • Guest run 43092276467 at the head d7c5595, x86-64 and AArch64 together: virt_smp, std_threads_count_the_cpus_and_give_back_ended_threads and a_user_copy_faults_in_mid_copy_or_ends_as_bad_address. Every mapping in those boots goes through the new inventory, thread stacks are mapped and unmapped across CPUs, and a copy's buffer is unmapped under it. It is queued; 43081326308, at the measurement branch, is cancelled as not code-identical.
  • Owed on metal, in issues/user-memory-hot-paths-are-slower-than-2-mib-pages.md. The benches are down, so it is not a landing condition: an A/B on both benches of osbench_system and osbench_mem, main from before and after. Main after must read mem.map_unmap_4k and mem.munmap_part.4k faster, and no other row of the two groups slower.
  • The speed-up this PR exists for is not yet measured on metal. The host timing above is relative and under load. The staged A/B (memperf-inv-ab 2bc40c8 against memperf-ab-base 8248c23) never booted.

Records

Review r1 (2b3d442)

  • Fixed: range_heap charged each range for the deleted by-start map, and its comment named two indexes and two removal lists. It now charges the entry, the gap's place by length, and three times the range's place in the one list a removal gathers, its peak while doubling.
  • Taken: the mixed-load test compares the free index with the page map's gaps exactly.
  • Not taken: asserting each gap removal. It would add three checks to the code the A/B was staged to measure. A drifted index still fails loudly at the next insert into a stale gap, where the overlap assert holds.
  • Checks at 2b3d442: cargo test -p kernel --lib --features sched-check (454 passed), and clippy on the host library with tests, on x86_64-unknown-none and on aarch64-unknown-none-softfloat, with the adopted lints and -D warnings. Each exited 0 (logs).

Not run, and why

  • Full cargo run -- --ci host: under the owner's testing policy only the touched crate's tests and lints run; the merge queue runs the rest.

🤖 Generated with Claude Code

… in place

The free ranges were held twice: by start in one map and by length in a set,
both rewritten on every insert and removal, though the entries already say
where every gap is. Now only the set by `(length, start)` stays, the one
placement needs; an insert or removal finds the gap or gaps it changes from
the neighbouring entries.

An insert joins the neighbours it continues directly, from the two lookups it
makes anyway, rather than inserting and then walking every entry around the
range; a removal takes its entries one at a time without first listing their
starts. A map and unmap of one page so makes about half the tree lookups and
one allocation where it made two.

On the host (load average about 90, min of 300 batches), one page placed,
inserted and removed in a space of 16 to 512 ranges: the insert path 116-176
ns to 50-82, the removal 148-177 to 105-145; allocations per pair 2 to 1.
Driven with the inventory it replaces through 6,000,000 random changes, every
answer and the whole state after each were equal; two seeded defects were
caught. The mixed-load test now asks every placement's exact answer of a
page map.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Japabu

Japabu commented Nov 11, 2026

Copy link
Copy Markdown
Collaborator Author

Review of 55fafd6

What I read: the commit, kernel/pure/vm.rs in full, and its callers in kernel/src/mm/space.rs (put, release, room, settle, range_heap). Also the evidence at fffbc92a (README, harness, diff, mutation and bench logs, host checks), memperf-inv-ab against memperf-ab-base, and main from 34cb4c9 to f79df95.

Architecture

The owner is right and the representation is simpler. The by-start map repeated what the entries already say: the gap below an entry starts where the entry before it ends. Deleting the map removes a second index and the code that kept the two in step (release, add_free, take_free). Production lines are about even (+54/−50) and the tests grew by 17 lines. An insert now joins only its two neighbours, which is enough because the entries are always kept joined.

Invariants and complexity

  • I1: entries are disjoint and lie inside [floor, top).
  • I2: no entry is continued by the entry right after it; such pairs are always joined.
  • I3: free holds exactly one (b−a, a) for each maximal uncovered [a, b).

Each operation keeps them:

  • insert. The overlap check now reads the entries themselves (lo <= start && end <= hi, vm.rs:258), so an insert over held memory still panics. The one gap [lo, hi) becomes at most two. Joining with the entry before and the entry after restores I2, because I2 held before the insert. Continuation carries across a join on both sides, since offsets add and Copy lengths compose: (a ⊖ b) ⊖ c = a ⊖ (b + c). Cost: two map lookups, at most three set operations and at most two map operations, O(log n).
  • remove. split_at leaves the gaps unchanged. Each entry taken merges the gap before it, and the k+1 gaps from lo to hi become one. lo is read after split_at(start), so lo ≤ start. Cost: O((k+1) log n), with one allocation.
  • update. Extents do not change, so I3 holds, and join restores I2.
  • place is unchanged.

As far as I can trace, it is correct. One check stays silent: the results of free.remove are ignored (vm.rs:259,294,302), as main's take_free ignored them, so a broken I3 does not fail where it breaks.

The differential's coverage

It is strong where it compares:

  • Both inventories run the same 6,000,000 random operations over spaces of 64, 512 and 4,096 pages, and every answer is compared: placements, removals' contents and queries.
  • Every entry is compared after every operation.
  • Neighbours often continue each other: few objects, offsets that follow the address, and copies.
  • Half the inserts follow a placement.
  • Both seeded defects were caught at the first state where the two diverged.
  • The patches' line numbers (257, 273, 298) match this PR's vm.rs, which supports the README's statement that new.rs is this file; the harness copies themselves are not in the evidence.

Where it is weaker:

  • The gap index itself is compared only through 24 placements per step. A stale or missing gap that none of those sizes selects goes unseen until one does. Main's by_len and the new free have the same type and could have been compared exactly.
  • remove with an empty range is not driven. No caller makes one.

The committed mixed-load test checks exact placements against a page map, an independent oracle, and caught both unit mutations.

Overlap with #1223's records

#1223's records commit adds the paragraph that puts 735 + 720 ns of the T14's mem.map_unmap_4k in the inventory, "not yet split further" (user-memory-hot-paths…md:135 there). This PR changes exactly that cost, and its Open section already says it owes the same record its A/B. #1223 is going back for a design change. Move the paragraph here so that it lands together with this PR's result; then no edit is left to whichever PR lands second.

Findings

BLOCKER

  1. kernel/src/mm/space.rs:209-216 — range_heap() still charges for the deleted map. It charges charge::map_entry::<u64, u64>() per range, which was the cost of the by-start free map this PR deletes. Its comment, "the free range beside it and that one's place by length, and its place in the lists a removal gathers", describes two free indexes and two removal lists. Now each gap has one set entry, and there is one list: the Vec that remove grows by doubling.

    As a result, every space is charged to its job, per range, for memory that no longer exists, and the comment is false. Fix: drop that term and rewrite the comment. Derive the removal-list term again for the one growing Vec, whose peak while it doubles is up to three times its length, or reserve the Vec's size in advance.

PR description (the orchestrator's to correct): "put a third of the T14's mem.map_unmap_4k … in the inventory: 735 ns to insert the page and 720 to remove it" — 1,455 of 2,070 ns is about 70%, not a third.

SUGGESTION

  • vm.rs:259,294,302 — assert the gap removals, as the overlap check already asserts, so an index that drifts from the entries fails at the change that broke it.
  • vm.rs:615-636 — the in-file test can compare v.free with the page map's gaps directly. That check is exact and cheaper than seven placements, and it would make "The free index is exactly the gaps" what the test literally checks.

Evidence and metal

  • Host checks at the head. Kernel library tests (454) and clippy on the host, x86-64 and aarch64, each exit 0, and the vm:: tests are listed in the log.
  • A/B branches. memperf-inv-ab is the head plus the row, and the row's hunk is identical to memperf-ab-base's. Between 34cb4c9 and f79df95, every changed line in kernel/, sdk/, system/ and build/ is a comment, so the base is valid.
  • The fix keeps the A/B valid. It changes only a constant in the charge's size. room resizes in steps of 32 ranges, and the one-page map/unmap loop never crosses a step.
  • What metal must show, on both benches:
    • mem.map_unmap_4k and mem.munmap_part.4k are MOVED faster.
    • No other row in osbench_system or osbench_mem shows a REGRESSION; rows that map and unmap may improve.
    • Every inventory-arm boot passed. A failed boot is investigated, not just excluded, because every mapping goes through this structure.
    • The record update with the A/B lands in this PR.
  • Guest rows: none. The structure is pure and checked on the host against main's, and the metal boots run it under the whole system on both benches. An aarch64 guest would run the same code, which has no architecture-specific behaviour, and would show nothing the host tests do not.

SEND BACK for the one fix. After it, the PR is LAND-pending-metal on the conditions above, and the queued A/B at 2bc40c8 still applies.

…cord says where map/unmap's time went

`range_heap` still charged each range for the by-start free map this PR
deletes, and its comment named two indexes and two removal lists. It now
charges a range's entry, its gap's place by length, and its share of the
one list a removal gathers, at up to three times its length while that
list doubles.

The mixed-load test compares the free index with the page map's gaps
exactly. #1223's paragraph putting the T14's `mem.map_unmap_4k` in the
inventory moves here, with this PR's host split.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Japabu

Japabu commented Nov 11, 2026

Copy link
Copy Markdown
Collaborator Author

Re-review of 2b3d442

Scope: whether round 1's findings are resolved, and whether the changed code is correct.

What I read:

Round 1

  • B1, resolved. range_heap now charges three things:

    • the entry;
    • the gap's (length, start) set entry;
    • three (u64, SpaceEntry) slots. That is exactly the element remove returns (vm.rs:284; space.rs:40).

    While the Vec doubles from c to 2c, it holds 3c slots for c + 1 elements, so fewer than three per element. A one-element removal's first allocation of four slots is more than three, but the charge is pooled over at least RANGE_GRAIN (32) ranges, 96 slots. The comment is true.

  • The test compares v.free with the page map's gaps exactly. The space's top is FLOOR + PAGES * PAGE, so the map covers it whole, and the set's order is the inventory's (length, start). A stale or missing gap now reds at the step that made it.

  • The description says about 70%: 1,455 of 2,070 ns is 70.3%.

  • The mem.map_unmap_4k paragraph is here and gone from An unmap walks no futex bucket while no thread of its space is in a futex wait #1223. The two PRs' hunks in that record touch separate lines.

  • The declined assert is acceptable. A stale gap still panics at the insert's overlap check, and the test now catches a missing one.

Findings

None. The changed code is correct.

Evidence

  • Host checks at 2b3d442, each exit 0, with a fresh kernel build and the vm:: tests listed.
  • The queued A/B at 2bc40c8 still measures this code. The r1 change is the charge's constant and a test, and room never crosses a 32-range step in the one-page loop.
  • Guest run 43081326308 is queued at 2bc40c8 (55fafd6 plus the row). It is code-identical to the head but for that constant, so it serves the boot-before-flash rule.

Landing conditions

  1. Guest run 43081326308 green before any flash.
  2. The A/B by rule.txt as amended, on both benches:
    • mem.map_unmap_4k and mem.munmap_part.4k MOVED faster;
    • no REGRESSION in any other row of osbench_system or osbench_mem;
    • every inventory-arm boot passed, or its failure was investigated and shown to have a cause outside the change.
  3. The record's "Not yet measured on metal" is replaced by the A/B.

LAND, pending the guest run and metal above.

With no bench booting, the record names the A/B on both benches that the
single free index still owes, and what main must show in it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Japabu

Japabu commented Nov 11, 2026

Copy link
Copy Markdown
Collaborator Author

Orchestrator: landing under the benches-down rule. Review r2 LAND. Guest run 43092276467 at head d7c5595: success. Head differs from the reviewed 2b3d442 only in its record (owed-on-metal bullet; verified). The A/B is owed on metal; the speed-up is not yet measured on metal.

@Japabu
Japabu marked this pull request as ready for review November 11, 2026 21:53
@Japabu
Japabu added this pull request to the merge queue Nov 11, 2026
Merged via the queue into main with commit 8188095 Nov 11, 2026
11 checks passed
@Japabu
Japabu deleted the memperf-inventory branch November 11, 2026 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant