memcg: fix unit mismatch in memcg oom limit calculation

PATCH In review Johannes Weiner 4 participants 7 messages Nov 9, 2010 – Nov 16, 2010

Subsystems (3)

Memory management memcg Memory management - OOM killer

lore

Share & tools

lore

Johannes Weiner

author maintainer

2010-11-09 11:05 UTC #0 lore raw

PATCH 1/1 memcg: fix unit mismatch in memcg oom limit calculation

Adding the number of swap pages to the byte limit of a memory control group makes no sense. Convert the pages to bytes before adding them.

The only user of this code is the OOM killer, and the way it is used means that the error results in a higher OOM badness value. Since the cgroup limit is the same for all tasks in the cgroup, the error should have no practical impact at the moment.

But let's not wait for future or changing users to trip over it.

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

---
 mm/memcontrol.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

mm/memcontrol.c +3 -2

@@ -1552,8 +1552,9 @@ u64 mem_cgroup_get_limit(struct mem_cgro
 	u64 limit;
 	u64 memsw;
 
-	limit = res_counter_read_u64(&memcg->res, RES_LIMIT) +
-			total_swap_pages;
+	limit = res_counter_read_u64(&memcg->res, RES_LIMIT);
+	limit += total_swap_pages << PAGE_SHIFT;
+
 	memsw = res_counter_read_u64(&memcg->memsw, RES_LIMIT);
 	/*
 	 * If memsw is finite and limits the amount of swap space available

Greg Thelen

Reviewed-by on v1 1/1

2010-11-09 20:00 UTC #1 lore raw

Quoted 9 lines · Johannes Weiner

Adding the number of swap pages to the byte limit of a memory control group makes no sense. Convert the pages to bytes before adding them.
The only user of this code is the OOM killer, and the way it is used means that the error results in a higher OOM badness value. Since the cgroup limit is the same for all tasks in the cgroup, the error should have no practical impact at the moment.
But let's not wait for future or changing users to trip over it.

Thanks for the fix.

Quoted 1 line

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

Reviewed-by Greg Thelen <[email protected]>

Quoted 8 lines

---
 mm/memcontrol.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

mm/memcontrol.c +0 -0

@@ -1552,8 +1552,9 @@ u64 mem_cgroup_get_limit(struct mem_cgro
 	u64 limit;

mm/memcontrol.c u64 mem_cgroup_get_limit(struct mem_cgro · line 1560

Quoted 10 lines

mm/memcontrol.c +3 -2

 	u64 memsw;
 
-	limit = res_counter_read_u64(&memcg->res, RES_LIMIT) +
-			total_swap_pages;
+	limit = res_counter_read_u64(&memcg->res, RES_LIMIT);
+	limit += total_swap_pages << PAGE_SHIFT;
+
 	memsw = res_counter_read_u64(&memcg->memsw, RES_LIMIT);
 	/*
 	 * If memsw is finite and limits the amount of swap space available

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

David Rientjes

reviewer

2010-11-09 21:31 UTC #2 lore raw

Quoted 10 lines · Greg Thelen

Quoted 8 lines · Johannes Weiner
Adding the number of swap pages to the byte limit of a memory control group makes no sense. Convert the pages to bytes before adding them.
The only user of this code is the OOM killer, and the way it is used means that the error results in a higher OOM badness value. Since the cgroup limit is the same for all tasks in the cgroup, the error should have no practical impact at the moment.

Quoted 7 lines

Quoted 1 line
But let's not wait for future or changing users to trip over it.
Thanks for the fix.
Quoted 1 line
Signed-off-by Johannes Weiner <[email protected]>
Reviewed-by Greg Thelen <[email protected]>

Nice catch, but it's done in the opposite way: the oom killer doesn't use byte limits but page limits. So this needs to be

	(res_counter_read_u64(&memcg->res, RES_LIMIT) >> PAGE_SHIFT) +
			total_swap_pages;

Greg Thelen

2010-11-09 22:01 UTC #3 lore raw

Re: mem_cgroup_get_limit() return type, was [patch] memcg: fix unit mismatch in memcg oom limit calculation

Quoted 14 lines · David Rientjes

Quoted 12 lines · Greg Thelen
Quoted 9 lines · Johannes Weiner
Adding the number of swap pages to the byte limit of a memory control group makes no sense. Convert the pages to bytes before adding them.
The only user of this code is the OOM killer, and the way it is used means that the error results in a higher OOM badness value. Since the cgroup limit is the same for all tasks in the cgroup, the error should have no practical impact at the moment.
But let's not wait for future or changing users to trip over it.

Quoted 8 lines

Quoted 2 lines
Thanks for the fix.
Nice catch, but it's done in the opposite way: the oom killer doesn't use byte limits but page limits. So this needs to be
	(res_counter_read_u64(&memcg->res, RES_LIMIT) >> PAGE_SHIFT) +
			total_swap_pages;

In -mm, the oom killer queries memcg for a byte limit using mem_cgroup_get_limit(). The following is from mem_cgroup_out_of_memory():

	limit = mem_cgroup_get_limit(mem) >> PAGE_SHIFT;

So I think the "[patch] memcg: fix unit mismatch in memcg oom limit calculation" is correct. Although a simpler interface, would involve changing mem_cgroup_get_limit() to return a page count instead of a byte count and thus save the oom killer from having to do the conversion:

commit d12e5eded4505a673a7d77d8adab7fce30c7a680

Author: Greg Thelen <[email protected]>
Date:   Tue Nov 9 13:46:38 2010 -0800

memcg: change mem_cgroup_get_limit() return type

The mem_cgroup_get_limit() interface routine returns a byte count. The only consumer of this data is the oom killer, which really wants a page count.

This change converts mem_cgroup_get_limit() to return a page count rather than a byte count. This makes the memcg interface more consistent with the rest of the mm. This even makes the memcg interface more consistent. Most other memcg interface routines operate on page counts, not byte counts.

Signed-off-by Greg Thelen <[email protected]>

include/linux/memcontrol.h +2 -2

index 3433784..0a8720e 100644
@@ -163,7 +163,7 @@ unsigned long mem_cgroup_page_stat(struct mem_cgroup *mem,
 
 unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order,
 						gfp_t gfp_mask);
-u64 mem_cgroup_get_limit(struct mem_cgroup *mem);
+unsigned long mem_cgroup_get_limit(struct mem_cgroup *mem);
 
 #else /* CONFIG_CGROUP_MEM_RES_CTLR */
 struct mem_cgroup;

@@ -368,7 +368,7 @@ unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order,
 }
 
 static inline
