linux-kernel - Re: [PATCH] mm/page_alloc: increase default min_free

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 20 Feb 2020 17:53:42 -0800
From:   John Hubbard <jhubbard@...dia.com>
To:     Joel Savitz <jsavitz@...hat.com>, <linux-kernel@...r.kernel.org>
CC:     Andrew Morton <akpm@...ux-foundation.org>,
        Rafael Aquini <aquini@...hat.com>, <linux-mm@...ck.org>
Subject: Re: [PATCH] mm/page_alloc: increase default min_free_kbytes bound

On 2/20/20 7:01 AM, Joel Savitz wrote:
> 
> Currently, the vm.min_free_kbytes sysctl value is capped at a hardcoded
> 64M in init_per_zone_wmark_min (unless it is overridden by khugepaged
> initialization).
> 
> This value has not been modified since 2005, and enterprise-grade
> systems now frequently have hundreds of GB of RAM and multiple 10, 40,
> or even 100 GB NICs. We have seen page allocation failures on heavily
> loaded systems related to NIC drivers. These issues were resolved by an
> increase to vm.min_free_kbytes.
> 
> This patch increases the hardcoded value by a factor of 4 as a temporary
> solution.
> 
> Further work to make the calculation of vm.min_free_kbytes more
> consistent throughout the kernel would be desirable.
> 
> As an example of the inconsistency of the current method, this value is
> recalculated by init_per_zone_wmark_min() in the case of memory hotplug
> which will override the value set by set_recommended_min_free_kbytes()
> called during khugepaged initialization even if khugepaged remains
> enabled, however an on/off toggle of khugepaged will then recalculate
> and set the value via set_recommended_min_free_kbytes().
> 
> Signed-off-by: Joel Savitz <jsavitz@...hat.com>
> ---
>  mm/page_alloc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 3c4eb750a199..32cbfb13e958 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -7867,8 +7867,8 @@ int __meminit init_per_zone_wmark_min(void)
>  		min_free_kbytes = new_min_free_kbytes;
>  		if (min_free_kbytes < 128)
>  			min_free_kbytes = 128;
> -		if (min_free_kbytes > 65536)
> -			min_free_kbytes = 65536;
> +		if (min_free_kbytes > 262144)
> +			min_free_kbytes = 262144;


Would it be any better to at least use symbols, instead of numbers, in the
routine? Like this:

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3c4eb750a199..e705636bb644 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -149,6 +149,9 @@ DEFINE_STATIC_KEY_FALSE(init_on_free);
 #endif
 EXPORT_SYMBOL(init_on_free);
 
+static const int MIN_FREE_KBYTES_LOWER_LIMIT = 128;
+static const int MIN_FREE_KBYTES_UPPER_LIMIT = 262144;
+
 static int __init early_init_on_alloc(char *buf)
 {
        int ret;
@@ -7865,10 +7868,10 @@ int __meminit init_per_zone_wmark_min(void)
 
        if (new_min_free_kbytes > user_min_free_kbytes) {
                min_free_kbytes = new_min_free_kbytes;
-               if (min_free_kbytes < 128)
-                       min_free_kbytes = 128;
-               if (min_free_kbytes > 65536)
-                       min_free_kbytes = 65536;
+               if (min_free_kbytes < MIN_FREE_KBYTES_LOWER_LIMIT)
+                       min_free_kbytes = MIN_FREE_KBYTES_LOWER_LIMIT;
+               if (min_free_kbytes > MIN_FREE_KBYTES_UPPER_LIMIT)
+                       min_free_kbytes = MIN_FREE_KBYTES_UPPER_LIMIT;
        } else {
                pr_warn("min_free_kbytes is not updated to %d because user defined value %d is preferred\n",
                                new_min_free_kbytes, user_min_free_kbytes);


thanks,
-- 
John Hubbard
NVIDIA


>  	} else {
>  		pr_warn("min_free_kbytes is not updated to %d because user defined value %d is preferred\n",
>  				new_min_free_kbytes, user_min_free_kbytes);
>