Fix some nits in 1G page handling.
ClosedPublic
Actions

Authored by markj on Sep 17 2020, 4:34 PM.

Details

Reviewers

alc
kib

Commits

rS365906: Fix some nits in 1G page support in the amd64 pmap.

Summary

Move assertions out of the main loop. This makes the assertion condition simpler and makes the code easier to follow IMO.
Fix va_next updates. In some cases we were not doing the wraparound check before continuing the loop.
Use the right va_next. In pmap_advise() and pmap_copy() we would step through 1G pages 2M at a time.
Write "non-transparent" instead of "non-transient" in the assertion message. I believe this was the original intent.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 33628
Build 30871: arc lint + arc unit

Event Timeline

markj requested review of this revision.Sep 17 2020, 4:34 PM

markj created this revision.

Harbormaster completed remote builds in B33606: Diff 77139.Sep 17 2020, 4:34 PM

markj added reviewers: alc, kib.Sep 17 2020, 4:38 PM

kib accepted this revision.Sep 17 2020, 5:12 PM

This revision is now accepted and ready to land.Sep 17 2020, 5:12 PM

alc added inline comments.Sep 17 2020, 6:00 PM

sys/amd64/amd64/pmap.c
5946	Shouldn't this really say something like, "partial removal of non-transparent 1G mapping "? The key word being "partial".
5951	There is an extra space here.
6237	Ditto.
7381	Ditto.
7530	Why aren't we copying the mapping here?
8589	Ditto.

markj marked 2 inline comments as done.Sep 17 2020, 8:51 PM

markj added inline comments.

sys/amd64/amd64/pmap.c
5946	I changed it to "partial update of non-transparent 1G mapping" in all places.
7530	I don't think there is any reason not to do it.

Implement handling for 1G pages in pmap_copy().

This revision now requires review to proceed.Sep 18 2020, 4:52 PM

Harbormaster completed remote builds in B33627: Diff 77188.Sep 18 2020, 4:52 PM

Drop a test printf().

Harbormaster completed remote builds in B33628: Diff 77189.Sep 18 2020, 4:57 PM

Hm, unless I miss something, does pmap_copy() break MINHERIT_ZERO ?

In D26463#589164, @kib wrote:

Hm, unless I miss something, does pmap_copy() break MINHERIT_ZERO ?

From my reading, pmap_copy() is only used for VM_INHERIT_COPY and _SHARED.

In D26463#589165, @markj wrote:

In D26463#589164, @kib wrote:

Hm, unless I miss something, does pmap_copy() break MINHERIT_ZERO ?

From my reading, pmap_copy() is only used for VM_INHERIT_COPY and _SHARED.

Right, pmap_copy() is only called per map entry.

This revision is now accepted and ready to land.Sep 18 2020, 6:26 PM

alc accepted this revision.Sep 19 2020, 7:17 AM

Closed by commit rS365906: Fix some nits in 1G page support in the amd64 pmap. (authored by markj). · Explain WhySep 19 2020, 3:22 PM

This revision was automatically updated to reflect the committed changes.

markj added a commit: rS365906: Fix some nits in 1G page support in the amd64 pmap..

Herald added a subscriber: imp. · View Herald TranscriptSep 19 2020, 3:22 PM

alc added inline comments.Sep 20 2020, 12:50 AM

head/sys/amd64/amd64/pmap.c
6504 ↗	(On Diff #77221)	Can't pmap_pml4e() now return NULL under LA57?
6528 ↗	(On Diff #77221)	I don't understand why we do this unconditionally. We don't use `mp` except in the then clause of the below if statement.
6536 ↗	(On Diff #77221)	Using PG_FRAME here will include X86_PG_PDE_PAT in the compared bits.
6566 ↗	(On Diff #77221)	Ditto.
6572 ↗	(On Diff #77221)	Ditto.
6582 ↗	(On Diff #77221)	I find it curious that the resident count is handled one way and the wired count another way.

alc added inline comments.Sep 20 2020, 12:57 AM

head/sys/amd64/amd64/pmap.c
6221 ↗	(On Diff #77221)	Can't pmap_pml4e() now return NULL under LA57?

kib added inline comments.Sep 20 2020, 10:09 AM

head/sys/amd64/amd64/pmap.c
6221 ↗	(On Diff #77221)	Yes this is systematic error. D26492.

markj marked 3 inline comments as done.Sep 20 2020, 4:59 PM

markj added inline comments.

head/sys/amd64/amd64/pmap.c
6536 ↗	(On Diff #77221)	I believe we can actually make this assertion stronger and write KASSERT((origpte & PG_V) == 0 \|\| ((origpte ^ pten) & ~(PG_W \| PG_M)) == 0, (...));