Date:   Wed, 8 Mar 2023 17:10:41 -0500
From:   Peter Xu <peterx@redhat.com>
To:     James Houghton <jthoughton@google.com>
Cc:     Mike Kravetz <mike.kravetz@oracle.com>,
        Hugh Dickins <hughd@google.com>,
        Muchun Song <songmuchun@bytedance.com>,
        "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
        David Hildenbrand <david@redhat.com>,
        David Rientjes <rientjes@google.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Jiaqi Yan <jiaqiyan@google.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/2] mm: rmap: merge HugeTLB mapcount logic with THPs
Message-ID: <ZAkH4cImuFttLuDQ@x1n>
References: <20230306230004.1387007-1-jthoughton@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230306230004.1387007-1-jthoughton@google.com>
Precedence: bulk

On Mon, Mar 06, 2023 at 11:00:02PM +0000, James Houghton wrote:
> HugeTLB pages may soon support being mapped with PTEs. To allow for this
> case, merge HugeTLB's mapcount scheme with THP's.
> 
> The first patch of this series comes from the HugeTLB high-granularity
> mapping series[1], though with some updates, as the original version
> was buggy[2] and incomplete.
> 
> I am sending this change as part of this smaller series in hopes that it
> can be more thoroughly scrutinized.
> 
> I haven't run any THP performance tests with this series applied.
> HugeTLB pages don't currently support being mapped with
> `compound=false`, but this mapcount scheme will make collapsing
> compound=false mappings in HugeTLB pages quite slow. This can be
> optimized with future patches (likely by taking advantage of HugeTLB's
> alignment guarantees).
> 
> Matthew Wilcox is working on a mapcounting scheme[3] that will avoid
> the use of each subpage's mapcount. If this series is applied, Matthew's
> new scheme will automatically apply to HugeTLB pages.

Is this the plan?

I may have not followed closely on the latest development of Matthew's
idea.  The thing is if the design requires ptes being installed / removed
at the same time for the whole folio, then it may not work directly for HGM
if HGM wants to support at least postcopy, iiuc, because if we install the
whole folio ptes at the same time it seems to beat the whole purpose of
having HGM..

The patch (especially patch 1) looks good.  So it's a pure question just to
make sure we're on the same page.  IIUC your other mapcount proposal may
work, but it still needs to be able to take care of ptes in less-than-folio
sizes whatever it'll look like at last.

A trivial comment on patch 2 since we're at it: does "a future plan on some
arch to support 512GB huge page" justify itself?  It would be better
justified, IMHO, when that support is added (and decided to use HGM)?

What I feel like is missing (rather than patch 2 itself) is some guard to
make sure thp mapcountings will not be abused with new hugetlb sizes
coming.

How about another BUG_ON() squashed into patch 1 (probably somewhere in
page_add_file|anon_rmap()) to make sure folio_size() is always smaller than
COMPOUND_MAPPED / 2)?

-- 
Peter Xu