Introduce vm_page_astate.
ClosedPublic
Actions

Authored by markj on Dec 3 2019, 8:42 PM.

Details

Reviewers

alc
jeff
kib
dougm
manu

Commits

rS355586: Introduce vm_page_astate.

Summary

This is a 32-bit structure embedded in each vm_page, consisting mostly
of page queue state. The use of a structure makes it easy to store a
snapshot of a page's queue state in a stack variable and use cmpset
loops to update that state without requiring the page lock.

This change merely adds the structure and updates references to atomic
state fields. No functional change intended.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Dec 3 2019, 8:42 PM

Herald added a reviewer: manu. · View Herald TranscriptDec 3 2019, 8:42 PM

Herald added subscribers: bdragon, jhibbits, emaste, andrew. · View Herald Transcript

Harbormaster completed remote builds in B27917: Diff 65173.Dec 3 2019, 8:42 PM

markj edited reviewers, added: alc, jeff, kib, dougm; removed: manu.Dec 3 2019, 8:43 PM

Herald added a reviewer: manu. · View Herald TranscriptDec 3 2019, 8:43 PM

My only comments are whether we want to stylistically hide the union with accessors. As we change synchronization rules I find them helpful. For example, once I correct a few more cases of direct valid bit manipulation we can add much stronger asserts about that.

sys/amd64/amd64/pmap.c
6105 ↗	(On Diff #65173)	Is there a reason we're not using accessors?
sys/arm/include/pmap.h
50 ↗	(On Diff #65173)	This doesn't really need to be defined in every pmap. I believe they all support it now.
sys/dev/virtio/balloon/virtio_balloon.c
335 ↗	(On Diff #65173)	Again why not an accessor? I think I don't like the ->a. everywhere and moving accesses into an accessor makes it less likely for someone to modify a field directly in error.
sys/vm/vm_page.c
516 ↗	(On Diff #65173)	I would resort to group assignments.
3212 ↗	(On Diff #65173)	Does every arch support atomic_load_16 now?
3426 ↗	(On Diff #65173)	If this is available everywhere we should update the atomic man page.

markj added inline comments.Dec 3 2019, 9:03 PM

sys/amd64/amd64/pmap.c
6105 ↗	(On Diff #65173)	In an earlier version I added vm_page_aflags_load() but thought it was too verbose. We cannot #define aflags (it's a common variable name) and I dislike the practice of #defining field names. I can use vm_page_aflags_load() if you prefer or can implement a different suggestion.
sys/dev/virtio/balloon/virtio_balloon.c
335 ↗	(On Diff #65173)	We already have vm_page_queue(), I will convert this line to use. There should really be no direct accesses of a.queue outside of vm_page.c and maybe vm_pageout.c, so I do not see a reason to add a direct accessor. (vm_page_queue() is higher-level.)
sys/vm/vm_page.c
3212 ↗	(On Diff #65173)	Yes, they all always have. It's 16-bit atomic updates that are not completely supported.
3426 ↗	(On Diff #65173)	AFAIK it does not claim anywhere that atomic_load_(8\|16) cannot be used in MI code.

kib added inline comments.Dec 3 2019, 9:49 PM

sys/vm/vm_page.h
249 ↗	(On Diff #65173)	I would say 'state accessed atomically'

my style quibbles shouldn't hold this up.

sys/amd64/amd64/pmap.c
6105 ↗	(On Diff #65173)	could be bool vm_page_aflag(m, bits)

This revision is now accepted and ready to land.Dec 4 2019, 1:54 AM

andrew added inline comments.Dec 4 2019, 7:37 AM

sys/vm/vm_page.h
760–761 ↗	(On Diff #65173)	This should be updated with the new name and size.

markj added inline comments.Dec 4 2019, 3:36 PM

sys/amd64/amd64/pmap.c
6105 ↗	(On Diff #65173)	I had considered that too but we don't have anything like it in the tree, and it's not immediately obvious whether vm_page_aflag() would return true only if all of the specified bits are set.
sys/vm/vm_page.h
760–761 ↗	(On Diff #65173)	This whole section will be removed in the next patch in this series: the use of a structure allows the compiler to generate these offsets.