mm: add sys_madvise2 and MADV_NAME to name vmas

Userspace processes often have multiple allocators that each do anonymous mmaps to get memory. When examining memory usage of individual processes or systems as a whole, it is useful to be able to break down the various heaps that were allocated by each layer and examine their size, RSS, and physical memory usage.

mm/vma_name.c new file +38 -0

+/**
+ * vma_name_get
+ *
+ * Increment the refcount of an existing vma_name.  No locks are needed because
+ * the caller should already be holding a reference, so refcount >= 1.
+ */
+void vma_name_get(struct vma_name *vma_name)
+{
+	if (WARN_ON(!vma_name))
+		return;
+
+	WARN_ON(!atomic_read(&vma_name->refcount));
+
+	atomic_inc(&vma_name->refcount);
+}
+
+/**
+ * vma_name_put
+ *
+ * Decrement the refcount of an existing vma_name and free it if necessary.
+ * No locks needed, takes the cache lock if it needs to remove the vma_name from
+ * the cache.
+ */
+void vma_name_put(struct vma_name *vma_name)
+{
+	int ret;
+
+	if (WARN_ON(!vma_name))
+		return;
+
+	WARN_ON(!atomic_read(&vma_name->refcount));
+
+	/* fast path: refcount > 1, decrement and return */
+	if (atomic_add_unless(&vma_name->refcount, -1, 1))
+		return;
+
+	/* slow path: take the lock, decrement, and erase node if count is 0 */
+	write_lock(&vma_name_cache_lock);

Quoted 6 lines · Colin Cross

Userspace processes often have multiple allocators that each do anonymous mmaps to get memory. When examining memory usage of individual processes or systems as a whole, it is useful to be able to break down the various heaps that were allocated by each layer and examine their size, RSS, and physical memory usage.

What is the advantage of this? It looks like it is going to add cache line contention (atomic_inc/atomic_dec) to every vma operation especially in the envision use case of heavy vma_name sharing.

I would expect this will result in a bloated vm_area_struct and a slower mm subsystem.

Have you done any benchmarks that stress the mm subsystem?

How can adding glittler to /proc/<pid>/maps and /proc/<pid>/smaps justify putting a hand break on the linux kernel?

Quoted 6 lines · Eric W. Biederman

Colin Cross <[email protected]> writes:

What is the advantage of this? It looks like it is going to add cache line contention (atomic_inc/atomic_dec) to every vma operation especially in the envision use case of heavy vma_name sharing.

Quoted 2 lines

I would expect this will result in a bloated vm_area_struct and a slower mm subsystem.

The advantage is better tracking of the impact of various userspace allocations on the overall system. Userspace could track allocations on its own, but it cannot track things like physical memory usage or Kernel SamePage Merging per allocation.

I expect "hand break" is overstating the impact.

How can adding glittler to /proc/<pid>/maps and /proc/<pid>/smaps justify putting a hand break on the linux kernel?

Quoted 2 lines · Eric W. Biederman

How can adding glittler to /proc/<pid>/maps and /proc/<pid>/smaps justify putting a hand break on the linux kernel?

It's not just glitter, it's potentially very useful for making perf work nicely with JVM, for example, to know about JIT codegen regions and GC regions.

The implementation seems very heavy-weight though and I'm not convinced a new syscall makes sense.

Quoted 7 lines · Pekka Enberg

Quoted 2 lines · Eric W. Biederman

How can adding glittler to /proc/<pid>/maps and /proc/<pid>/smaps justify putting a hand break on the linux kernel?

It's not just glitter, it's potentially very useful for making perf work nicely with JVM, for example, to know about JIT codegen regions and GC regions.

Ah yes. The old let's make it possible to understand the performance and behavior by making the bottleneck case even slower. At least for variants of GC that use occasionally make have use of mprotect that seems to be exactly what this patch proposes.

Quoted 2 lines

The implementation seems very heavy-weight though and I'm not convinced a new syscall makes sense.

Strongly agreed. Oleg's idea of a simple integer (that can be though of as a 4 or 8 byte string) seems much more practical.

What puzzles me is what is the point? What is gained by putting this knowledge in the kernel that can not be determend from looking at how user space has allocated the memory? The entire concept feels like a layering violation. Instead of modifying the malloc in glibc or the jvm or whatever it is propsed to modify the kernel.

Even after all of the discussion I am still seeing glitter and hand breaks.

Quoted 6 lines · Colin Cross

Userspace processes often have multiple allocators that each do anonymous mmaps to get memory. When examining memory usage of individual processes or systems as a whole, it is useful to be able to break down the various heaps that were allocated by each layer and examine their size, RSS, and physical memory usage.

What is the advantage of this? It looks like it is going to add cache line contention (atomic_inc/atomic_dec) to every vma operation

especially in the envision use case of heavy vma_name sharing.

I would expect this will result in a bloated vm_area_struct and a slower mm subsystem.

Have you done any benchmarks that stress the mm subsystem?

How can adding glittler to /proc/<pid>/maps and /proc/<pid>/smaps justify putting a hand break on the linux kernel?

Eric

Quoted 45 lines

mm/vma_name.c new file +45 -0

+/**
+ * vma_name_get
+ *
+ * Increment the refcount of an existing vma_name.  No locks are needed because
+ * the caller should already be holding a reference, so refcount >= 1.
+ */
+void vma_name_get(struct vma_name *vma_name)
+{
+	if (WARN_ON(!vma_name))
+		return;
+
+	WARN_ON(!atomic_read(&vma_name->refcount));
+
+	atomic_inc(&vma_name->refcount);
+}
+
+/**
+ * vma_name_put
+ *
+ * Decrement the refcount of an existing vma_name and free it if necessary.
+ * No locks needed, takes the cache lock if it needs to remove the vma_name from
+ * the cache.
+ */
+void vma_name_put(struct vma_name *vma_name)
+{
+	int ret;
+
+	if (WARN_ON(!vma_name))
+		return;
+
+	WARN_ON(!atomic_read(&vma_name->refcount));
+
+	/* fast path: refcount > 1, decrement and return */
+	if (atomic_add_unless(&vma_name->refcount, -1, 1))
+		return;
+
+	/* slow path: take the lock, decrement, and erase node if count is 0 */
+	write_lock(&vma_name_cache_lock);
+
+	ret = atomic_dec_return(&vma_name->refcount);
+	if (ret == 0)
+		rb_erase(&vma_name->rb_node, &vma_name_cache);
+
+	write_unlock(&vma_name_cache_lock);
+

mm/vma_name.c line 88

Quoted 3 lines

mm/vma_name.c new file +3 -0

+	if (ret == 0)
+		kfree(vma_name);
+}

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

Userspace processes often have multiple allocators that each do anonymous mmaps to get memory. When examining memory usage of individual processes or systems as a whole, it is useful to be able to break down the various heaps that were allocated by each layer and examine their size, RSS, and physical memory usage.

Quoted 6 lines · Colin Cross

Userspace processes often have multiple allocators that each do anonymous mmaps to get memory. When examining memory usage of individual processes or systems as a whole, it is useful to be able to break down the various heaps that were allocated by each layer and examine their size, RSS, and physical memory usage.

So why not 'abuse' deleted files?

Quoted 8 lines · Peter Zijlstra

Quoted 6 lines · Colin Cross

Userspace processes often have multiple allocators that each do anonymous mmaps to get memory. When examining memory usage of individual processes or systems as a whole, it is useful to be able to break down the various heaps that were allocated by each layer and examine their size, RSS, and physical memory usage.

Quoted 2 lines

So why not 'abuse' deleted files?

That's effectively what ashmem does for this use case, but it has its issues when allocators ask the kernel for memory multiple times. There are two ways to implement it in userspace, either reusing the same fd or using a new fd for every allocation.

Reusing the same fd fundamentally changes the semantics of the memory.
It requires the allocator to keep a global fd and offset variable,
and extend the file and map the new region to get the kernel to merge the mappings.

This inherently ties the memory together - AFAICT the kernel will not reclaim any of the memory until either the whole file is unmapped and the fd is closed, or userspace manually calls MADV_REMOVE.

It's not immediately clear from the madvise man page what would happen after a fork if one process calls MADV_REMOVE on MAP_PRIVATE tmpfs memory, but if it really goes directly to the backing store won't those pages disappear for both processes?

The names of named anonymous vmas are shown in /proc/pid/maps as [anon:<name>]. The name of all named vmas are shown in /proc/pid/smaps in a new "Name" field that is only present for named vmas.

static long madvise_behavior(struct vm_area_struct * vma,

mm/madvise.c +17 -3

 		     struct vm_area_struct **prev,
-		     unsigned long start, unsigned long end, int behavior)
+		     unsigned long start, unsigned long end, int behavior,
+		     void *arg, size_t arg_len)
 {
 	struct mm_struct * mm = vma->vm_mm;
 	int error = 0;
 	pgoff_t pgoff;
 	unsigned long new_flags = vma->vm_flags;
+	struct vma_name *new_name = vma->vm_name;
 
 	switch (behavior) {
 	case MADV_NORMAL:

@@ -93,16 +97,28 @@ static long madvise_behavior(struct vm_area_struct * vma,
 		if (error)
 			goto out;
 		break;
+	case MADV_NAME:
+		if (arg) {
+			new_name = vma_name_get_from_str(arg, arg_len);
+			if (!new_name) {
+				error = -ENOMEM;
+				goto out;
+			}
+		} else {
+			new_name = NULL;
+		}
+		break;
 	}
 
-	if (new_flags == vma->vm_flags) {
+	if (new_flags == vma->vm_flags && new_name == vma->vm_name) {
 		*prev = vma;
 		goto out;
 	}
 
 	pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT);
 	*prev = vma_merge(mm, *prev, start, end, new_flags, vma->anon_vma,
-				vma->vm_file, pgoff, vma_policy(vma));
+				vma->vm_file, pgoff, vma_policy(vma),
+				new_name);
 	if (*prev) {
 		vma = *prev;
 		goto success;

Quoted 5 lines · Colin Cross

The names of named anonymous vmas are shown in /proc/pid/maps as [anon:<name>]. The name of all named vmas are shown in /proc/pid/smaps in a new "Name" field that is only present for named vmas.

And this is the only purpose, yes?

Quoted 46 lines

static long madvise_behavior(struct vm_area_struct * vma,
struct vm_area_struct **prev,

mm/madvise.c +17 -3

-                  unsigned long start, unsigned long end, int behavior)
+                  unsigned long start, unsigned long end, int behavior,
+                  void *arg, size_t arg_len)
 {
      struct mm_struct * mm = vma->vm_mm;
      int error = 0;
      pgoff_t pgoff;
      unsigned long new_flags = vma->vm_flags;
+     struct vma_name *new_name = vma->vm_name;
 
      switch (behavior) {
      case MADV_NORMAL:

@@ -93,16 +97,28 @@ static long madvise_behavior(struct vm_area_struct * vma,
              if (error)
                      goto out;
              break;
+     case MADV_NAME:
+             if (arg) {
+                     new_name = vma_name_get_from_str(arg, arg_len);
+                     if (!new_name) {
+                             error = -ENOMEM;
+                             goto out;
+                     }
+             } else {
+                     new_name = NULL;
+             }
+             break;
      }
 
-     if (new_flags == vma->vm_flags) {
+     if (new_flags == vma->vm_flags && new_name == vma->vm_name) {
              *prev = vma;
              goto out;
      }
 
      pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT);
      *prev = vma_merge(mm, *prev, start, end, new_flags, vma->anon_vma,
-                             vma->vm_file, pgoff, vma_policy(vma));
+                             vma->vm_file, pgoff, vma_policy(vma),
+                             new_name);
      if (*prev) {
              vma = *prev;
              goto success;

mm/madvise.c success: · line 152

Quoted 10 lines

mm/madvise.c +7 -0

@@ -127,8 +143,17 @@ success:
       * vm_flags is protected by the mmap_sem held in write mode.
       */
      vma->vm_flags = new_flags;
+     if (vma->vm_name != new_name) {
+             if (vma->vm_name)
+                     vma_name_put(vma->vm_name);
+             if (new_name)
+                     vma_name_get(new_name);
+             vma->vm_name = new_name;
+     }

So we change vma->vm_name after vma_merge(). But given that is_mergeable_vma() checks vma->vm_name with this patch, this means that we can have 2 vma's with the same ->vm_name which should be merged?

IOW. Suppose that we have vma with vm_start = START, end = START + 2 * PAGE_SIZE. Suppose that an application does

        MADV_NAME(START, PAGE_SIZE, "MY_NAME");
        MADV_NAME(START + PAGE_SIZE, PAGE_SIZE, "MY_NAME");

The 1st MADV_NAME will split this vma, the 2nd won't merge. Not that I think this is buggy, just a bit inconsistent imho.

And I guess vma_name_get(new_name) is not needed, you can simply nullify it after changing ->vm_name to avoid vma_name_put() below.

Quoted 4 lines · Oleg Nesterov

        MADV_NAME(START, PAGE_SIZE, "MY_NAME");
        MADV_NAME(START + PAGE_SIZE, PAGE_SIZE, "MY_NAME");

Quoted 2 lines

The 1st MADV_NAME will split this vma, the 2nd won't merge. Not that I think this is buggy, just a bit inconsistent imho.

My intention is that any vmas that would be merged without names would be merged if they have the same name. I copied the logic used for vm_flags, but I'll take another look.

vma_adjust() does vma_set_policy(vma, vma_policy(next)) and this is doubly wrong:

1. This leaks vma->vm_policy if it is not NULL and not equal to next->vm_policy.

This can happen if vma_merge() expands "area", not prev (case 8).

2. This sets the wrong policy if vma_merge() joins prev and area, area is the vma the caller needs to update and it still has the old policy.

Revert 1444f92c "mm: merging memory blocks resets mempolicy" which introduced these problems.

Change mbind_range() to recheck mpol_equal() after vma_merge() to fix the problem 1444f92c tried to address.

Signed-off-by Oleg Nesterov <[email protected]>

Cc <[email protected]>

---
 mm/mempolicy.c |    6 +++++-
 mm/mmap.c      |    2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

mm/mempolicy.c +0 -0

index 7431001..4baf12e 100644

Quoted 8 lines · Oleg Nesterov

vma_adjust() does vma_set_policy(vma, vma_policy(next)) and this is doubly wrong:

1. This leaks vma->vm_policy if it is not NULL and not equal to next->vm_policy.

This can happen if vma_merge() expands "area", not prev (case 8).

Quoted 6 lines

2. This sets the wrong policy if vma_merge() joins prev and area, area is the vma the caller needs to update and it still has the old policy.

Revert 1444f92c "mm: merging memory blocks resets mempolicy" which introduced these problems.

Yes, I believe 1444f92c is wrong and should be reverted.

Quoted 10 lines

Change mbind_range() to recheck mpol_equal() after vma_merge() to fix the problem 1444f92c tried to address.

Signed-off-by Oleg Nesterov <[email protected]>

Cc <[email protected]>

---
 mm/mempolicy.c |    6 +++++-
 mm/mmap.c      |    2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

mm/mempolicy.c static int mbind_range(struct mm_struct *mm, unsigned long start, · line 737

Quoted 12 lines

mm/mempolicy.c +3 -1

index 7431001..4baf12e 100644
@@ -732,7 +732,10 @@ static int mbind_range(struct mm_struct *mm, unsigned long start,
                if (prev) {
                        vma = prev;
                        next = vma->vm_next;
-                       continue;
+                       if (mpol_equal(vma_policy(vma), new_pol))
+                               continue;
+                       /* vma_merge() joined vma && vma->next, case 8 */

case 3 makes the same scenario?

Acked-by KOSAKI Motohiro <[email protected]>

I can be easily wrong, but to me vma_adjust() and its usage looks a bit overcomplicated. Perhaps it makes sense to distinguish mmapped/hole cases. mbind_range/madvise/etc need vma_join(vma, ...), not prev/anon_vma/file. Perhaps. not sure.

Quoted 1 line

case 3 makes the same scenario?

Not really, afaics. "case 3" is when vma_merge() "merges" a hole with vma, mbind_range() works with the already mmapped regions.

vma_adjust() does vma_set_policy(vma, vma_policy(next)) and this is doubly wrong:

1. This leaks vma->vm_policy if it is not NULL and not equal to next->vm_policy.

This can happen if vma_merge() expands "area", not prev (case 8).

2. This sets the wrong policy if vma_merge() joins prev and area, area is the vma the caller needs to update and it still has the old policy.

Revert 1444f92c "mm: merging memory blocks resets mempolicy" which introduced these problems.

Change mbind_range() to recheck mpol_equal() after vma_merge() to fix the problem 1444f92c tried to address.

Quoted 2 lines · On Mon, 8 Jul 2013 20:05:01 +0200 Oleg Nesterov <[email protected]> wrote:

Change mbind_range() to recheck mpol_equal() after vma_merge() to fix the problem 1444f92c tried to address.

So I assume the kernel still passes Steven's testcase from the 1444f92c changelog?

Quoted 8 lines · Oleg Nesterov

Quoted 5 lines · Colin Cross

The names of named anonymous vmas are shown in /proc/pid/maps as [anon:<name>]. The name of all named vmas are shown in /proc/pid/smaps in a new "Name" field that is only present for named vmas.

And this is the only purpose, yes?

Quoted 10 lines · Colin Cross

Quoted 8 lines · Oleg Nesterov

Quoted 5 lines · Colin Cross

The names of named anonymous vmas are shown in /proc/pid/maps as [anon:<name>]. The name of all named vmas are shown in /proc/pid/smaps in a new "Name" field that is only present for named vmas.

And this is the only purpose, yes?

The heuristics used for the thread stack annotation is not working always:

https://lkml.org/lkml/2013/6/26/256

Maybe we can get rid of the heuristic if there is an explicit interface to mark vma's?

OTOH, a new flag bit instead of a string would be enough to mark the thread stacks correctly.

Quoted 12 lines · Dave Hansen

include/linux/mm_types.h struct vm_area_struct { · line 295

Quoted 6 lines · Colin Cross

include/linux/mm_types.h +1 -0

@@ -289,6 +291,7 @@ struct vm_area_struct {
 #ifdef CONFIG_NUMA
      struct mempolicy *vm_policy;    /* NUMA policy for the VMA */
 #endif
+     struct vma_name *vm_name;
 };

You could probably get rid of the extra pointer by unioning this with vm_file. They should not ever get used together. We would, however, have to transition away from checking vma->vm_file for "is this vma file-backed"? We are out of VM_* flag space which is a bummer.

I considered trying to union with vm_file, but like you said the lack of space in vm_flags makes it hard.

It would also prevent naming file
backed mappings, which is supported by this patch set. It's not required by my primary use case, but it could be useful to have the dynamic linker identify the various elf segments.

I wonder if it
would be worth trying to make names a struct file? They would be significantly larger than a struct vma_name, but reuse all the existing refcounting code.

Quoted 8 lines

Here's one more idea: instead of having a kernel pointer, let's let userspace hand the kernel a userspace address, and the kernel will hang on to it. Userspace is responsible for keeping it valid, kind of like ARGV[]. When the kernel goes to dump out the /proc/$pid/maps fields, it can do a copy_from_user() to get the string back out. If this fails, it can just go and treat it like a non-named VMA, or could output "userspace sucks".

Quoted 4 lines

That way, the kernel isn't dealing with refcounting and allocating strings. It's got security concerns, just like /proc/$pid/cmdline since it'll let you dig around in another process's address space via /proc. But, I think they're manageable.

How do you deal with merging adjacent vmas with the same name? The whole point of the refcounted strings is to allow comparing strings for equality by comparing pointers. You could say that a named vma never gets merged, but then you might as well use separate tmpfs files, and you end up at least doubling the number of vmas in some processes I've seen.