Page MenuHomeFreeBSD

Remove the page lock dependency on busy sleeps by using sleepq to avoid races.

Authored by jeff on Aug 13 2019, 8:08 PM.



This uses the sleepq lock itself to synchronize busy sleep/wakeup and avoids the need for the page lock. I need the lock test and conditional acquire/release for a later patch. It could be elided for now but it allows for simplifying some consumers already.

I didn't realize some diffs from a different patchset snuck in and I will correct that. I made a note where this occurred.

Diff Detail

rS FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jeff created this revision.Aug 13 2019, 8:08 PM
jeff edited the summary of this revision. (Show Details)Aug 13 2019, 8:16 PM
jeff added reviewers: alc, kib, markj.
markj added inline comments.Aug 13 2019, 8:34 PM
966 ↗(On Diff #60758)

If the page lock isn't held and the object lock isn't held, what prevents m from being freed after the call to vm_page_busy_sleep() and before the dereference of m->object?

jeff added inline comments.Aug 13 2019, 8:43 PM
966 ↗(On Diff #60758)

Nothing prevents it. I believe we handle the race gracefully.

m->object could become NULL at any point after we drop the object lock. However, this means the busy holder is removing the page and will unbusy at some point, waking us up, or avoiding the sleep below.

All vm_page_busy_sleep callers are basically doing what we call "WAITFAIL" elsewhere. They don't return with the busy lock held. They restart compound operations and look for the correct page again.

It is possible that the page could be freed, reallocated, and we sleep for the wrong thing. However, I believe this is harmless and rare. It would be possible to treat the object lock as an interlock here with a slightly more complicated and expensive sleepq operation. Cases that don't hold the object lock are protected by a wire count.

3961–3963 ↗(On Diff #60758)

This is a merge bug and related to a later diff.

378 ↗(On Diff #60758)

Merge bug for later diff.

markj added inline comments.Aug 13 2019, 8:46 PM
966 ↗(On Diff #60758)

The wiring doesn't prevent the page from being removed from its object, in which case m->object will become NULL. See vm_object_terminate_pages() for instance.

jeff added inline comments.Aug 13 2019, 8:48 PM
966 ↗(On Diff #60758)

Yes I understand but it does prevent the page from being reallocated to some other purpose. So there is no chance of a spurious sleep.

markj added inline comments.Aug 13 2019, 8:52 PM
966 ↗(On Diff #60758)

We still need to check that obj is non-NULL before dereferencing it.

kib added inline comments.Aug 13 2019, 8:53 PM
932 ↗(On Diff #60758)

I wonder if this needs atomc_cmpset_int_rel. Before, the vm_page_unlock() provided the release semantic, sunbusy acts as unlock. vm_page_xunbusy does use _rel.

961 ↗(On Diff #60758)

Then assert that the object ref count is > 0 ?

I would prefer to convert the nonshared argument to flags, and add two flags, one for nonshared, another for obj_locked, instead of using VM_OBJECT_WOWNED().

jeff added inline comments.Aug 14 2019, 10:50 PM
961 ↗(On Diff #60758)

I copied the comment from elsewhere. I'm not comfortable enough with all callers to add a new assert. This may be used in the object tear-down path for example. We rely on object type stability elsewhere.

I thought about the flag. It is a tradeoff. With a flag you are more explicit and so less likely to have bugs where the caller doesn't realize the object lock is dropped. However, you're then subject to bugs where the flag was just specified wrong. With the silent operation you won't have bugs with releasing/re-acquiring the lock but the caller may not anticipate losing synchronization.

If you follow all of my patch sets you will see that ultimately there are few cases that are called with any lock held so I settled on this method.

jeff updated this revision to Diff 60810.Aug 14 2019, 11:11 PM

Address review feedback. Clean up patch. Switch to fcmpset and rel semantics.

jeff updated this revision to Diff 61023.Aug 20 2019, 12:46 AM

Respond to review feedback. Eliminate the object locking changes for now.

markj accepted this revision.Aug 20 2019, 1:21 AM
This revision is now accepted and ready to land.Aug 20 2019, 1:21 AM
kib accepted this revision.Aug 20 2019, 11:29 AM
jeff updated this revision to Diff 61698.Sep 5 2019, 6:31 PM

Resolve identity change issues

This revision now requires review to proceed.Sep 5 2019, 6:31 PM
kib accepted this revision.Sep 5 2019, 8:52 PM
This revision is now accepted and ready to land.Sep 5 2019, 8:52 PM
alc accepted this revision.Sep 6 2019, 2:37 PM
alc added inline comments.Sep 6 2019, 2:41 PM
236–237 ↗(On Diff #61698)

I'm curious as to why you didn't further simplify cases like this one to:

if (vm_page_sleep_if_busy(m, "ttmpbs")) {
jeff added inline comments.Sep 6 2019, 2:44 PM
236–237 ↗(On Diff #61698)

This is turned into a vm_page_busy_acquire() in another patch I have in review.

if (vm_page_busy_acquire(m, VM_ALLOC_WAITFAIL) == 0) {
        goto retry;
markj accepted this revision.Sep 8 2019, 8:04 PM