Skip to contentSkip to conversation

mm acct accounting fix

PATCH In review Kirill Korotaev 2 participants 4 messages May 11, 2005 – May 12, 2005

Subsystems (2)
Memory management Memory mapping
lore
Share & tools
lore

Kirill Korotaev

author
#0 lore raw

PATCH 1/1 mm acct accounting fix

Sorry, forgot to write that all these patches are against 2.6.12-rc4...

This patch fixes mm->total_vm and mm->locked_vm acctounting in case when move_page_tables() fails inside move_vma().

Signed-off-by Kirill Korotaev <[email protected]>
mm/mremap.c renamed from mm/mremap.c.mmacct +1 -0
@@ -213,6 +213,7 @@ static unsigned long move_vma(struct vm_
 		old_len = new_len;
 		old_addr = new_addr;
 		new_addr = -ENOMEM;
+		new_len = 0;
 	}
 
 	/* Conceal VM_ACCOUNT so old reservation is not undone */
#1 lore raw
Quoted 10 lines · Kirill Korotaev

Sorry, forgot to write that all these patches are against 2.6.12-rc4...

This patch fixes mm->total_vm and mm->locked_vm acctounting in case when move_page_tables() fails inside move_vma().

Signed-off-by Kirill Korotaev <[email protected]>

mm/mremap.c static unsigned long move_vma(struct vm_ · line 219

Quoted 7 lines
mm/mremap.c renamed from mm/mremap.c.mmacct +1 -0
@@ -213,6 +213,7 @@ static unsigned long move_vma(struct vm_
 		old_len = new_len;
 		old_addr = new_addr;
 		new_addr = -ENOMEM;
+		new_len = 0;
 	}
 
 	/* Conceal VM_ACCOUNT so old reservation is not undone */

Are you sure?

The way it's supposed to work is that the do_munmap(,,old_len) which follows, which normally unmaps the area moved from, when unsuccessful unmaps the area moved to: which will "mistakenly" decrement total_vm etc. by old_len, which needs to be bumped back up by that amount before leaving.

Hugh

author
#2 lore raw
Quoted 21 lines
Quoted 10 lines

Sorry, forgot to write that all these patches are against 2.6.12-rc4...

This patch fixes mm->total_vm and mm->locked_vm acctounting in case when move_page_tables() fails inside move_vma().

Signed-off-by Kirill Korotaev <[email protected]>

mm/mremap.c static unsigned long move_vma(struct vm_ · line 219

Quoted 7 lines
mm/mremap.c renamed from mm/mremap.c.mmacct +1 -0
@@ -213,6 +213,7 @@ static unsigned long move_vma(struct vm_
 		old_len = new_len;
 		old_addr = new_addr;
 		new_addr = -ENOMEM;
+		new_len = 0;
 	}
 
 	/* Conceal VM_ACCOUNT so old reservation is not undone */

Are you sure?

Quoted 4 lines

The way it's supposed to work is that the do_munmap(,,old_len) which follows, which normally unmaps the area moved from, when unsuccessful unmaps the area moved to: which will "mistakenly" decrement total_vm etc. by old_len, which needs to be bumped back up by that amount before leaving.

Yup, I was wrong. Sorry for taking your time.
It would be nice to accept this small patch with a comment.

Kirill

mm/mremap.c renamed from mm/mremap.c.mmacct +6 -1
@@ -224,6 +224,12 @@ static unsigned long move_vma(struct vm_
 			split = 1;
 	}
 
+	/*
+	 * if we failed to move page tables we still do total_vm increment
+	 * since do_munmap() will decrement it by old_len == new_len
+	 */
+	mm->total_vm += new_len >> PAGE_SHIFT;
+
 	if (do_munmap(mm, old_addr, old_len) < 0) {
 		/* OOM: unable to split vma, just get accounts right */
 		vm_unacct_memory(excess >> PAGE_SHIFT);

@@ -237,7 +243,6 @@ static unsigned long move_vma(struct vm_
 			vma->vm_next->vm_flags |= VM_ACCOUNT;
 	}
 
-	mm->total_vm += new_len >> PAGE_SHIFT;
 	__vm_stat_account(mm, vma->vm_flags, vma->vm_file, new_len>>PAGE_SHIFT);
 	if (vm_flags & VM_LOCKED) {
 		mm->locked_vm += new_len >> PAGE_SHIFT;
#3 lore raw
Quoted 2 lines · Kirill Korotaev

It would be nice to accept this small patch with a comment.

Adding such a comment is okay, but moving the total_vm += line away from the __vm_stat_account and locked_vm += seems odd to me
- I'd rather you added your comment after the do_munmap(), but no big deal (mmap_sem is held for writing throughout).

Hugh

mm/mremap.c renamed from mm/mremap.c.mmacct +6 -1
@@ -224,6 +224,12 @@ static unsigned long move_vma(struct vm_
 			split = 1;
 	}
 
+	/*
+	 * if we failed to move page tables we still do total_vm increment
+	 * since do_munmap() will decrement it by old_len == new_len
+	 */
+	mm->total_vm += new_len >> PAGE_SHIFT;
+
 	if (do_munmap(mm, old_addr, old_len) < 0) {
 		/* OOM: unable to split vma, just get accounts right */
 		vm_unacct_memory(excess >> PAGE_SHIFT);

@@ -237,7 +243,6 @@ static unsigned long move_vma(struct vm_
 			vma->vm_next->vm_flags |= VM_ACCOUNT;
 	}
 
-	mm->total_vm += new_len >> PAGE_SHIFT;
 	__vm_stat_account(mm, vma->vm_flags, vma->vm_file, new_len>>PAGE_SHIFT);
 	if (vm_flags & VM_LOCKED) {
 		mm->locked_vm += new_len >> PAGE_SHIFT;