The charging code can encounter a charge size that is bigger than a regular page in two situations: one is a batched charge to fill the per-cpu stocks, the other is a huge page charge.

This code is distributed over two functions, however, and only the outer one is aware of huge pages.  In case the charging fails, the inner function will tell the outer function to retry if the charge size is bigger than regular pages--assuming batched charging is the only case.  And the outer function will retry forever charging a huge page.

This patch makes sure the inner function can distinguish between batch charging and a single huge page charge.  It will only signal another attempt if batch charging failed, and go into regular reclaim when it is called on behalf of a huge page.

Signed-off-by Johannes Weiner <[email protected]>

The charging code can encounter a charge size that is bigger than a regular page in two situations: one is a batched charge to fill the per-cpu stocks, the other is a huge page charge.

This code is distributed over two functions, however, and only the outer one is aware of huge pages. In case the charging fails, the inner function will tell the outer function to retry if the charge size is bigger than regular pages--assuming batched charging is the only case. And the outer function will retry forever charging a huge page.

This patch makes sure the inner function can distinguish between batch charging and a single huge page charge. It will only signal another attempt if batch charging failed, and go into regular reclaim when it is called on behalf of a huge page.

Signed-off-by Johannes Weiner <[email protected]>

---
 mm/memcontrol.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

mm/memcontrol.c +9 -2

index d572102..73ea323 100644
@@ -1837,8 +1837,15 @@ static int __mem_cgroup_do_charge(struct mem_cgroup *mem, gfp_t gfp_mask,
 		flags |= MEM_CGROUP_RECLAIM_NOSWAP;
 	} else
 		mem_over_limit = mem_cgroup_from_res_counter(fail_res, res);
-
-	if (csize > PAGE_SIZE) /* change csize and retry */
+	/*
+	 * csize can be either a huge page (HPAGE_SIZE), a batch of
+	 * regular pages (CHARGE_SIZE), or a single regular page
+	 * (PAGE_SIZE).
+	 *
+	 * Never reclaim on behalf of optional batching, retry with a
+	 * single page instead.
+	 */
+	if (csize == CHARGE_SIZE)
 		return CHARGE_RETRY;
 
 	if (!(gfp_mask & __GFP_WAIT))

Signature · 1 line

1.7.3.5

include/linux/res_counter.h +12 -0

+static inline bool res_counter_check_margin(struct res_counter *cnt,
+					    unsigned long bytes)
+{
+	bool ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cnt->lock, flags);
+	ret = cnt->limit - cnt->usage >= bytes;
+	spin_unlock_irqrestore(&cnt->lock, flags);
+	return ret;
+}
+
 static inline bool res_counter_check_under_soft_limit(struct res_counter *cnt)
 {
 	bool ret;

mm/memcontrol.c +0 -0

index 73ea323..c28072f 100644
@@ -1111,6 +1111,15 @@ static bool mem_cgroup_check_under_limit(struct mem_cgroup *mem)
 	return false;

Quoted 21 lines · On Mon, 31 Jan 2011 15:03:54 +0100 Johannes Weiner <[email protected]> wrote:

include/linux/res_counter.h +12 -0

+static inline bool res_counter_check_margin(struct res_counter *cnt,
+					    unsigned long bytes)
+{
+	bool ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cnt->lock, flags);
+	ret = cnt->limit - cnt->usage >= bytes;
+	spin_unlock_irqrestore(&cnt->lock, flags);
+	return ret;
+}
+
 static inline bool res_counter_check_under_soft_limit(struct res_counter *cnt)
 {
 	bool ret;

mm/memcontrol.c +0 -0

index 73ea323..c28072f 100644
@@ -1111,6 +1111,15 @@ static bool mem_cgroup_check_under_limit(struct mem_cgroup *mem)
 	return false;

mm/memcontrol.c static bool mem_cgroup_check_under_limit(struct mem_cgroup *mem) · line 1121

Quoted 10 lines

mm/memcontrol.c +8 -0

 }
 
+static bool mem_cgroup_check_margin(struct mem_cgroup *mem, unsigned long bytes)
+{
+	if (!res_counter_check_margin(&mem->res, bytes))
+		return false;
+	if (do_swap_account && !res_counter_check_margin(&mem->memsw, bytes))
+		return false;
+	return true;
+}

argh.

If you ever have a function with the string "check" in its name, it's a good sign that you did something wrong.

Check what? Against what? Returning what?

mem_cgroup_check_under_limit() isn't toooo bad - the name tells you what's being checked and tells you what to expect the return value to mean.

But "res_counter_check_margin" and "mem_cgroup_check_margin" are just awful. Something like

bool res_counter_may_charge(counter, bytes)

would be much clearer.

If we really want to stick with the "check" names (perhaps as an ironic reference to res_counter's past mistakes) then please at least document the sorry things?

Review static bool mem_cgroup_check_under_limit(struct mem_cgroup *mem) · line 1121

Quoted 12 lines · On Mon, 31 Jan 2011 15:03:54 +0100 Johannes Weiner <[email protected]> wrote:

(quoted diff) +8 -0

@@ -1111,6 +1111,15 @@ static bool mem_cgroup_check_under_limit(struct mem_cgroup *mem)
 	return false;
 }
 
