Re: [PATCH v4] Documentation/mm: Initial page table documentation

[Linus Walleij <linus.walleij@linaro.org>]

On Tue, Jun 13, 2023 at 4:45 PM Matthew Wilcox <willy@infradead.org> wrote:
> On Tue, Jun 13, 2023 at 10:39:06AM +0200, Linus Walleij wrote:

> > +Paged virtual memory was invented along with virtual memory as a concept in
> > +1962 on the Ferranti Atlas Computer which was the first computer with paged
> > +virtual memory. The feature migrated to newer computers and became a de facto
> > +feature of all Unix-like systems as time went by. In 1985 the feature was
> > +included in the Intel 80386, which was the CPU Linux 1.0 was developed on.
>
> I still don't think the origin story is useful.  It's trivia and doesn't
> help someone understand what they need to know.

I think history is pretty important for understanding concepts, otherwise
they appear as not invented but emergent. However the question about
what is important and what is trivia is a question of professional technical
writing. The documentation maintainer works with technical writing and
can decide the value of this.

> > +Page tables map virtual addresses as seen by the CPU program counter into
> > +physical addresses as seen on the external memory bus.
>
> This makes it sound like virtual addresses are only used for
> instructions.  I had better wording earlier, but there's no point in
> repeating it.  Just: I dissent.

You are right I should reword this to be about memory accesses, I
might have missed some of your feedback so I will go back and check
what you said.

> > +Linux defines page tables as a hierarchy which is currently five levels in
> > +height. The target architecture code for each supported architecture will then
> > +map this to the restrictions of the target hardware.
>
> The word "target" isn't adding any value in this paragraph.

Thanks dropping it.

I guess the word is a byproduct of doing too much cross compiling...

> Honestly, I don't like much about this document.  The writing is
> flabby and untargetted.

Please refarain from using this kind of unproffessional wording in your
professional communication. Be technical, precise and to the point and do
not use value statements.

> Much of my last review was ignored.  I'm just
> going to stop here since I have low confidence that any suggestions
> would be incorporated.

Your statement is incorrect. Your feedback is seen as valuable
and it is read, reacted on and incorporated.

I did rewrite the document thoroughly in reaction to your comments,
and in order so you feel respected I am going to illustrate.

You wrote:

> This reads backwards to me.  The index point in the hierarchy (what an
> unusual turn of phrase!) is surely the virtual address, since the
> hierarchy is indexed by virtual addresses.  If this paragraph is
> supposed to define what a pfn is, how about simply:
>
> The pfn of a page of memory is the physical address of the page divided
> by PAGE_SIZE

Which I took as a good suggestion, and the paragraph is rewritten
from this (which you criticized):

> +The physical address corresponding to the virtual address is commonly
> +defined by the index point in the hierarchy, and this is called a **page frame
> +number** or **pfn**. The first entry on the top level to the first entry in the
> +second and so on down the hierarchy will point out the virtual address for the
> +physical memory address 0, which will be *pfn 0* and the highest pfn will be
> +the last page of physical memory the external address bus of the CPU can
> +address.

To this (current wording):

> The physical address corresponding to the virtual address is often referenced
> by the underlying physical page frame. The **page frame number** or **pfn**
> is the physical address of the page (as seen on the external memory bus)
> divided by `PAGE_SIZE`.

Which obviously includes some of your wording (divided by PAGE_SIZE etc).
I also got review comments from other reviewers and that is reflected in the
current wording.

You wrote:

> Your arrows are backwards.  The PTE doesn't point to the PMD; the PMD
> points to PTEs.

And this was changed to what you suggested, and it was a pretty
fundamental and important change.

You wrote:

> You have rather too many forward references in this description for my
> taste.  Start with the PTE, then the PMD, then  PUD, P4D, PGD.

And this was changed to exactly what you suggested.

You wrote:

> array, not list

And this was changed (everywhere) to what you suggested.

You wrote:

> I think a document like this that talks about page tables really needs to
> include a description of how some PMDs / PUDs / ... may not be pointers
> to lower levels, but direct pointers to the actual memory (ie THPs /
> hugetlb pages).

And the document now includes this (Mike Rapoport made the same
comment).

So your claim that "Much of my last review was ignored." is simply
incorrect. It was not ignored, rather it has thoroughly shaped the document.

Yours,
Linus Walleij