vmspace: Convert to refcount(9)
ClosedPublic
Actions

Authored by markj on Nov 2 2020, 5:58 PM.

Details

Reviewers

kib
alc
mmel
dougm

Commits

rS367334: vmspace: Convert to refcount(9)

Summary

This is mostly mechanical except for vmspace_exit(). There, I used the
new refcount_release_if_last() to avoid switching to vmspace0 unless
other processes are sharing the vmspace. In that case, upon switching
to vmspace0 we can unconditionally release the reference.

Remove the volatile qualifier from vm_refcnt now that accesses are
protected using refcount(9) KPIs.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Nov 2 2020, 5:58 PM

Herald added a subscriber: ae. · View Herald TranscriptNov 2 2020, 5:58 PM

markj requested review of this revision.Nov 2 2020, 5:58 PM

Harbormaster completed remote builds in B34568: Diff 79077.Nov 2 2020, 5:58 PM

markj added reviewers: kib, alc, mmel, dougm.Nov 2 2020, 5:58 PM

markj added a parent revision: D27056: refcount(9): Add refcount_release_if_last() and refcount_load().

kib added inline comments.Nov 2 2020, 10:03 PM

sys/vm/vm_glue.c
553 ↗	(On Diff #79077)	Should this be `refcount_load > 1` ?
sys/vm/vm_map.c
350 ↗	(On Diff #79077)	s/dummy/kernel/

Address feedback.

Harbormaster completed remote builds in B34589: Diff 79108.Nov 2 2020, 10:52 PM

kib accepted this revision.Nov 2 2020, 11:51 PM

This revision is now accepted and ready to land.Nov 2 2020, 11:51 PM

alc accepted this revision.Nov 3 2020, 6:01 PM

alc added inline comments.

sys/vm/vm_map.c
365–366 ↗	(On Diff #79108)	There is nothing wrong with the patch here. I just want to make an observation. Setting aside the exact KASSERT()s in some of the pmap implementations, we really don't need the pmap to be active on the underlying processor. We really only need that the pmap not be active on any other processor, because some of the pmap implementations intentionally perform non-atomic read-and-clear operations to minimize the cost of checking the dirty bit and clearing the PTE. I wonder how often we pointlessly activate the pmap here.
400 ↗	(On Diff #79108)	I would suggest merging this "if" statement with the one above it.

Tested on ARM and ARM64

markj marked an inline comment as done.Nov 4 2020, 4:26 PM

markj added inline comments.

sys/vm/vm_map.c
365–366 ↗	(On Diff #79108)	By "pointlessly activate the pmap" do you mean switching to vmspace0's pmap and back again? I believe it should be fairly rare since it requires a process to exit while using a shared vmspace. I ran a kernel build with the following dtrace script running in the background: `dtrace -n 'fbt::vmspace_exit:entry {@[args[0]->td_proc->p_vmspace->vm_refcnt] = count();}'` and got: 61 1 1 53328 i.e., in all but one case the vmspace had one reference at the time the function was called. The sole exception was a case where p->p_vm == &vmspace0. I'm not sure how that arises.