+static bool mem_cgroup_check_margin(struct mem_cgroup *mem, unsigned long bytes)
+{
+	if (!res_counter_check_margin(&mem->res, bytes))
+		return false;
+	if (do_swap_account && !res_counter_check_margin(&mem->memsw, bytes))
+		return false;
+	return true;
+}

argh.

If you ever have a function with the string "check" in its name, it's a good sign that you did something wrong.

Check what? Against what? Returning what?

mem_cgroup_check_under_limit() isn't toooo bad - the name tells you what's being checked and tells you what to expect the return value to mean.

But "res_counter_check_margin" and "mem_cgroup_check_margin" are just awful. Something like

bool res_counter_may_charge(counter, bytes)

would be much clearer.

If we really want to stick with the "check" names (perhaps as an ironic reference to res_counter's past mistakes) then please at least document the sorry things?

Maybe it would be better to use res_counter_margin(cnt) >= wanted throughout the code.

include/linux/res_counter.h +1 -0

@@ -182,6 +182,14 @@ static inline bool res_counter_check_under_limit(struct res_counter *cnt)
 	return ret;
 }
 
+/**

Quoted 4 lines · On Tue, 1 Feb 2011 01:04:55 +0100 Johannes Weiner <[email protected]> wrote:

(quoted diff) +0 -0

@@ -182,6 +182,14 @@ static inline bool res_counter_check_under_limit(struct res_counter *cnt)
 	return ret;
 }
 

Review static inline bool res_counter_check_under_limit(struct res_counter *cnt) · line 195

Quoted 11 lines

(quoted diff) +8 -0

+/**
+ * res_counter_check_margin - check if the counter allows charging
+ * @cnt: the resource counter to check
+ * @bytes: the number of bytes to check the remaining space against
+ *
+ * Returns a boolean value on whether the counter can be charged
+ * @bytes or whether this would exceed the limit.
+ */
 static inline bool res_counter_check_margin(struct res_counter *cnt,
 					    unsigned long bytes)
 {

mem_cgroup_check_margin() needs some lipstick too.

Quoted 2 lines · On Tue, 1 Feb 2011 01:04:55 +0100 Johannes Weiner <[email protected]> wrote:

Maybe it would be better to use res_counter_margin(cnt) >= wanted throughout the code.

yup.

Soft limit reclaim continues until the usage is below the current soft limit, but the documented semantics are actually that soft limit reclaim will push usage back until the soft limits are met again.

Signed-off-by Johannes Weiner <[email protected]>

Soft limit reclaim continues until the usage is below the current soft limit, but the documented semantics are actually that soft limit reclaim will push usage back until the soft limits are met again.

Signed-off-by Johannes Weiner <[email protected]>

Since transparent huge pages, checking whether memory cgroups are below their limits is no longer enough, but the actual amount of chargeable space is important.

To not have more than one limit-checking interface, replace memory_cgroup_check_under_limit() and memory_cgroup_check_margin() with a single memory_cgroup_margin() that returns the chargeable space and leaves the comparison to the callsite.

Soft limits are now checked the other way round, by using the already existing function that returns the amount by which soft limits are
exceeded: res_counter_soft_limit_excess().

Also remove all the corresponding functions on the res_counter side that are now no longer used.

Signed-off-by Johannes Weiner <[email protected]>

Since transparent huge pages, checking whether memory cgroups are below their limits is no longer enough, but the actual amount of chargeable space is important.

To not have more than one limit-checking interface, replace memory_cgroup_check_under_limit() and memory_cgroup_check_margin() with a single memory_cgroup_margin() that returns the chargeable space and leaves the comparison to the callsite.

Soft limits are now checked the other way round, by using the already existing function that returns the amount by which soft limits are
exceeded: res_counter_soft_limit_excess().

Also remove all the corresponding functions on the res_counter side that are now no longer used.

Signed-off-by Johannes Weiner <[email protected]>

If reclaim after a failed charging was unsuccessful, the limits are checked again, just in case they settled by means of other tasks.

This is all fine as long as every charge is of size PAGE_SIZE, because in that case, being below the limit means having at least PAGE_SIZE bytes available.

But with transparent huge pages, we may end up in an endless loop where charging and reclaim fail, but we keep going because the limits are not yet exceeded, although not allowing for a huge page.

Fix this up by explicitely checking for enough room, not just whether we are within limits.

Signed-off-by Johannes Weiner <[email protected]>

Huge page coverage should obviously have less priority than the continued execution of a process.

Never kill a process when charging it a huge page fails.  Instead, give up after the first failed reclaim attempt and fall back to regular pages.

Signed-off-by Johannes Weiner <[email protected]>

Huge page coverage should obviously have less priority than the continued execution of a process.

Never kill a process when charging it a huge page fails. Instead, give up after the first failed reclaim attempt and fall back to regular pages.

Signed-off-by Johannes Weiner <[email protected]>