Page MenuHomeFreeBSD

amd64 pmap: explain pteindex
ClosedPublic

Authored by kib on Jun 8 2020, 1:27 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 23 2024, 3:42 AM
Unknown Object (File)
Feb 9 2024, 3:06 AM
Unknown Object (File)
Jan 22 2024, 5:40 PM
Unknown Object (File)
Jan 16 2024, 2:02 PM
Unknown Object (File)
Jan 13 2024, 6:52 PM
Unknown Object (File)
Dec 20 2023, 2:23 PM
Unknown Object (File)
Dec 1 2023, 10:59 PM
Unknown Object (File)
Nov 28 2023, 10:20 AM
Subscribers

Details

Summary

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

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 31995

Event Timeline

kib requested review of this revision.Jun 8 2020, 1:27 PM
sys/amd64/amd64/pmap.c
3789

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

3791

Duplicate '='

3798

s/is it/it is/

3799

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

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

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

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

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

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

3826

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

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

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

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

3822

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

3834–3836

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

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.