diff mbox series

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

Message ID CAPm50aLPxJCiVTqqwiz00oMNiqHggB84sXB3x=tv_HUAd5UktQ@mail.gmail.com (mailing list archive)
State New
Headers show
Series [v2] mm/huge_memory.c: disable THP with large THP size on small present memory | expand

Commit Message

Hao Peng Oct. 8, 2021, 1:22 a.m. UTC 
From: Peng Hao <flyingpeng@tencent.com>

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(-)

--
2.27.0

Comments

Kirill A. Shutemov Oct. 8, 2021, 9:51 a.m. UTC | #1 
On Fri, Oct 08, 2021 at 09:22:00AM +0800, Hao Peng wrote:
> From: Peng Hao <flyingpeng@tencent.com>
> 
> 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)

?

>                 transparent_hugepage_flags = 0;
>                 return 0;
>         }
> --
> 2.27.0
>
Hao Peng Oct. 9, 2021, 1:28 a.m. UTC | #2 
On Fri, Oct 8, 2021 at 5:51 PM Kirill A. Shutemov <kirill@shutemov.name> wrote:
>
> On Fri, Oct 08, 2021 at 09:22:00AM +0800, Hao Peng wrote:
> > From: Peng Hao <flyingpeng@tencent.com>
> >
> > 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. In addition, THP is disabled by default, but you can also enable THP
dynamically.
Thanks.
> >                 transparent_hugepage_flags = 0;
> >                 return 0;
> >         }
> > --
> > 2.27.0
> >
>
> --
>  Kirill A. Shutemov
David Rientjes Oct. 10, 2021, 10:43 p.m. UTC | #3 
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.
Hao Peng Oct. 11, 2021, 3:55 a.m. UTC | #4 
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.
>
diff mbox series

Patch

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))) {
                transparent_hugepage_flags = 0;
                return 0;
        }