KRN-1117: simplify stackdepot trie insertion - #5
Open
mfleming wants to merge 13 commits into
Open
Conversation
The main loop mixed child insertion, splitting existing children on a diverging nibble, and promoting an internal child to a leaf, which made the ownership of the walk state hard to follow. Split those three transitions into dedicated helpers so the loop reads as a decision tree over the child slot's current state. Also skip the x86_64 frame-compression KUnit case under CONFIG_UML, since the UML build pulls in the x86_64 arch bits but can't exercise the direct-map addresses the test asserts against.
Follow-up to 812b3f5 ("Simplify stack_depot_trie_insert_locked() control flow"). The missing-children and no-matching-child cases both end up calling the same helper to append the remaining path, so fold them into one branch and rename trie_insert_child_locked() to trie_insert_path_locked() to match what it actually does. Teach trie_child_array_find_slot() to accept a NULL array (treated as empty, returning pos = 0) so the caller doesn't need a separate NULL check or a conditional pos argument. Document the helper's return contract while we're there. While here, funnel every insertion case through a single trie_finish_insert() return via break, and drop a one-line comment on each of the four cases in the loop so the shape is obvious at a glance.
These helpers are all file-static and only ever run under the trie writer lock or pool_lock, so the _locked tag doesn't tell the reader anything the surrounding code doesn't already make obvious. Rename them to the shorter forms; no behaviour change.
The retry path in stack_depot_trie_save() only re-ran the same preallocate-then-insert sequence after releasing the leftover pool page and side-table prealloc, which the out_free path already handles. Drop it and inline __stack_depot_trie_alloc_prealloc() so the allocation and locking sit in one place.
Taking pool_lock just to read new_pool is overkill. new_pool can change between this check and the actual insertion anyway, so the test is only an opportunistic hint about whether we need to preallocate a page. Drop the lock and READ_ONCE() the pointer, matching what the hash stackdepot save path already does.
The new_pool hint was racy. Another writer could publish a pool between our READ_ONCE() and the trie insert, leaving us without a preallocated page when we needed one. Drop the hint and always try the pool-page prealloc once the side-table prealloc has succeeded. If the insert doesn't consume the page we still hand it to depot_keep_new_pool() or free it, so keep/free semantics are unchanged. The out_free label goes away too now that side-table prealloc failure has nothing to clean up.
The old code mixed child/node/children/array in various ways that made following it a bit confusing. Rename the container to struct stack_depot_trie_children with a nodes[] flexible member, and update the helpers, locals, and pending list to match. A trie node now has a parent and children, and a children container owns nodes; each individual child is a stack_depot_trie_node. No behaviour change.
The generic trie_pool_alloc_insert() and trie_build_split() helpers grew several unrelated parameters and workspace slots to cover all three insertion cases at once, which made each caller harder to follow than the work it was actually doing. Inline the pool reservation and rollback into trie_insert_path(), trie_split_child(), and trie_promote_child() so each path allocates only what it needs and releases the exact set it took on failure. Make trie_pool_alloc_children() capacity-based and self-sizing, and let trie_pool_release_children() derive the size from the object it frees. Zero the insertion workspace once up front and drop the size-carrying release variant. Split handling now has a common setup, a single divergence branch that builds the new tail and orders the two suffixes, and shared finalisation. Node sizing is expressed via trie_node_bytes_for(mode, nr_entries) and local names spell out prefix/tail intent. No behaviour change intended. Validated with git diff --check, strict checkpatch on the diff, kbuild of lib/stackdepot.o and lib/tests/stackdepot_kunit.o, and stackdepot KUnit under UML.
mfleming
force-pushed
the
mfleming/KRN-1117-simplify-trie-insert
branch
from
July 29, 2026 14:03
3852423 to
8d43f65
Compare
Drop the in-place tail append so published children are immutable and readers no longer need to tolerate transient NULL slots during a lockless search. trie_children_copy() now accepts a NULL source so the first-child case shares the copy-and-publish path. Build the new node chain incrementally with local pointers instead of the stack_depot_trie_alloc scratch arrays, and roll back on allocation failure by walking parent links from the leaf.
The trie insert and split paths were hard to follow. Insertion sized the path in one pass, allocated in another via a global stack_depot_trie_insert_alloc workspace whose arrays were sized to CONFIG_STACKDEPOT_MAX_FRAMES, then built the chain in a third pass. Split reused the same workspace and its bulk rollback loop. The workspace also demanded init-time allocation from memblock or kvzalloc. Fold all of that into a single trie_path_alloc() shared by insert and split. It walks @entries once, allocates each node and its path children as it goes, and rolls back only what it allocated on failure. The workspace, its init-time allocation, the sizing pass, and the bulk rollback all go away. trie_children_copy() is split into trie_children_init(), which initialises an unpublished container from an immutable source, and trie_children_insert(), which shifts and inserts into an unpublished container. Callers now express "copy then insert" directly, and the immutable/unpublished invariants are visible at each step. Validation: git diff --check, strict checkpatch, kernel-doc, kbuild of lib/stackdepot.o and lib/tests/stackdepot_kunit.o, and the UML stackdepot KUnit suite.
The trie code carried several overlapping vocabularies -- leaf, COW/generation, tail -- for the same underlying concepts. Reading the insert and split paths meant translating between them. Settle on one glossary so the algorithm reads directly. node one frame run in the trie children immutable container of child-node pointers child one node inside a children container path linear sequence of nodes for a stack path_root first node of that sequence stack_id optional non-zero ID set only where a stored stack ends prefix shared portion of a split child suffix divergent portion(s) of a split child pos index within a children container slot root-or-child pointer location the writer updates leaf_id and the "leaf" node concept go away; ordinary nodes carry an optional stack_id and there is no distinct leaf type. The RCU/COW and "generation" comments on children go away in favour of describing the replacement children container as immutable once published. Tail becomes suffix in the split path. While here, drop the __stack_depot_ prefix from private static trie helpers. We were using __ merely to mean "private", which conflicts with the kernel convention of reserving __ for genuine lower-level or public-interface pairings. These helpers are all file-local, so name them consistently with the trie_ prefix instead. No behaviour change. Validated with git diff --check, strict checkpatch, kernel-doc, kbuild objects, and UML stackdepot KUnit (8/8).
trie_finish_insert() is three lines and has a single caller. Folding it into stack_depot_trie_insert() makes the successful stack-ID update and return visible at the call site.
Rename trie_side_table_publish_new_node() to trie_side_table_publish() and reorder its arguments to (node, stack_id). Document that the path must be fully initialised before the call, that publication commits the path with no rollback, and that the side-table mapping must be installed before the trie slot that makes @node reachable. The two-node split variant had a single caller, so inline it into trie_split_child() and drop trie_side_table_publish_split_nodes(). Similarly drop trie_children_replace_at() and express the init plus pointer store at its callers, where the intent reads directly. Add kernel-doc for trie_pool_alloc()/trie_pool_release() clarifying that @SiZe is a byte count, and for trie_pool_alloc_children()/ trie_pool_release_children() clarifying that @capacity counts child-pointer entries rather than bytes. Document trie_drain_pending_children() -- when it must run relative to pool_lock and allocation, the FIFO stop condition, and the paired node+children reclaim -- and document trie_reparent_children() as a reparent-before-retirement step, adding a lockdep assertion for the writer lock. Normalize trie function prototypes to keep the return type on the same line as the name where the signature fits in 100 columns. Validation: diff review, strict checkpatch, and kernel-doc all clean. UML stackdepot KUnit reports 8/8 passing on the behaviour-changing revision; the drain-comment refinement afterwards is comment-only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is a review-helper series for the stackdepot trie port. It keeps the trie behavior intact while simplifying the insertion/preallocation control flow:
stack_depot_trie_insert()into smaller case helperstrie_child_array_find_slot()handle empty child arrays_lockedsuffixes from private helpersstack_depot_trie_save()and remove the retry pathValidation
git diff --checklib/stackdepot.o lib/tests/stackdepot_kunit.ostackdepot8/8 passed withstackdepot.trie_enabled=1 stackdepot_kunit.trie_pool_limit=8192