memrlimit improve error handling

include/linux/res_counter.h renamed from include/linux/res_counter.h~memrlimit-cgroup-fix-attach-task +6 -0

+/*
+ * Add the value val to the resource counter and check if we are
+ * still under the limit.
+ */
+static inline bool res_counter_add_check(struct res_counter *cnt,
+						unsigned long val)

Quoted 6 lines · On Fri, 20 Jun 2008 20:31:42 +0530 Balbir Singh <[email protected]>…

include/linux/res_counter.h renamed from include/linux/res_counter.h~memrlimit-cgroup-fix-attach-task +6 -0

+/*
+ * Add the value val to the resource counter and check if we are
+ * still under the limit.
+ */
+static inline bool res_counter_add_check(struct res_counter *cnt,
+						unsigned long val)

include/linux/res_counter.h static inline void res_counter_reset_fai · line 172

Quoted 10 lines

include/linux/res_counter.h renamed from include/linux/res_counter.h~memrlimit-cgroup-fix-attach-task +10 -0

+{
+	bool ret = false;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cnt->lock, flags);
+	if (cnt->usage + val <= cnt->limit)
+		ret = true;
+	spin_unlock_irqrestore(&cnt->lock, flags);
+	return ret;
+}

The comment and the function name imply that thins function will "Add the value val to the resource counter". But it doesn't do that at all. In fact the first arg could be a `const struct res_counter *'.

Perhaps res_counter_can_add() would be more accurate.

Fix the variable tmp being used in write_strategy. This patch replaces tmp with val, the fact that it is an output parameter can be interpreted from the pass by reference.

Quoted 3 lines · On Fri, 20 Jun 2008 20:31:52 +0530 Balbir Singh <[email protected]>…

Fix the variable tmp being used in write_strategy. This patch replaces tmp with val, the fact that it is an output parameter can be interpreted from the pass by reference.

Paul's "CGroup Files: Convert res_counter_write() to be a cgroups
write_string() handler" (memrlimit-setup-the-memrlimit-controller-cgroup-files-convert-res_counter_write-to-be-a-cgroups-write_string-handler-memrlimitcgroup.patch) deleted memrlimit_cgroup_write_strategy(), so problem solved ;)

memrlimit cgroup does not handle error cases after may_expand_vm(). This BUG was reported by Kamezawa, with the test case below to reproduce it

This patch adds better handling support to fix the reported problem.

Reported-By [email protected]

Signed-off-by Balbir Singh <[email protected]>

mm/mremap.c renamed from mm/mremap.c~memrlimit-cgroup-add-better-error-handling +2 -0

@@ -256,6 +257,7 @@ unsigned long do_mremap(unsigned long ad
 	struct vm_area_struct *vma;
 	unsigned long ret = -EINVAL;
 	unsigned long charged = 0;
+	int vm_expanded = 0;
 
 	if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE))
 		goto out;

@@ -349,6 +351,7 @@ unsigned long do_mremap(unsigned long ad
 		goto out;
 	}
 
