Add unlocked variants of grab functions.

[jeff]

Summary

This uses the radix functionality from https://reviews.freebsd.org/D23446 to locklessly lookup and busy or wire a page. It has fallbacks to object locking for insert.

I don't like the naming. _unlocked is long and some of these functions will acquire the lock if they fail and some will simply fail. So I'd like suggestions.

It is actually possibly to busy sleep for a page without the object lock and I will implement that as a separate pass. This should restrict cases that acquire the object lock to only the cases where the page is not present in the object.

The vast majority of in-tree users can directly use the unlocked variants. I tried to see if I could eliminate the direct locking variants but there are a few cases that are challenging. It may be worthwhile to chase these down just for API simplicity.

[jeff]

Try to name more consistently. Minor refactoring. Use the unlocked sleep.

[markj]

sys/vm/vm_page.c

1730 ↗

(On Diff #68096)

This should be an atomic load.

1737 ↗

(On Diff #68096)

Don't we potentially need to wake up waiters if we are the last sharer?

4321 ↗

(On Diff #68096)

vm_page_grab_check() might be a better name. It is consistent with vm_page_alloc_check() and vm_page_grab_asserts sounds like "asserts" is the object, not "vm_page".

4330 ↗

(On Diff #68096)

You can get rid of the extra quotes here.

[jeff]

sys/vm/vm_page.c

1730 ↗

(On Diff #68096)

For the reader or the machine?

1737 ↗

(On Diff #68096)

If we are the last sharer we should execute the case below.

4321 ↗

(On Diff #68096)

ok.

The naming I really don't like is _unlocked after everything. I could probably do a sweep of the tree and convert everything to _unlocked and drop it from the name. Any outside sources doing direct object manipulation will suddenly be wrong though.

If you have suggestions I'm all ears.

[kib]

sys/vm/vm_page.c

1730 ↗

(On Diff #68096)

For compiler. Atomic_load() does not add anything to the generated load, except it ensures that the load indeed happen (but could be reordered or coalesced). I think it would be somewhat clearer to see atomic_load() for lock-kind word, as well.

1740 ↗

(On Diff #68096)

Should we assert that x is exclusive busy owner ?

[jeff]

Use atomic_load. Add an assert.

[markj]

sys/vm/vm_page.c

4601 ↗

(On Diff #68395)

Why is it sufficient to hold a wiring? Without the object lock or page busied the page's <obj, pindex> identity is not stable.

[jeff]

sys/vm/vm_page.c

4601 ↗

(On Diff #68395)

I think this is actually harmless because the page in question will have a NULL object pointer and its next pointer will either be clear or point to a page without a NULL object pointer. pindex will remain unchanged. I suppose that could create a spin loop until the NULL pointer makes it to the list. So from that pov I need to be more careful here and perhaps retry less.

Another option would be to pass the expected object pointer in with the page for validation.

Once grab_next fails grab_pages_unlocked will fall back to the locked loop. So the subsequent pages will be valid. I don't think it's possible to get incorrect results this way.

[jeff]

Fully handle sleep fail/nowait cases. Simplify a few lockless functions by
introducing another helper. Handle the object NULL assignment in a single
place with a compiler barrier.

I am currently retesting all of this on stress2.

[jeff]

sys/vm/vm_page.c

4672 ↗

(On Diff #68789)

We could actually handle more cases by falling back to a lockless lookup. Then the grab_next_unlocked return would be conclusive.

[jeff]

Sometimes if you keep re-arranging pieces they shuffle into a more compact
representation.

This update constrains all of the lockless lookup code to a single function
that can return with fairly strong guarantees about page state. This
really simplifies the handful of consumers. It allows all of the sleeping
and retrying cases to be handled by the lockless code. The function itself
is tricky but now the visibility of questionable page identity is contained.

[jeff]

Upload the correct diff

[kib]

sys/vm/vm_page.c

889 ↗

(On Diff #68830)

Although it is obvious for careful reader that the function drops the object lock, it might be surprising to not so careful one. Also we traditionally point that out.

So I think it is useful to note that the function might drop the object lock if owned.

1651 ↗

(On Diff #68830)

Is this comment still relevant ? I believe it described a release store.
Right now there is nothing preventing observer from seeing updated radix with old m->object.

[markj]

sys/vm/vm_page.c

841 ↗

(On Diff #68830)

vm_page_grab_trybusy() is also used by non-grab functions. You could rename it to vm_page_trybusy() and group it with the other functions in this file that deal with page busying.

936 ↗

(On Diff #68830)

Why tryacquire instead of trybusy? I don't think anything should be using this function to wire the page. If not, then we should rename the function to e.g., vm_page_acquire().

4439 ↗

(On Diff #68830)

The use of TAILQ_NEXT here is technically fine but breaks the locking protocol. In the absence of an "atomic" TAILQ_NEXT it would probably be good to note this in the comment.

4452 ↗

(On Diff #68830)

Maybe use __predict_true here.

4487 ↗

(On Diff #68830)

"Spurious errors" is kind of ambiguous. "False negatives"?

4639 ↗

(On Diff #68830)

You might add a two line vm_page_grab_wire() for this, since it gets copied around a lot.

4755 ↗

(On Diff #68830)

I think "next" should be called "pred" or "prev".

[jeff]

sys/vm/vm_page.c

936 ↗

(On Diff #68830)

I found no reason to preclude wiring the page although it would be unusual.

1651 ↗

(On Diff #68830)

The release store is in vm_radix_remove(). The acquire is in vm_radix_lookup_unlocked().

4439 ↗

(On Diff #68830)

That was the intent of the comment but I can clarify.

4639 ↗

(On Diff #68830)

This and _valid() don't want to do it via _acquire or tryacquire because they may decide not to keep the page and this simplified error handling. So they should be the only two IIRC.

[jeff]

Review feedback.