[v2 1/2] mm: khugepaged: recalculate min_free_kbytes after memory hotplug as expected by khugepaged

[v2 1/2] mm: khugepaged: recalculate min_free_kbytes after memory hotplug as expected by khugepaged

[Vijay Balakrishna]

When memory is hotplug added or removed the min_free_kbytes must be
recalculated based on what is expected by khugepaged.  Currently
after hotplug, min_free_kbytes will be set to a lower default and higher
default set when THP enabled is lost. This leaves the system with small
min_free_kbytes which isn't suitable for systems especially with network
intensive loads.  Typical failure symptoms include HW WATCHDOG reset,
soft lockup hang notices, NETDEVICE WATCHDOG timeouts, and OOM process
kills.

Fixes: f000565adb77 ("thp: set recommended min free kbytes")

Signed-off-by: Vijay Balakrishna <vijayb@linux.microsoft.com>
Cc: stable@vger.kernel.org
Reviewed-by: Pavel Tatashin <pasha.tatashin@soleen.com>
---
 include/linux/khugepaged.h |  5 +++++
 mm/khugepaged.c            | 13 +++++++++++--
 mm/memory_hotplug.c        |  3 +++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/include/linux/khugepaged.h b/include/linux/khugepaged.h
index bc45ea1efbf7..c941b7377321 100644
--- a/include/linux/khugepaged.h
+++ b/include/linux/khugepaged.h
@@ -15,6 +15,7 @@ extern int __khugepaged_enter(struct mm_struct *mm);
 extern void __khugepaged_exit(struct mm_struct *mm);
 extern int khugepaged_enter_vma_merge(struct vm_area_struct *vma,
 				      unsigned long vm_flags);
+extern void khugepaged_min_free_kbytes_update(void);
 #ifdef CONFIG_SHMEM
 extern void collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr);
 #else
@@ -85,6 +86,10 @@ static inline void collapse_pte_mapped_thp(struct mm_struct *mm,
 					   unsigned long addr)
 {
 }
