Re: [PATCH v1 08/11] mm/sparse-vmemmap: use hugepages for PUD compound pagemaps

From: Dan Williams <dan.j.williams@intel.com>
To: Joao Martins <joao.m.martins@oracle.com>
Cc: Linux MM <linux-mm@kvack.org>, Ira Weiny <ira.weiny@intel.com>,
	 linux-nvdimm <linux-nvdimm@lists.01.org>,
	Matthew Wilcox <willy@infradead.org>,
	 Jason Gunthorpe <jgg@ziepe.ca>, Jane Chu <jane.chu@oracle.com>,
	 Muchun Song <songmuchun@bytedance.com>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	 Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH v1 08/11] mm/sparse-vmemmap: use hugepages for PUD compound pagemaps
Date: Mon, 7 Jun 2021 12:47:24 -0700	[thread overview]
Message-ID: <CAPcyv4hOavDWA+OJjrXe3mTDN5dZOnFVo3SOX3QGccy1ut6MNA@mail.gmail.com> (raw)
In-Reply-To: <51dfb7d8-269d-5692-7c88-7b4084e996cc@oracle.com>

On Mon, Jun 7, 2021 at 5:03 AM Joao Martins <joao.m.martins@oracle.com> wrote:
>
>
>
> On 6/1/21 8:30 PM, Dan Williams wrote:
> > Sorry for the delay, and the sync up on IRC. I think this subject
> > needs to change to "optimize memory savings for compound pud memmap
> > geometry", and move this to the back of the series to make it clear
> > which patches are base functionality and which extend the idea
> > further.
>
> OK
>
> > As far as I can see this patch can move to the end of the
> > series.
>
> Maybe its prefered that this patch could be deferred to out of the series as
> a followup improvement, and leave this series with the base feature set only?

My preference is to keep it in just clarify that it's an optimization
and make sure it does not come before any fundamental patches that
implement the base support.

> > Some additional changelog feedback below:
> >
> >
> > On Thu, Mar 25, 2021 at 4:10 PM Joao Martins <joao.m.martins@oracle.com> wrote:
> >>
> >> Right now basepages are used to populate the PUD tail pages, and it
> >> picks the address of the previous page of the subsection that preceeds
> >
> > s/preceeds/precedes/
> >
> Yeap.
>
> >> the memmap we are initializing.  This is done when a given memmap
> >> address isn't aligned to the pgmap @align (which is safe to do because
> >> @ranges are guaranteed to be aligned to @align).
> >
> > You know what would help is if you could draw an ascii art picture of
> > the before and after of the head page vs tail page arrangement. I can
> > see how the words line up to the code, but it takes a while to get the
> > picture in my head and I think future work in this area will benefit
> > from having a place in Documentation that draws a picture of the
> > layout of the various geometries.
> >
> Makes sense, I will add docs. Mike K. and others had similar trouble following
> the page structs arrangement which ultimately lead to this section of commentary
> at the beginning of the new source file added here ...
>
> https://www.ozlabs.org/~akpm/mmotm/broken-out/mm-hugetlb-free-the-vmemmap-pages-associated-with-each-hugetlb-page.patch

Ah, looks good, but that really belongs in Documentation/ not a comment block.

>
> > I've used asciiflow.com for these types of diagrams in the past.
> >
>
> ... so perhaps I can borrow some of that and place it to
> a common place like in Documentation/vm/compound_pagemaps.rst
>
> This patch specifically would need a new diagram added on top covering
> the PMD page case.

Sounds good.

[..]
> > Other than that the implementation looks ok to me, modulo previous
> > comments about @block type and the use of the "geometry" term.
> >
> OK.
>
> Btw, speaking of geometry, could you have a look at this thread:
>
> https://lore.kernel.org/linux-mm/8c922a58-c901-1ad9-5d19-1182bd6dea1e@oracle.com/
>
> .. and let me know what you think?

Ok, will take a look