Page MenuHomeFreeBSD

Remove the volatile qualifier from busy_lock.
ClosedPublic

Authored by markj on Tue, Jul 28, 3:08 PM.

Details

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.

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 requested review of this revision.Tue, Jul 28, 3:08 PM
markj created this revision.
kib accepted this revision.Tue, Jul 28, 3:45 PM
This revision is now accepted and ready to land.Tue, Jul 28, 3:45 PM
alc accepted this revision.Tue, Jul 28, 6:13 PM

(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 added inline comments.Tue, Jul 28, 6:14 PM
sys/vm/vm_page.h
246 ↗(On Diff #75071)

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

markj marked an inline comment as done.Tue, Jul 28, 7:44 PM
markj added inline comments.
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 updated this revision to Diff 75087.Tue, Jul 28, 7:46 PM

Annotate the busy_lock field.

This revision now requires review to proceed.Tue, Jul 28, 7:46 PM
markj added inline comments.Tue, Jul 28, 7:48 PM
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 added inline comments.Tue, Jul 28, 9:30 PM
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 added inline comments.Tue, Jul 28, 9:34 PM
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 updated this revision to Diff 75124.Wed, Jul 29, 2:18 PM
markj marked 2 inline comments as done.

Address feedback.

markj added inline comments.Wed, Jul 29, 2:19 PM
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.

kib accepted this revision.Wed, Jul 29, 2:54 PM
This revision is now accepted and ready to land.Wed, Jul 29, 2:54 PM
alc accepted this revision.Wed, Jul 29, 5:08 PM
This revision was automatically updated to reflect the committed changes.