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

[jeff]

Summary

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.

[markj]

sys/vm/vm_page.c

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]

sys/vm/vm_page.c

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.

sys/vm/vm_pageout.c

378 ↗

(On Diff #60758)

Merge bug for later diff.

[markj]

sys/vm/vm_page.c

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]

sys/vm/vm_page.c

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]

sys/vm/vm_page.c

966 ↗

(On Diff #60758)

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

[kib]

sys/vm/vm_page.c

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]

sys/vm/vm_page.c

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]

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

[jeff]

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

[jeff]

Resolve identity change issues

[alc]

sys/dev/drm2/ttm/ttm_bo_vm.c

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")) {
        ttm_mem_io_unlock(man);

[jeff]

sys/dev/drm2/ttm/ttm_bo_vm.c

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) {
        ttm_mem_io_unlock(man);
        ttm_bo_unreserve(bo);
        goto retry;
}