pctrie: define lookup_readonly
Needs ReviewPublic
Actions

Authored by dougm on Jun 26 2025, 6:44 AM.

Details

Reviewers

alc
kib
markj

Summary

pctrie_lookup() does not currently modify the pctrie it is examining. Some callers depend on this when they invoke it holding a read lock. Define a readonly version of pctrie_lookup() that promises not to modify the pctrie in the future, and use it in place of pctrie_lookup() in places that hold only a read lock.

No functional change is expected.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

dougm requested review of this revision.Jun 26 2025, 6:44 AM

dougm created this revision.

dougm added reviewers: alc, kib, markj.Jun 26 2025, 7:03 AM

markj added inline comments.Jun 26 2025, 3:31 PM

sys/kern/subr_pctrie.c
343 ↗	(On Diff #157647)	A bit more detail would be helpful, for instance, "Returns the value stored at the index without mutating the trie and assuming that access is synchronized by a read lock or mutex."
sys/vm/vm_page.c
1767	Since a write lock is of course sufficient.
sys/vm/vm_radix.h
84 ↗	(On Diff #157647)	Shouldn't vm_page_lookup() assert that the write lock is held then?

dougm added inline comments.Jun 26 2025, 7:57 PM

sys/vm/vm_radix.h

84 ↗

(On Diff #157647)

If the first line of vm_page_lookup() is

VM_OBJECT_ASSERT_WLOCKED(object);

then I crash at boot time like this:

panic: Lock vmobject not exclusively locked @ ../../../vm/vm_page.c:1751

cpuid = 13
time = 1750967544
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe014a207a30
vpanic() at vpanic+0x136/frame 0xfffffe014a207b60
panic() at panic+0x43/frame 0xfffffe014a207bc0
__rw_assert() at __rw_assert+0x94/frame 0xfffffe014a207bd0
vm_page_lookup() at vm_page_lookup+0x27/frame 0xfffffe014a207bf0
vm_fault() at vm_fault+0xd81/frame 0xfffffe014a207d60
vm_fault_trap() at vm_fault_trap+0x65/frame 0xfffffe014a207da0
trap_pfault() at trap_pfault+0x27b/frame 0xfffffe014a207e10
trap() at trap+0x5c2/frame 0xfffffe014a207f30
calltrap() at calltrap+0x8/frame 0xfffffe014a207f30
--- trap 0xc, rip = 0x23e896, rsp = 0x8208f46d0, rbp = 0x8208f4730 ---

Is there a different assertion I should be using here?

Update comments, asserts, convert one more lookup to lookup_readonly.

dougm marked 3 inline comments as done.Jun 26 2025, 8:22 PM

kib added inline comments.Jun 26 2025, 10:26 PM

sys/vm/vm_radix.h
84 ↗	(On Diff #157647)	The point is that if your intent is to allow vm_radix_lookup() to modify trie, then the exclusive lock must be held. Otherwise, what is the point of adding _readonly()? Or put it differently, why vm_page_lookup() cannot be switched to vm_radix_lookup_readonly()?

Avoid defining new pctrie methods; just copy the pctrie (root) before the search and search the copy so that only the root of the copy can be modified.

Update to account for call to vm_page_lookup that was dropped from vm_fault.c.

dougm mentioned this in D51237: vm: don't use iterators for page allocation.Jul 23 2025, 4:20 AM