[PATCH 1/2] uprobes/core: Make background page replacement logic account for rss_stat counters

[PATCH 1/2] uprobes/core: Make background page replacement logic account for rss_stat counters

[Srikar Dronamraju]

From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Background page replacement logic adds a new anonymous page instead of a
filebacked(while inserting a breakpoint) / anonymous page(while removing a
breakpoint). Hence logic should take care to update the rss_stat counters
accordingly. This bug became apparent courtesy commit c3f0327f8e9d7

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 kernel/events/uprobes.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 29e881b..c5caeec 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -160,6 +160,11 @@ static int __replace_page(struct vm_area_struct *vma, struct page *page, struct 
 	get_page(kpage);
 	page_add_new_anon_rmap(kpage, vma, addr);
 
+	if (!PageAnon(page)) {
+		dec_mm_counter(mm, MM_FILEPAGES);
+		inc_mm_counter(mm, MM_ANONPAGES);
+	}
+
 	flush_cache_page(vma, addr, pte_pfn(*ptep));
 	ptep_clear_flush(vma, addr, ptep);
 	set_pte_at_notify(mm, addr, ptep, mk_pte(kpage, vma->vm_page_prot));


--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/2] uprobes/core: Decrement uprobe count before the pages are unmapped

[Srikar Dronamraju]

From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Uprobes has a hook(uprobe_munmap) in unmap path to keep the uprobes count
sane. In the exit path this hook gets called in unlink_file_vma. However by
the time unlink_file_vma is called, the pages would have been unmapped
(unmap_vmas) and the rss_stat counts accounted (zap_pte_range). If
the exiting process has probepoints, uprobe_munmap checks if the breakpoint
instruction was around before decrementing the probe count.

This results in a filebacked page being reread by uprobe_munmap and hence 
it does not find the breakpoint.

This patch fixes this problem by moving the hook to unmap_single_vma.
Since unmap_single_vma may not unmap the complete vma, add start and end
parameters to uprobe_munmap.
This bug became apparent courtesy commit c3f0327f8e9d7.

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 include/linux/uprobes.h |    5 +++--
 kernel/events/uprobes.c |    4 ++--
 mm/memory.c             |    3 +++
 mm/mmap.c               |    8 ++++----
 4 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index d594d3b..efe4b33 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -107,7 +107,7 @@ extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
 extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
 extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
 extern int uprobe_mmap(struct vm_area_struct *vma);
-extern void uprobe_munmap(struct vm_area_struct *vma);
+extern void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end);
 extern void uprobe_free_utask(struct task_struct *t);
 extern void uprobe_copy_process(struct task_struct *t);
 extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
@@ -134,7 +134,8 @@ static inline int uprobe_mmap(struct vm_area_struct *vma)
 {
 	return 0;
 }
-static inline void uprobe_munmap(struct vm_area_struct *vma)
+static inline void
+uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end)
 {
 }
 static inline void uprobe_notify_resume(struct pt_regs *regs)
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index c5caeec..985be4d 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1112,7 +1112,7 @@ int uprobe_mmap(struct vm_area_struct *vma)
 /*
  * Called in context of a munmap of a vma.
  */
