Page MenuHomeFreeBSD

Fix some errors in the page busying code
ClosedPublic

Authored by markj on Dec 14 2020, 3:42 PM.

Details

Summary

In vm_page_busy_acquire(), load the object pointer using
atomic_load_ptr() as we do elsewhere. Per the comment, the object
identity must be consistent across sleeps.

In vm_page_grab_sleep(), pass the correct pindex to
_vm_page_busy_sleep(). I'm not sure why clang doesn't warn about this.
The pindex is used to check the page's identity before going to sleep.
In particular, vm_page_grab_sleep() is used by unlocked grab, so the
object lock is not necessarily held when verifying the page's identity,
and the pindex may change if the page is moved, or freed and
re-allocated. I believe this can result in spurious VM_PAGER_FAILs from
vm_page_grab_valid_unlocked() or early termination of
vm_page_grab_pages_unlocked().

In vm_page_grab_pages(), pass the correct pindex to
vm_page_grab_sleep(). Otherwise I believe vm_page_grab_pages() will
effectively spin when attempting to busy a busy page after the first
index in the range.

Test Plan

This was noticed while doing some refactoring, I have no test case for these bugs.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj requested review of this revision.Dec 14 2020, 3:42 PM
markj edited the test plan for this revision. (Show Details)
markj added reviewers: alc, kib, jeff.
This revision is now accepted and ready to land.Dec 14 2020, 9:29 PM
alc added inline comments.
sys/vm/vm_page.c
896

Why isn't the pindex also cached? The page could move to a different offset within the same object.

901

I'm puzzled by the "m->object == NULL" case here. We appear to be allowing for the page to be removed from an object but not the possibility that is removed and quickly added to a different object.

sys/vm/vm_page.c
901

Responding to both comments here. I believe that consumers are supposed to guarantee that pages will not be freed or moved to another object if VM_ALLOC_WAITOK is specified. More concretely, VM_ALLOC_WAITOK must not be specified if the object has OBJ_ANON set. Otherwise the page must be wired, so it may be removed from its object but will not be freed, and vm_page_remove() preserves the pindex.

I think is would be correct to add the following assertions:

if (obj != NULL && (obj->flags & OBJ_ANON) != 0)
    KASSERT((allocflags & (VM_ALLOC_NOWAIT | VM_ALLOC_WAITFAIL)) != 0);
else
    KASSERT(vm_page_wired(m));

At least, I'm able to boot with this change applied.

This revision was automatically updated to reflect the committed changes.