Page MenuHomeFreeBSD

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

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

Details

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

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.Sat, Nov 30, 9:01 AM
jeff edited the summary of this revision. (Show Details)Sat, Nov 30, 9:12 AM
jeff added reviewers: kib, markj, alc, dougm.
jeff set the repository for this revision to rS FreeBSD src repository.
jeff added inline comments.Sat, Nov 30, 9:14 AM
sys/arm/nvidia/drm2/tegra_bo.c
68–71 ↗(On Diff #65065)

This section is super weird.

alc added inline comments.Sat, Nov 30, 6:07 PM
sys/amd64/sgx/sgx.c
392 ↗(On Diff #65065)

This assignment no longer serves any purpose.

730 ↗(On Diff #65065)

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 ↗(On Diff #65065)

Ditto.

jeff added inline comments.Sat, Nov 30, 9:16 PM
sys/amd64/sgx/sgx.c
743 ↗(On Diff #65065)

will do.

sys/dev/md/md.c
1105 ↗(On Diff #65065)

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.Sat, Nov 30, 10:37 PM
sys/arm/nvidia/drm2/tegra_bo.c
68–71 ↗(On Diff #65065)

I suspect that tegra_bo sets PG_FICTITIOUS on aperture pages to satisfy MGTDEVICE pager interface.

sys/dev/md/md.c
1105 ↗(On Diff #65065)

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.

jeff updated this revision to Diff 65085.Sun, Dec 1, 4:46 AM

Address review feedback

kib accepted this revision.Sun, Dec 1, 8:50 PM
This revision is now accepted and ready to land.Sun, Dec 1, 8:50 PM
markj added inline comments.Mon, Dec 2, 4:43 PM
sys/vm/vm_glue.c
366 ↗(On Diff #65085)

Can't we just call vm_page_grab(ksobj, i, VM_ALLOC_NOCREAT) instead?

jeff added inline comments.Mon, Dec 2, 10:41 PM
sys/vm/vm_glue.c
366 ↗(On Diff #65085)

Yeah possibly I will do that in a follow up now that I have verified this diff with stress and builds. I don't want to start over.