-u64 mem_cgroup_get_limit(struct mem_cgroup *mem)
+unsigned long mem_cgroup_get_limit(struct mem_cgroup *mem)
 {
 	return 0;
 }

mm/memcontrol.c +7 -6

index 7b9ecdc..90efb5d 100644
@@ -1569,22 +1569,23 @@ static int mem_cgroup_count_children(struct mem_cgroup *mem)
 }
 
 /*
- * Return the memory (and swap, if configured) limit for a memcg.
+ * Return the memory (and swap, if configured) limit for a memcg expressed as
+ * a page count.
  */
-u64 mem_cgroup_get_limit(struct mem_cgroup *memcg)
+unsigned long mem_cgroup_get_limit(struct mem_cgroup *memcg)
 {
 	u64 limit;
 	u64 memsw;
 
-	limit = res_counter_read_u64(&memcg->res, RES_LIMIT);
-	limit += total_swap_pages << PAGE_SHIFT;
+	limit = res_counter_read_u64(&memcg->res, RES_LIMIT) >> PAGE_SHIFT;
+	limit += total_swap_pages;
 
-	memsw = res_counter_read_u64(&memcg->memsw, RES_LIMIT);
+	memsw = res_counter_read_u64(&memcg->memsw, RES_LIMIT) >> PAGE_SHIFT;
 	/*
 	 * If memsw is finite and limits the amount of swap space available
 	 * to this memcg, return that limit.
 	 */
-	return min(limit, memsw);
+	return min(min(limit, memsw), ULONG_MAX);
 }
 
 /*

mm/oom_kill.c +1 -1

index 7dcca55..9ccc59f 100644
@@ -538,7 +538,7 @@ void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
 	struct task_struct *p;
 
 	check_panic_on_oom(CONSTRAINT_MEMCG, gfp_mask, 0, NULL);
-	limit = mem_cgroup_get_limit(mem) >> PAGE_SHIFT;
+	limit = mem_cgroup_get_limit(mem);
 	read_lock(&tasklist_lock);
 retry:
 	p = select_bad_process(&points, limit, mem, NULL);

David Rientjes

reviewer

2010-11-09 23:22 UTC #4 lore raw

Re: mem_cgroup_get_limit() return type, was [patch] memcg: fix unit mismatch in memcg oom limit calculation

Quoted 16 lines · Greg Thelen

Quoted 16 lines
Quoted 12 lines
Quoted 9 lines
Adding the number of swap pages to the byte limit of a memory control group makes no sense. Convert the pages to bytes before adding them.
The only user of this code is the OOM killer, and the way it is used means that the error results in a higher OOM badness value. Since the cgroup limit is the same for all tasks in the cgroup, the error should have no practical impact at the moment.
But let's not wait for future or changing users to trip over it.
Thanks for the fix.
Nice catch, but it's done in the opposite way: the oom killer doesn't use byte limits but page limits. So this needs to be

Quoted 9 lines

Quoted 2 lines
	(res_counter_read_u64(&memcg->res, RES_LIMIT) >> PAGE_SHIFT) +
			total_swap_pages;
In -mm, the oom killer queries memcg for a byte limit using mem_cgroup_get_limit(). The following is from mem_cgroup_out_of_memory():
	limit = mem_cgroup_get_limit(mem) >> PAGE_SHIFT;

Oops, I missed that. I think Johannes' patch is better because mem_cgroup_get_limit() may eventually be used elsewhere and the subsystem has byte granularity.

Thanks!

Greg Thelen

2010-11-10 01:28 UTC #5 lore raw

Re: mem_cgroup_get_limit() return type, was [patch] memcg: fix unit mismatch in memcg oom limit calculation

Quoted 25 lines · David Rientjes

Quoted 23 lines · Greg Thelen
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Adding the number of swap pages to the byte limit of a memory control group makes no sense. Convert the pages to bytes before adding them.
The only user of this code is the OOM killer, and the way it is used means that the error results in a higher OOM badness value. Since the cgroup limit is the same for all tasks in the cgroup, the error should have no practical impact at the moment.
But let's not wait for future or changing users to trip over it.
Quoted 8 lines
Quoted 2 lines
Thanks for the fix.
Nice catch, but it's done in the opposite way: the oom killer doesn't use byte limits but page limits. So this needs to be
	(res_counter_read_u64(&memcg->res, RES_LIMIT) >> PAGE_SHIFT) +
			total_swap_pages;
In -mm, the oom killer queries memcg for a byte limit using mem_cgroup_get_limit(). The following is from mem_cgroup_out_of_memory():

Quoted 6 lines

Quoted 2 lines
	limit = mem_cgroup_get_limit(mem) >> PAGE_SHIFT;
Oops, I missed that. I think Johannes' patch is better because mem_cgroup_get_limit() may eventually be used elsewhere and the subsystem has byte granularity.

I have no problem with mem_cgroup_get_limit() returning a byte count as you prefer. I think this approach does have an issue. "mem_cgroup_get_limit() >> PAGE_SHIFT" may not fit within unsigned long on 32-bit machines. Does this cause a problem for the 'limit' local variable in mem_cgroup_out_of_memory():
limit = mem_cgroup_get_limit(mem) >> PAGE_SHIFT;

Do we need something like the following in mem_cgroup_out_of_memory() to guard against this overflow?
limit = min(mem_cgroup_get_limit(mem) >> PAGE_SHIFT, ULONG_MAX);

KAMEZAWA Hiroyuki

Acked-by on v1 1/1

2010-11-16 04:08 UTC #6 lore raw

In reply to Johannes Weiner

Quoted 11 lines · On Tue, 9 Nov 2010 12:05:21 +0100 Johannes Weiner <[email protected]> wrote:

Adding the number of swap pages to the byte limit of a memory control group makes no sense. Convert the pages to bytes before adding them.
The only user of this code is the OOM killer, and the way it is used means that the error results in a higher OOM badness value. Since the cgroup limit is the same for all tasks in the cgroup, the error should have no practical impact at the moment.
But let's not wait for future or changing users to trip over it.
Signed-off-by Johannes Weiner <[email protected]>

Acked-by KAMEZAWA Hiroyuki <[email protected]>