-void uprobe_munmap(struct vm_area_struct *vma)
+void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end)
 {
 	struct list_head tmp_list;
 	struct uprobe *uprobe, *u;
@@ -1138,7 +1138,7 @@ void uprobe_munmap(struct vm_area_struct *vma)
 		list_del(&uprobe->pending_list);
 		vaddr = vma_address(vma, uprobe->offset);
 
-		if (vaddr >= vma->vm_start && vaddr < vma->vm_end) {
+		if (vaddr >= start && vaddr < end) {
 			/*
 			 * An unregister could have removed the probe before
 			 * unmap. So check before we decrement the count.
diff --git a/mm/memory.c b/mm/memory.c
index 6105f47..bf8b403 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1307,6 +1307,9 @@ static void unmap_single_vma(struct mmu_gather *tlb,
 	if (end <= vma->vm_start)
 		return;
 
+	if (vma->vm_file)
+		uprobe_munmap(vma, start, end);
+
 	if (vma->vm_flags & VM_ACCOUNT)
 		*nr_accounted += (end - start) >> PAGE_SHIFT;
 
diff --git a/mm/mmap.c b/mm/mmap.c
index b17a39f..15c21a1 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -218,7 +218,6 @@ void unlink_file_vma(struct vm_area_struct *vma)
 		mutex_lock(&mapping->i_mmap_mutex);
 		__remove_shared_vm_struct(vma, file, mapping);
 		mutex_unlock(&mapping->i_mmap_mutex);
-		uprobe_munmap(vma);
 	}
 }
 
@@ -548,10 +547,11 @@ again:			remove_next = 1 + (end > next->vm_end);
 		mapping = file->f_mapping;
 		if (!(vma->vm_flags & VM_NONLINEAR)) {
 			root = &mapping->i_mmap;
-			uprobe_munmap(vma);
+			uprobe_munmap(vma, vma->vm_start, vma->vm_end);
 
 			if (adjust_next)
-				uprobe_munmap(next);
+				uprobe_munmap(next, next->vm_start,
+							next->vm_end);
 		}
 
 		mutex_lock(&mapping->i_mmap_mutex);
@@ -632,7 +632,7 @@ again:			remove_next = 1 + (end > next->vm_end);
 
 	if (remove_next) {
 		if (file) {
-			uprobe_munmap(next);
+			uprobe_munmap(next, next->vm_start, next->vm_end);
 			fput(file);
 			if (next->vm_flags & VM_EXECUTABLE)
 				removed_exe_file_vma(mm);


--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[tip:perf/uprobes] uprobes/core: Make background page replacement logic account for rss_stat counters

[tip-bot for Srikar Dronamraju]

Commit-ID:  7396fa818d6278694a44840f389ddc40a3269a9a
Gitweb:     http://git.kernel.org/tip/7396fa818d6278694a44840f389ddc40a3269a9a
Author:     Srikar Dronamraju <srikar@linux.vnet.ibm.com>
AuthorDate: Wed, 11 Apr 2012 16:05:16 +0530
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 14 Apr 2012 13:25:47 +0200

uprobes/core: Make background page replacement logic account for rss_stat counters

Background page replacement logic adds a new anonymous page
instead of a file backed (while inserting a breakpoint) /
anonymous page (while removing a breakpoint).

Hence the uprobes logic should take care to update the
task->ss_stat counters accordingly.

This bug became apparent courtesy of commit c3f0327f8e9d
("mm: add rss counters consistency check").

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: Jim Keniston <jkenisto@linux.vnet.ibm.com>
Cc: Linux-mm <linux-mm@kvack.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Arnaldo Carvalho de Melo <acme@infradead.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Anton Arapov <anton@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20120411103516.23245.2700.sendpatchset@srdronam.in.ibm.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/uprobes.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 29e881b..c5caeec 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -160,6 +160,11 @@ static int __replace_page(struct vm_area_struct *vma, struct page *page, struct
 	get_page(kpage);
 	page_add_new_anon_rmap(kpage, vma, addr);
 
+	if (!PageAnon(page)) {
+		dec_mm_counter(mm, MM_FILEPAGES);
+		inc_mm_counter(mm, MM_ANONPAGES);
+	}
+
 	flush_cache_page(vma, addr, pte_pfn(*ptep));
 	ptep_clear_flush(vma, addr, ptep);
 	set_pte_at_notify(mm, addr, ptep, mk_pte(kpage, vma->vm_page_prot));

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[tip:perf/uprobes] uprobes/core: Decrement uprobe count before the pages are unmapped

[tip-bot for Srikar Dronamraju]

Commit-ID:  cbc91f71b51b8335f1fc7ccfca8011f31a717367
Gitweb:     http://git.kernel.org/tip/cbc91f71b51b8335f1fc7ccfca8011f31a717367
Author:     Srikar Dronamraju <srikar@linux.vnet.ibm.com>
AuthorDate: Wed, 11 Apr 2012 16:05:27 +0530
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 14 Apr 2012 13:25:48 +0200

uprobes/core: Decrement uprobe count before the pages are unmapped

Uprobes has a callback (uprobe_munmap()) in the unmap path to
maintain the uprobes count.

In the exit path this callback gets called in unlink_file_vma().
However by the time unlink_file_vma() is called, the pages would
have been unmapped (in unmap_vmas()) and the task->rss_stat counts
accounted (in zap_pte_range()).

If the exiting process has probepoints, uprobe_munmap() checks if
the breakpoint instruction was around before decrementing the probe
count.

This results in a file backed page being reread by uprobe_munmap()
and hence it does not find the breakpoint.

This patch fixes this problem by moving the callback to
unmap_single_vma(). Since unmap_single_vma() may not unmap the
complete vma, add start and end parameters to uprobe_munmap().

This bug became apparent courtesy of commit c3f0327f8e9d
("mm: add rss counters consistency check").

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: Jim Keniston <jkenisto@linux.vnet.ibm.com>
Cc: Linux-mm <linux-mm@kvack.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Arnaldo Carvalho de Melo <acme@infradead.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Anton Arapov <anton@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20120411103527.23245.9835.sendpatchset@srdronam.in.ibm.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/uprobes.h |    5 +++--
 kernel/events/uprobes.c |    4 ++--
 mm/memory.c             |    3 +++
 mm/mmap.c               |    8 ++++----
 4 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index d594d3b..efe4b33 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -107,7 +107,7 @@ extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
 extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
 extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
 extern int uprobe_mmap(struct vm_area_struct *vma);
-extern void uprobe_munmap(struct vm_area_struct *vma);
+extern void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end);
 extern void uprobe_free_utask(struct task_struct *t);
 extern void uprobe_copy_process(struct task_struct *t);
 extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
@@ -134,7 +134,8 @@ static inline int uprobe_mmap(struct vm_area_struct *vma)
 {
 	return 0;
 }
-static inline void uprobe_munmap(struct vm_area_struct *vma)
+static inline void
+uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end)
 {
 }
 static inline void uprobe_notify_resume(struct pt_regs *regs)
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index c5caeec..985be4d 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1112,7 +1112,7 @@ int uprobe_mmap(struct vm_area_struct *vma)
 /*
  * Called in context of a munmap of a vma.
  */
-void uprobe_munmap(struct vm_area_struct *vma)
+void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end)
 {
 	struct list_head tmp_list;
 	struct uprobe *uprobe, *u;
@@ -1138,7 +1138,7 @@ void uprobe_munmap(struct vm_area_struct *vma)
 		list_del(&uprobe->pending_list);
 		vaddr = vma_address(vma, uprobe->offset);
 
-		if (vaddr >= vma->vm_start && vaddr < vma->vm_end) {
+		if (vaddr >= start && vaddr < end) {
 			/*
 			 * An unregister could have removed the probe before
 			 * unmap. So check before we decrement the count.
diff --git a/mm/memory.c b/mm/memory.c
index 6105f47..bf8b403 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1307,6 +1307,9 @@ static void unmap_single_vma(struct mmu_gather *tlb,
 	if (end <= vma->vm_start)
 		return;
 
+	if (vma->vm_file)
+		uprobe_munmap(vma, start, end);
+
 	if (vma->vm_flags & VM_ACCOUNT)
 		*nr_accounted += (end - start) >> PAGE_SHIFT;
 
diff --git a/mm/mmap.c b/mm/mmap.c
index b17a39f..15c21a1 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -218,7 +218,6 @@ void unlink_file_vma(struct vm_area_struct *vma)
 		mutex_lock(&mapping->i_mmap_mutex);
 		__remove_shared_vm_struct(vma, file, mapping);
 		mutex_unlock(&mapping->i_mmap_mutex);
-		uprobe_munmap(vma);
 	}
 }
 
@@ -548,10 +547,11 @@ again:			remove_next = 1 + (end > next->vm_end);
 		mapping = file->f_mapping;
 		if (!(vma->vm_flags & VM_NONLINEAR)) {
 			root = &mapping->i_mmap;
-			uprobe_munmap(vma);
+			uprobe_munmap(vma, vma->vm_start, vma->vm_end);
 
 			if (adjust_next)
-				uprobe_munmap(next);
+				uprobe_munmap(next, next->vm_start,
+							next->vm_end);
 		}
 
 		mutex_lock(&mapping->i_mmap_mutex);
@@ -632,7 +632,7 @@ again:			remove_next = 1 + (end > next->vm_end);
 
 	if (remove_next) {
 		if (file) {
-			uprobe_munmap(next);
+			uprobe_munmap(next, next->vm_start, next->vm_end);
 			fput(file);
 			if (next->vm_flags & VM_EXECUTABLE)
 				removed_exe_file_vma(mm);

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [tip:perf/uprobes] uprobes/core: Decrement uprobe count before the pages are unmapped

[Ingo Molnar]

* tip-bot for Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:

> Commit-ID:  cbc91f71b51b8335f1fc7ccfca8011f31a717367
> Gitweb:     http://git.kernel.org/tip/cbc91f71b51b8335f1fc7ccfca8011f31a717367
> Author:     Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> AuthorDate: Wed, 11 Apr 2012 16:05:27 +0530
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Sat, 14 Apr 2012 13:25:48 +0200
> 
> uprobes/core: Decrement uprobe count before the pages are unmapped
> 
> Uprobes has a callback (uprobe_munmap()) in the unmap path to
> maintain the uprobes count.
> 
> In the exit path this callback gets called in unlink_file_vma().
> However by the time unlink_file_vma() is called, the pages would
> have been unmapped (in unmap_vmas()) and the task->rss_stat counts
> accounted (in zap_pte_range()).
> 
> If the exiting process has probepoints, uprobe_munmap() checks if
> the breakpoint instruction was around before decrementing the probe
> count.
> 
> This results in a file backed page being reread by uprobe_munmap()
> and hence it does not find the breakpoint.
> 
> This patch fixes this problem by moving the callback to
> unmap_single_vma(). Since unmap_single_vma() may not unmap the
> complete vma, add start and end parameters to uprobe_munmap().
> 
> This bug became apparent courtesy of commit c3f0327f8e9d
> ("mm: add rss counters consistency check").

Srikar, as a side note, please try to write more readable 
changelogs.

The original version, before I edited it, was:

> Uprobes has a hook(uprobe_munmap) in unmap path to keep the 
> uprobes count sane. In the exit path this hook gets called in 
> unlink_file_vma. However by the time unlink_file_vma is 
> called, the pages would have been unmapped (unmap_vmas) and 
> the rss_stat counts accounted (zap_pte_range). If the exiting 
> process has probepoints, uprobe_munmap checks if the 
> breakpoint instruction was around before decrementing the 
> probe count.
>
> This results in a filebacked page being reread by 
> uprobe_munmap and hence it does not find the breakpoint.
>
> This patch fixes this problem by moving the hook to 
> unmap_single_vma. Since unmap_single_vma may not unmap the 
> complete vma, add start and end parameters to uprobe_munmap. 
> This bug became apparent courtesy commit c3f0327f8e9d7.

I changed these details:

 - We use func() instead of func when talking about functions in 
   changelogs, to make them stand apart from types, variables, 
   and regular words better. Especially in your changelog it was 
   warranted, because you mention more than half a dozen of 
   function names.

 - A similar detail is 'rss_stat' - it's better to refer to
   'struct task_rss_stat' or task->rss_stat, so that the reader 
   has some context to place this structure into - and can
   distinguish data from function names.

 - We don't maintain the uprobes count to make it 'sane' - it's
   either correctly maintained or not. Readers of your changelog 
   have no idea what 'sane' means in that context.

 - We reference upstream commits not via their commit ID alone, 
   but by mentioning their title: which is in fact the more
   important piece of information in a *human* readable
   changelog. I.e. not:

     commit c3f0327f8e9d7

   but:

     commit c3f0327f8e9d ("mm: add rss counters consistency check").

 - In all prior uprobes commits I had to correct your
   usage of 'hooks' to 'callbacks' - which is how we 
   traditionally refer to callback functions in the mm/.

 - Small details like there's no such thing as 'filebacked' -
   it's "file backed". The phrase "became apparent courtesy 
   commit" has a serious shortage of prepositions, etc.

Fixing it all adds up for the maintainer. You should generally 
strive for making your changelog readable to any kernel hacker - 
not just to those intimately familiar with the code you are 
working on.

Thanks,

	Ingo

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>