Skip to contentSkip to conversation

mm: khugepaged: recalculate min_free_kbytes after memory hotplug as expected by khugepaged

PATCH Superseded Vijay Balakrishna 3 participants 7 messages Sep 15, 2020 – Sep 21, 2020

Subsystems (4)
Stable branch Memory management THP Memory hot(un)plug

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Vijay Balakrishna

author
#0 lore raw

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

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 <[email protected]>
Cc [email protected]
Reviewed-by Pavel Tatashin <[email protected]>
---
 include/linux/khugepaged.h |  5 +++++
 mm/khugepaged.c            | 13 +++++++++++--
 mm/memory_hotplug.c        |  3 +++
 3 files changed, 19 insertions(+), 2 deletions(-)
include/linux/khugepaged.h +5 -0
index bc45ea1efbf7..c941b7377321 100644
@@ -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 */
mm/khugepaged.c +11 -2
index cfa0dba5fd3b..4f7107476a6f 100644
@@ -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);
+}
mm/memory_hotplug.c +3 -0
index e9d5ab5d3ca0..3e19272c1fad 100644
@@ -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);
Signature · 1 line
2.28.0
author
#1 lore raw

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

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 <[email protected]>
Cc [email protected]
Reviewed-by Pavel Tatashin <[email protected]>
---
 mm/khugepaged.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
mm/khugepaged.c +4 -4
index 4f7107476a6f..b4b753ba411a 100644
@@ -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;
Signature · 1 line
2.28.0
#2 lore raw

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

Quoted 28 lines · 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 <[email protected]>
Cc [email protected]
Reviewed-by Pavel Tatashin <[email protected]>
---
 mm/khugepaged.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
mm/khugepaged.c +1 -1
index 4f7107476a6f..b4b753ba411a 100644
@@ -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) {
 		/*

mm/khugepaged.c static void set_recommended_min_free_kbytes(void) · line 2283

Quoted 5 lines
mm/khugepaged.c +1 -1
@@ -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.

mm/khugepaged.c static void set_recommended_min_free_kbytes(void) · line 2286

Quoted 4 lines
mm/khugepaged.c +1 -1
 	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.

Quoted 9 lines
mm/khugepaged.c +1 -1
 		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;
Signature · 1 line
2.28.0
Signature · 2 lines
Michal Hocko
SUSE Labs
author
#3 lore raw

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

Quoted 30 lines · Michal Hocko
Quoted 29 lines · 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 <[email protected]>
Cc [email protected]
Reviewed-by Pavel Tatashin <[email protected]>
---
  mm/khugepaged.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)
mm/khugepaged.c +1 -1
index 4f7107476a6f..b4b753ba411a 100644
@@ -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)
Quoted 6 lines

mm/khugepaged.c static void set_recommended_min_free_kbytes(void) · line 2283

Quoted 4 lines
mm/khugepaged.c +1 -1
  	/* 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.

Quoted 11 lines

mm/khugepaged.c static void set_recommended_min_free_kbytes(void) · line 2286

Quoted 5 lines
mm/khugepaged.c +1 -1
  	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

Quoted 11 lines
Quoted 10 lines
mm/khugepaged.c +1 -1
  		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;
Signature · 1 line
2.28.0
maintainer
#4 lore raw

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

In reply to Vijay Balakrishna

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?

Signature · 2 lines
Thanks
Sasha
#5 lore raw
In reply to Vijay Balakrishna
Quoted 8 lines · 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.

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.

Quoted 99 lines
Fixes f000565adb77 ("thp: set recommended min free kbytes")
Signed-off-by Vijay Balakrishna <[email protected]>
Cc [email protected]
Reviewed-by Pavel Tatashin <[email protected]>
---
 include/linux/khugepaged.h |  5 +++++
 mm/khugepaged.c            | 13 +++++++++++--
 mm/memory_hotplug.c        |  3 +++
 3 files changed, 19 insertions(+), 2 deletions(-)
include/linux/khugepaged.h +5 -0
index bc45ea1efbf7..c941b7377321 100644
@@ -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 */
mm/khugepaged.c +11 -2
index cfa0dba5fd3b..4f7107476a6f 100644
@@ -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);
+}
mm/memory_hotplug.c +3 -0
index e9d5ab5d3ca0..3e19272c1fad 100644
@@ -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);
Signature · 1 line
2.28.0
Signature · 2 lines
Michal Hocko
SUSE Labs
maintainer
#6 lore raw
In reply to Vijay Balakrishna

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?

Signature · 2 lines
Thanks
Sasha