Re: [PATCH v2] mm/huge_memory.c: disable THP with large THP size on small present memory

[Hao Peng <flyingpenghao@gmail.com>]

On Mon, Oct 11, 2021 at 6:43 AM David Rientjes <rientjes@google.com> wrote:
>
> 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.
>
I have considered this point, but I think the initial threshold is only a
rough estimate. And THP can be enabled at runtime, so this threshold
does not need to be accurate.
> 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 it is on an arm64 machine with a small memory system, such as a mobile phone,
it generally uses 4KB native page size, so the page size of THP is 2MB.
> 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).
>
The main reason for my modification is that excessively large THP page
size may make
min_free_kbytes too large when enabled, especially on systems without
swap that easily
trigger OOM.
> > In addition, THP is disabled by default, but you can also enable THP
> > dynamically.
> > Thanks.
>