Page MenuHomeFreeBSD

Centralize the logic in vfs_vmio_unwire() and sendfile_free_page().
ClosedPublic

Authored by markj on Jul 18 2019, 6:53 PM.

Details

Summary

These functions are similar and I would like to have their functionality
live in generic VM functions so that the details of vm_page
synchronization can be encapsulated in vm_page.c.

The change adds vm_page_release() and vm_page_release_locked(). They
both unwire the page and optionally attempt to free the page.
vm_page_release_locked() requires the object lock. The main difference
is that vfs_vmio_unwire() may attempt to free a mapped page, while
sendfile_free_page() does not. This change thus modifies the handling
of direct I/O and B_NOREUSE to avoid freeing a mapped page.

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

markj created this revision.Jul 18 2019, 6:53 PM
kib added inline comments.Jul 18 2019, 8:11 PM
sys/vm/vm_page.c
171 ↗(On Diff #59888)

Is this unrelated ?

3768 ↗(On Diff #59888)

What prevents other thread from busying the page after we tested for !xbusy but before TRYWLOCK() succeeded ?

markj added inline comments.Jul 18 2019, 8:16 PM
sys/vm/vm_page.c
171 ↗(On Diff #59888)

No, I renamed the old (static) vm_page_release() to vm_page_zone_release(), and renamed vm_page_import() as well for consistency.

3768 ↗(On Diff #59888)

Nothing, but there is a vm_page_busied(m) check after we acquire the object lock. This is identical to the logic in sendfile_free_page(). I accidentally removed a comment explaining that the first check is racy, I will restore it.

markj updated this revision to Diff 59893.Jul 18 2019, 8:21 PM

Restore a lost comment.

kib added inline comments.Jul 18 2019, 8:56 PM
sys/vm/vm_page.c
3768 ↗(On Diff #59888)

Then why the first (racy) check is not vm_page_busied() ?

markj updated this revision to Diff 59894.Jul 18 2019, 9:01 PM

Don't attempt to free the page if it is busy at all.

kib accepted this revision.Jul 18 2019, 9:14 PM
kib added inline comments.
sys/vm/vm_page.c
171 ↗(On Diff #59888)

I think it is useful to commit this part separately, and definitely merge it to 12.

This revision is now accepted and ready to land.Jul 18 2019, 9:14 PM
markj updated this revision to Diff 59969.Jul 20 2019, 8:33 PM

Rebase.

This revision now requires review to proceed.Jul 20 2019, 8:33 PM
dougm added inline comments.Jul 22 2019, 4:45 AM
sys/vm/vm_page.c
3758 ↗(On Diff #59969)

assert that m->object is not locked.

3797 ↗(On Diff #59969)

Consider defining these seven lines as a function of m and flags, and using it here and below.

markj updated this revision to Diff 60035.Jul 23 2019, 1:39 AM
markj marked 2 inline comments as done.
  • Use a common subrouting for releasing to the page queues.
  • Assert that the object is unlocked in vm_page_release().
markj added inline comments.Jul 23 2019, 1:39 AM
sys/vm/vm_page.c
3758 ↗(On Diff #59969)

Note that m->object may be NULL. I added the conditional assertion under the page lock, since the page lock is required in order to modify the object field.

kib accepted this revision.Jul 23 2019, 7:42 AM
This revision is now accepted and ready to land.Jul 23 2019, 7:42 AM
alc added inline comments.Jul 27 2019, 4:10 AM
sys/vm/vm_page.c
3782 ↗(On Diff #60035)

"managed" -> "unmanaged"

3823 ↗(On Diff #60035)

"managed" -> "unmanaged"

markj updated this revision to Diff 60206.Jul 27 2019, 11:51 PM

Fix typos.

This revision now requires review to proceed.Jul 27 2019, 11:51 PM
alc accepted this revision.Jul 28 2019, 9:37 PM
This revision is now accepted and ready to land.Jul 28 2019, 9:37 PM