fix text reporting in O(1) proc_pid_statm()

DISCUSSION Albert Cahalan 2 participants 7 messages Aug 24, 2004 – Aug 25, 2004

Subsystems (6)

Memory management MM core Scheduler exec and binfmt +2 more subsystems

lore

Share & tools

lore

Albert Cahalan

author

2004-08-24 23:09 UTC #0 lore raw

Quoted 4 lines

- *text = mm->exec_vm - ((mm->end_code - mm->start_code) >> PAGE_SHIFT);
- *data = mm->total_vm - mm->shared_vm;
+ *text = (mm->end_code - mm->start_code) >> PAGE_SHIFT;
+ *data = mm->total_vm - mm->shared_vm - *text;

It's actually still wrong. This has been broken for a very long time. It you can fix it, great. Otherwise this is a useless value, since /proc/*/stat provides start_code and end_code already.

The statm file is supposed to contain a field known as "trs" or "trss". This is like rss, but text-only. Likewise, statm also contains "drs" or "drss" for data. When you subtract start_code from end_code, you're generating a value known as "tsiz" (the text size). The statm file is supposed to supply trs, not tsiz.

Back in the days of a.out, statm also contained lrs for libraries. ELF broke this.

The statm VM size is supposed to count IO mappings. So if your X server maps 64 MB of video RAM, then the statm file should have a value 64 MB larger than the status file has.

William Lee Irwin III

2004-08-24 23:13 UTC #1 lore raw

Quoted 4 lines · At some point in the past, I wrote:

Quoted 4 lines
- *text = mm->exec_vm - ((mm->end_code - mm->start_code) >> PAGE_SHIFT);
- *data = mm->total_vm - mm->shared_vm;
+ *text = (mm->end_code - mm->start_code) >> PAGE_SHIFT;
+ *data = mm->total_vm - mm->shared_vm - *text;

Quoted 10 lines · Albert Cahalan

It's actually still wrong. This has been broken for a very long time. It you can fix it, great. Otherwise this is a useless value, since /proc/*/stat provides start_code and end_code already.
The statm file is supposed to contain a field known as "trs" or "trss". This is like rss, but text-only. Likewise, statm also contains "drs" or "drss" for data. When you subtract start_code from end_code, you're generating a value known as "tsiz" (the text size). The statm file is supposed to supply trs, not tsiz.

The current 2.6 semantics are purely virtual, so this merely reimplements those semantics more efficiently. The scheme you describe would require accounting at the time of pte modification to implement in a like fashion, which has been rejected.

Quoted 6 lines · Albert Cahalan

Back in the days of a.out, statm also contained lrs for libraries. ELF broke this.
The statm VM size is supposed to count IO mappings. So if your X server maps 64 MB of video RAM, then the statm file should have a value 64 MB larger than the status file has.

This would not be difficult to perform additional accounting for. I'll follow up with that shortly.

-- wli

William Lee Irwin III

2004-08-24 23:22 UTC #2 lore raw

Quoted 6 lines · Albert Cahalan

Quoted 6 lines
Back in the days of a.out, statm also contained lrs for libraries. ELF broke this.
The statm VM size is supposed to count IO mappings. So if your X server maps 64 MB of video RAM, then the statm file should have a value 64 MB larger than the status file has.

Quoted 2 lines · William Lee Irwin III

This would not be difficult to perform additional accounting for. I'll follow up with that shortly.

Account reserved memory properly as per acahalan's sepecified semantics.

fs/proc/task_mmu.c +1 -1

Index: mm4-2.6.8.1/fs/proc/task_mmu.c
===================================================================
@@ -19,7 +19,7 @@
 		"VmStk:\t%8lu kB\n"
 		"VmExe:\t%8lu kB\n"
 		"VmLib:\t%8lu kB\n",
-		mm->total_vm << (PAGE_SHIFT-10),
+		(mm->total_vm - mm->reserved_vm) << (PAGE_SHIFT-10),
 		mm->locked_vm << (PAGE_SHIFT-10),
 		mm->rss << (PAGE_SHIFT-10),
 		data << (PAGE_SHIFT-10),

