Page MenuHomeFreeBSD

amd64 pmap: explain pteindex
ClosedPublic

Authored by kib on Jun 8 2020, 1:27 PM.

Details

Summary

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib created this revision.Jun 8 2020, 1:27 PM
kib requested review of this revision.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.

markj accepted this revision.Jun 26 2020, 7:36 PM
markj added inline comments.
sys/amd64/amd64/pmap.c
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:"

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?

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
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.

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
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
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.