mm: Rework virtual memory accounting

RFC PATCH Applied Cyrill Gorcunov 4 participants 9 messages Dec 28, 2015 – Jan 22, 2016

Subsystems (7)

Stable branch Memory management MM core Scheduler +3 more subsystems

lore

Share & tools

lore

Cyrill Gorcunov

author

2015-12-28 21:10 UTC #0 lore raw

RFC PATCH 1/1 mm: Rework virtual memory accounting

Really sorry for delays. Konstantin, I slightly updated the changelog (to point where problem came from). Linus are you fine with accounting not only anonymous memory in VmData?

Guys, take a look please, comments are highly appreciated.
---

From: Konstantin Khlebnikov <[email protected]>

When inspecting a vague code inside prctl(PR_SET_MM_MEM) call (which testing the RLIMIT_DATA value to figure out if we're allowed to assign new @start_brk, @brk, @start_data, @end_data from mm_struct) it's been commited that RLIMIT_DATA in a form it's implemented now doesn't do anything useful because most of user-space libraries use mmap() syscall for dynamic memory allocations.

Linus suggested to convert RLIMIT_DATA rlimit into something suitable for anonymous memory accounting. But in this patch we go further, and the changes are bundled together as:

* keep vma counting if CONFIG_PROC_FS=n, will be used for limits
* replace mm->shared_vm with better defined mm->data_vm
* account anonymous executable areas as executable
* account file-backed growsdown/up areas as stack
* drop struct file* argument from vm_stat_account
* enforce RLIMIT_DATA for size of data areas

This way code looks cleaner: now code/stack/data
classification depends only on vm_flags state:

VM_EXEC & ~VM_WRITE -> code (VmExe + VmLib in proc)
VM_GROWSUP | VM_GROWSDOWN -> stack (VmStk)
VM_WRITE & ~VM_SHARED & !stack -> data (VmData)

The rest (VmSize - VmData - VmStk - VmExe - VmLib) could be called "shared", but that might be strange beast like readonly-private or VM_IO area.

 - RLIMIT_AS            limits whole address space "VmSize"
 - RLIMIT_STACK         limits stack "VmStk" (but each vma individually)
 - RLIMIT_DATA          now limits "VmData"

CC Quentin Casasnovas <[email protected]>

CC Vegard Nossum <[email protected]>

CC Linus Torvalds <[email protected]>

CC Andrew Morton <[email protected]>

CC Willy Tarreau <[email protected]>

CC Andy Lutomirski <[email protected]>

CC Kees Cook <[email protected]>

CC Vladimir Davydov <[email protected]>

CC Pavel Emelyanov <[email protected]>

CC Peter Zijlstra <[email protected]>

Signed-off-by Konstantin Khlebnikov <[email protected]>

Signed-off-by Cyrill Gorcunov <[email protected]>

---
 arch/ia64/kernel/perfmon.c |    3 --
 fs/proc/task_mmu.c         |    7 ++---
 include/linux/mm.h         |   13 ++-------
 include/linux/mm_types.h   |    2 -
 kernel/fork.c              |    5 +--
 mm/debug.c                 |    4 +-
 mm/mmap.c                  |   63 ++++++++++++++++++++++-----------------------
 mm/mprotect.c              |    8 ++++-
 mm/mremap.c                |    7 ++---
 9 files changed, 53 insertions(+), 59 deletions(-)

arch/ia64/kernel/perfmon.c +1 -2

Index: linux-ml.git/arch/ia64/kernel/perfmon.c
===================================================================
@@ -2332,8 +2332,7 @@ pfm_smpl_buffer_alloc(struct task_struct
 	 */
 	insert_vm_struct(mm, vma);
 
-	vm_stat_account(vma->vm_mm, vma->vm_flags, vma->vm_file,
-							vma_pages(vma));
+	vm_stat_account(vma->vm_mm, vma->vm_flags, vma_pages(vma));
 	up_write(&task->mm->mmap_sem);
 
 	/*

fs/proc/task_mmu.c +3 -4

Index: linux-ml.git/fs/proc/task_mmu.c
===================================================================
@@ -22,7 +22,7 @@
 
 void task_mem(struct seq_file *m, struct mm_struct *mm)
 {
-	unsigned long data, text, lib, swap, ptes, pmds;
+	unsigned long text, lib, swap, ptes, pmds;
 	unsigned long hiwater_vm, total_vm, hiwater_rss, total_rss;
 
 	/*

@@ -39,7 +39,6 @@ void task_mem(struct seq_file *m, struct
 	if (hiwater_rss < mm->hiwater_rss)
 		hiwater_rss = mm->hiwater_rss;
 
-	data = mm->total_vm - mm->shared_vm - mm->stack_vm;
 	text = (PAGE_ALIGN(mm->end_code) - (mm->start_code & PAGE_MASK)) >> 10;
 	lib = (mm->exec_vm << (PAGE_SHIFT-10)) - text;
 	swap = get_mm_counter(mm, MM_SWAPENTS);

@@ -65,7 +64,7 @@ void task_mem(struct seq_file *m, struct
 		mm->pinned_vm << (PAGE_SHIFT-10),
 		hiwater_rss << (PAGE_SHIFT-10),
 		total_rss << (PAGE_SHIFT-10),
-		data << (PAGE_SHIFT-10),
+		mm->data_vm << (PAGE_SHIFT-10),
 		mm->stack_vm << (PAGE_SHIFT-10), text, lib,
 		ptes >> 10,
 		pmds >> 10,

@@ -85,7 +84,7 @@ unsigned long task_statm(struct mm_struc
 	*shared = get_mm_counter(mm, MM_FILEPAGES);
 	*text = (PAGE_ALIGN(mm->end_code) - (mm->start_code & PAGE_MASK))
 								>> PAGE_SHIFT;
-	*data = mm->total_vm - mm->shared_vm;
+	*data = mm->data_vm + mm->stack_vm;
 	*resident = *shared + get_mm_counter(mm, MM_ANONPAGES);
 	return mm->total_vm;
 }

include/linux/mm.h +3 ~~-10~~

Index: linux-ml.git/include/linux/mm.h
===================================================================
@@ -1898,7 +1898,9 @@ extern void mm_drop_all_locks(struct mm_
 extern void set_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file);
 extern struct file *get_mm_exe_file(struct mm_struct *mm);
 
-extern int may_expand_vm(struct mm_struct *mm, unsigned long npages);
+extern bool may_expand_vm(struct mm_struct *, vm_flags_t, unsigned long npages);
+extern void vm_stat_account(struct mm_struct *, vm_flags_t, long npages);
+
 extern struct vm_area_struct *_install_special_mapping(struct mm_struct *mm,
 				   unsigned long addr, unsigned long len,
 				   unsigned long flags,

@@ -2116,15 +2118,6 @@ typedef int (*pte_fn_t)(pte_t *pte, pgta
 extern int apply_to_page_range(struct mm_struct *mm, unsigned long address,
 			       unsigned long size, pte_fn_t fn, void *data);
 
-#ifdef CONFIG_PROC_FS
-void vm_stat_account(struct mm_struct *, unsigned long, struct file *, long);
-#else
-static inline void vm_stat_account(struct mm_struct *mm,
-			unsigned long flags, struct file *file, long pages)
-{
-	mm->total_vm += pages;
-}
-#endif /* CONFIG_PROC_FS */
 
 #ifdef CONFIG_DEBUG_PAGEALLOC
 extern bool _debug_pagealloc_enabled;

include/linux/mm_types.h +1 -1

Index: linux-ml.git/include/linux/mm_types.h
===================================================================
@@ -426,7 +426,7 @@ struct mm_struct {
 	unsigned long total_vm;		/* Total pages mapped */
 	unsigned long locked_vm;	/* Pages that have PG_mlocked set */
 	unsigned long pinned_vm;	/* Refcount permanently increased */
-	unsigned long shared_vm;	/* Shared pages (files) */
+	unsigned long data_vm;		/* VM_WRITE & ~VM_SHARED/GROWSDOWN */
 	unsigned long exec_vm;		/* VM_EXEC & ~VM_WRITE */
 	unsigned long stack_vm;		/* VM_GROWSUP/DOWN */
 	unsigned long def_flags;

kernel/fork.c +2 -3

Index: linux-ml.git/kernel/fork.c
===================================================================
@@ -413,7 +413,7 @@ static int dup_mmap(struct mm_struct *mm
 	RCU_INIT_POINTER(mm->exe_file, get_mm_exe_file(oldmm));
 
 	mm->total_vm = oldmm->total_vm;
-	mm->shared_vm = oldmm->shared_vm;
+	mm->data_vm = oldmm->data_vm;
 	mm->exec_vm = oldmm->exec_vm;
 	mm->stack_vm = oldmm->stack_vm;

@@ -432,8 +432,7 @@ static int dup_mmap(struct mm_struct *mm
 		struct file *file;
 
 		if (mpnt->vm_flags & VM_DONTCOPY) {
-			vm_stat_account(mm, mpnt->vm_flags, mpnt->vm_file,
-							-vma_pages(mpnt));
+			vm_stat_account(mm, mpnt->vm_flags, -vma_pages(mpnt));
 			continue;
 		}
 		charge = 0;

mm/debug.c +2 -2

Index: linux-ml.git/mm/debug.c
===================================================================
@@ -175,7 +175,7 @@ void dump_mm(const struct mm_struct *mm)
 		"mmap_base %lu mmap_legacy_base %lu highest_vm_end %lu\n"
 		"pgd %p mm_users %d mm_count %d nr_ptes %lu nr_pmds %lu map_count %d\n"
 		"hiwater_rss %lx hiwater_vm %lx total_vm %lx locked_vm %lx\n"
-		"pinned_vm %lx shared_vm %lx exec_vm %lx stack_vm %lx\n"
+		"pinned_vm %lx data_vm %lx exec_vm %lx stack_vm %lx\n"
 		"start_code %lx end_code %lx start_data %lx end_data %lx\n"
 		"start_brk %lx brk %lx start_stack %lx\n"
 		"arg_start %lx arg_end %lx env_start %lx env_end %lx\n"

@@ -209,7 +209,7 @@ void dump_mm(const struct mm_struct *mm)
 		mm_nr_pmds((struct mm_struct *)mm),
 		mm->map_count,
 		mm->hiwater_rss, mm->hiwater_vm, mm->total_vm, mm->locked_vm,
-		mm->pinned_vm, mm->shared_vm, mm->exec_vm, mm->stack_vm,
+		mm->pinned_vm, mm->data_vm, mm->exec_vm, mm->stack_vm,
 		mm->start_code, mm->end_code, mm->start_data, mm->end_data,
 		mm->start_brk, mm->brk, mm->start_stack,
 		mm->arg_start, mm->arg_end, mm->env_start, mm->env_end,

mm/mmap.c +31 ~~-32~~

141 lines not on this page · raw message

mm/mprotect.c +6 -2

Index: linux-ml.git/mm/mprotect.c
===================================================================
@@ -278,6 +278,10 @@ mprotect_fixup(struct vm_area_struct *vm
 	 * even if read-only so there is no need to account for them here
 	 */
 	if (newflags & VM_WRITE) {
+		/* Check space limits when area turns into data. */
+		if (!may_expand_vm(mm, newflags, nrpages) &&
+				may_expand_vm(mm, oldflags, nrpages))
+			return -ENOMEM;
 		if (!(oldflags & (VM_ACCOUNT|VM_WRITE|VM_HUGETLB|
 						VM_SHARED|VM_NORESERVE))) {
 			charged = nrpages;

@@ -334,8 +338,8 @@ success:
 		populate_vma_page_range(vma, start, end, NULL);
 	}
 
-	vm_stat_account(mm, oldflags, vma->vm_file, -nrpages);
-	vm_stat_account(mm, newflags, vma->vm_file, nrpages);
+	vm_stat_account(mm, oldflags, -nrpages);
+	vm_stat_account(mm, newflags, nrpages);
 	perf_event_mmap(vma);
 	return 0;

mm/mremap.c +4 -3

Index: linux-ml.git/mm/mremap.c
===================================================================
@@ -317,7 +317,7 @@ static unsigned long move_vma(struct vm_
 	 * If this were a serious issue, we'd add a flag to do_munmap().
 	 */
 	hiwater_vm = mm->hiwater_vm;
-	vm_stat_account(mm, vma->vm_flags, vma->vm_file, new_len>>PAGE_SHIFT);
+	vm_stat_account(mm, vma->vm_flags, new_len >> PAGE_SHIFT);
 
 	if (do_munmap(mm, old_addr, old_len) < 0) {
 		/* OOM: unable to split vma, just get accounts right */

@@ -379,7 +379,8 @@ static struct vm_area_struct *vma_to_res
 			return ERR_PTR(-EAGAIN);
 	}
 
-	if (!may_expand_vm(mm, (new_len - old_len) >> PAGE_SHIFT))
+	if (!may_expand_vm(mm, vma->vm_flags,
+				(new_len - old_len) >> PAGE_SHIFT))
 		return ERR_PTR(-ENOMEM);
 
 	if (vma->vm_flags & VM_ACCOUNT) {

@@ -541,7 +542,7 @@ SYSCALL_DEFINE5(mremap, unsigned long, a
 				goto out;
 			}
 
-			vm_stat_account(mm, vma->vm_flags, vma->vm_file, pages);
+			vm_stat_account(mm, vma->vm_flags, pages);
 			if (vma->vm_flags & VM_LOCKED) {
 				mm->locked_vm += pages;
 				locked = true;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Linus Torvalds

2015-12-28 22:22 UTC #1 lore raw

Quoted 3 lines · Cyrill Gorcunov

Really sorry for delays. Konstantin, I slightly updated the changelog (to point where problem came from). Linus are you fine with accounting not only anonymous memory in VmData?

The patch looks ok to me. I guess if somebody relies on old behavior we may have to tweak it a bit, but on the whole this looks sane and I'd be happy to merge it in the 4.5 merge window (and maybe even have it marked for stable if it works out)

Linus

Cyrill Gorcunov

author

2015-12-29 09:43 UTC #2 lore raw

Quoted 9 lines · Linus Torvalds

Quoted 3 lines · Cyrill Gorcunov
Really sorry for delays. Konstantin, I slightly updated the changelog (to point where problem came from). Linus are you fine with accounting not only anonymous memory in VmData?
The patch looks ok to me. I guess if somebody relies on old behavior we may have to tweak it a bit, but on the whole this looks sane and I'd be happy to merge it in the 4.5 merge window (and maybe even have it marked for stable if it works out)

Thank you!

Christian Borntraeger

2016-01-22 19:42 UTC #3 lore raw

In reply to Linus Torvalds

Quoted 10 lines · Linus Torvalds

Quoted 3 lines · Cyrill Gorcunov
Really sorry for delays. Konstantin, I slightly updated the changelog (to point where problem came from). Linus are you fine with accounting not only anonymous memory in VmData?
The patch looks ok to me. I guess if somebody relies on old behavior we may have to tweak it a bit, but on the whole this looks sane and I'd be happy to merge it in the 4.5 merge window (and maybe even have it marked for stable if it works out)

Just want to mention that this patch breaks older versions of valgrind (including the current release)
https://bugs.kde.org/show_bug.cgi?id=357833
It is fixed in trunk (and even triggered some good cleanups, so the valgrind developers do NOT want it to get reverted). Rawhide already has the valgrind fix, others might not, so if we consider this for stable, things might break here and there, but in general this looks like a good cleanup.

Christian

Cyrill Gorcunov

author

2016-01-22 20:20 UTC #4 lore raw

Quoted 20 lines · Christian Borntraeger

Quoted 10 lines · Linus Torvalds
Quoted 3 lines · Cyrill Gorcunov
Really sorry for delays. Konstantin, I slightly updated the changelog (to point where problem came from). Linus are you fine with accounting not only anonymous memory in VmData?
The patch looks ok to me. I guess if somebody relies on old behavior we may have to tweak it a bit, but on the whole this looks sane and I'd be happy to merge it in the 4.5 merge window (and maybe even have it marked for stable if it works out)
Just want to mention that this patch breaks older versions of valgrind (including the current release)
https://bugs.kde.org/show_bug.cgi?id=357833
It is fixed in trunk (and even triggered some good cleanups, so the valgrind developers do NOT want it to get reverted). Rawhide already has the valgrind fix, others might not, so if we consider this for stable, things might break here and there, but in general this looks like a good cleanup.

Quoted 1 line

Christian

Thanks a huge for the report, Christian. I think this won't go for stable for now, lets see if there are other tools which do the same trick setting up zero to rlimit data. If indeed this would make more problems than solve it, we might need to find a way for backward compatibility.

Andrew Morton

maintainer

2016-01-22 20:21 UTC #5 lore raw

In reply to Christian Borntraeger

Quoted 12 lines · On Fri, 22 Jan 2016 20:42:11 +0100 Christian Borntraeger…

Quoted 10 lines · Linus Torvalds
Quoted 3 lines · Cyrill Gorcunov
Really sorry for delays. Konstantin, I slightly updated the changelog (to point where problem came from). Linus are you fine with accounting not only anonymous memory in VmData?
The patch looks ok to me. I guess if somebody relies on old behavior we may have to tweak it a bit, but on the whole this looks sane and I'd be happy to merge it in the 4.5 merge window (and maybe even have it marked for stable if it works out)

Quoted 8 lines

Just want to mention that this patch breaks older versions of valgrind (including the current release)
https://bugs.kde.org/show_bug.cgi?id=357833
It is fixed in trunk (and even triggered some good cleanups, so the valgrind developers do NOT want it to get reverted). Rawhide already has the valgrind fix, others might not, so if we consider this for stable, things might break here and there, but in general this looks like a good cleanup.

OK, thanks - that sounds reasonable, although a bit worrisome - what other userspace was affected? In some cases people won't find out for years...

84638335900f199 ("mm: rework virtual memory accounting") did not have the cc:stable tag so it should avoid the -stable dragnet.

Linus Torvalds

2016-01-22 20:30 UTC #6 lore raw

In reply to Christian Borntraeger

Quoted 4 lines · Christian Borntraeger

Just want to mention that this patch breaks older versions of valgrind (including the current release)
https://bugs.kde.org/show_bug.cgi?id=357833

Ugh. Looks like valgrind is doing something that fundamentally can't be "tweaked" around in the algorithm. Setting the data limit to zero will never work with any model that starts accounting any mmap, so we can't just tweak things a bit..

Quoted 2 lines

It is fixed in trunk (and even triggered some good cleanups, so the valgrind developers do NOT want it to get reverted).

Hmm. If we start getting complaints from users, I suspect we'll just have to revert. The fact that the valgrind developers are ok with the change doesn't much matter - all that matters is whether users are ok with it.

The only saving grace is that valgrind is fairly specialized, so it's not like it breaks some core workflow. But I could easily see people who run valgrind as part of some regression suite having their day-to-day work broken.

So I'll let it slide for now, but if I start seeing complaints, I think we'll just have to revert and wait for fixed valgrind versions to actually percolate out to people and re-do it later.. (The "percolate out to people" tends to take a _loong_ time, though).

Linus

Andrew Morton

maintainer

2015-12-28 23:10 UTC #7 lore raw

In reply to Cyrill Gorcunov

Quoted 26 lines · On Tue, 29 Dec 2015 00:10:15 +0300 Cyrill Gorcunov <[email protected]> wrote:

When inspecting a vague code inside prctl(PR_SET_MM_MEM) call (which testing the RLIMIT_DATA value to figure out if we're allowed to assign new @start_brk, @brk, @start_data, @end_data from mm_struct) it's been commited that RLIMIT_DATA in a form it's implemented now doesn't do anything useful because most of user-space libraries use mmap() syscall for dynamic memory allocations.
Linus suggested to convert RLIMIT_DATA rlimit into something suitable for anonymous memory accounting. But in this patch we go further, and the changes are bundled together as:
* keep vma counting if CONFIG_PROC_FS=n, will be used for limits
* replace mm->shared_vm with better defined mm->data_vm
* account anonymous executable areas as executable
* account file-backed growsdown/up areas as stack
* drop struct file* argument from vm_stat_account
* enforce RLIMIT_DATA for size of data areas
This way code looks cleaner: now code/stack/data classification depends only on vm_flags state:
VM_EXEC & ~VM_WRITE -> code (VmExe + VmLib in proc)
VM_GROWSUP | VM_GROWSDOWN -> stack (VmStk)
VM_WRITE & ~VM_SHARED & !stack -> data (VmData)

Quoted 7 lines

The rest (VmSize - VmData - VmStk - VmExe - VmLib) could be called "shared", but that might be strange beast like readonly-private or VM_IO area.
 - RLIMIT_AS            limits whole address space "VmSize"
 - RLIMIT_STACK         limits stack "VmStk" (but each vma individually)
 - RLIMIT_DATA          now limits "VmData"

This clashes with mm-mmapc-remove-redundant-local-variables-for-may_expand_vm.patch, below. I resolved it thusly:

bool may_expand_vm(struct mm_struct *mm, vm_flags_t flags, unsigned long npages)

{
	if (mm->total_vm + npages > rlimit(RLIMIT_AS) >> PAGE_SHIFT)
		return false;

	if ((flags & (VM_WRITE | VM_SHARED | (VM_STACK_FLAGS &
				(VM_GROWSUP | VM_GROWSDOWN)))) == VM_WRITE)
		return mm->data_vm + npages <= rlimit(RLIMIT_DATA);

	return true;
}

From: Chen Gang <[email protected]>
Subject: mm/mmap.c: remove redundant local variables for may_expand_vm()

Simplify may_expand_vm()

[[email protected]: further simplification, per Naoya Horiguchi]

Signed-off-by Chen Gang <[email protected]>

Cc Naoya Horiguchi <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 mm/mmap.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

mm/mmap.c renamed from mm/mmap.c~mm-mmapc-remove-redundant-local-variables-for-may_expand_vm +1 -8

diff -puN mm/mmap.c~mm-mmapc-remove-redundant-local-variables-for-may_expand_vm mm/mmap.c
@@ -2988,14 +2988,7 @@ out:
  */
 int may_expand_vm(struct mm_struct *mm, unsigned long npages)
 {
-	unsigned long cur = mm->total_vm;	/* pages */
-	unsigned long lim;
-
-	lim = rlimit(RLIMIT_AS) >> PAGE_SHIFT;
-
-	if (cur + npages > lim)
-		return 0;
-	return 1;
+	return mm->total_vm + npages <= rlimit(RLIMIT_AS) >> PAGE_SHIFT;
 }
 
 static int special_mapping_fault(struct vm_area_struct *vma,

Cyrill Gorcunov

author

2015-12-29 09:48 UTC #8 lore raw

Quoted 1 line · Andrew Morton

On Tue, 29 Dec 2015 00:10:15 +0300 Cyrill Gorcunov <[email protected]> wrote:

...

Quoted 6 lines

This clashes with mm-mmapc-remove-redundant-local-variables-for-may_expand_vm.patch, below. I resolved it thusly:
bool may_expand_vm(struct mm_struct *mm, vm_flags_t flags, unsigned long npages)

Quoted 10 lines

{
	if (mm->total_vm + npages > rlimit(RLIMIT_AS) >> PAGE_SHIFT)
		return false;

	if ((flags & (VM_WRITE | VM_SHARED | (VM_STACK_FLAGS &
				(VM_GROWSUP | VM_GROWSDOWN)))) == VM_WRITE)
		return mm->data_vm + npages <= rlimit(RLIMIT_DATA);

	return true;
}

Thanks, Andrew!