Make page busy state deterministic on free. Pages associated with objects must be xbusy on free. Pages not associated with objects must not be busy.
ClosedPublic
Actions

Authored by jeff on Dec 15 2019, 2:53 AM.

Details

Reviewers

alc
dougm
kib
markj

Commits

rS356002: Make page busy state deterministic on free. Pages must be xbusy when

Summary

This closes the loop on xbusy protecting page identity. It makes lockless lookup much cleaner because I don't have to worry about code saying if (busy) unbusy(). Because busy does not have a strong ownership concept you can unlock another threads lock. As you can see very few changes were required to make the busy state consistent at this point.

I personally think the vm_page_replace() API is cleaner now. It is effectively always _checked(). We don't strictly need a pindex argument although in some cases it does provide for an additional assert.

It is not absolutely necessary that I assert the state but I prefer the stronger guarantee this gives. In the future it may provide an avenue to resolve the NOFREE flag on vm objects.

Test Plan

passes stress2

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jeff created this revision.Dec 15 2019, 2:53 AM

Herald added a subscriber: emaste. · View Herald TranscriptDec 15 2019, 2:53 AM

Harbormaster completed remote builds in B28160: Diff 65676.Dec 15 2019, 2:53 AM

jeff retitled this revision from Make page busy state deterministic on free. Pages associated with objects must be xbusy on free. Pages not associated with objects must not be busy. to Make page busy state deterministic on free. Pages associated with objects must be xbusy on free. Pages not associated with objects must not be busy..Dec 15 2019, 3:06 AM

jeff edited the summary of this revision. (Show Details)

jeff edited the test plan for this revision. (Show Details)

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 TranscriptDec 15 2019, 3:06 AM

jeff added inline comments.Dec 15 2019, 3:06 AM

sys/dev/xen/gntdev/gntdev.c
834 ↗	(On Diff #65676)	Is mres == NULL possible?
sys/dev/xen/privcmd/privcmd.c
178 ↗	(On Diff #65676)	I don't know if mres == NULL is even possible?

markj added inline comments.Dec 16 2019, 9:40 PM

sys/vm/vm_fault.c
1258 ↗	(On Diff #65676)	Instead of adding vm_page_remove_hold(), which has no other callers, can we wrap all of this into a single function? I don't really like the direct manipulation of the reference count.
sys/vm/vm_kern.c
589 ↗	(On Diff #65676)	Can we require that one of VM_ALLOC_{NOWAIT,WAITOK,WAITFAIL} be specified? I find this KPI very confusing otherwise.
sys/vm/vm_page.c
1623 ↗	(On Diff #65676)	This is just vm_page_remove_hold().
2796 ↗	(On Diff #65676)	Why can't we use vm_page_replace() here? We know that there are no other references to the page.
3921 ↗	(On Diff #65676)	I will fix this.

kib added inline comments.Dec 18 2019, 12:20 AM

sys/dev/xen/gntdev/gntdev.c
834 ↗	(On Diff #65676)	I suspect not.
sys/vm/vm_fault.c
1258 ↗	(On Diff #65676)	I find vm_page_remove_hold() existence quite logical. I.e., even if these three calls are wrapped into a helper, remove_hold() is still quite understandable standalone piece of code.