* Re: [RFC PATCH] Add user-space support for resetting mm->hiwater_rss (peak RSS)
[not found] <1418223544-11382-1-git-send-email-petrcermak@chromium.org>
@ 2014-12-12 19:51 ` Bjorn Helgaas
2014-12-15 17:12 ` [PATCH 1/2] task_mmu: Reduce excessive indentation in clear_refs_write Petr Cermak
2014-12-15 17:15 ` [PATCH 2/2] task_mmu: Add user-space support for resetting mm->hiwater_rss (peak RSS) Petr Cermak
2 siblings, 0 replies; 4+ messages in thread
From: Bjorn Helgaas @ 2014-12-12 19:51 UTC (permalink / raw)
To: Petr Cermak; +Cc: linux-kernel, Primiano Tucci, Andrew Morton, linux-mm
[+cc akpm, linux-mm]
On Wed, Dec 10, 2014 at 02:59:04PM +0000, Petr Cermak wrote:
> Being able to reset mm->hiwater_rss (resident set size high water mark) from
> user space would enable fine grained iterative memory profiling. I propose a
> very short patch for doing so below. I would like to get some feedback on the
> user-space interface to do this. Would it be best to:
>
> 1. Add an extra value to /proc/PID/clear_refs to reset VmHWM? (The proposed
> patch uses this approach.)
>
> 2. Add a new write-only pseudo-file for this purpose (e.g.,
> /proc/pid/reset_hwm)?
>
> The driving use-case for this would be getting the peak RSS value, which can be
> retrieved from the VmHWM field in /proc/pid/status, per benchmark iteration or
> test scenario.
>
> Signed-off-by: Petr Cermak <petrcermak@chromium.org>
> ---
> Documentation/filesystems/proc.txt | 3 ++
> fs/proc/task_mmu.c | 106 +++++++++++++++++++++----------------
> include/linux/mm.h | 5 ++
> 3 files changed, 68 insertions(+), 46 deletions(-)
>
> diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
> index eb8a10e..2c277e9 100644
> --- a/Documentation/filesystems/proc.txt
> +++ b/Documentation/filesystems/proc.txt
> @@ -488,6 +488,9 @@ To clear the bits for the file mapped pages associated with the process
> To clear the soft-dirty bit
> > echo 4 > /proc/PID/clear_refs
>
> +To reset the peak resident set size ("high water mark")
> + > echo 5 > /proc/PID/clear_refs
> +
> Any other value written to /proc/PID/clear_refs will have no effect.
>
> The /proc/pid/pagemap gives the PFN, which can be used to find the pageflags
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 4e0388c..86b23b2 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -712,6 +712,7 @@ enum clear_refs_types {
> CLEAR_REFS_ANON,
> CLEAR_REFS_MAPPED,
> CLEAR_REFS_SOFT_DIRTY,
> + CLEAR_REFS_MM_HIWATER_RSS,
> CLEAR_REFS_LAST,
> };
>
> @@ -818,56 +819,69 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
> return -ESRCH;
> mm = get_task_mm(task);
> if (mm) {
> - struct clear_refs_private cp = {
> - .type = type,
> - };
This function suffers from excessive indentation, even before your patch.
In the "!mm" case, all we do is put the task struct and return, so I think
you should add a preliminary patch that does nothing but change this, e.g.,
if (!mm) {
put_task_struct(task);
return count;
}
down_read(&mm->mmap_sem);
...
You'll have to rework the declarations a bit, and maybe you want a goto
instead of a return, so the put_task_struct() isn't duplicated. The point
is that this will remove a level of indentation for the main body of the
function.
So it'd be nice to fix that with an initial patch that does nothing but fix
the style. Then the follow-on patch that adds your functionality will be
much smaller and easier to review. You're doing a simple thing, and the
patch should look simple :)
Bjorn
> - struct mm_walk clear_refs_walk = {
> - .pmd_entry = clear_refs_pte_range,
> - .mm = mm,
> - .private = &cp,
> - };
> - down_read(&mm->mmap_sem);
> - if (type == CLEAR_REFS_SOFT_DIRTY) {
> - for (vma = mm->mmap; vma; vma = vma->vm_next) {
> - if (!(vma->vm_flags & VM_SOFTDIRTY))
> - continue;
> - up_read(&mm->mmap_sem);
> - down_write(&mm->mmap_sem);
> + if (type == CLEAR_REFS_MM_HIWATER_RSS) {
> + /*
> + * Writing 5 to /proc/pid/clear_refs resets the peak
> + * resident set size.
> + */
> + down_write(&mm->mmap_sem);
> + reset_mm_hiwater_rss(mm);
> + up_write(&mm->mmap_sem);
> + } else {
> + struct clear_refs_private cp = {
> + .type = type,
> + };
> + struct mm_walk clear_refs_walk = {
> + .pmd_entry = clear_refs_pte_range,
> + .mm = mm,
> + .private = &cp,
> + };
> + down_read(&mm->mmap_sem);
> + if (type == CLEAR_REFS_SOFT_DIRTY) {
> for (vma = mm->mmap; vma; vma = vma->vm_next) {
> - vma->vm_flags &= ~VM_SOFTDIRTY;
> - vma_set_page_prot(vma);
> + if (!(vma->vm_flags & VM_SOFTDIRTY))
> + continue;
> + up_read(&mm->mmap_sem);
> + down_write(&mm->mmap_sem);
> + for (vma = mm->mmap; vma;
> + vma = vma->vm_next) {
> + vma->vm_flags &= ~VM_SOFTDIRTY;
> + vma_set_page_prot(vma);
> + }
> + downgrade_write(&mm->mmap_sem);
> + break;
> }
> - downgrade_write(&mm->mmap_sem);
> - break;
> + mmu_notifier_invalidate_range_start(mm, 0, -1);
> }
> - mmu_notifier_invalidate_range_start(mm, 0, -1);
> - }
> - for (vma = mm->mmap; vma; vma = vma->vm_next) {
> - cp.vma = vma;
> - if (is_vm_hugetlb_page(vma))
> - continue;
> - /*
> - * Writing 1 to /proc/pid/clear_refs affects all pages.
> - *
> - * Writing 2 to /proc/pid/clear_refs only affects
> - * Anonymous pages.
> - *
> - * Writing 3 to /proc/pid/clear_refs only affects file
> - * mapped pages.
> - *
> - * Writing 4 to /proc/pid/clear_refs affects all pages.
> - */
> - if (type == CLEAR_REFS_ANON && vma->vm_file)
> - continue;
> - if (type == CLEAR_REFS_MAPPED && !vma->vm_file)
> - continue;
> - walk_page_range(vma->vm_start, vma->vm_end,
> - &clear_refs_walk);
> + for (vma = mm->mmap; vma; vma = vma->vm_next) {
> + cp.vma = vma;
> + if (is_vm_hugetlb_page(vma))
> + continue;
> + /*
> + * Writing 1 to /proc/pid/clear_refs affects all
> + * pages.
> + *
> + * Writing 2 to /proc/pid/clear_refs only
> + * affects Anonymous pages.
> + *
> + * Writing 3 to /proc/pid/clear_refs only
> + * affects file mapped pages.
> + *
> + * Writing 4 to /proc/pid/clear_refs affects all
> + * pages.
> + */
> + if (type == CLEAR_REFS_ANON && vma->vm_file)
> + continue;
> + if (type == CLEAR_REFS_MAPPED && !vma->vm_file)
> + continue;
> + walk_page_range(vma->vm_start, vma->vm_end,
> + &clear_refs_walk);
> + }
> + if (type == CLEAR_REFS_SOFT_DIRTY)
> + mmu_notifier_invalidate_range_end(mm, 0, -1);
> + flush_tlb_mm(mm);
> + up_read(&mm->mmap_sem);
> }
> - if (type == CLEAR_REFS_SOFT_DIRTY)
> - mmu_notifier_invalidate_range_end(mm, 0, -1);
> - flush_tlb_mm(mm);
> - up_read(&mm->mmap_sem);
> mmput(mm);
> }
> put_task_struct(task);
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index b464611..8a51ef4 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1350,6 +1350,11 @@ static inline void update_hiwater_vm(struct mm_struct *mm)
> mm->hiwater_vm = mm->total_vm;
> }
>
> +static inline void reset_mm_hiwater_rss(struct mm_struct *mm)
> +{
> + mm->hiwater_rss = get_mm_rss(mm);
> +}
> +
> static inline void setmax_mm_hiwater_rss(unsigned long *maxrss,
> struct mm_struct *mm)
> {
> --
> 2.2.0.rc0.207.ga3a616c
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
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>
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH 1/2] task_mmu: Reduce excessive indentation in clear_refs_write
[not found] <1418223544-11382-1-git-send-email-petrcermak@chromium.org>
2014-12-12 19:51 ` [RFC PATCH] Add user-space support for resetting mm->hiwater_rss (peak RSS) Bjorn Helgaas
@ 2014-12-15 17:12 ` Petr Cermak
2014-12-15 17:15 ` [PATCH 2/2] task_mmu: Add user-space support for resetting mm->hiwater_rss (peak RSS) Petr Cermak
2 siblings, 0 replies; 4+ messages in thread
From: Petr Cermak @ 2014-12-15 17:12 UTC (permalink / raw)
To: linux-kernel
Cc: Petr Cermak, linux-mm, Andrew Morton, Bjorn Helgaas, Primiano Tucci
This is a purely cosmetic fix for clear_refs_write(). It removes excessive
indentation as suggested by Bjorn Helgaas <bhelgaas@google.com>. This is to
make upcoming changes to the file more readable.
Cc: linux-mm@kvack.org
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Primiano Tucci <primiano@chromium.org>
Cc: Petr Cermak <petrcermak@chromium.org>
Signed-off-by: Petr Cermak <petrcermak@chromium.org>
---
fs/proc/task_mmu.c | 100 +++++++++++++++++++++++++++--------------------------
1 file changed, 51 insertions(+), 49 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 246eae8..3ee8541 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -852,59 +852,61 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
if (!task)
return -ESRCH;
mm = get_task_mm(task);
- if (mm) {
- struct clear_refs_private cp = {
- .type = type,
- };
- struct mm_walk clear_refs_walk = {
- .pmd_entry = clear_refs_pte_range,
- .mm = mm,
- .private = &cp,
- };
- down_read(&mm->mmap_sem);
- if (type == CLEAR_REFS_SOFT_DIRTY) {
- for (vma = mm->mmap; vma; vma = vma->vm_next) {
- if (!(vma->vm_flags & VM_SOFTDIRTY))
- continue;
- up_read(&mm->mmap_sem);
- down_write(&mm->mmap_sem);
- for (vma = mm->mmap; vma; vma = vma->vm_next) {
- vma->vm_flags &= ~VM_SOFTDIRTY;
- vma_set_page_prot(vma);
- }
- downgrade_write(&mm->mmap_sem);
- break;
- }
- mmu_notifier_invalidate_range_start(mm, 0, -1);
- }
+ if (!mm)
+ goto out_task;
+
+ struct clear_refs_private cp = {
+ .type = type,
+ };
+ struct mm_walk clear_refs_walk = {
+ .pmd_entry = clear_refs_pte_range,
+ .mm = mm,
+ .private = &cp,
+ };
+ down_read(&mm->mmap_sem);
+ if (type == CLEAR_REFS_SOFT_DIRTY) {
for (vma = mm->mmap; vma; vma = vma->vm_next) {
- cp.vma = vma;
- if (is_vm_hugetlb_page(vma))
- continue;
- /*
- * Writing 1 to /proc/pid/clear_refs affects all pages.
- *
- * Writing 2 to /proc/pid/clear_refs only affects
- * Anonymous pages.
- *
- * Writing 3 to /proc/pid/clear_refs only affects file
- * mapped pages.
- *
- * Writing 4 to /proc/pid/clear_refs affects all pages.
- */
- if (type == CLEAR_REFS_ANON && vma->vm_file)
+ if (!(vma->vm_flags & VM_SOFTDIRTY))
continue;
- if (type == CLEAR_REFS_MAPPED && !vma->vm_file)
- continue;
- walk_page_range(vma->vm_start, vma->vm_end,
- &clear_refs_walk);
+ up_read(&mm->mmap_sem);
+ down_write(&mm->mmap_sem);
+ for (vma = mm->mmap; vma; vma = vma->vm_next) {
+ vma->vm_flags &= ~VM_SOFTDIRTY;
+ vma_set_page_prot(vma);
+ }
+ downgrade_write(&mm->mmap_sem);
+ break;
}
- if (type == CLEAR_REFS_SOFT_DIRTY)
- mmu_notifier_invalidate_range_end(mm, 0, -1);
- flush_tlb_mm(mm);
- up_read(&mm->mmap_sem);
- mmput(mm);
+ mmu_notifier_invalidate_range_start(mm, 0, -1);
}
+ for (vma = mm->mmap; vma; vma = vma->vm_next) {
+ cp.vma = vma;
+ if (is_vm_hugetlb_page(vma))
+ continue;
+ /*
+ * Writing 1 to /proc/pid/clear_refs affects all pages.
+ *
+ * Writing 2 to /proc/pid/clear_refs only affects anonymous
+ * pages.
+ *
+ * Writing 3 to /proc/pid/clear_refs only affects file mapped
+ * pages.
+ *
+ * Writing 4 to /proc/pid/clear_refs affects all pages.
+ */
+ if (type == CLEAR_REFS_ANON && vma->vm_file)
+ continue;
+ if (type == CLEAR_REFS_MAPPED && !vma->vm_file)
+ continue;
+ walk_page_range(vma->vm_start, vma->vm_end, &clear_refs_walk);
+ }
+ if (type == CLEAR_REFS_SOFT_DIRTY)
+ mmu_notifier_invalidate_range_end(mm, 0, -1);
+ flush_tlb_mm(mm);
+ up_read(&mm->mmap_sem);
+ mmput(mm);
+
+out_task:
put_task_struct(task);
return count;
--
2.2.0.rc0.207.ga3a616c
--
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>
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH 2/2] task_mmu: Add user-space support for resetting mm->hiwater_rss (peak RSS)
[not found] <1418223544-11382-1-git-send-email-petrcermak@chromium.org>
2014-12-12 19:51 ` [RFC PATCH] Add user-space support for resetting mm->hiwater_rss (peak RSS) Bjorn Helgaas
2014-12-15 17:12 ` [PATCH 1/2] task_mmu: Reduce excessive indentation in clear_refs_write Petr Cermak
@ 2014-12-15 17:15 ` Petr Cermak
2014-12-17 22:30 ` Andrew Morton
2 siblings, 1 reply; 4+ messages in thread
From: Petr Cermak @ 2014-12-15 17:15 UTC (permalink / raw)
To: linux-kernel
Cc: Petr Cermak, linux-mm, Andrew Morton, Bjorn Helgaas, Primiano Tucci
Peak resident size of a process can be reset by writing "5" to
/proc/pid/clear_refs. The driving use-case for this would be getting the
peak RSS value, which can be retrieved from the VmHWM field in
/proc/pid/status, per benchmark iteration or test scenario.
Cc: linux-mm@kvack.org
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Primiano Tucci <primiano@chromium.org>
Cc: Petr Cermak <petrcermak@chromium.org>
Signed-off-by: Petr Cermak <petrcermak@chromium.org>
---
Documentation/filesystems/proc.txt | 3 +++
fs/proc/task_mmu.c | 15 ++++++++++++++-
include/linux/mm.h | 5 +++++
3 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index aae9dd1..eab62e3 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -488,6 +488,9 @@ To clear the bits for the file mapped pages associated with the process
To clear the soft-dirty bit
> echo 4 > /proc/PID/clear_refs
+To reset the peak resident set size ("high water mark")
+ > echo 5 > /proc/PID/clear_refs
+
Any other value written to /proc/PID/clear_refs will have no effect.
The /proc/pid/pagemap gives the PFN, which can be used to find the pageflags
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 3ee8541..7967535 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -747,6 +747,7 @@ enum clear_refs_types {
CLEAR_REFS_ANON,
CLEAR_REFS_MAPPED,
CLEAR_REFS_SOFT_DIRTY,
+ CLEAR_REFS_MM_HIWATER_RSS,
CLEAR_REFS_LAST,
};
@@ -855,6 +856,17 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
if (!mm)
goto out_task;
+ if (type == CLEAR_REFS_MM_HIWATER_RSS) {
+ /*
+ * Writing 5 to /proc/pid/clear_refs resets the peak resident
+ * set size.
+ */
+ down_write(&mm->mmap_sem);
+ reset_mm_hiwater_rss(mm);
+ up_write(&mm->mmap_sem);
+ goto out_mm;
+ }
+
struct clear_refs_private cp = {
.type = type,
};
@@ -904,8 +916,9 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
mmu_notifier_invalidate_range_end(mm, 0, -1);
flush_tlb_mm(mm);
up_read(&mm->mmap_sem);
- mmput(mm);
+out_mm:
+ mmput(mm);
out_task:
put_task_struct(task);
diff --git a/include/linux/mm.h b/include/linux/mm.h
index c0a67b8..f3f6cee 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1368,6 +1368,11 @@ static inline void update_hiwater_vm(struct mm_struct *mm)
mm->hiwater_vm = mm->total_vm;
}
+static inline void reset_mm_hiwater_rss(struct mm_struct *mm)
+{
+ mm->hiwater_rss = get_mm_rss(mm);
+}
+
static inline void setmax_mm_hiwater_rss(unsigned long *maxrss,
struct mm_struct *mm)
{
--
2.2.0.rc0.207.ga3a616c
--
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>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH 2/2] task_mmu: Add user-space support for resetting mm->hiwater_rss (peak RSS)
2014-12-15 17:15 ` [PATCH 2/2] task_mmu: Add user-space support for resetting mm->hiwater_rss (peak RSS) Petr Cermak
@ 2014-12-17 22:30 ` Andrew Morton
0 siblings, 0 replies; 4+ messages in thread
From: Andrew Morton @ 2014-12-17 22:30 UTC (permalink / raw)
To: Petr Cermak; +Cc: linux-kernel, linux-mm, Bjorn Helgaas, Primiano Tucci
On Mon, 15 Dec 2014 17:15:33 +0000 Petr Cermak <petrcermak@chromium.org> wrote:
> Peak resident size of a process can be reset by writing "5" to
> /proc/pid/clear_refs. The driving use-case for this would be getting the
> peak RSS value, which can be retrieved from the VmHWM field in
> /proc/pid/status, per benchmark iteration or test scenario.
The term "reset" is ambiguous - it often means "reset it to zero".
This?
--- a/Documentation/filesystems/proc.txt~task_mmu-add-user-space-support-for-resetting-mm-hiwater_rss-peak-rss-fix
+++ a/Documentation/filesystems/proc.txt
@@ -488,7 +488,8 @@ To clear the bits for the file mapped pa
To clear the soft-dirty bit
> echo 4 > /proc/PID/clear_refs
-To reset the peak resident set size ("high water mark")
+To reset the peak resident set size ("high water mark") to the process's
+current value:
> echo 5 > /proc/PID/clear_refs
Any other value written to /proc/PID/clear_refs will have no effect.
--- a/fs/proc/task_mmu.c~task_mmu-add-user-space-support-for-resetting-mm-hiwater_rss-peak-rss-fix
+++ a/fs/proc/task_mmu.c
@@ -859,7 +859,7 @@ static ssize_t clear_refs_write(struct f
if (type == CLEAR_REFS_MM_HIWATER_RSS) {
/*
* Writing 5 to /proc/pid/clear_refs resets the peak resident
- * set size.
+ * set size to this mm's current rss value.
*/
down_write(&mm->mmap_sem);
reset_mm_hiwater_rss(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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 4+ messages in thread