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

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

Details

Reviewers

alc
dougm
kib
markj

Commits

rS356933: Reduce object locking in vm_fault. Once we have an exclusively busied page we

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 - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jeff created this revision.Jan 4 2020, 8:26 PM

Harbormaster completed remote builds in B28481: Diff 66350.Jan 4 2020, 8:26 PM

jeff mentioned this in D23033: Don't hold the object lock across getpages..Jan 4 2020, 8:38 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 - subversion.

Herald added a subscriber: imp. · View Herald TranscriptJan 4 2020, 8:45 PM

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.