Page MenuHomeFreeBSD

Reduce object locking in vm_fault. The object is only needed until we have an exclusive busy.
ClosedPublic

Authored by jeff on Jan 4 2020, 8:26 PM.

Details

Summary

This drastically shortens maximum hold times on object locks in fault. We will basically only hold it through page allocation. It also eliminates some ugly lock/unlock/relock style code.

I intend to continue to refactor fault into smaller functions but I wanted to introduce the locking change first since it has the largest functional impact.

Diff Detail

Repository
rS 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

jeff created this revision.Jan 4 2020, 8:26 PM
jeff edited the summary of this revision. (Show Details)Jan 4 2020, 8:45 PM
jeff added reviewers: alc, dougm, kib, markj.
jeff set the repository for this revision to rS FreeBSD src repository.
jeff added inline comments.Jan 4 2020, 9:11 PM
sys/vm/vm_fault.c
340 ↗(On Diff #66350)

We could almost drop the lock before pmap_enter() but we are relying on it to prevent invalidation of pages for read-only faults.

633 ↗(On Diff #66350)

I don't really like this but my only other option is to perhaps make this bool part of the fault_state.

962 ↗(On Diff #66350)

Default objects hold the lock for the full duration.

1114 ↗(On Diff #66350)

I don't like the extra label but I am continuing to break this function up and simplify control flow in later patches.

1210 ↗(On Diff #66350)

Fixed indentation while adding the extra TRYWLOCK.

1462 ↗(On Diff #66350)

This is actually a fairly substantial win.

kib added inline comments.Jan 6 2020, 1:48 AM
sys/vm/vm_fault.c
337 ↗(On Diff #66350)

So we potentially dirty the object even despite the pmap_enter() could fail, same for prefault.

962 ↗(On Diff #66350)

This explanation should be added to the comment, perhaps ?

1185 ↗(On Diff #66350)

'A valid page has been found, busied by us' (modulo my english).

1210 ↗(On Diff #66350)

And remove useless () around conditions while there, please.

jeff added inline comments.Jan 6 2020, 2:23 AM
sys/vm/vm_fault.c
337 ↗(On Diff #66350)

Yeah it's safe to move this down. I can do so.

1185 ↗(On Diff #66350)

A valid page has been found and exclusively busied.

1210 ↗(On Diff #66350)

will do.

jeff updated this revision to Diff 66661.Jan 12 2020, 9:07 PM

Review feedback. Mostly style and comments. Re-shuffle vm_fault_soft_fast() to properly handle the error case.

kib accepted this revision.Jan 13 2020, 8:39 PM
This revision is now accepted and ready to land.Jan 13 2020, 8:39 PM
markj accepted this revision.Jan 13 2020, 8:46 PM