KRN-1117: Add path-compressed trie storage to stackdepot

[caleb-kan]

Summary

This ports the KRN-1117 stackdepot redesign to Linux 6.18.

DOG KASAN hosts can exhaust stackdepot pools because stackdepot currently deduplicates only byte-identical full stacks. The RFC and userspace prototype showed that these stacks still share many frame sequences, even when the whole stack is unique.

This adds an opt-in path-compressed trie backend for persistent stack saves. The trie stores shared frame runs once and compresses kernel text frames when the arch helper can reconstruct them safely.

stackdepot.trie_enabled defaults off. The next step is DOG one-off testing and validation.

Design Notes

The legacy hash backend is still used for:

• STACK_DEPOT_FLAG_GET
• explicit hash saves
• overlong stacks
• page_owner count-helper paths

The trie path is used only for persistent, non-refcounted stack saves.

This keeps refcounted eviction out of scope and preserves existing hash semantics for the callers that still need them.

Review Focus

The parts I would most like reviewed are:

• trie save/insert routing
• RCU/COW publication
• retired trie object reuse
• side-table handle lifetime
• KMSAN extra-bit handling
• no-spin / constrained-context behavior
• x86-64 and arm64 frame compression
• caller migration to stack_depot_fetch_into()

Validation

Based on linux-6.18.y at v6.18.35.

Passed:

• git diff --check greg/linux-6.18.y...HEAD
• strict checkpatch on the effective diff
• focused full-file checkpatch for stackdepot files and mm/page_owner.c
• focused arm64 sparse build for lib/stackdepot.o and lib/tests/stackdepot_kunit.o
• stackdepot KUnit: 136/136
• lockdep / PROVE_RCU / raw-lock KUnit: 136/136
• KCSAN KUnit: 136/136
• runtime stackdepot.trie_enabled enable/disable smoke
• trie-enabled Generic KASAN boot smoke
• sampled drgn trie materialization and frame-compression checks

Rollout

This is still experimental. DOG needs to answer:

• whether the memory savings hold on real Generic KASAN workload
• whether stackdepot pool exhaustion is avoided
• whether KASAN reports still have usable allocation/free stacks
• whether stack_depot_save() cost is acceptable

[mfleming]

is it safe for storage to be NULL here? This seems like overly defensive programming again. What's the invariant we should be checking?

[mfleming]

instead of using a status flag for this, I would really prefer to see this expressed in code, e.g.

   found = trie_find_child(root, parent, entries[0], &children, &node, &pos, &found);
   if (!found)
      append_here();

   matched = node_match(node, entries, nr_entries);

   if (matched < node->run.nr_entries)
      split_node();

   if (matched < nr_entries) {
      parent = node;
      entries += matched;
      nr_entries -= matched;
      continue;
   }

   if (node->leaf_id)
      found_leaf();
   else
      promote_node();

For me, this is much easier to follow and makes the algorithm crystal clear. This is how things are done in kernel/bpf/lpm_trie.c, for example.

[mfleming]

we just looked up the entry before this, how is this condition possible?

[mfleming]

this is an invariant bug. This needs to be WARN_ON() or WARN_ON_ONCE() (if it needs to be ratelimited). This should never happen.

[mfleming]

is it not possible to make this unnecessary by minimising our use of the stack_depot_trie_root type? Beyond some initial checks I would just expend to use root->node and then all this checking of root vs parent goes away?

[mfleming]

i'm not sure why we need all of this validation? Why can't this just be:

   static inline size_t stack_depot_trie_node_size(const struct stack_depot_frame_run *run)
   {
      return ALIGN(struct_size(node, data,
             stack_depot_frame_run_bytes(run)), sizeof(unsigned long));
   }

[mfleming]

how is this even possible? we have no old array and no new array?

[mfleming]

please don't use these kinds of nested blocks. It took me 30 seconds to realise the open brakcet wasn't associated with the if() condition.

If you need variables declarations just stick them at the top of the fuction, or inside a nested block guarded by a condition, but not a naked one like this.

[mfleming]

i don't follow why we only check whether we can append way down here inside the append_prepare() function after checking whether we have a new array available. The logic flow seems really weird to reason about.

Either we need to append or we need a new array, why are we worrying about both cases in one code path?

[mfleming]

this is only ever called with the logs already held. Drop the lockdep_assert_held() and hoist this function into trie_side_publish(), please.

[mfleming]

it would really help reviewers to know that this is safe because A) concurrent writers are prohibited by stcak_depot_trie_workspace_lock and B) only readers need to see the updated value.

[mfleming]

stack_depot_trie_slot_binsesarch() or something would be more descriptive here, I think. The salient point is "slot to insert the child" not "lower bound".

[mfleming]

The trie insertion path is currently hard to review because most internal helpers are written defensively, as if they are public APIs receiving hostile inputs. That obscures the actual invariants.

In particular, the path around __stack_depot_trie_insert_plan(), trie_publish_append_prepare(), trie_promote_child(), trie_split_subtree_prepare(), and trie_split_child() repeatedly revalidates facts the writer path should already know:

• all trie writers are serialized by stack_depot_trie_workspace_lock
• published trie nodes have valid run metadata
• child arrays are sorted; only tail append mutates a live array
• non-tail insert/replace/split uses a private replacement array
• the planner determines the operation kind and exact slot/storage needs
• side-table storage is reserved before publish

If those are the intended invariants, I think they should be documented once near the writer lock / child-array definition, and the internal helpers should rely on them. Today the code keeps rediscovering state: e.g. trie_publish_append_prepare() branches on new_storage == NULL, reruns lower_bound(), rechecks appendability, and republishes a side-table update inline; trie_split_subtree_prepare() validates slot counts, topology, split bounds, and side-table publish failure after the planner supposedly made this operation valid; __stack_depot_trie_node_size() revalidates runs loaded from trie-owned nodes.

This makes the code look like it supports many impossible interleavings and partial states. A simpler contract would be: boundary/planning code validates once, then trusted build/publish helpers consume an explicit planned op (append-in-place, append-replace, promote, split) plus exact slots. Internal misuse can be WARN_ON_ONCE() or left to the invariant, not threaded through normal -EINVAL paths.

I’m not asking for a rewrite of the trie algorithm. I’m asking to narrow the helper contracts so invalid states are designed out rather than checked everywhere. That should make the insert path much easier to review and reduce the rollback/error-path surface.