Page MenuHomeFreeBSD
Differential D38964

vm: read-locked fault handling for backing objects ; simplest take
ClosedPublic
Actions

Authored by mjg on Mar 7 2023, 10:06 PM.
Tags
None
Referenced Files
F102992342: D38964.id118657.diff
Tue, Nov 19, 2:08 PM
Unknown Object (File)
Sun, Nov 17, 1:45 PM
Unknown Object (File)
Fri, Nov 15, 1:58 AM
Unknown Object (File)
Fri, Nov 15, 1:51 AM
Unknown Object (File)
Fri, Nov 15, 1:34 AM
Unknown Object (File)
Fri, Nov 15, 1:28 AM
Unknown Object (File)
Fri, Nov 15, 1:20 AM
Unknown Object (File)
Fri, Nov 15, 1:19 AM
View All 78 Files
Subscribers
emaste
imp

Details

Reviewers
kib
markj
Commits
rGfdb1dbb1cc06: vm: read-locked fault handling for backing objects
rG73b951cd3914: vm: move up object lock asserts in fault functions
rGbdfd1adc9908: vm: add VM_OBJECT_UNLOCK
Summary
commit 4988ddd9f529239ebb510ca8c5d05654f1ef9553
Author: Mateusz Guzik <mjg@FreeBSD.org>
Date:   Tue Mar 7 20:56:54 2023 +0000

    vm: read-locked fault handling for backing objects
    
    This is almost the simplest patch which manages to avoid write locking
    for backing objects, as a result mostly fixing vm object contention
    problems.
    
    What is not fixed:
    1. cacheline ping pong due to read-locks
    2. cacheline ping pong due to pip
    3. cacheling ping pong due to object busying
    4. write locking on first object
    
    On top of it the use VM_OBJECT_UNLOCK instead of explicitly tracking the
    state is slower multithreaded that it needs to be, done for simplicity
    for the time being.
    
    Sample lock profiling results doing -j 104 buildkernel on tmpfs:
    before:
    71446200 (rw:vmobject)
    14689706 (sx:vm map (user))
    4166251 (rw:pmap pv list)
    2799924 (spin mutex:turnstile chain)
    
    after:
    19940411 (rw:vmobject)
    8166012 (rw:pmap pv list)
    6017608 (sx:vm map (user))
    1151416 (sleep mutex:pipe mutex)
    
    Reviewed by:
    Differential Revision:  https://reviews.freebsd.org/D38964

commit 388c60608388bbe484322fbd01f03551602f0db5
Author: Mateusz Guzik <mjg@FreeBSD.org>
Date:   Tue Mar 7 20:47:44 2023 +0000

    vm: add VM_OBJECT_UNLOCK
    
    Reviewed by:
    Differential Revision:  https://reviews.freebsd.org/D38964

commit be2b02c0b73e1e46e0148ec0f825e902b62d3630
Author: Mateusz Guzik <mjg@FreeBSD.org>
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:  https://reviews.freebsd.org/D38964

Replacement for D36038.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

mjg created this revision.Mar 7 2023, 10:06 PM
Herald added a subscriber: imp. · View Herald TranscriptMar 7 2023, 10:06 PM
mjg requested review of this revision.Mar 7 2023, 10:06 PM
mjg added inline comments.Mar 7 2023, 10:08 PM
sys/vm/vm_fault.c
1475

note that for example here the code already unlocked the object and locked it again later. if this is correct behavior as is, the patch does not change it.

mjg added inline comments.Mar 7 2023, 10:59 PM
sys/vm/vm_fault.c
1444

this bit may be a little iffy but is not strictly needed for the scalability boost

what it can do instead is set a flag which disables readlocking and FAULT_RESTART

mjg updated this revision to Diff 118508.Mar 7 2023, 11:18 PM
  • add the readlock disablement thing
sys/vm/vm_fault.c
1444

so I added the restart here. I checked this case does not happen in cases I'm worried about anyway.

mjg updated this revision to Diff 118509.Mar 7 2023, 11:21 PM
kib added inline comments.Mar 7 2023, 11:51 PM
sys/vm/vm_fault.c
1070

