[PATCH] proc: revert /proc/<pid>/maps [stack:TID] annotation

[PATCH] proc: revert /proc/<pid>/maps [stack:TID] annotation

[Johannes Weiner]

b764375 ("procfs: mark thread stack correctly in proc/<pid>/maps")
added [stack:TID] annotation to /proc/<pid>/maps. Finding the task of
a stack VMA requires walking the entire thread list, turning this into
quadratic behavior: a thousand threads means a thousand stacks, so the
rendering of /proc/<pid>/maps needs to look at a million threads. The
cost is not in proportion to the usefulness as described in the patch.

Drop the [stack:TID] annotation to make /proc/<pid>/maps (and
/proc/<pid>/numa_maps) usable again for higher thread counts.

The [stack] annotation inside /proc/<pid>/task/<tid>/maps is retained,
as identifying the stack VMA there is an O(1) operation.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 Documentation/filesystems/proc.txt |  9 ++----
 fs/proc/task_mmu.c                 | 66 +++++++++++++-------------------------
 fs/proc/task_nommu.c               | 48 +++++++++++----------------
 include/linux/mm.h                 |  3 +-
 mm/util.c                          | 27 +---------------
 5 files changed, 47 insertions(+), 106 deletions(-)

diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index e95aa1c..4e95796 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -356,7 +356,7 @@ address           perms offset  dev   inode      pathname
 a7cb1000-a7cb2000 ---p 00000000 00:00 0
 a7cb2000-a7eb2000 rw-p 00000000 00:00 0
 a7eb2000-a7eb3000 ---p 00000000 00:00 0
-a7eb3000-a7ed5000 rw-p 00000000 00:00 0          [stack:1001]
+a7eb3000-a7ed5000 rw-p 00000000 00:00 0
 a7ed5000-a8008000 r-xp 00000000 03:00 4222       /lib/libc.so.6
 a8008000-a800a000 r--p 00133000 03:00 4222       /lib/libc.so.6
 a800a000-a800b000 rw-p 00135000 03:00 4222       /lib/libc.so.6
@@ -388,7 +388,6 @@ is not associated with a file:
 
  [heap]                   = the heap of the program
  [stack]                  = the stack of the main process
- [stack:1001]             = the stack of the thread with tid 1001
  [vdso]                   = the "virtual dynamic shared object",
                             the kernel system call handler
 
@@ -396,10 +395,8 @@ is not associated with a file:
 
 The /proc/PID/task/TID/maps is a view of the virtual memory from the viewpoint
 of the individual tasks of a process. In this file you will see a mapping marked
-as [stack] if that task sees it as a stack. This is a key difference from the
-content of /proc/PID/maps, where you will see all mappings that are being used
-as stack by all of those tasks. Hence, for the example above, the task-level
-map, i.e. /proc/PID/task/TID/maps for thread 1001 will look like this:
+as [stack] if that task sees it as a stack. Hence, for the example above, the
+task-level map, i.e. /proc/PID/task/TID/maps for thread 1001 will look like this:
 
 08048000-08049000 r-xp 00000000 03:00 8312       /opt/test
 08049000-0804a000 rw-p 00001000 03:00 8312       /opt/test
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 65a1b6c..81aa553 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -259,23 +259,29 @@ static int do_maps_open(struct inode *inode, struct file *file,
 				sizeof(struct proc_maps_private));
 }
 
-static pid_t pid_of_stack(struct proc_maps_private *priv,
-				struct vm_area_struct *vma, bool is_pid)
+/*
+ * Indicate if the VMA is a stack for the given task; for
+ * /proc/PID/maps that is the stack of the main task.
+ */
+static int is_stack(struct proc_maps_private *priv,
+		    struct vm_area_struct *vma, int is_pid)
 {
-	struct inode *inode = priv->inode;
-	struct task_struct *task;
-	pid_t ret = 0;
+	int stack = 0;
+
+	if (is_pid) {
+		stack = vma->vm_start <= vma->vm_mm->start_stack &&
+			vma->vm_end >= vma->vm_mm->start_stack;
+	} else {
+		struct inode *inode = priv->inode;
+		struct task_struct *task;
 
-	rcu_read_lock();
-	task = pid_task(proc_pid(inode), PIDTYPE_PID);
-	if (task) {
-		task = task_of_stack(task, vma, is_pid);
+		rcu_read_lock();
+		task = pid_task(proc_pid(inode), PIDTYPE_PID);
 		if (task)
-			ret = task_pid_nr_ns(task, inode->i_sb->s_fs_info);
+			stack = vma_is_stack_for_task(vma, task);
+		rcu_read_unlock();
 	}
-	rcu_read_unlock();
-
-	return ret;
+	return stack;
 }
 
 static void
@@ -335,8 +341,6 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
 
 	name = arch_vma_name(vma);
 	if (!name) {
-		pid_t tid;
-
 		if (!mm) {
 			name = "[vdso]";
 			goto done;
@@ -348,21 +352,8 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
 			goto done;
 		}
 
-		tid = pid_of_stack(priv, vma, is_pid);
-		if (tid != 0) {
-			/*
-			 * Thread stack in /proc/PID/task/TID/maps or
-			 * the main process stack.
-			 */
-			if (!is_pid || (vma->vm_start <= mm->start_stack &&
-			    vma->vm_end >= mm->start_stack)) {
-				name = "[stack]";
-			} else {
-				/* Thread stack in /proc/PID/maps */
-				seq_pad(m, ' ');
-				seq_printf(m, "[stack:%d]", tid);
-			}
-		}
+		if (is_stack(priv, vma, is_pid))
+			name = "[stack]";
 	}
 
 done:
@@ -1613,19 +1604,8 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
 		seq_file_path(m, file, "\n\t= ");
 	} else if (vma->vm_start <= mm->brk && vma->vm_end >= mm->start_brk) {
 		seq_puts(m, " heap");
-	} else {
-		pid_t tid = pid_of_stack(proc_priv, vma, is_pid);
-		if (tid != 0) {
-			/*
-			 * Thread stack in /proc/PID/task/TID/maps or
-			 * the main process stack.
-			 */
-			if (!is_pid || (vma->vm_start <= mm->start_stack &&
-			    vma->vm_end >= mm->start_stack))
-				seq_puts(m, " stack");
-			else
-				seq_printf(m, " stack:%d", tid);
-		}
+	} else if (is_stack(proc_priv, vma, is_pid)) {
+		seq_puts(m, " stack");
 	}
 
 	if (is_vm_hugetlb_page(vma))
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index e0d64c9..60ab72e 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -123,23 +123,25 @@ unsigned long task_statm(struct mm_struct *mm,
 	return size;
 }
 