include/linux/sched.h +1 -1

Index: mm4-2.6.8.1/include/linux/sched.h
===================================================================
@@ -226,7 +226,7 @@
 	unsigned long start_brk, brk, start_stack;
 	unsigned long arg_start, arg_end, env_start, env_end;
 	unsigned long rlimit_rss, rss, total_vm, locked_vm, shared_vm;
-	unsigned long exec_vm, stack_vm, def_flags;
+	unsigned long exec_vm, stack_vm, reserved_vm, def_flags;
 
 	unsigned long saved_auxv[40]; /* for /proc/PID/auxv */

mm/mmap.c +2 -0

Index: mm4-2.6.8.1/mm/mmap.c
===================================================================
@@ -749,6 +749,8 @@
 		mm->stack_vm += pages;
 	if (flags & VM_EXEC)
 		mm->exec_vm += pages;
+	if (flags & (VM_RESERVED|VM_IO))
+		mm->reserved_vm += pages;
 }
 
 /*

William Lee Irwin III

2004-08-24 23:29 UTC #3 lore raw

Quoted 2 lines · William Lee Irwin III

Quoted 2 lines
This would not be difficult to perform additional accounting for. I'll follow up with that shortly.

Quoted 1 line · William Lee Irwin III

Account reserved memory properly as per acahalan's sepecified semantics.

Unrelated fix. Unaccount VM_DONTCOPY vmas properly; the child inherits the whole of the parent's virtual accounting from the memcpy() in copy_mm(), but the VM_DONTCOPY check here is where a decision is made for the child not to inherit the vmas corresponding to some accounted memory usages. Hence, unaccount them when skipping over them here.

-- wli

kernel/fork.c +4 -1

Index: mm4-2.6.8.1/kernel/fork.c
===================================================================
@@ -391,8 +391,11 @@
 	for (mpnt = current->mm->mmap ; mpnt ; mpnt = mpnt->vm_next) {
 		struct file *file;
 
-		if(mpnt->vm_flags & VM_DONTCOPY)
+		if (mpnt->vm_flags & VM_DONTCOPY) {
+			__vm_stat_account(mm, mpnt->vm_flags, mpnt->vm_file,
+							-vma_pages(mpnt));
 			continue;
+		}
 		charge = 0;
 		if (mpnt->vm_flags & VM_ACCOUNT) {
 			unsigned int len = (mpnt->vm_end - mpnt->vm_start) >> PAGE_SHIFT;

William Lee Irwin III

2004-08-25 00:02 UTC #4 lore raw

Quoted 1 line · William Lee Irwin III

Quoted 1 line
Account reserved memory properly as per acahalan's sepecified semantics.

Quoted 5 lines · William Lee Irwin III

Unrelated fix. Unaccount VM_DONTCOPY vmas properly; the child inherits the whole of the parent's virtual accounting from the memcpy() in copy_mm(), but the VM_DONTCOPY check here is where a decision is made for the child not to inherit the vmas corresponding to some accounted memory usages. Hence, unaccount them when skipping over them here.

Unrelated improvement (not mandatory or a fix for a bug). Remove the accounting overhead when CONFIG_PROC_FS is not defined.

include/linux/mm.h +8 -0

Index: mm4-2.6.8.1/include/linux/mm.h
===================================================================
@@ -754,7 +754,15 @@
 		int write);
 extern int remap_page_range(struct vm_area_struct *vma, unsigned long from,
 		unsigned long to, unsigned long size, pgprot_t prot);
+
+#ifdef CONFIG_PROC_FS
 void __vm_stat_account(struct mm_struct *, unsigned long, struct file *, long);
+#else
+static inline void __vm_stat_account(struct mm_struct *mm,
+			unsigned long flags, struct file *file, long pages)
+{
+}
+#endif /* CONFIG_PROC_FS */
 
 static inline void vm_stat_account(struct vm_area_struct *vma)
 {

mm/mmap.c +2 -0

Index: mm4-2.6.8.1/mm/mmap.c
===================================================================
@@ -729,6 +729,7 @@
 	return NULL;
 }
 