Free requires write lock for fs->object.

1452

Mark, this comment needs to be updated for the removal of the default pager.

mjg added inline comments.Mar 8 2023, 12:06 AM
sys/vm/vm_fault.c
1070

why? only the page is modified and to my reading it has to be xbusy on entry

mjg added inline comments.Mar 8 2023, 12:07 AM
sys/vm/vm_fault.c
1070

now that i wrote it, i think i see vm_page_free_toq -> vm_page_free_prep -> vm_page_object_remove

I'll think about what to do here

kib added inline comments.Mar 8 2023, 12:08 AM
sys/vm/vm_fault.c
1070

Because the page is freed from the object.

This is actually asserted in fault_page_free(), and then in vm_page_free()->vm_page_free_prep()->vm_page_object_remove().

kib added inline comments.Mar 8 2023, 12:51 AM
sys/vm/vm_fault.c
1070

In fact I think that the situation is rare, in the sense that to get the object read-locked and need to free the page, you must find invalid page on the queue. If the fault handler allocated the page, the thread already locked the object exclusive.

It would be fine to leave the page alone, but it would prevent the shadowing from working.

markj added inline comments.Mar 8 2023, 4:12 PM
sys/vm/vm_fault.c
216

The write lock must be held in order to free a page.

1444

It would be worth adding a debug counter for this case. Akin to vm.stats.page and vm.stats.object we could have vm.stats.fault for tracking how often "rare" code paths in the fault handler actually get executed.

1452

D38985

sys/vm/vm_object.h
271

BTW, this is a cheaper implementation of VM_OBJECT_DROP.

mjg updated this revision to Diff 118538.Mar 8 2023, 5:28 PM
  • write lock for fault_page_free and restart if not possible
mjg added a comment.EditedMar 8 2023, 5:30 PM

So the idea is to tryupgrade and if that fails drop the page on the floor and goto RetryFault, which I would hope sorts out the problem -- the bad page will be found second time around with the write lock or be sorted out by someone else.

I verified with the printf that the condition *does* occur (about once for buildkernel on tmpfs) and several times on zfs, but it does not impede on lock contention reduction.

I have to say this *really* needs a stable lockless way to do the fault. seqc around obj collapse and freeing would definitely help here.

mjg added inline comments.Mar 8 2023, 5:31 PM
sys/vm/vm_fault.c
1615

bunch of ifs instead of switch because of the last case doing 'break' of the outer loop -- i find this to be much more readable

kib accepted this revision.Mar 9 2023, 12:46 AM
This revision is now accepted and ready to land.Mar 9 2023, 12:46 AM
mjg updated this revision to Diff 118577.Mar 9 2023, 3:30 PM
  • whack WIP leftovers
  • update commitm essage with numbers

the patch is under test by pho

This revision now requires review to proceed.Mar 9 2023, 3:30 PM
mjg edited the summary of this revision. (Show Details)Mar 9 2023, 3:31 PM
mjg mentioned this in D36038: vm: read-locked backing object fault handling.
emaste added a subscriber: emaste.Mar 9 2023, 3:47 PM
markj added inline comments.Mar 9 2023, 4:10 PM
sys/vm/vm_fault.c
130

This isn't a "fault parameter", it belongs under "control state" .

markj accepted this revision.Mar 9 2023, 4:10 PM
This revision is now accepted and ready to land.Mar 9 2023, 4:10 PM
Closed by commit rG73b951cd3914: vm: move up object lock asserts in fault functions (authored by mjg). · Explain WhyMar 11 2023, 11:08 AM
This revision was automatically updated to reflect the committed changes.
mjg added a commit: rG73b951cd3914: vm: move up object lock asserts in fault functions.
mjg added a commit: rGbdfd1adc9908: vm: add VM_OBJECT_UNLOCK.
mjg added a commit: rGfdb1dbb1cc06: vm: read-locked fault handling for backing objects.

Revision Contents
Changeset List

PathSize
sys/
vm/
47 lines
2 lines

Diff 118508

View Options

sys/vm/vm_fault.c

Loading...
View Options

sys/vm/vm_object.h

Loading...