Fix a few places that free a page from an object without busy held.
ClosedPublic
Actions

Authored by jeff on Nov 30 2019, 9:01 AM.

Details

Reviewers

kib
markj
alc
dougm

Commits

rS355314: Fix a few places that free a page from an object without busy held. This is

Summary

Like D22610 this is a nop with current code but gives me stronger asserts. I am building towards a patch that gets rid of the following code in vm_page_free()

if (vm_page_xbusied(m))
        vm_page_xunbusy(m);

This is unsafe with lockless lookup since we do not have ownership information in non-debug kernels.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 27865
Build 26037: arc lint + arc unit

Event Timeline

jeff created this revision.Nov 30 2019, 9:01 AM

Herald added a subscriber: andrew. · View Herald TranscriptNov 30 2019, 9:01 AM

Harbormaster completed remote builds in B27857: Diff 65065.Nov 30 2019, 9:01 AM

jeff edited the summary of this revision. (Show Details)Nov 30 2019, 9:12 AM

jeff added reviewers: kib, markj, alc, dougm.

jeff set the repository for this revision to rS FreeBSD src repository - subversion.

Herald added a subscriber: imp. · View Herald TranscriptNov 30 2019, 9:12 AM

jeff added inline comments.Nov 30 2019, 9:14 AM

sys/arm/nvidia/drm2/tegra_bo.c
68–71	This section is super weird.

alc added inline comments.Nov 30 2019, 6:07 PM

sys/amd64/sgx/sgx.c
392	This assignment no longer serves any purpose.
729	VM_ALLOC_NOCREAT? If only to clarify the intent, which is only to lookup and busy. Further, performing an allocation might mask an error that has arisen elsewhere.
743	Ditto.

jeff added inline comments.Nov 30 2019, 9:16 PM

sys/amd64/sgx/sgx.c
743	will do.
sys/dev/md/md.c
1105	I also want to bring attention to this while I'm here. There are some cases that do if (!vm_page_wired()) free() or some variant. I think here we can be reasonably certain that the pages are not visible somewhere that they can be wired and that's why this never panics. However, for consistency sake and future proofing it might be better to have some free variant which does remove and leaves the page if it's still wired? Many places that call pagers repeat some variant of this. Also if (vm_page_remove()) vm_page_free(), etc.

kib added inline comments.Nov 30 2019, 10:37 PM

sys/arm/nvidia/drm2/tegra_bo.c
68–71	I suspect that tegra_bo sets PG_FICTITIOUS on aperture pages to satisfy MGTDEVICE pager interface.
sys/dev/md/md.c
1105	I remember Mark proposed an idea for similar interface some time ago as well. Also please note that the whole vm_object_page_remove() follows this pattern, but with tweaks.