+#ifdef CONFIG_PROC_FS
 void __vm_stat_account(struct mm_struct *mm, unsigned long flags,
 						struct file *file, long pages)
 {

@@ -752,6 +753,7 @@
 	if (flags & (VM_RESERVED|VM_IO))
 		mm->reserved_vm += pages;
 }
+#endif /* CONFIG_PROC_FS */
 
 /*
  * The caller must hold down_write(current->mm->mmap_sem).

Albert Cahalan

author

2004-08-25 02:53 UTC #5 lore raw

In reply to William Lee Irwin III

Quoted 18 lines · William Lee Irwin III

Quoted 4 lines · At some point in the past, I wrote:
Quoted 4 lines
- *text = mm->exec_vm - ((mm->end_code - mm->start_code) >> PAGE_SHIFT);
- *data = mm->total_vm - mm->shared_vm;
+ *text = (mm->end_code - mm->start_code) >> PAGE_SHIFT;
+ *data = mm->total_vm - mm->shared_vm - *text;
Quoted 10 lines · Albert Cahalan
It's actually still wrong. This has been broken for a very long time. It you can fix it, great. Otherwise this is a useless value, since /proc/*/stat provides start_code and end_code already.
The statm file is supposed to contain a field known as "trs" or "trss". This is like rss, but text-only. Likewise, statm also contains "drs" or "drss" for data. When you subtract start_code from end_code, you're generating a value known as "tsiz" (the text size). The statm file is supposed to supply trs, not tsiz.

Quoted 4 lines

The current 2.6 semantics are purely virtual, so this merely reimplements those semantics more efficiently. The scheme you describe would require accounting at the time of pte modification to implement in a like fashion, which has been rejected.

Hmmm, why not reject RSS too then? It's the same thing.

If trs and drs and so on were kept, then rss would just be the sum of them. Per-permission tracking seems about right:

rss[perms] += change;

(where "perms" is rwx plus shared/private and dirty/clean)

At some point, it would be good to have per-vma rss. This would be displayed by the pmap command's -x option. (added to /proc/*/maps right before the filename)

William Lee Irwin III

2004-08-25 03:04 UTC #6 lore raw

Quoted 4 lines · William Lee Irwin III

Quoted 4 lines
The current 2.6 semantics are purely virtual, so this merely reimplements those semantics more efficiently. The scheme you describe would require accounting at the time of pte modification to implement in a like fashion, which has been rejected.

Quoted 9 lines · Albert Cahalan

Hmmm, why not reject RSS too then? It's the same thing. If trs and drs and so on were kept, then rss would just be the sum of them. Per-permission tracking seems about right:
rss[perms] += change;
(where "perms" is rwx plus shared/private and dirty/clean) At some point, it would be good to have per-vma rss. This would be displayed by the pmap command's -x option. (added to /proc/*/maps right before the filename)

Generally the way these patches have operated is accounting only the interesting combinations of those with respect to reporting in the interest of conserving space in the mm. These have also been combined with various checks on e.g. address ranges and some special casing of hugetlb. So it's not been quite as simple as all that.

The code from my 2.6.0-test7-bk1 forward port of the Red Hat patches for this was as follows:

static inline void vm_account(struct vm_area_struct *vma, pte_t pte,
unsigned long addr, long adjustment)

{
	struct mm_struct *mm = vma->vm_mm;
	unsigned long pfn;
	struct page *page;

	if (!pte_present(pte))
		return;

	pfn = pte_pfn(pte);
	if (!pfn_valid(pfn))
		goto out;

	page = pfn_to_page(pfn);
	if (PageReserved(page))
		goto out;

	if (vma->vm_flags & VM_EXECUTABLE)
		mm->text += adjustment;
	else if (vma->vm_flags & (VM_STACK_FLAGS & (VM_GROWSUP | VM_GROWSDOWN))) {
		mm->data += adjustment;
		mm->stack += adjustment;
	} else if (addr >= TASK_UNMAPPED_BASE)
		mm->lib += adjustment;
	else
		mm->data += adjustment;

	if (page_mapping(page))
		mm->shared += adjustment;

out:

	if (pte_write(pte))
		mm->dirty += adjustment;
}

-- wli