Page MenuHomeFreeBSD

Remove the volatile qualifier from busy_lock.
ClosedPublic

Authored by markj on Jul 28 2020, 3:08 PM.
Tags
None
Referenced Files
Unknown Object (File)
Jan 7 2024, 1:14 AM
Unknown Object (File)
Jan 7 2024, 1:14 AM
Unknown Object (File)
Nov 12 2023, 1:37 AM
Unknown Object (File)
Oct 11 2023, 12:39 AM
Unknown Object (File)
Sep 27 2023, 11:52 PM
Unknown Object (File)
Sep 18 2023, 11:17 AM
Unknown Object (File)
Sep 14 2023, 5:52 AM
Unknown Object (File)
Sep 11 2023, 11:54 PM
Subscribers

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

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 32645
Build 30098: arc lint + arc unit

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

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

986

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

sys/vm/vm_page.h
782–790

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–247

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

markj added inline comments.
sys/vm/vm_page.c
986

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
782–790

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–247

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–247

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
782–790

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
782–790

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.