-static pid_t pid_of_stack(struct proc_maps_private *priv,
-				struct vm_area_struct *vma, bool is_pid)
+static int is_stack(struct proc_maps_private *priv,
+		    struct vm_area_struct *vma, int is_pid)
 {
-	struct inode *inode = priv->inode;
-	struct task_struct *task;
-	pid_t ret = 0;
-
-	rcu_read_lock();
-	task = pid_task(proc_pid(inode), PIDTYPE_PID);
-	if (task) {
-		task = task_of_stack(task, vma, is_pid);
+	int stack = 0;
+
+	if (is_pid) {
+		stack = vma->vm_start <= mm->start_stack &&
+			vma->vm_end >= mm->start_stack;
+	} else {
+		struct inode *inode = priv->inode;
+		struct task_struct *task;
+
+		rcu_read_lock();
+		task = pid_task(proc_pid(inode), PIDTYPE_PID);
 		if (task)
-			ret = task_pid_nr_ns(task, inode->i_sb->s_fs_info);
+			stack = vma_is_stack_for_task(vma, task);
+		rcu_read_unlock();
 	}
-	rcu_read_unlock();
-
-	return ret;
+	return stack;
 }
 
 /*
@@ -181,21 +183,9 @@ static int nommu_vma_show(struct seq_file *m, struct vm_area_struct *vma,
 	if (file) {
 		seq_pad(m, ' ');
 		seq_file_path(m, file, "");
-	} else if (mm) {
-		pid_t tid = pid_of_stack(priv, vma, is_pid);
-
-		if (tid != 0) {
-			seq_pad(m, ' ');
-			/*
-			 * Thread stack in /proc/PID/task/TID/maps or
-			 * the main process stack.
-			 */
-			if (!is_pid || (vma->vm_start <= mm->start_stack &&
-			    vma->vm_end >= mm->start_stack))
-				seq_printf(m, "[stack]");
-			else
-				seq_printf(m, "[stack:%d]", tid);
-		}
+	} else if (mm && is_stack(priv, vma, is_pid)) {
+		seq_pad(m, ' ');
+		seq_printf(m, "[stack]");
 	}
 
 	seq_putc(m, '\n');
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 7286d5b..0cbed33 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1343,8 +1343,7 @@ static inline int stack_guard_page_end(struct vm_area_struct *vma,
 		!vma_growsup(vma->vm_next, addr);
 }
 
