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 - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib requested review of this revision.Jun 8 2020, 1:27 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 marked an inline comment as done.

Handle the first round of comments.

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.

sys/amd64/amd64/pmap.c
3826 ↗(On Diff #73656)

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

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 marked an inline comment as done.

Separately mention the root page.

This revision is now accepted and ready to land.Jun 26 2020, 8:42 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 marked 3 inline comments as done.

Alc suggestions.

This revision now requires review to proceed.Jun 26 2020, 10:02 PM
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 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.