Page MenuHomeFreeBSD

pctrie: define lookup_readonly
Needs ReviewPublic

Authored by dougm on Jun 26 2025, 6:44 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Oct 14, 6:12 PM
Unknown Object (File)
Fri, Sep 26, 7:31 AM
Unknown Object (File)
Thu, Sep 25, 12:15 PM
Unknown Object (File)
Tue, Sep 23, 6:26 PM
Unknown Object (File)
Sep 18 2025, 8:05 PM
Unknown Object (File)
Sep 17 2025, 10:48 AM
Unknown Object (File)
Sep 5 2025, 10:00 PM
Unknown Object (File)
Sep 5 2025, 5:34 AM
Subscribers
None

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.
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?

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.

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.