Page MenuHomeFreeBSD

Remove the volatile qualifier from busy_lock.
ClosedPublic

Authored by markj on Jul 28 2020, 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 - subversion
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.Jul 28 2020, 3:08 PM
markj created this revision.
This revision is now accepted and ready to land.Jul 28 2020, 3:45 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.

sys/vm/vm_page.h
246 ↗(On Diff #75071)

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

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>

?

Annotate the busy_lock field.

This revision now requires review to proceed.Jul 28 2020, 7:46 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.

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.

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 marked 2 inline comments as done.

Address feedback.

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.

This revision is now accepted and ready to land.Jul 29 2020, 2:54 PM
This revision was automatically updated to reflect the committed changes.