Page MenuHomeFreeBSD

vm: expand rlock usage in fault handling
Needs ReviewPublic

Authored by mjg on Wed, Aug 3, 10:14 PM.

Details

Reviewers
markj
alc
kib
dougm
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).

This largely eliminates vm object contention when doing -j 104 buildkernel:

before:

103744637 (rw:vm object)   
6091096 (spin mutex:turnstile chain)  
4682478 (rw:pmap pv list)  
4658491 (sleep mutex:process lock)  
3075988 (sx:vm map (user))  
1431320 (sleep mutex:pmap)

after:

22508492 (rw:pmap pv list)
5903096 (sleep mutex:process lock)
2476543 (sx:vm map (user))  
1119649 (sleep mutex:pmap) 
665686 (sleep mutex:vm page free queue)
588529 (sleep mutex:vm active pagequeue)
455094 (sleep mutex:umtxql)   
285416 (rw:vm object)
Test Plan

will ask pho. built tinderbox fwiw.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

mjg requested review of this revision.Wed, Aug 3, 10:14 PM
mjg created this revision.
mjg edited the summary of this revision. (Show Details)
sys/vm/vm_fault.c
1052

The lock state should go into faultstate.

1414

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

1630

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?

  • add UNLOCKED state
  • fold stuff back into vm_fault_object
sys/vm/vm_fault.c
1414

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.

  • set the unlocked state
sys/vm/vm_fault.c
1414

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.

sys/vm/vm_fault.c
1414

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.

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.

sys/vm/vm_fault.c
1385

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

1503

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.

sys/vm/vm_fault.c
1055

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.

sys/vm/vm_fault.c
1082

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.

1371

Is it time to finally have VM_OBJECT_UNLOCK() ?

1486

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

1638

This should be a separate commit.

1659

Is this change of the comment style needed?

sys/vm/vm_fault.c
1082

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);
}
sys/vm/vm_fault.c
1371

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

sys/vm/vm_fault.c
1371

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