+	vm_expanded = 1;
 	if (vma->vm_flags & VM_ACCOUNT) {
 		charged = (new_len - old_len) >> PAGE_SHIFT;
 		if (security_vm_enough_memory(charged))

Hi Balbir,

Below I've a few comments on this particular patch, then report other problems I've seen with memrlimits configured into my 2.6.25-rc5-mm3 kernel - I've not tried _using_ them. My own opinion would be that -mm already contains enough breakage, we don't need memrlimits yet.

Quoted 8 lines

memrlimit cgroup does not handle error cases after may_expand_vm(). This BUG was reported by Kamezawa, with the test case below to reproduce it

This patch adds better handling support to fix the reported problem.

Reported-By [email protected]

Signed-off-by Balbir Singh <[email protected]>

I wrote a similar patch when I saw Kame's report, because I was interested in getting that accounting right. Comparing the two, a couple of notes on the details...

In mm/mmap.c:

mm/mmap.c unsigned long do_brk(unsigned long addr, · line 1995

Quoted 5 lines

mm/mmap.c renamed from mm/mmap.c~memrlimit-cgroup-add-better-error-handling +1 -0

@@ -1982,6 +1992,7 @@ unsigned long do_brk(unsigned long addr,
 	struct rb_node ** rb_link, * rb_parent;
 	pgoff_t pgoff = addr >> PAGE_SHIFT;
 	int error;
+	int ret = -ENOMEM;

I think we don't want int error returned from some parts of do_brk() and int ret returned from other parts: please use int error throughout. It would probably be nicer to add int error rather than int ret in acct_stack_growth() too, but that doesn't matter much.

In mm/mremap.c:

Quoted 17 lines

mm/mremap.c renamed from mm/mremap.c~memrlimit-cgroup-add-better-error-handling +2 -0

@@ -256,6 +257,7 @@ unsigned long do_mremap(unsigned long ad
 	struct vm_area_struct *vma;
 	unsigned long ret = -EINVAL;
 	unsigned long charged = 0;
+	int vm_expanded = 0;
 
 	if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE))
 		goto out;

@@ -349,6 +351,7 @@ unsigned long do_mremap(unsigned long ad
 		goto out;
 	}
 
+	vm_expanded = 1;
 	if (vma->vm_flags & VM_ACCOUNT) {
 		charged = (new_len - old_len) >> PAGE_SHIFT;
 		if (security_vm_enough_memory(charged))

mm/mremap.c out: · line 421

Quoted 8 lines

mm/mremap.c renamed from mm/mremap.c~memrlimit-cgroup-add-better-error-handling +3 -0

@@ -411,6 +414,9 @@ out:
 	if (ret & ~PAGE_MASK)
 		vm_unacct_memory(charged);
 out_nc:
+	if (vm_expanded)
+		memrlimit_cgroup_uncharge_as(mm,
+				(new_len - old_len) >> PAGE_SHIFT);
 	return ret;
 }

See how vm_unacct_memory(charged) is only called if (ret & ~PAGE_MASK)? If ret is a valid address being returned, we do not want to uncharge. So I believe you need to do likewise with your uncharge_as().

And please handle them both in the same way: either follow the same "charged" style as is being used for vm_unacct_memory, rather than a boolean; or convert vm_unacct_memory over to use your boolean style:
but it's unhelpful to have the two using different techniques.

In kernel/fork.c:
nothing, but when I had a quick look there, again the error handling appeared to be broken e.g. if allocate_mm fails, where's the uncharge? But be careful: there's a particular point where enough of the new mm is set up that it gets torn down by normal exit_mmap.

And while looking at copy_mm, do I see an extra down_write and up_write of mmap_sem, just to guard some memrlimit charging?

Don't add overhead when memrlimits are CONFIGed off; but can't it be moved into dup_mm() where mmap_sem is already down_write?

Please go through all your charges, again, to double check that you've got the uncharging right in the error cases.

I was interested in these because I find that after running kernel build on tmpfs swapping loads in a 700M memcg (but you may well hit other rc5-mm3 bugs if you try that, I've fixes to send out) for some

hours on x86_64, my shutdown hits the kernel/res_counter.c:49 res_counter_uncharge_locked() WARN_ON(counter->usage < val) several times, called from memrlimit_cgroup_uncharge_as called from exit_mmap.

I have no idea what the cause is; but I've not seen it on i386, and I'm not seeing it after shorter runs. It could even be some error introduced by other patches in what I'm testing: so don't worry too much about it yet, but please keep an eye out for it.

(If I'd thought harder, I'd have been less interested in the charge_as leaks: those WARN_ONs come when too much is being uncharged, not when too little.)

But the issue which worries me most, because it's mislocking which affects anyone with CONFIG_MM_OWNER=y, is seen when I have CONFIG_DEBUG_SPINLOCK_SLEEP=y:

 BUG: sleeping function called from invalid context at kernel/rwsem.c:48
 in_atomic():1, irqs_disabled():0
 1 lock held by blogd/830:
  #0:  (tasklist_lock){..--}, at: [<78125869>] mm_update_next_owner+0x1dd/0x1fa
 Pid: 830, comm: blogd Not tainted 2.6.26-rc5-mm2 #2
  [<78120ea2>] __might_sleep+0xed/0xf2
  [<78367947>] down_write+0x13/0x43
  [<781257b6>] mm_update_next_owner+0x12a/0x1fa
  [<7812595a>] exit_mm+0xd4/0xe5
  [<78125f9d>] do_exit+0x1e7/0x2bc
  [<781260fb>] do_group_exit+0x61/0x8a
  [<78126134>] sys_exit_group+0x10/0x13
  [<78102dd9>] sysenter_past_esp+0x6a/0xa5

Your memrlimit-cgroup-mm-owner-callback-changes-to-add-task-info.patch thinks it can acquire mmap_sem while it has read_lock(&tasklist_lock). Sorry, no: you'll have to rework that somehow.

(In passing, I'll add that I'm not a great fan of these memrlimits:
to me it's loony to be charging people for virtual address space, it's _virtual_, and process A can have as much as it likes without affecting process B in any way. You're following the lead of RLIMIT_AS, but I've always thought RLIMIT_AS a lame attempt to move into the mmap decade, after RLIMIT_DATA and RLIMIT_STACK no longer made sense.

Taking Alan Cox's Committed_AS as a limited resource charged per mm makes much more sense to me: but yes, it's not perfect, and it is a lot harder to get its accounting right, and to maintain that down the line. Okay, you've gone for the easier option of tracking total_vm, getting that right is a more achievable target. And I accept that I may be too pessimistic about it: total_vm may often enough give a rough approximation to something else worth limiting.)