Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 10 Oct 2021 15:43:53 -0700 (PDT)
From:   David Rientjes <rientjes@google.com>
To:     Hao Peng <flyingpenghao@gmail.com>
cc:     "Kirill A. Shutemov" <kirill@shutemov.name>,
        David Hildenbrand <david@redhat.com>,
        akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
Subject: Re: [PATCH v2] mm/huge_memory.c: disable THP with large THP size on
 small present memory
In-Reply-To: <CAPm50a+E6mm_qA9h9MSvh4K+WA8Qf6mU=2yig5GyVw9GFJzr8g@mail.gmail.com>
Message-ID: <aece8474-4881-4c9-362-a7cb211e5933@google.com>
References: <CAPm50aLPxJCiVTqqwiz00oMNiqHggB84sXB3x=tv_HUAd5UktQ@mail.gmail.com> <20211008095123.73b4bubwrpdj6tuz@box.shutemov.name> <CAPm50a+E6mm_qA9h9MSvh4K+WA8Qf6mU=2yig5GyVw9GFJzr8g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Sat, 9 Oct 2021, Hao Peng wrote:

> > > After setting the page size to 64k on ARM64, the supported huge page
> > > size is 512M and 1TB. Therefore, if the thp is enabled, the size
> > > of the thp is 512M. But if THP is enabled, min_free_kbytes will
> > > be recalculated. At this time, min_free_kbytes is calculated based
> > > on the size of THP.
> > >
> > > On an arm64 server with 64G memory, the page size is 64k, with thp
> > > enabled.
> > > cat /proc/sys/vm/min_free_kbytes
> > > 3335104
> > >
> > > Therefore, when judging whether to enable THP by default, consider
> > > the size of thp.
> > >
> > > V2: title suggested by David Hildenbrand
> > >
> > > Signed-off-by: Peng Hao <flyingpeng@tencent.com>
> > > ---
> > >  mm/huge_memory.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > > index 5e9ef0fc261e..03c7f571b3ae 100644
> > > --- a/mm/huge_memory.c
> > > +++ b/mm/huge_memory.c
> > > @@ -437,7 +437,7 @@ static int __init hugepage_init(void)
> > >          * where the extra memory used could hurt more than TLB overhead
> > >          * is likely to save.  The admin can still enable it through /sys.
> > >          */
> > > -       if (totalram_pages() < (512 << (20 - PAGE_SHIFT))) {
> > > +       if (totalram_pages() < (512 << (HPAGE_PMD_SHIFT - PAGE_SHIFT))) {
> >
> > On x86-64 HPAGE_PMD_SHIFT is 21, so you double the amount of memory
> > required to enabled THP by default. It doesn't seem to be the intent of
> > the patch.
> >
> > What about something like
> >
> >         if (totalram_pages() < 256 * HPAGE_PMD_NR)
> >
> > ?
> >
> I think that setting the threshold to 512M here is also a rough
> estimate. If it is 512M
> of memory and 2M of THP is used, there are only 256 pages in total.
> This is actually
> too small.

So does this mean that the original intent of the patch is what you 
proposed?  It's not discussed in the changelog so it's unclear.

The "extra memory used could hurt more..." statement in the comment 
depends on other system-wide settings like max_ptes_none and whether you 
default to faulting hugepages if eligible.  There are scenarios where 
there is no extra memory used, so I think the intent is for sane default 
behavior and, as you mention, it can always be enabled at runtime as well.

By using 64KB native page sizes on small memory capacity systems, you're 
already opting into this memory bloat.

If we are trying to avoid memory bloat then we likely shouldn't be 
defaulting max_ptes_none to 511 either and that would be a bigger 
consideration than a minimum memory capacity to enable thp.

Or maybe you are questioning the adjustment to min_free_kbytes and whether 
that is rational or not for small machine sizes (but large page sizes).

> In addition, THP is disabled by default, but you can also enable THP
> dynamically.
> Thanks.