+
+static inline void khugepaged_min_free_kbytes_update(void)
+{
+}
 #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
 
 #endif /* _LINUX_KHUGEPAGED_H */
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index cfa0dba5fd3b..4f7107476a6f 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -56,6 +56,9 @@ enum scan_result {
 #define CREATE_TRACE_POINTS
 #include <trace/events/huge_memory.h>
 
+static struct task_struct *khugepaged_thread __read_mostly;
+static DEFINE_MUTEX(khugepaged_mutex);
+
 /* default scan 8*512 pte (or vmas) every 30 second */
 static unsigned int khugepaged_pages_to_scan __read_mostly;
 static unsigned int khugepaged_pages_collapsed;
@@ -2292,8 +2295,6 @@ static void set_recommended_min_free_kbytes(void)
 
 int start_stop_khugepaged(void)
 {
-	static struct task_struct *khugepaged_thread __read_mostly;
-	static DEFINE_MUTEX(khugepaged_mutex);
 	int err = 0;
 
 	mutex_lock(&khugepaged_mutex);
@@ -2320,3 +2321,11 @@ int start_stop_khugepaged(void)
 	mutex_unlock(&khugepaged_mutex);
 	return err;
 }
+
+void khugepaged_min_free_kbytes_update(void)
+{
+	mutex_lock(&khugepaged_mutex);
+	if (khugepaged_enabled() && khugepaged_thread)
+		set_recommended_min_free_kbytes();
+	mutex_unlock(&khugepaged_mutex);
+}
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index e9d5ab5d3ca0..3e19272c1fad 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -36,6 +36,7 @@
 #include <linux/memblock.h>
 #include <linux/compaction.h>
 #include <linux/rmap.h>
+#include <linux/khugepaged.h>
 
 #include <asm/tlbflush.h>
 
@@ -857,6 +858,7 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages,
 	zone_pcp_update(zone);
 
 	init_per_zone_wmark_min();
+	khugepaged_min_free_kbytes_update();
 
 	kswapd_run(nid);
 	kcompactd_run(nid);
@@ -1600,6 +1602,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	pgdat_resize_unlock(zone->zone_pgdat, &flags);
 
 	init_per_zone_wmark_min();
+	khugepaged_min_free_kbytes_update();
 
 	if (!populated_zone(zone)) {
 		zone_pcp_reset(zone);
-- 
2.28.0

[v2 2/2] mm: khugepaged: avoid overriding min_free_kbytes set by user

[Vijay Balakrishna]

set_recommended_min_free_kbytes need to honor min_free_kbytes set by the
user.  Post start-of-day THP enable or memory hotplug operations can
lose user specified min_free_kbytes, in particular when it is higher than
calculated recommended value.  Also modifying "recommended_min" variable
type to "int" from "unsigned long" to avoid undesired result noticed
during testing.  It is due to comparing "unsigned long" with "int" type.

Signed-off-by: Vijay Balakrishna <vijayb@linux.microsoft.com>
Cc: stable@vger.kernel.org
Reviewed-by: Pavel Tatashin <pasha.tatashin@soleen.com>
---
 mm/khugepaged.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 4f7107476a6f..b4b753ba411a 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2253,7 +2253,7 @@ static void set_recommended_min_free_kbytes(void)
 {
 	struct zone *zone;
 	int nr_zones = 0;
-	unsigned long recommended_min;
+	int recommended_min;
 
 	for_each_populated_zone(zone) {
 		/*
@@ -2280,12 +2280,12 @@ static void set_recommended_min_free_kbytes(void)
 
 	/* don't ever allow to reserve more than 5% of the lowmem */
 	recommended_min = min(recommended_min,
-			      (unsigned long) nr_free_buffer_pages() / 20);
+			      (int) nr_free_buffer_pages() / 20);
 	recommended_min <<= (PAGE_SHIFT-10);
 
-	if (recommended_min > min_free_kbytes) {
+	if (recommended_min > user_min_free_kbytes) {
 		if (user_min_free_kbytes >= 0)
-			pr_info("raising min_free_kbytes from %d to %lu to help transparent hugepage allocations\n",
+			pr_info("raising min_free_kbytes from %d to %d to help transparent hugepage allocations\n",
 				min_free_kbytes, recommended_min);
 
 		min_free_kbytes = recommended_min;
-- 
2.28.0

Re: [v2 2/2] mm: khugepaged: avoid overriding min_free_kbytes set by user

[Michal Hocko]

On Tue 15-09-20 14:10:58, Vijay Balakrishna wrote:
> set_recommended_min_free_kbytes need to honor min_free_kbytes set by the
> user.  Post start-of-day THP enable or memory hotplug operations can
> lose user specified min_free_kbytes, in particular when it is higher than
> calculated recommended value.  Also modifying "recommended_min" variable
> type to "int" from "unsigned long" to avoid undesired result noticed
> during testing.  It is due to comparing "unsigned long" with "int" type.
> 
> Signed-off-by: Vijay Balakrishna <vijayb@linux.microsoft.com>
> Cc: stable@vger.kernel.org
> Reviewed-by: Pavel Tatashin <pasha.tatashin@soleen.com>
> ---
>  mm/khugepaged.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 4f7107476a6f..b4b753ba411a 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2253,7 +2253,7 @@ static void set_recommended_min_free_kbytes(void)
>  {
>  	struct zone *zone;
>  	int nr_zones = 0;
> -	unsigned long recommended_min;
> +	int recommended_min;
>  
>  	for_each_populated_zone(zone) {
>  		/*
> @@ -2280,12 +2280,12 @@ static void set_recommended_min_free_kbytes(void)
>  
>  	/* don't ever allow to reserve more than 5% of the lowmem */
>  	recommended_min = min(recommended_min,
> -			      (unsigned long) nr_free_buffer_pages() / 20);
> +			      (int) nr_free_buffer_pages() / 20);

nr_free_buffer_pages can oveflow in int on very large machines.

>  	recommended_min <<= (PAGE_SHIFT-10);
>  
> -	if (recommended_min > min_free_kbytes) {
> +	if (recommended_min > user_min_free_kbytes) {

This can decrease the size theoretically. Because user_min_free_kbytes
is -1 by default and recommended_min might be <= min_free_kbytes.

You need to check both. Also can we make user_min_free_kbytes 0 by
default? From a quick look, nobody should really care.

>  		if (user_min_free_kbytes >= 0)
> -			pr_info("raising min_free_kbytes from %d to %lu to help transparent hugepage allocations\n",
> +			pr_info("raising min_free_kbytes from %d to %d to help transparent hugepage allocations\n",
>  				min_free_kbytes, recommended_min);
>  
>  		min_free_kbytes = recommended_min;
> -- 
> 2.28.0
> 

-- 
Michal Hocko
SUSE Labs

Re: [v2 2/2] mm: khugepaged: avoid overriding min_free_kbytes set by user

[Vijay Balakrishna]

On 9/16/2020 12:59 AM, Michal Hocko wrote:
> On Tue 15-09-20 14:10:58, Vijay Balakrishna wrote:
>> set_recommended_min_free_kbytes need to honor min_free_kbytes set by the
>> user.  Post start-of-day THP enable or memory hotplug operations can
>> lose user specified min_free_kbytes, in particular when it is higher than
>> calculated recommended value.  Also modifying "recommended_min" variable
>> type to "int" from "unsigned long" to avoid undesired result noticed
>> during testing.  It is due to comparing "unsigned long" with "int" type.
>>
>> Signed-off-by: Vijay Balakrishna <vijayb@linux.microsoft.com>
>> Cc: stable@vger.kernel.org
>> Reviewed-by: Pavel Tatashin <pasha.tatashin@soleen.com>
>> ---
>>   mm/khugepaged.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index 4f7107476a6f..b4b753ba411a 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -2253,7 +2253,7 @@ static void set_recommended_min_free_kbytes(void)
>>   {
>>   	struct zone *zone;
>>   	int nr_zones = 0;
>> -	unsigned long recommended_min;
>> +	int recommended_min;
>>   
>>   	for_each_populated_zone(zone) {
>>   		/*
>> @@ -2280,12 +2280,12 @@ static void set_recommended_min_free_kbytes(void)
>>   
>>   	/* don't ever allow to reserve more than 5% of the lowmem */
>>   	recommended_min = min(recommended_min,
>> -			      (unsigned long) nr_free_buffer_pages() / 20);
>> +			      (int) nr_free_buffer_pages() / 20);
> 
> nr_free_buffer_pages can oveflow in int on very large machines.

Good point.  I will address it.

> 
>>   	recommended_min <<= (PAGE_SHIFT-10);
>>   
>> -	if (recommended_min > min_free_kbytes) {
>> +	if (recommended_min > user_min_free_kbytes) {
> 
> This can decrease the size theoretically. Because user_min_free_kbytes
> is -1 by default and recommended_min might be <= min_free_kbytes.
> 
> You need to check both. Also can we make user_min_free_kbytes 0 by
> default? From a quick look, nobody should really care.

Let me rework.

Thanks,
Vijay

> 
>>   		if (user_min_free_kbytes >= 0)
>> -			pr_info("raising min_free_kbytes from %d to %lu to help transparent hugepage allocations\n",
>> +			pr_info("raising min_free_kbytes from %d to %d to help transparent hugepage allocations\n",
>>   				min_free_kbytes, recommended_min);
>>   
>>   		min_free_kbytes = recommended_min;
>> -- 
>> 2.28.0
>>
> 

Re: [v2 2/2] mm: khugepaged: avoid overriding min_free_kbytes set by user

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v5.8.10, v5.4.66, v4.19.146, v4.14.198, v4.9.236, v4.4.236.

v5.8.10: Build OK!
v5.4.66: Build OK!
v4.19.146: Build OK!
v4.14.198: Build OK!
v4.9.236: Build OK!
v4.4.236: Failed to apply! Possible dependencies:
    0b57d6ba0bd1 ("mm/mmap.c: remove redundant local variables for may_expand_vm()")
    1170532bb49f ("mm: convert printk(KERN_<LEVEL> to pr_<level>")
    5a6e75f8110c ("shmem: prepare huge= mount option and sysfs knob")
    756a025f0009 ("mm: coalesce split strings")
    84638335900f ("mm: rework virtual memory accounting")
    8cee852ec53f ("mm, procfs: breakdown RSS for anon, shmem and file in /proc/pid/status")
    b46e756f5e47 ("thp: extract khugepaged from mm/huge_memory.c")
    d07e22597d1d ("mm: mmap: add new /proc tunable for mmap_base ASLR")
    d977d56ce5b3 ("mm: warn about VmData over RLIMIT_DATA")
    d9fe4fab1197 ("x86/mm/pat: Add untrack_pfn_moved for mremap")
    eca56ff906bd ("mm, shmem: add internal shmem resident memory accounting")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha

Re: [v2 1/2] mm: khugepaged: recalculate min_free_kbytes after memory hotplug as expected by khugepaged

[Michal Hocko]

On Tue 15-09-20 14:10:57, Vijay Balakrishna wrote:
> When memory is hotplug added or removed the min_free_kbytes must be
> recalculated based on what is expected by khugepaged.  Currently
> after hotplug, min_free_kbytes will be set to a lower default and higher
> default set when THP enabled is lost. This leaves the system with small
> min_free_kbytes which isn't suitable for systems especially with network
> intensive loads.  Typical failure symptoms include HW WATCHDOG reset,
> soft lockup hang notices, NETDEVICE WATCHDOG timeouts, and OOM process
> kills.

This changelog still doesn't explain the underlying problem IMO. It
sounds it is papering over some other problem. I do agree that the
current behavior is non intuitive and surprising. Justifying the change
by that inconsistency is likely much better than arguing with soft
lockups and OOMs which shouldn't be a result of this inconsistency.
 
> Fixes: f000565adb77 ("thp: set recommended min free kbytes")
> 
> Signed-off-by: Vijay Balakrishna <vijayb@linux.microsoft.com>
> Cc: stable@vger.kernel.org
> Reviewed-by: Pavel Tatashin <pasha.tatashin@soleen.com>
> ---
>  include/linux/khugepaged.h |  5 +++++
>  mm/khugepaged.c            | 13 +++++++++++--
>  mm/memory_hotplug.c        |  3 +++
>  3 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/khugepaged.h b/include/linux/khugepaged.h
> index bc45ea1efbf7..c941b7377321 100644
> --- a/include/linux/khugepaged.h
> +++ b/include/linux/khugepaged.h
> @@ -15,6 +15,7 @@ extern int __khugepaged_enter(struct mm_struct *mm);
>  extern void __khugepaged_exit(struct mm_struct *mm);
>  extern int khugepaged_enter_vma_merge(struct vm_area_struct *vma,
>  				      unsigned long vm_flags);
> +extern void khugepaged_min_free_kbytes_update(void);
>  #ifdef CONFIG_SHMEM
>  extern void collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr);
>  #else
> @@ -85,6 +86,10 @@ static inline void collapse_pte_mapped_thp(struct mm_struct *mm,
>  					   unsigned long addr)
>  {
>  }
> +
> +static inline void khugepaged_min_free_kbytes_update(void)
> +{
> +}
>  #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>  
>  #endif /* _LINUX_KHUGEPAGED_H */
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index cfa0dba5fd3b..4f7107476a6f 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -56,6 +56,9 @@ enum scan_result {
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/huge_memory.h>
>  
> +static struct task_struct *khugepaged_thread __read_mostly;
> +static DEFINE_MUTEX(khugepaged_mutex);
> +
>  /* default scan 8*512 pte (or vmas) every 30 second */
>  static unsigned int khugepaged_pages_to_scan __read_mostly;
>  static unsigned int khugepaged_pages_collapsed;
> @@ -2292,8 +2295,6 @@ static void set_recommended_min_free_kbytes(void)
>  
>  int start_stop_khugepaged(void)
>  {
> -	static struct task_struct *khugepaged_thread __read_mostly;
> -	static DEFINE_MUTEX(khugepaged_mutex);
>  	int err = 0;
>  
>  	mutex_lock(&khugepaged_mutex);
> @@ -2320,3 +2321,11 @@ int start_stop_khugepaged(void)
>  	mutex_unlock(&khugepaged_mutex);
>  	return err;
>  }
> +
> +void khugepaged_min_free_kbytes_update(void)
> +{
> +	mutex_lock(&khugepaged_mutex);
> +	if (khugepaged_enabled() && khugepaged_thread)
> +		set_recommended_min_free_kbytes();
> +	mutex_unlock(&khugepaged_mutex);
> +}
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index e9d5ab5d3ca0..3e19272c1fad 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -36,6 +36,7 @@
>  #include <linux/memblock.h>
>  #include <linux/compaction.h>
>  #include <linux/rmap.h>
> +#include <linux/khugepaged.h>
>  
>  #include <asm/tlbflush.h>
>  
> @@ -857,6 +858,7 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages,
>  	zone_pcp_update(zone);
>  
>  	init_per_zone_wmark_min();
> +	khugepaged_min_free_kbytes_update();
>  
>  	kswapd_run(nid);
>  	kcompactd_run(nid);
> @@ -1600,6 +1602,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
>  	pgdat_resize_unlock(zone->zone_pgdat, &flags);
>  
>  	init_per_zone_wmark_min();
> +	khugepaged_min_free_kbytes_update();
>  
>  	if (!populated_zone(zone)) {
>  		zone_pcp_reset(zone);
> -- 
> 2.28.0

-- 
Michal Hocko
SUSE Labs

Re: [v2 1/2] mm: khugepaged: recalculate min_free_kbytes after memory hotplug as expected by khugepaged

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: f000565adb77 ("thp: set recommended min free kbytes").

The bot has tested the following trees: v5.8.10, v5.4.66, v4.19.146, v4.14.198, v4.9.236, v4.4.236.

v5.8.10: Build OK!
v5.4.66: Build OK!
v4.19.146: Failed to apply! Possible dependencies:
    013de2d6671d ("csky: MMU and page table management")
    27e1f8273113 ("khugepaged: enable collapse pmd for pte-mapped THP")
    426dcd4b600f ("hexagon: switch to NO_BOOTMEM")
    6471f52af786 ("alpha: switch to NO_BOOTMEM")
    9143a9359d05 ("csky: Kernel booting")
    aca52c398389 ("mm: remove CONFIG_HAVE_MEMBLOCK")
    bc3ec75de545 ("dma-mapping: merge direct and noncoherent ops")
    c32e64e852f3 ("csky: Build infrastructure")
    ca9a46f8a4f0 ("mm/memory_hotplug: online_pages cannot be 0 in online_pages()")
    de1193f0be66 ("mm, memory_hotplug: update pcp lists everytime onlining a memory block")
    e0a9317d9004 ("hexagon: use generic dma_noncoherent_ops")
    e900a918b098 ("mm: shuffle initial free memory to improve memory-side-cache utilization")
    f3ecc0ff0457 ("dma-mapping: move the dma_coherent flag to struct device")
    f406f222d4b2 ("hexagon: implement the sync_sg_for_device DMA operation")

v4.14.198: Failed to apply! Possible dependencies:
    008ef0969dd9 ("btrfs: drop lock parameter from update_ioctl_balance_args and rename")
    17ef445f9bef ("Documentation/filesystems: update documentation of file_operations")
    27e1f8273113 ("khugepaged: enable collapse pmd for pte-mapped THP")
    3d8f7615319b ("vfs: implement readahead(2) using POSIX_FADV_WILLNEED")
    45cd0faae371 ("vfs: add the fadvise() file operation")
    5740c99e9d30 ("vfs: dedupe: return int")
    6e8b704df584 ("fs: update documentation to mention __poll_t and match the code")
    87eb5eb24232 ("vfs: dedupe: rationalize args")
    b4e98d9ac775 ("mm: account pud page tables")
    c4812909f5d5 ("mm: introduce wrappers to access mm->nr_ptes")
    ca9a46f8a4f0 ("mm/memory_hotplug: online_pages cannot be 0 in online_pages()")
    de1193f0be66 ("mm, memory_hotplug: update pcp lists everytime onlining a memory block")
    e900a918b098 ("mm: shuffle initial free memory to improve memory-side-cache utilization")
    f51d2b59120f ("btrfs: allow to set compression level for zlib")
    f5c29bd9dbd3 ("Btrfs: add __init macro to btrfs init functions")

v4.9.236: Failed to apply! Possible dependencies:
    0b89ede62963 ("s390/mm: fork vs. 5 level page tabel")
    27e1f8273113 ("khugepaged: enable collapse pmd for pte-mapped THP")
    505a60e22560 ("asm-generic: introduce 5level-fixup.h")
    a00cc7d9dd93 ("mm, x86: add support for PUD-sized transparent hugepages")
    b4e98d9ac775 ("mm: account pud page tables")
    c2febafc6773 ("mm: convert generic code to 5-level paging")
    c4812909f5d5 ("mm: introduce wrappers to access mm->nr_ptes")
    c763ea2650df ("x86/kconfig: Sort the 'config X86' selects alphabetically")
    d94e068573f2 ("x86/kconfig: Move 64-bit only arch Kconfig selects to 'config X86_64'")

v4.4.236: Failed to apply! Possible dependencies:
    0b57d6ba0bd1 ("mm/mmap.c: remove redundant local variables for may_expand_vm()")
    1170532bb49f ("mm: convert printk(KERN_<LEVEL> to pr_<level>")
    27e1f8273113 ("khugepaged: enable collapse pmd for pte-mapped THP")
    5a6e75f8110c ("shmem: prepare huge= mount option and sysfs knob")
    756a025f0009 ("mm: coalesce split strings")
    84638335900f ("mm: rework virtual memory accounting")
    8cee852ec53f ("mm, procfs: breakdown RSS for anon, shmem and file in /proc/pid/status")
    b46e756f5e47 ("thp: extract khugepaged from mm/huge_memory.c")
    d07e22597d1d ("mm: mmap: add new /proc tunable for mmap_base ASLR")
    d977d56ce5b3 ("mm: warn about VmData over RLIMIT_DATA")
    d9fe4fab1197 ("x86/mm/pat: Add untrack_pfn_moved for mremap")
    eca56ff906bd ("mm, shmem: add internal shmem resident memory accounting")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha