Page MenuHomeFreeBSD
Differential D25187

amd64 pmap: explain pteindex
ClosedPublic
Actions

Authored by kib on Jun 8 2020, 1:27 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 14, 9:21 AM
Unknown Object (File)
Sat, Nov 2, 3:50 PM
Unknown Object (File)
Tue, Oct 29, 8:10 PM
Unknown Object (File)
Tue, Oct 29, 1:29 AM
Unknown Object (File)
Fri, Oct 25, 4:33 PM
Unknown Object (File)
Fri, Oct 25, 1:57 PM
Unknown Object (File)
Tue, Oct 22, 9:31 AM
Unknown Object (File)
Oct 20 2024, 7:57 AM
View All Files
Subscribers
emaste
imp

Details

Reviewers
alc
markj
Commits
rS362706: amd64 pmap: explain ptepindex.
Summary

Add description of how the page table pages are numbered by pmap_allocpte().

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kib created this revision.Jun 8 2020, 1:27 PM
Herald added a subscriber: imp. · View Herald TranscriptJun 8 2020, 1:27 PM
kib requested review of this revision.Jun 8 2020, 1:27 PM
Harbormaster completed remote builds in B31561: Diff 72824.Jun 8 2020, 1:27 PM
emaste added a subscriber: emaste.Jun 23 2020, 2:37 PM
markj added inline comments.Jun 25 2020, 3:32 PM
sys/amd64/amd64/pmap.c
3789 ↗(On Diff #72824)

"The page index of the PTE at address va is defined as follows" would be more clear to me.

3791 ↗(On Diff #72824)

Duplicate '='

3798 ↗(On Diff #72824)

s/is it/it is/

3799 ↗(On Diff #72824)

If I understand this correctly, "height" might be clearer than "rank" in the context of a tree.

kib marked 3 inline comments as done.Jun 25 2020, 7:49 PM
kib added inline comments.
sys/amd64/amd64/pmap.c
3789 ↗(On Diff #72824)

IMO it is too vague. I used the basis of your sentence, but extended it as following:

* The page index of the page containing PTE at address va is defined
* as follows:
3791 ↗(On Diff #72824)

It was a split formula, I reformatted to move formulas to display to avoid line break.

kib updated this revision to Diff 73656.Jun 25 2020, 7:50 PM
kib marked an inline comment as done.

Handle the first round of comments.

Harbormaster completed remote builds in B31963: Diff 73656.Jun 25 2020, 7:50 PM
markj accepted this revision.Jun 26 2020, 7:36 PM
markj added inline comments.
sys/amd64/amd64/pmap.c
3813 ↗(On Diff #73656)

Should it be, "for the page table page (last level)"?

For the other entries, we are missing "the" as well.

3826 ↗(On Diff #73656)

Maybe clarify also that the PML4 page always has pindex 0?

3789 ↗(On Diff #72824)

It should be "the PTE".

"the page" sounds like there is a unique page, but really we are talking about each PTP in the hierarchy. So maybe: "The page indices of the page table pages encountered while translating virtual address va are defined as follows:"

This revision is now accepted and ready to land.Jun 26 2020, 7:36 PM
kib marked 2 inline comments as done.Jun 26 2020, 8:02 PM
kib added inline comments.
sys/amd64/amd64/pmap.c
3826 ↗(On Diff #73656)

PML4 page does not have index for the purpose of _pmap_allocpte() which uses this numbering. The PML4 page is statically allocated at the pinit() time.

In fact, the pteindex 0 is assigned to the pte which maps at zero.

markj added inline comments.Jun 26 2020, 8:03 PM
sys/amd64/amd64/pmap.c
3826 ↗(On Diff #73656)

Indeed, I just thought it would be useful to clarify this fact for completeness.

kib updated this revision to Diff 73709.Jun 26 2020, 8:05 PM
kib marked an inline comment as done.

Change herald sentence as suggested by Mark. Add articles.

This revision now requires review to proceed.Jun 26 2020, 8:05 PM
Harbormaster completed remote builds in B31988: Diff 73709.Jun 26 2020, 8:05 PM
kib updated this revision to Diff 73713.Jun 26 2020, 8:28 PM
kib marked an inline comment as done.

Separately mention the root page.

Harbormaster completed remote builds in B31991: Diff 73713.Jun 26 2020, 8:28 PM
markj accepted this revision.Jun 26 2020, 8:42 PM
This revision is now accepted and ready to land.Jun 26 2020, 8:42 PM
alc added inline comments.Jun 26 2020, 9:02 PM
sys/amd64/amd64/pmap.c
3818–3819 ↗(On Diff #73713)

What follows will be clearer if this sentence explicitly defines ptepindex as the page index of a page table page.

3822 ↗(On Diff #73713)

"... the PDE that maps the page table page."

3831–3833 ↗(On Diff #73713)

I think that this sentence is hard to understand.

kib updated this revision to Diff 73719.Jun 26 2020, 10:02 PM
kib marked 3 inline comments as done.

Alc suggestions.

This revision now requires review to proceed.Jun 26 2020, 10:02 PM
Harbormaster completed remote builds in B31995: Diff 73719.Jun 26 2020, 10:02 PM
markj accepted this revision.Jun 27 2020, 4:09 AM
markj added inline comments.
sys/amd64/amd64/pmap.c
3818 ↗(On Diff #73719)

There should be a comma after "page indices", otherwise it is not really clear what "i.e." refers to.

This revision is now accepted and ready to land.Jun 27 2020, 4:09 AM
kib updated this revision to Diff 73739.Jun 27 2020, 4:27 AM
kib marked an inline comment as done.

comma

This revision now requires review to proceed.Jun 27 2020, 4:27 AM
Harbormaster completed remote builds in B32004: Diff 73739.Jun 27 2020, 4:27 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 27 2020, 7:29 PM
Closed by commit rS362706: amd64 pmap: explain ptepindex. (authored by kib). · Explain Why
This revision was automatically updated to reflect the committed changes.
kib added a commit: rS362706: amd64 pmap: explain ptepindex..

Revision Contents
Changeset List

PathSize
head/
sys/
amd64/
amd64/
23 lines

Diff 73787

View Options

head/sys/amd64/amd64/pmap.c

Loading...