vm: read-locked backing object fault handling

[mjg]

Summary

It is likely that the backing object has the page ready to use, which can be handled with a read lock only (or even without locks, but I did not go for it right now).

commit 19d3829d1c48ca41b223e1079ab58f50240a8ae4 (HEAD -> vm15local)
Author: Mateusz Guzik <[e-mail]>
Date:   Sat Aug 20 11:17:46 2022 +0000

    vm: read-locked backing object fault handling
    
    This largely eliminates vm object contention when doing -j 104
    buildkernel of stable/12:
    
    before:
    
    103744637 (rw:vm object)
    6091096 (spin mutex:turnstile chain)
    4682478 (rw:pmap pv list)
    4658491 (sleep mutex:process lock)
    
    after:
    
    22508492 (rw:pmap pv list)
    5903096 (sleep mutex:process lock)
    2476543 (sx:vm map (user))
    1119649 (sleep mutex:pmap)
    
    Reviewed by:
    Differential Revision:

commit f1b93cc2b18a627d64afa34043faa3bb3948195b
Author: Mateusz Guzik <[e-mail]>
Date:   Sun Aug 7 13:07:59 2022 +0000

    vm: include function name when checking vm_fault_object retval
    
    Reviewed by:
    Differential Revision:

commit 0417436c08afed77fbead93eeb192e6b67dce815
Author: Mateusz Guzik <[e-mail]>
Date:   Sun Aug 7 13:05:47 2022 +0000

    vm: move up object lock asserts in fault functions
    
    No functional changes.
    
    Reviewed by:
    Differential Revision:

commit d8eda53492c18f75a81e6b3f171e45403ffae517
Author: Mateusz Guzik <[e-mail]>
Date:   Sat Aug 20 12:27:23 2022 +0000

    vm: employ vm_page_aflag_set_cond in vm_fault_busy_sleep
    
    Reviewed by:
    Differential Revision:

commit 78765a4a0308b22d15bd86bb70e89a510799c538
Author: Mateusz Guzik <[e-mail]>
Date:   Sat Aug 20 12:26:39 2022 +0000

    vm: add vm_page_aflag_set_cond
    
    Reduces ping-ponging if the bits to set are already there.
    
    Reviewed by:
    Differential Revision:

Test Plan

will ask pho. built tinderbox fwiw.

[markj]

sys/vm/vm_fault.c

1053

The lock state should go into faultstate.

1461

It's better to handle this in unlock_and_deallocate().

1652

If the lock upgrade fails, then we just retry the lookup. Why not keep vm_fault_object() intact and retry the lookup within that function?

[mjg]

• add UNLOCKED state
• fold stuff back into vm_fault_object

[mjg]

sys/vm/vm_fault.c

1461

this would require patching all other callers of the routine, or reworking it internally to wrap a vriant which grabs objlocked argument. i explicitly did not do that to avoid extra changes in the area given the entire thing will need to be refactored down the road.

[mjg]

• set the unlocked state

[markj]

sys/vm/vm_fault.c

1461

I suggested to put the objlock state in the faultstate structure and handle it without passing around an extra parameter. Why not do it? That way, vm_fault_lock_vnode() can be simplified as well, and you don't have to modify every caller.

[mjg]

sys/vm/vm_fault.c

1461

The patch as implemented confines rlock to the loop, without affecting other code. Most notably everything outside of it will only ever see a write lock, just like without the patch. Moving the lock state to the faultstate struct would require patching *all* places which unlock and it's just a lot of churn for no benefit that I see.

[mjg]

refactor the change. rlocked lookup is moved into a dedicated routine, read-lock is not seen anywhere else modulo one func which got adjusted for it.

[mjg]

sys/vm/vm_fault.c

1443

I don't think this bit is pretty, but given unlocked operation later I suspect this will require a dedicated variant anyway.

1555

I don't expect this to ever be a real problem -- the page has to go invalid in-between the initial check and xbusy, which I strongly suspect will be rare. leaving the code like this avoids modifying vm_fault_object.

[mjg]

sys/vm/vm_fault.c

1116

i moved this and the same assert in vm_fault_object up, i dn't understand why they were pushed below. this can be a separate commit.

[kib]

sys/vm/vm_fault.c

1143

You can do lockless lookup for the page (and may be checking its validity), only to select rlock vs. wlock there, probably eliminating almost all lock upgrades.

1434

Is it time to finally have VM_OBJECT_UNLOCK() ?

1538

Why did you omitted the checks for dead object flag and state?

1659

This should be a separate commit.

1688

Is this change of the comment style needed?

[mjg]

sys/vm/vm_fault.c

1143

what is rlock for in this case if the page was found and validated locklessly?

how about something of this sort, with failing case wlocking the object

static enum fault_status
vm_fault_object_unlocked(struct faultstate *fs)
{

        VM_OBJECT_ASSERT_UNLOCKED(fs->object);

        fs->m = vm_page_lookup_unlocked(fs->object, fs->pindex);
        if (fs->m == NULL)
                return (FAULT_CONTINUE);

        if (!vm_page_all_valid(fs->m))
                return (FAULT_CONTINUE);

        if (!vm_page_tryxbusy(fs->m)) {
                vm_fault_busy_sleep_unlocked(fs);
                return (FAULT_RESTART);
        }

        if (fs->m->object != fs->object || fs->m->pindex != fs->pindex ||
            !vm_page_all_valid(fs->m)) {
                vm_page_xunbusy(fs->m);
                fs->m = NULL;
                return (FAULT_CONTINUE);
        }

        /*
         * Finally make sure the object is still alive.
         */
        if ((fs->object->flags & OBJ_DEAD) != 0) {
                vm_page_xunbusy(fs->m);
                fs->m = NULL;
                return (FAULT_CONTINUE);
        }

        return (FAULT_SOFT);
}

[markj]

sys/vm/vm_fault.c

1434

We already have VM_OBJECT_DROP(), BTW. I think VM_OBJECT_UNLOCK() makes more sense as a name though.

[kib]

sys/vm/vm_fault.c

1434

_DROP() calls the lock class method, which at least gives more overhead than plain rw_unlock(). It also intended to be used in pair with _PICKUP.

[mjg]

• added vm_page_aflag_set_cond. vm_page_aflag most of the time finds bit not already set, so I did not patch it.
• pre-lookup a page before deciding which lock to take
• rename funcs to have _rlocked and _wlocked suffixes

[mjg]

sys/vm/vm_fault.c

1434

This would make things slower multi-threaded. write unlock does not pre-read the lock and adding that would require an extra transaction. read unlock happens to read upfront, but that's only a current artifact of the implementation

1688

no, but it matches other comments in the file and since I'm moving the code around I don't see why not

[mjg]

so can i get some flames on this?

[mjg]

replaced with D38964