Re: [PATCH v7 3/5] mm/hugetlb_vmemmap: move comment block to Documentation/vm

[Joao Martins <joao.m.martins@oracle.com>]

On 3/4/22 15:21, Jonathan Corbet wrote:
> Joao Martins <joao.m.martins@oracle.com> writes:
> 
>> In preparation for device-dax for using hugetlbfs compound page tail
>> deduplication technique, move the comment block explanation into a
>> common place in Documentation/vm.
>>
>> Cc: Muchun Song <songmuchun@bytedance.com>
>> Cc: Mike Kravetz <mike.kravetz@oracle.com>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
>> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>>  Documentation/vm/index.rst         |   1 +
>>  Documentation/vm/vmemmap_dedup.rst | 175 +++++++++++++++++++++++++++++
>>  mm/hugetlb_vmemmap.c               | 168 +--------------------------
>>  3 files changed, 177 insertions(+), 167 deletions(-)
>>  create mode 100644 Documentation/vm/vmemmap_dedup.rst
> 
> Thanks for remembering to add this to the index.rst file!  That said, I
> get the impression you didn't actually build the docs afterward and look
> at the result; there are a number of things here that won't render the
> way you might like.
> 
Had some environment struggles to render the end result. I had no errors, though,
only two warnings on the diagrams part. I've this properly now and I see the
rendering issues you mention.

>> diff --git a/Documentation/vm/vmemmap_dedup.rst b/Documentation/vm/vmemmap_dedup.rst
>> new file mode 100644
>> index 000000000000..8143b2ce414d
>> --- /dev/null
>> +++ b/Documentation/vm/vmemmap_dedup.rst
>> @@ -0,0 +1,175 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +.. _vmemmap_dedup:
> 
> This label isn't needed, I'd take it out.
> 
I've removed it.

>> +==================================
>> +Free some vmemmap pages of HugeTLB
>> +==================================
>> +
>> +The struct page structures (page structs) are used to describe a physical
>> +page frame. By default, there is a one-to-one mapping from a page frame to
>> +it's corresponding page struct.
>> +
>> +HugeTLB pages consist of multiple base page size pages and is supported by
>> +many architectures. See hugetlbpage.rst in the Documentation directory for

While at it, I'll replace hugetlbpage.rst in the Documentation directory to be:

See :ref:`Documentation/vm/hugetlbpage.rst <hugetlbpage>` for more details.

>> +When the system boot up, every HugeTLB page has more than one struct page
>> +structs which size is (unit: pages):
>> +
>> +   struct_size = HugeTLB_Size / PAGE_SIZE * sizeof(struct page) / PAGE_SIZE
> 
> This, for example, needs to be in a literal block or you won't get what
> you expect; that's true of all of the code samples and ascii-art
> sections.  Easiest way to do that is to end the preceding text line with
> :: instead of :

I've added :: prior to code blocks, and also had some issues with the page table
diagrams in this patch and (the one after this). I also added '::' for the diagrams
to be able to render it properly.

I'll respin with the proper docs fixed. Below what I have staged for this patch.

diff --git a/Documentation/vm/vmemmap_dedup.rst b/Documentation/vm/vmemmap_dedup.rst
index de958bbbf78c..aad48ab713c1 100644
--- a/Documentation/vm/vmemmap_dedup.rst
+++ b/Documentation/vm/vmemmap_dedup.rst
@@ -1,7 +1,5 @@
 .. SPDX-License-Identifier: GPL-2.0

-.. _vmemmap_dedup:
-
 =========================================
 A vmemmap diet for HugeTLB and Device DAX
 =========================================
@@ -13,12 +11,13 @@ The struct page structures (page structs) are used to describe a physical
 page frame. By default, there is a one-to-one mapping from a page frame to
 it's corresponding page struct.

-HugeTLB pages consist of multiple base page size pages and is supported by
-many architectures. See hugetlbpage.rst in the Documentation directory for
-more details. On the x86-64 architecture, HugeTLB pages of size 2MB and 1GB
-are currently supported. Since the base page size on x86 is 4KB, a 2MB
-HugeTLB page consists of 512 base pages and a 1GB HugeTLB page consists of
-4096 base pages. For each base page, there is a corresponding page struct.
+HugeTLB pages consist of multiple base page size pages and is supported by many
+architectures. See :ref:`Documentation/admin-guide/mm/hugetlbpage.rst
+<hugetlbpage>` for more details. On the x86-64 architecture, HugeTLB pages of
+size 2MB and 1GB are currently supported. Since the base page size on x86 is
+4KB, a 2MB HugeTLB page consists of 512 base pages and a 1GB HugeTLB page
+consists of 4096 base pages. For each base page, there is a corresponding page
+struct.

 Within the HugeTLB subsystem, only the first 4 page structs are used to
 contain unique information about a HugeTLB page. __NR_USED_SUBPAGE provides
@@ -47,24 +46,24 @@ page.
 +--------------+-----------+-----------+-----------+-----------+-----------+

 When the system boot up, every HugeTLB page has more than one struct page
-structs which size is (unit: pages):
+structs which size is (unit: pages)::

    struct_size = HugeTLB_Size / PAGE_SIZE * sizeof(struct page) / PAGE_SIZE

 Where HugeTLB_Size is the size of the HugeTLB page. We know that the size
 of the HugeTLB page is always n times PAGE_SIZE. So we can get the following
-relationship.
+relationship::

    HugeTLB_Size = n * PAGE_SIZE

-Then,
+Then::

    struct_size = n * PAGE_SIZE / PAGE_SIZE * sizeof(struct page) / PAGE_SIZE
                = n * sizeof(struct page) / PAGE_SIZE

 We can use huge mapping at the pud/pmd level for the HugeTLB page.

-For the HugeTLB page of the pmd level mapping, then
+For the HugeTLB page of the pmd level mapping, then::

    struct_size = n * sizeof(struct page) / PAGE_SIZE
                = PAGE_SIZE / sizeof(pte_t) * sizeof(struct page) / PAGE_SIZE
@@ -82,7 +81,7 @@ x86-64 and arm64). So if we use pmd level mapping for a HugeTLB page, the
 size of struct page structs of it is 8 page frames which size depends on the
 size of the base page.

-For the HugeTLB page of the pud level mapping, then
+For the HugeTLB page of the pud level mapping, then::

    struct_size = PAGE_SIZE / sizeof(pmd_t) * struct_size(pmd)
                = PAGE_SIZE / 8 * 8 (pages)
@@ -98,7 +97,7 @@ Next, we take the pmd level mapping of the HugeTLB page as an example to
 show the internal implementation of this optimization. There are 8 pages
 struct page structs associated with a HugeTLB page which is pmd mapped.

-Here is how things look before optimization.
+Here is how things look before optimization::

     HugeTLB                  struct pages(8 pages)         page frame(8 pages)
  +-----------+ ---virt_to_page---> +-----------+   mapping to   +-----------+
@@ -131,7 +130,7 @@ Therefore, we can remap pages 1 to 7 to page 0. Only 1 page of page
structs
 will be used for each HugeTLB page. This will allow us to free the remaining
 7 pages to the buddy allocator.

-Here is how things look after remapping.
+Here is how things look after remapping::

     HugeTLB                  struct pages(8 pages)         page frame(8 pages)
  +-----------+ ---virt_to_page---> +-----------+   mapping to   +-----------+