Details

Reviewers

alc
kib
markj

Commits

rG7becd87c988c: vm_grab: use iterator for grab lookup

Summary

When a page lookup fails in a vm_page_grab operation, use the results of the failed search, stored in an iterator, to begin the search for a predecessor to use in a call to vm_page_alloc_after(), to avoid doing that search from the root in vm_page_alloc(), as happens now.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

dougm requested review of this revision.Mar 15 2025, 4:19 AM

dougm created this revision.

In vm_page_grab_lookup, use the iterator for the lookup. Duh.

kib added inline comments.Mar 15 2025, 4:20 PM

sys/vm/vm_page.c
4825–4826	Code only sleeps if the page cannot be busied as requested. Initially this comment made me very confused.
4830	The first sentence of the comment should be an assert instead. Overall this is too detailed comment for the internal function. I would leave only the first paragraph of the comment, edited, IMO.
4887	I am not sure about the semantic of VM_ALLOC_WAITFAIL. If we sleep inside vm_page_grab_lookup(), should we also return NULL for VM_ALLOC_WAITFAIL? (This is the pre-existing bug, if indeed a bug).

kib added inline comments.Mar 15 2025, 4:25 PM

sys/vm/vm_page.c
4887	BTW, same question about VM_ALLOC_NOWAIT.

Update after (expected) checkin of D49391.

dougm mentioned this in D49391: vm_page: update legend documenting allocation.Mar 23 2025, 5:14 PM

markj added inline comments.Mar 23 2025, 10:31 PM

sys/vm/vm_page.c
4887	From my reading of this diff, the bug is in the patch, not the existing code. Without the patch, vm_page_grab_sleep(WAITFAIL) will sleep and then return false, and then vm_page_grab() returns NULL. That's the behaviour I'd expect. With the patch, vm_page_grab_lookup(WAITFAIL) will sleep and return NULL, whereupon we try to allocate a page at the pindex. That's incorrect: after sleeping, we don't know the state of the VM object since the VM object lock was dropped. At that point, WAITFAIL means, "signal failure to the caller and let them handle it". The problem with the patch is that the vm_page_grab_lookup() interface is not rich enough: it needs to tell the caller both whether it found and busied a page, and whether it had to sleep in order to do so.

Backing out the last update to the patch, since I've evidently completely failed to understand @kib's concerns, and so won't try to eliminate the WAITFAIL case from page grabbing.

In vm_page_grab, if VM_ALLOC_WAITFAIL is set in allocflags and vm_page_grab_lookup returns NULL, return NULL.

Add a *found parameter to vm_page_grab_lookup to distinguish found-but-sleep cases from not-found cases.

markj added inline comments.Mar 24 2025, 10:40 PM

sys/vm/vm_page.c
5082	A portion of the page is zeroed whether or not it was resident already, but the comment implies that it is zeroed only if it had to be paged in.

Apply suggested comment edit.

kib added inline comments.Mar 25 2025, 4:46 AM

sys/vm/vm_page.c
4853
5081–5086
5088	I suggest to merge the two paragraphs: The object must be locked on entry. This routine may sleep. The lock will, however, be released and reacquired if the routine sleeps.

Apply suggestions from @kib.

kib added inline comments.Mar 25 2025, 8:16 AM

sys/vm/vm_page.c
4831	This and the next herald comment could be compactified in the same way, by merging the two paragraphs.
4849	So we normally return non-NULL m and simultaneously set found to false. IMO this should be either fixed or documented that found is only meaningful when the return value is NULL.
4880	Could we get mpred there with pindex equal our pindex? I do not see why it is impossible, but I am sure that such mpred is wrong (and asserted by) vm_page_alloc_after().