-extern struct task_struct *task_of_stack(struct task_struct *task,
-				struct vm_area_struct *vma, bool in_group);
+int vma_is_stack_for_task(struct vm_area_struct *vma, struct task_struct *t);
 
 extern unsigned long move_page_tables(struct vm_area_struct *vma,
 		unsigned long old_addr, struct vm_area_struct *new_vma,
diff --git a/mm/util.c b/mm/util.c
index bafd4c5..4f841e5 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -260,36 +260,11 @@ void __vma_link_list(struct mm_struct *mm, struct vm_area_struct *vma,
 }
 
 /* Check if the vma is being used as a stack by this task */
-static int vm_is_stack_for_task(struct task_struct *t,
-				struct vm_area_struct *vma)
+int vma_is_stack_for_task(struct vm_area_struct *vma, struct task_struct *t)
 {
 	return (vma->vm_start <= KSTK_ESP(t) && vma->vm_end >= KSTK_ESP(t));
 }
 
-/*
- * Check if the vma is being used as a stack.
- * If is_group is non-zero, check in the entire thread group or else
- * just check in the current task. Returns the task_struct of the task
- * that the vma is stack for. Must be called under rcu_read_lock().
- */
-struct task_struct *task_of_stack(struct task_struct *task,
-				struct vm_area_struct *vma, bool in_group)
-{
-	if (vm_is_stack_for_task(task, vma))
-		return task;
-
-	if (in_group) {
-		struct task_struct *t;
-
-		for_each_thread(task, t) {
-			if (vm_is_stack_for_task(t, vma))
-				return t;
-		}
-	}
-
-	return NULL;
-}
-
 #if defined(CONFIG_MMU) && !defined(HAVE_ARCH_PICK_MMAP_LAYOUT)
 void arch_pick_mmap_layout(struct mm_struct *mm)
 {
-- 
2.7.0

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

Re: [PATCH] proc: revert /proc/<pid>/maps [stack:TID] annotation

[Andrew Morton]

On Tue, 19 Jan 2016 13:02:39 -0500 Johannes Weiner <hannes@cmpxchg.org> wrote:

> b764375 ("procfs: mark thread stack correctly in proc/<pid>/maps")
> added [stack:TID] annotation to /proc/<pid>/maps. Finding the task of
> a stack VMA requires walking the entire thread list, turning this into
> quadratic behavior: a thousand threads means a thousand stacks, so the
> rendering of /proc/<pid>/maps needs to look at a million threads. The
> cost is not in proportion to the usefulness as described in the patch.
> 
> Drop the [stack:TID] annotation to make /proc/<pid>/maps (and
> /proc/<pid>/numa_maps) usable again for higher thread counts.
> 
> The [stack] annotation inside /proc/<pid>/task/<tid>/maps is retained,
> as identifying the stack VMA there is an O(1) operation.

Four years ago, ouch.

Any thoughts on the obvious back-compatibility concerns?  ie, why did
Siddhesh implement this in the first place?  My bad for not ensuring
that the changelog told us this.

https://lkml.org/lkml/2012/1/14/25 has more info: 

: Memory mmaped by glibc for a thread stack currently shows up as a
: simple anonymous map, which makes it difficult to differentiate between
: memory usage of the thread on stack and other dynamic allocation. 
: Since glibc already uses MAP_STACK to request this mapping, the
: attached patch uses this flag to add additional VM_STACK_FLAGS to the
: resulting vma so that the mapping is treated as a stack and not any
: regular anonymous mapping.  Also, one may use vm_flags to decide if a
: vma is a stack.

But even that doesn't really tell us what the actual *value* of the
patch is to end-users.


I note that this patch is a partial revert - the smaps and numa_maps
parts of b764375 remain in place.  What's up with that?

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

Re: [PATCH] proc: revert /proc/<pid>/maps [stack:TID] annotation

[Kirill A. Shutemov]

On Tue, Jan 19, 2016 at 02:14:30PM -0800, Andrew Morton wrote:
> On Tue, 19 Jan 2016 13:02:39 -0500 Johannes Weiner <hannes@cmpxchg.org> wrote:
> 
> > b764375 ("procfs: mark thread stack correctly in proc/<pid>/maps")
> > added [stack:TID] annotation to /proc/<pid>/maps. Finding the task of
> > a stack VMA requires walking the entire thread list, turning this into
> > quadratic behavior: a thousand threads means a thousand stacks, so the
> > rendering of /proc/<pid>/maps needs to look at a million threads. The
> > cost is not in proportion to the usefulness as described in the patch.
> > 
> > Drop the [stack:TID] annotation to make /proc/<pid>/maps (and
> > /proc/<pid>/numa_maps) usable again for higher thread counts.
> > 
> > The [stack] annotation inside /proc/<pid>/task/<tid>/maps is retained,
> > as identifying the stack VMA there is an O(1) operation.
> 
> Four years ago, ouch.
> 
> Any thoughts on the obvious back-compatibility concerns?  ie, why did
> Siddhesh implement this in the first place?  My bad for not ensuring
> that the changelog told us this.
> 
> https://lkml.org/lkml/2012/1/14/25 has more info: 
> 
> : Memory mmaped by glibc for a thread stack currently shows up as a
> : simple anonymous map, which makes it difficult to differentiate between
> : memory usage of the thread on stack and other dynamic allocation. 
> : Since glibc already uses MAP_STACK to request this mapping, the
> : attached patch uses this flag to add additional VM_STACK_FLAGS to the
> : resulting vma so that the mapping is treated as a stack and not any
> : regular anonymous mapping.  Also, one may use vm_flags to decide if a
> : vma is a stack.
> 
> But even that doesn't really tell us what the actual *value* of the
> patch is to end-users.

I doubt it can be very useful as it's unreliable: if two stacks are
allocated end-to-end (which is not good idea, but still) it can only
report [stack:XXX] for the first one as they are merged into one VMA.
Any other anon VMA merged with the stack will be also claimed as stack,
which is not always correct.

I think report the VMA as anon is the best we can know about it,
everything else just rather expensive guesses.

> I note that this patch is a partial revert - the smaps and numa_maps
> parts of b764375 remain in place.  What's up with that?
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
 Kirill A. Shutemov

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

Re: [PATCH] proc: revert /proc/<pid>/maps [stack:TID] annotation

[Johannes Weiner]

On Tue, Jan 19, 2016 at 02:14:30PM -0800, Andrew Morton wrote:
> On Tue, 19 Jan 2016 13:02:39 -0500 Johannes Weiner <hannes@cmpxchg.org> wrote:
> 
> > b764375 ("procfs: mark thread stack correctly in proc/<pid>/maps")
> > added [stack:TID] annotation to /proc/<pid>/maps. Finding the task of
> > a stack VMA requires walking the entire thread list, turning this into
> > quadratic behavior: a thousand threads means a thousand stacks, so the
> > rendering of /proc/<pid>/maps needs to look at a million threads. The
> > cost is not in proportion to the usefulness as described in the patch.
> > 
> > Drop the [stack:TID] annotation to make /proc/<pid>/maps (and
> > /proc/<pid>/numa_maps) usable again for higher thread counts.
> > 
> > The [stack] annotation inside /proc/<pid>/task/<tid>/maps is retained,
> > as identifying the stack VMA there is an O(1) operation.
> 
> Four years ago, ouch.
> 
> Any thoughts on the obvious back-compatibility concerns?  ie, why did
> Siddhesh implement this in the first place?  My bad for not ensuring
> that the changelog told us this.

I thought about storing the TID of the thread using the VMA as the
stack directly inside vm_area_struct; maybe using vm_private_data?
However, that's a bit of work and ugliness that I wouldn't want to
commit to until we know that people ended up using this in practice.

> I note that this patch is a partial revert - the smaps and numa_maps
> parts of b764375 remain in place.  What's up with that?

I left the stack annotations in the thread-specific files because that
sounds useful and is cheap enough - we only have to test the vma range
against that thread's stack pointer. The last changelog paragraph says
that for maps, I'll update it to include smaps and numa_maps.

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

Re: [PATCH] proc: revert /proc/<pid>/maps [stack:TID] annotation

[Siddhesh Poyarekar]

On 20 January 2016 at 03:44, Andrew Morton <akpm@linux-foundation.org> wrote:
> Any thoughts on the obvious back-compatibility concerns?  ie, why did
> Siddhesh implement this in the first place?  My bad for not ensuring
> that the changelog told us this.
>
> https://lkml.org/lkml/2012/1/14/25 has more info:
>
> : Memory mmaped by glibc for a thread stack currently shows up as a
> : simple anonymous map, which makes it difficult to differentiate between
> : memory usage of the thread on stack and other dynamic allocation.
> : Since glibc already uses MAP_STACK to request this mapping, the
> : attached patch uses this flag to add additional VM_STACK_FLAGS to the
> : resulting vma so that the mapping is treated as a stack and not any
> : regular anonymous mapping.  Also, one may use vm_flags to decide if a
> : vma is a stack.
>
> But even that doesn't really tell us what the actual *value* of the
> patch is to end-users.

The end users needed a way to identify thread stacks programmatically
and there wasn't a way to do that.  I'm afraid I no longer remember
(or have access to the resources that would aid my memory since I
changed employers) the details of their requirement.  However, I did
do this on my own time because I thought it was an interesting project
for me and nobody really gave any feedback then as to its utility, so
as far as I am concerned you could roll back the main thread maps
information since the information is available in the thread-specific
files.

Siddhesh
-- 
http://siddhesh.in

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

Re: [PATCH] proc: revert /proc/<pid>/maps [stack:TID] annotation

[Siddhesh Poyarekar]

On 20 January 2016 at 05:00, Kirill A. Shutemov <kirill@shutemov.name> wrote:
> I doubt it can be very useful as it's unreliable: if two stacks are
> allocated end-to-end (which is not good idea, but still) it can only
> report [stack:XXX] for the first one as they are merged into one VMA.
> Any other anon VMA merged with the stack will be also claimed as stack,
> which is not always correct.

It is quite uncommon because you will always have an intervening guard
page that separates the two stack vmas.  To have the vmas merge, you
will have to disable guard pages which is an even worse idea.

A more relevant argument about its unreliability is context changes
due to makecontext/setcontext, which could momentarily show the heap
or some other arbitrary vma as a stack.

Siddhesh
-- 
http://siddhesh.in

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

Re: [PATCH] proc: revert /proc/<pid>/maps [stack:TID] annotation

[Andrew Morton]

On Wed, 20 Jan 2016 08:47:39 +0530 Siddhesh Poyarekar <siddhesh.poyarekar@gmail.com> wrote:

> On 20 January 2016 at 03:44, Andrew Morton <akpm@linux-foundation.org> wrote:
> > Any thoughts on the obvious back-compatibility concerns?  ie, why did
> > Siddhesh implement this in the first place?  My bad for not ensuring
> > that the changelog told us this.
> >
> > https://lkml.org/lkml/2012/1/14/25 has more info:
> >
> > : Memory mmaped by glibc for a thread stack currently shows up as a
> > : simple anonymous map, which makes it difficult to differentiate between
> > : memory usage of the thread on stack and other dynamic allocation.
> > : Since glibc already uses MAP_STACK to request this mapping, the
> > : attached patch uses this flag to add additional VM_STACK_FLAGS to the
> > : resulting vma so that the mapping is treated as a stack and not any
> > : regular anonymous mapping.  Also, one may use vm_flags to decide if a
> > : vma is a stack.
> >
> > But even that doesn't really tell us what the actual *value* of the
> > patch is to end-users.
> 
> The end users needed a way to identify thread stacks programmatically
> and there wasn't a way to do that.  I'm afraid I no longer remember
> (or have access to the resources that would aid my memory since I
> changed employers) the details of their requirement.  However, I did
> do this on my own time because I thought it was an interesting project
> for me and nobody really gave any feedback then as to its utility, so
> as far as I am concerned you could roll back the main thread maps
> information since the information is available in the thread-specific
> files.

OK, thanks.  I was thinking of queueing this for 4.6 to let it bake in
-next for a cycle, but quadratic performance is bad and nobody will
test such an obscure feature in -next so maybe I'll jam it into 4.5 and we
wait and see.

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

[PATCH] proc: add missing 'mm' variable in nommu is_stack()

[Arnd Bergmann]

A recent revert left an incomplete function in fs/proc/task_nommu.c,
causing a build error for any NOMMU configuration with procfs:

fs/proc/task_nommu.c:132:28: error: 'mm' undeclared (first use in this function)
   stack = vma->vm_start <= mm->start_stack &&

Evidently, there is just a missing variable that is available
in the calling function but not inside of is_stack(). This
adds it.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: e87d4fd02f40 ("proc: revert /proc/<pid>/maps [stack:TID] annotation")
---
This came up today on my ARM randconfig builds with linux-next.
I did not run the kernel to see if the code actually works, but
it seems straightforward enough.

diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 60ab72e38f78..faacb0c0d857 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -126,6 +126,7 @@ unsigned long task_statm(struct mm_struct *mm,
 static int is_stack(struct proc_maps_private *priv,
 		    struct vm_area_struct *vma, int is_pid)
 {
+	struct mm_struct *mm = vma->vm_mm;
 	int stack = 0;
 
 	if (is_pid) {

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

Re: [PATCH] proc: revert /proc/<pid>/maps [stack:TID] annotation

[Colin Cross]

On Tue, Jan 19, 2016 at 3:30 PM, Kirill A. Shutemov
<kirill@shutemov.name> wrote:
> On Tue, Jan 19, 2016 at 02:14:30PM -0800, Andrew Morton wrote:
>> On Tue, 19 Jan 2016 13:02:39 -0500 Johannes Weiner <hannes@cmpxchg.org> wrote:
>>
>> > b764375 ("procfs: mark thread stack correctly in proc/<pid>/maps")
>> > added [stack:TID] annotation to /proc/<pid>/maps. Finding the task of
>> > a stack VMA requires walking the entire thread list, turning this into
>> > quadratic behavior: a thousand threads means a thousand stacks, so the
>> > rendering of /proc/<pid>/maps needs to look at a million threads. The
>> > cost is not in proportion to the usefulness as described in the patch.
>> >
>> > Drop the [stack:TID] annotation to make /proc/<pid>/maps (and
>> > /proc/<pid>/numa_maps) usable again for higher thread counts.
>> >
>> > The [stack] annotation inside /proc/<pid>/task/<tid>/maps is retained,
>> > as identifying the stack VMA there is an O(1) operation.
>>
>> Four years ago, ouch.
>>
>> Any thoughts on the obvious back-compatibility concerns?  ie, why did
>> Siddhesh implement this in the first place?  My bad for not ensuring
>> that the changelog told us this.
>>
>> https://lkml.org/lkml/2012/1/14/25 has more info:
>>
>> : Memory mmaped by glibc for a thread stack currently shows up as a
>> : simple anonymous map, which makes it difficult to differentiate between
>> : memory usage of the thread on stack and other dynamic allocation.
>> : Since glibc already uses MAP_STACK to request this mapping, the
>> : attached patch uses this flag to add additional VM_STACK_FLAGS to the
>> : resulting vma so that the mapping is treated as a stack and not any
>> : regular anonymous mapping.  Also, one may use vm_flags to decide if a
>> : vma is a stack.
>>
>> But even that doesn't really tell us what the actual *value* of the
>> patch is to end-users.
>
> I doubt it can be very useful as it's unreliable: if two stacks are
> allocated end-to-end (which is not good idea, but still) it can only
> report [stack:XXX] for the first one as they are merged into one VMA.
> Any other anon VMA merged with the stack will be also claimed as stack,
> which is not always correct.
>
> I think report the VMA as anon is the best we can know about it,
> everything else just rather expensive guesses.

An alternative to guessing is the anonymous VMA naming patch used on
Android, https://lkml.org/lkml/2013/10/30/518.  It allows userspace to
name anonymous memory however it wishes, and prevents vma merging
adjacent regions with different names.  Android uses it to label
native heap memory, but it would work well for stacks too.

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

Re: [PATCH] proc: revert /proc/<pid>/maps [stack:TID] annotation

[Kirill A. Shutemov]

On Mon, Jan 25, 2016 at 01:30:00PM -0800, Colin Cross wrote:
> On Tue, Jan 19, 2016 at 3:30 PM, Kirill A. Shutemov
> <kirill@shutemov.name> wrote:
> > On Tue, Jan 19, 2016 at 02:14:30PM -0800, Andrew Morton wrote:
> >> On Tue, 19 Jan 2016 13:02:39 -0500 Johannes Weiner <hannes@cmpxchg.org> wrote:
> >>
> >> > b764375 ("procfs: mark thread stack correctly in proc/<pid>/maps")
> >> > added [stack:TID] annotation to /proc/<pid>/maps. Finding the task of
> >> > a stack VMA requires walking the entire thread list, turning this into
> >> > quadratic behavior: a thousand threads means a thousand stacks, so the
> >> > rendering of /proc/<pid>/maps needs to look at a million threads. The
> >> > cost is not in proportion to the usefulness as described in the patch.
> >> >
> >> > Drop the [stack:TID] annotation to make /proc/<pid>/maps (and
> >> > /proc/<pid>/numa_maps) usable again for higher thread counts.
> >> >
> >> > The [stack] annotation inside /proc/<pid>/task/<tid>/maps is retained,
> >> > as identifying the stack VMA there is an O(1) operation.
> >>
> >> Four years ago, ouch.
> >>
> >> Any thoughts on the obvious back-compatibility concerns?  ie, why did
> >> Siddhesh implement this in the first place?  My bad for not ensuring
> >> that the changelog told us this.
> >>
> >> https://lkml.org/lkml/2012/1/14/25 has more info:
> >>
> >> : Memory mmaped by glibc for a thread stack currently shows up as a
> >> : simple anonymous map, which makes it difficult to differentiate between
> >> : memory usage of the thread on stack and other dynamic allocation.
> >> : Since glibc already uses MAP_STACK to request this mapping, the
> >> : attached patch uses this flag to add additional VM_STACK_FLAGS to the
> >> : resulting vma so that the mapping is treated as a stack and not any
> >> : regular anonymous mapping.  Also, one may use vm_flags to decide if a
> >> : vma is a stack.
> >>
> >> But even that doesn't really tell us what the actual *value* of the
> >> patch is to end-users.
> >
> > I doubt it can be very useful as it's unreliable: if two stacks are
> > allocated end-to-end (which is not good idea, but still) it can only
> > report [stack:XXX] for the first one as they are merged into one VMA.
> > Any other anon VMA merged with the stack will be also claimed as stack,
> > which is not always correct.
> >
> > I think report the VMA as anon is the best we can know about it,
> > everything else just rather expensive guesses.
> 
> An alternative to guessing is the anonymous VMA naming patch used on
> Android, https://lkml.org/lkml/2013/10/30/518.  It allows userspace to
> name anonymous memory however it wishes, and prevents vma merging
> adjacent regions with different names.  Android uses it to label
> native heap memory, but it would work well for stacks too.

I don't think preventing vma merging is fair price for the feature: you
would pay extra in every find_vma() (meaning all page faults).

I think it would be nice to have a way to store this kind of sideband info
without impacting critical code path.

One other use case I see for such sideband info is storing hits from
MADV_HUGEPAGE/MADV_NOHUGEPAGE: need to split vma just for these hints is
unfortunate.

-- 
 Kirill A. Shutemov

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

Re: [PATCH] proc: revert /proc/<pid>/maps [stack:TID] annotation

[Colin Cross]

On Mon, Jan 25, 2016 at 3:14 PM, Kirill A. Shutemov
<kirill@shutemov.name> wrote:
> On Mon, Jan 25, 2016 at 01:30:00PM -0800, Colin Cross wrote:
>> On Tue, Jan 19, 2016 at 3:30 PM, Kirill A. Shutemov
>> <kirill@shutemov.name> wrote:
>> > On Tue, Jan 19, 2016 at 02:14:30PM -0800, Andrew Morton wrote:
>> >> On Tue, 19 Jan 2016 13:02:39 -0500 Johannes Weiner <hannes@cmpxchg.org> wrote:
>> >>
>> >> > b764375 ("procfs: mark thread stack correctly in proc/<pid>/maps")
>> >> > added [stack:TID] annotation to /proc/<pid>/maps. Finding the task of
>> >> > a stack VMA requires walking the entire thread list, turning this into
>> >> > quadratic behavior: a thousand threads means a thousand stacks, so the
>> >> > rendering of /proc/<pid>/maps needs to look at a million threads. The
>> >> > cost is not in proportion to the usefulness as described in the patch.
>> >> >
>> >> > Drop the [stack:TID] annotation to make /proc/<pid>/maps (and
>> >> > /proc/<pid>/numa_maps) usable again for higher thread counts.
>> >> >
>> >> > The [stack] annotation inside /proc/<pid>/task/<tid>/maps is retained,
>> >> > as identifying the stack VMA there is an O(1) operation.
>> >>
>> >> Four years ago, ouch.
>> >>
>> >> Any thoughts on the obvious back-compatibility concerns?  ie, why did
>> >> Siddhesh implement this in the first place?  My bad for not ensuring
>> >> that the changelog told us this.
>> >>
>> >> https://lkml.org/lkml/2012/1/14/25 has more info:
>> >>
>> >> : Memory mmaped by glibc for a thread stack currently shows up as a
>> >> : simple anonymous map, which makes it difficult to differentiate between
>> >> : memory usage of the thread on stack and other dynamic allocation.
>> >> : Since glibc already uses MAP_STACK to request this mapping, the
>> >> : attached patch uses this flag to add additional VM_STACK_FLAGS to the
>> >> : resulting vma so that the mapping is treated as a stack and not any
>> >> : regular anonymous mapping.  Also, one may use vm_flags to decide if a
>> >> : vma is a stack.
>> >>
>> >> But even that doesn't really tell us what the actual *value* of the
>> >> patch is to end-users.
>> >
>> > I doubt it can be very useful as it's unreliable: if two stacks are
>> > allocated end-to-end (which is not good idea, but still) it can only
>> > report [stack:XXX] for the first one as they are merged into one VMA.
>> > Any other anon VMA merged with the stack will be also claimed as stack,
>> > which is not always correct.
>> >
>> > I think report the VMA as anon is the best we can know about it,
>> > everything else just rather expensive guesses.
>>
>> An alternative to guessing is the anonymous VMA naming patch used on
>> Android, https://lkml.org/lkml/2013/10/30/518.  It allows userspace to
>> name anonymous memory however it wishes, and prevents vma merging
>> adjacent regions with different names.  Android uses it to label
>> native heap memory, but it would work well for stacks too.
>
> I don't think preventing vma merging is fair price for the feature: you
> would pay extra in every find_vma() (meaning all page faults).
>
> I think it would be nice to have a way to store this kind of sideband info
> without impacting critical code path.
>
> One other use case I see for such sideband info is storing hits from
> MADV_HUGEPAGE/MADV_NOHUGEPAGE: need to split vma just for these hints is
> unfortunate.

In practice we don't see many extra VMAs from naming; alignment
requirements, guard pages, and permissions differences are usually
enough to keep adjacent anonymous VMAs from merging.  Here's an
example from a process on Android:
7f9086c000-7f9086d000 rw-p 00006000 fd:00 1495
  /system/lib64/libhardware_legacy.so
7f9086d000-7f9086e000 rw-p 00000000 00:00 0
7f9086e000-7f9086f000 rw-p 00000000 00:00 0
  [anon:linker_alloc]
7f90875000-7f90876000 r--p 00000000 00:00 0
  [anon:linker_alloc]
7f9087c000-7f9087d000 r--p 00000000 00:00 0
  [anon:linker_alloc]
7f90901000-7f90902000 ---p 00000000 00:00 0
  [anon:thread stack guard page]
7f90902000-7f90a00000 rw-p 00000000 00:00 0
  [stack:410]
7f90a00000-7f90c00000 rw-p 00000000 00:00 0
  [anon:libc_malloc]
7f90c02000-7f90c03000 ---p 00000000 00:00 0
  [anon:thread stack guard page]
7f90c03000-7f90d01000 rw-p 00000000 00:00 0
  [stack:409]
7f90d01000-7f90d02000 ---p 00000000 00:00 0
  [anon:thread stack guard page]
7f90d02000-7f90e00000 rw-p 00000000 00:00 0
  [stack:408]
7f90e00000-7f91200000 rw-p 00000000 00:00 0
  [anon:libc_malloc]
7f91206000-7f91207000 r--p 00000000 00:00 0
  [anon:linker_alloc]
7f91237000-7f91238000 ---p 00000000 00:00 0
  [anon:thread signal stack guard page]
7f91238000-7f9123c000 rw-p 00000000 00:00 0
  [anon:thread signal stack]
7f9123c000-7f9123d000 ---p 00000000 00:00 0
  [anon:thread signal stack guard page]
7f9123d000-7f91241000 rw-p 00000000 00:00 0
  [anon:thread signal stack]
7f91246000-7f91247000 ---p 00000000 00:00 0
  [anon:thread signal stack guard page]
7f91247000-7f9124b000 rw-p 00000000 00:00 0
  [anon:thread signal stack]
7f9124b000-7f9124c000 ---p 00000000 00:00 0
  [anon:thread signal stack guard page]
7f9124c000-7f91250000 rw-p 00000000 00:00 0
  [anon:thread signal stack]

I only see 2 extra VMAs here, the "[stack:410]" and "[stack:408]"
regions would have been merged with the following "[anon:libc_malloc]"
regions.

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

Re: [PATCH] proc: revert /proc/<pid>/maps [stack:TID] annotation

[Kirill A. Shutemov]

On Mon, Jan 25, 2016 at 03:53:01PM -0800, Colin Cross wrote:
> On Mon, Jan 25, 2016 at 3:14 PM, Kirill A. Shutemov
> <kirill@shutemov.name> wrote:
> > On Mon, Jan 25, 2016 at 01:30:00PM -0800, Colin Cross wrote:
> >> On Tue, Jan 19, 2016 at 3:30 PM, Kirill A. Shutemov
> >> <kirill@shutemov.name> wrote:
> >> > On Tue, Jan 19, 2016 at 02:14:30PM -0800, Andrew Morton wrote:
> >> >> On Tue, 19 Jan 2016 13:02:39 -0500 Johannes Weiner <hannes@cmpxchg.org> wrote:
> >> >>
> >> >> > b764375 ("procfs: mark thread stack correctly in proc/<pid>/maps")
> >> >> > added [stack:TID] annotation to /proc/<pid>/maps. Finding the task of
> >> >> > a stack VMA requires walking the entire thread list, turning this into
> >> >> > quadratic behavior: a thousand threads means a thousand stacks, so the
> >> >> > rendering of /proc/<pid>/maps needs to look at a million threads. The
> >> >> > cost is not in proportion to the usefulness as described in the patch.
> >> >> >
> >> >> > Drop the [stack:TID] annotation to make /proc/<pid>/maps (and
> >> >> > /proc/<pid>/numa_maps) usable again for higher thread counts.
> >> >> >
> >> >> > The [stack] annotation inside /proc/<pid>/task/<tid>/maps is retained,
> >> >> > as identifying the stack VMA there is an O(1) operation.
> >> >>
> >> >> Four years ago, ouch.
> >> >>
> >> >> Any thoughts on the obvious back-compatibility concerns?  ie, why did
> >> >> Siddhesh implement this in the first place?  My bad for not ensuring
> >> >> that the changelog told us this.
> >> >>
> >> >> https://lkml.org/lkml/2012/1/14/25 has more info:
> >> >>
> >> >> : Memory mmaped by glibc for a thread stack currently shows up as a
> >> >> : simple anonymous map, which makes it difficult to differentiate between
> >> >> : memory usage of the thread on stack and other dynamic allocation.
> >> >> : Since glibc already uses MAP_STACK to request this mapping, the
> >> >> : attached patch uses this flag to add additional VM_STACK_FLAGS to the
> >> >> : resulting vma so that the mapping is treated as a stack and not any
> >> >> : regular anonymous mapping.  Also, one may use vm_flags to decide if a
> >> >> : vma is a stack.
> >> >>
> >> >> But even that doesn't really tell us what the actual *value* of the
> >> >> patch is to end-users.
> >> >
> >> > I doubt it can be very useful as it's unreliable: if two stacks are
> >> > allocated end-to-end (which is not good idea, but still) it can only
> >> > report [stack:XXX] for the first one as they are merged into one VMA.
> >> > Any other anon VMA merged with the stack will be also claimed as stack,
> >> > which is not always correct.
> >> >
> >> > I think report the VMA as anon is the best we can know about it,
> >> > everything else just rather expensive guesses.
> >>
> >> An alternative to guessing is the anonymous VMA naming patch used on
> >> Android, https://lkml.org/lkml/2013/10/30/518.  It allows userspace to
> >> name anonymous memory however it wishes, and prevents vma merging
> >> adjacent regions with different names.  Android uses it to label
> >> native heap memory, but it would work well for stacks too.
> >
> > I don't think preventing vma merging is fair price for the feature: you
> > would pay extra in every find_vma() (meaning all page faults).
> >
> > I think it would be nice to have a way to store this kind of sideband info
> > without impacting critical code path.
> >
> > One other use case I see for such sideband info is storing hits from
> > MADV_HUGEPAGE/MADV_NOHUGEPAGE: need to split vma just for these hints is
> > unfortunate.
> 
> In practice we don't see many extra VMAs from naming; alignment
> requirements, guard pages, and permissions differences are usually
> enough to keep adjacent anonymous VMAs from merging.  Here's an
> example from a process on Android:
> 7f9086c000-7f9086d000 rw-p 00006000 fd:00 1495
>   /system/lib64/libhardware_legacy.so
> 7f9086d000-7f9086e000 rw-p 00000000 00:00 0
> 7f9086e000-7f9086f000 rw-p 00000000 00:00 0
>   [anon:linker_alloc]
> 7f90875000-7f90876000 r--p 00000000 00:00 0
>   [anon:linker_alloc]
> 7f9087c000-7f9087d000 r--p 00000000 00:00 0
>   [anon:linker_alloc]
> 7f90901000-7f90902000 ---p 00000000 00:00 0
>   [anon:thread stack guard page]
> 7f90902000-7f90a00000 rw-p 00000000 00:00 0
>   [stack:410]
> 7f90a00000-7f90c00000 rw-p 00000000 00:00 0
>   [anon:libc_malloc]
> 7f90c02000-7f90c03000 ---p 00000000 00:00 0
>   [anon:thread stack guard page]
> 7f90c03000-7f90d01000 rw-p 00000000 00:00 0
>   [stack:409]
> 7f90d01000-7f90d02000 ---p 00000000 00:00 0
>   [anon:thread stack guard page]
> 7f90d02000-7f90e00000 rw-p 00000000 00:00 0
>   [stack:408]
> 7f90e00000-7f91200000 rw-p 00000000 00:00 0
>   [anon:libc_malloc]
> 7f91206000-7f91207000 r--p 00000000 00:00 0
>   [anon:linker_alloc]
> 7f91237000-7f91238000 ---p 00000000 00:00 0
>   [anon:thread signal stack guard page]
> 7f91238000-7f9123c000 rw-p 00000000 00:00 0
>   [anon:thread signal stack]
> 7f9123c000-7f9123d000 ---p 00000000 00:00 0
>   [anon:thread signal stack guard page]
> 7f9123d000-7f91241000 rw-p 00000000 00:00 0
>   [anon:thread signal stack]
> 7f91246000-7f91247000 ---p 00000000 00:00 0
>   [anon:thread signal stack guard page]
> 7f91247000-7f9124b000 rw-p 00000000 00:00 0
>   [anon:thread signal stack]
> 7f9124b000-7f9124c000 ---p 00000000 00:00 0
>   [anon:thread signal stack guard page]
> 7f9124c000-7f91250000 rw-p 00000000 00:00 0
>   [anon:thread signal stack]
> 
> I only see 2 extra VMAs here, the "[stack:410]" and "[stack:408]"
> regions would have been merged with the following "[anon:libc_malloc]"
> regions.

Fair enough.

I wanted to trick you to implemented feature I want. Failed. ;)

The naming approach looks good to me. Storing strings in userspace is
somewhat unusual, but probably okay.

-- 
 Kirill A. Shutemov

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