Remove the volatile qualifier from busy_lock.

[markj]

Summary

Use atomic(9) to load the lock state. Some places were doing this
already, so it was inconsistent. In initialization code, the lock state
is still initialized with plain stores.

[alc]

(My questions are all unrelated to the direct purpose of the change.)

sys/vm/vm_page.c

941 ↗

(On Diff #75071)

I'm surprised to see an acquire here. The acquire performed by the sbusy should suffice.

986 ↗

(On Diff #75071)

Why is a release used here? Note that the other case, where sharers > 1, does not use a release.

sys/vm/vm_page.h

776–784 ↗

(On Diff #75071)

Does anything that follows a "claim" call examine fields that are synchronized by the busy lock? I'm wondering about possible memory ordering issues.

[alc]

sys/vm/vm_page.h

246 ↗

(On Diff #75071)

Documentation that all accesses should use atomic_*() would be good.

[markj]

sys/vm/vm_page.c

986 ↗

(On Diff #75071)

I would expect both cases to use a release store. Threads holding an sbusy lock may still modify page state (set valid or dirty bits, but not clear them).

sys/vm/vm_page.h

776–784 ↗

(On Diff #75071)

I don't think so. In all cases it's used immediately before freeing a page (or at least removing it from its object). Are you worried about issues like:

thread A, CPU 1:

vm_page_xbusy(m);
<update valid or dirty state>
<hand off m to thread B>

thread B, CPU 2:

vm_page_xbusy_claim(m);
<observes old valid or dirty state>

?

[markj]

Annotate the busy_lock field.

[markj]

sys/vm/vm_page.h

246 ↗

(On Diff #75071)

I simply added a locking annotation, but re-reading your comment makes me wonder if you were looking for something more descriptive.

[alc]

sys/vm/vm_page.h

246 ↗

(On Diff #75071)

I think that the annotation suffices, but I would revisit the description/definition of "(A)". Specifically, the phrase "the field is atomic" is not very clear.

[alc]

sys/vm/vm_page.h

776–784 ↗

(On Diff #75071)

Yes, exactly. I would suggest adding a comment to say that this function cannot be used in that way.

[markj]

Address feedback.

[markj]

sys/vm/vm_page.h

776–784 ↗

(On Diff #75071)

I suppose we could introduce a vm_page_xbusy_disown(), akin to lockmgr_disown(), to provide any required synchronization if the need arises.