memcg : more fixes and clean up for 2.6.28-rc

From: KAMEZAWA Hiroyuki <[email protected]>

A fix for 987eba66e0e6aa654d60881a14731a353ee0acb4

A clean up for mem_cgroup_move_parent().
- remove unnecessary initialization of local variable.
- rename charge_size -> page_size
- remove unnecessary (wrong) comment.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

From: KAMEZAWA Hiroyuki <[email protected]>

A fix for 987eba66e0e6aa654d60881a14731a353ee0acb4

A clean up for mem_cgroup_move_parent().
- remove unnecessary initialization of local variable.
- rename charge_size -> page_size
- remove unnecessary (wrong) comment.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

---
 mm/memcontrol.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Index: mmotm-0107/mm/memcontrol.c
===================================================================

mm/memcontrol.c +1 -1

@@ -2265,7 +2265,7 @@ static int mem_cgroup_move_parent(struct
 	struct cgroup *cg = child->css.cgroup;
 	struct cgroup *pcg = cg->parent;
 	struct mem_cgroup *parent;
-	int charge = PAGE_SIZE;
+	int page_size;
 	unsigned long flags;
 	int ret;
 

Quoted 27 lines · KAMEZAWA Hiroyuki

From: KAMEZAWA Hiroyuki <[email protected]>

A fix for 987eba66e0e6aa654d60881a14731a353ee0acb4

A clean up for mem_cgroup_move_parent().
- remove unnecessary initialization of local variable.
- rename charge_size -> page_size
- remove unnecessary (wrong) comment.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

---
 mm/memcontrol.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Index: mmotm-0107/mm/memcontrol.c
===================================================================

mm/memcontrol.c +1 -1

@@ -2265,7 +2265,7 @@ static int mem_cgroup_move_parent(struct
 	struct cgroup *cg = child->css.cgroup;
 	struct cgroup *pcg = cg->parent;
 	struct mem_cgroup *parent;
-	int charge = PAGE_SIZE;
+	int page_size;
 	unsigned long flags;
 	int ret;
 

mm/memcontrol.c static int mem_cgroup_move_parent(struct · line 2282

Quoted 8 lines

mm/memcontrol.c +2 -2

@@ -2278,22 +2278,23 @@ static int mem_cgroup_move_parent(struct
 		goto out;
 	if (isolate_lru_page(page))
 		goto put;
-	/* The page is isolated from LRU and we have no race with splitting */
-	charge = PAGE_SIZE << compound_order(page);
+
+	page_size = PAGE_SIZE << compound_order(page);

Okay, so you remove the wrong comment, but that does not make the code right. What protects compound_order from reading garbage because the page is currently splitting?

Quoted 28 lines · On Mon, 24 Jan 2011 11:14:02 +0100 Johannes Weiner <[email protected]> wrote:

Quoted 27 lines · KAMEZAWA Hiroyuki

From: KAMEZAWA Hiroyuki <[email protected]>

A fix for 987eba66e0e6aa654d60881a14731a353ee0acb4

A clean up for mem_cgroup_move_parent().
- remove unnecessary initialization of local variable.
- rename charge_size -> page_size
- remove unnecessary (wrong) comment.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

---
 mm/memcontrol.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Index: mmotm-0107/mm/memcontrol.c
===================================================================

mm/memcontrol.c +1 -1

@@ -2265,7 +2265,7 @@ static int mem_cgroup_move_parent(struct
 	struct cgroup *cg = child->css.cgroup;
 	struct cgroup *pcg = cg->parent;
 	struct mem_cgroup *parent;
-	int charge = PAGE_SIZE;
+	int page_size;
 	unsigned long flags;
 	int ret;
 

Quoted 13 lines

mm/memcontrol.c static int mem_cgroup_move_parent(struct · line 2282

Quoted 8 lines

mm/memcontrol.c +2 -2

@@ -2278,22 +2278,23 @@ static int mem_cgroup_move_parent(struct
 		goto out;
 	if (isolate_lru_page(page))
 		goto put;
-	/* The page is isolated from LRU and we have no race with splitting */
-	charge = PAGE_SIZE << compound_order(page);
+
+	page_size = PAGE_SIZE << compound_order(page);

Okay, so you remove the wrong comment, but that does not make the code right. What protects compound_order from reading garbage because the page is currently splitting?

==
static int mem_cgroup_move_account(struct page_cgroup *pc,
struct mem_cgroup *from, struct mem_cgroup *to, bool uncharge, int charge_size)

{
        int ret = -EINVAL;
        unsigned long flags;

        if ((charge_size > PAGE_SIZE) && !PageTransHuge(pc->page))
                return -EBUSY;

==

This is called under compound_lock(). Then, if someone breaks THP, -EBUSY and retry.

Quoted 57 lines

Quoted 28 lines · On Mon, 24 Jan 2011 11:14:02 +0100 Johannes Weiner <[email protected]> wrote:

Quoted 27 lines · KAMEZAWA Hiroyuki

From: KAMEZAWA Hiroyuki <[email protected]>

A fix for 987eba66e0e6aa654d60881a14731a353ee0acb4

A clean up for mem_cgroup_move_parent().
 - remove unnecessary initialization of local variable.
 - rename charge_size -> page_size
 - remove unnecessary (wrong) comment.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

---
 mm/memcontrol.c |   17 +++++++++--------

 1 file changed, 9 insertions(+), 8 deletions(-)

Index: mmotm-0107/mm/memcontrol.c
===================================================================

mm/memcontrol.c +1 -1

@@ -2265,7 +2265,7 @@ static int mem_cgroup_move_parent(struct
   struct cgroup *cg = child->css.cgroup;
   struct cgroup *pcg = cg->parent;
   struct mem_cgroup *parent;
- int charge = PAGE_SIZE;
+ int page_size;
   unsigned long flags;
   int ret;
 

Quoted 13 lines

mm/memcontrol.c static int mem_cgroup_move_parent(struct · line 2282

Quoted 8 lines

mm/memcontrol.c +2 -2

@@ -2278,22 +2278,23 @@ static int mem_cgroup_move_parent(struct
           goto out;
   if (isolate_lru_page(page))
           goto put;
- /* The page is isolated from LRU and we have no race with splitting */
- charge = PAGE_SIZE << compound_order(page);
+
+ page_size = PAGE_SIZE << compound_order(page);

Okay, so you remove the wrong comment, but that does not make the code right.  What protects compound_order from reading garbage because the page is currently splitting?

==
static int mem_cgroup_move_account(struct page_cgroup *pc, struct mem_cgroup *from, struct mem_cgroup *to, bool uncharge, int charge_size)
{
        int ret = -EINVAL;
        unsigned long flags;

        if ((charge_size > PAGE_SIZE) && !PageTransHuge(pc->page)) return -EBUSY;
==

Quoted 2 lines

This is called under compound_lock(). Then, if someone breaks THP, -EBUSY and retry.

This charge_size contains exactly the garbage you just read from an unprotected compound_order().  It could be anything if the page is split concurrently.

2011/1/24 Johannes Weiner <[email protected]>:

Quoted 58 lines · KAMEZAWA Hiroyuki

Quoted 57 lines

Quoted 28 lines · On Mon, 24 Jan 2011 11:14:02 +0100 Johannes Weiner <[email protected]> wrote:

Quoted 27 lines · KAMEZAWA Hiroyuki

From: KAMEZAWA Hiroyuki <[email protected]>

A fix for 987eba66e0e6aa654d60881a14731a353ee0acb4

A clean up for mem_cgroup_move_parent().
- remove unnecessary initialization of local variable.
- rename charge_size -> page_size
- remove unnecessary (wrong) comment.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

---
 mm/memcontrol.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Index: mmotm-0107/mm/memcontrol.c
===================================================================

mm/memcontrol.c +1 -1

@@ -2265,7 +2265,7 @@ static int mem_cgroup_move_parent(struct
  struct cgroup *cg = child->css.cgroup;
  struct cgroup *pcg = cg->parent;
  struct mem_cgroup *parent;
- int charge = PAGE_SIZE;
+ int page_size;
  unsigned long flags;
  int ret;
 

Quoted 13 lines

mm/memcontrol.c static int mem_cgroup_move_parent(struct · line 2282

Quoted 8 lines

mm/memcontrol.c +2 -2

@@ -2278,22 +2278,23 @@ static int mem_cgroup_move_parent(struct
          goto out;
  if (isolate_lru_page(page))
          goto put;
- /* The page is isolated from LRU and we have no race with splitting */
- charge = PAGE_SIZE << compound_order(page);
+
+ page_size = PAGE_SIZE << compound_order(page);

Okay, so you remove the wrong comment, but that does not make the code right. What protects compound_order from reading garbage because the page is currently splitting?

==
static int mem_cgroup_move_account(struct page_cgroup *pc,
struct mem_cgroup *from, struct mem_cgroup *to, bool uncharge, int charge_size)

{
        int ret = -EINVAL;
        unsigned long flags;

        if ((charge_size > PAGE_SIZE) && !PageTransHuge(pc->page))
                return -EBUSY;

==

Quoted 6 lines

Quoted 2 lines

This is called under compound_lock(). Then, if someone breaks THP, -EBUSY and retry.

This charge_size contains exactly the garbage you just read from an unprotected compound_order(). It could be anything if the page is split concurrently.

Then, my recent fix to LRU accounting which use compound_order() is racy, too ?

I'll replace compound_order() with
if (PageTransHuge(page))
size = HPAGE_SIZE.

Does this work ?

From: KAMEZAWA Hiroyuki <[email protected]>

This is a fix for
ca3e021417eed30ec2b64ce88eb0acf64aa9bc29

mem_cgroup_disabled() should be checked at splitting.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

From: KAMEZAWA Hiroyuki <[email protected]>

This is a fix for
ca3e021417eed30ec2b64ce88eb0acf64aa9bc29

mem_cgroup_disabled() should be checked at splitting.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

From: KAMEZAWA Hiroyuki <[email protected]>

Current memory cgroup's code tends to assume page_size == PAGE_SIZE but THP does HPAGE_SIZE charge.

This is one of fixes for supporing THP. This modifies mem_cgroup_check_under_limit to take page_size into account.

Total fixes for do_charge()/reclaim memory will follow this patch.

TODO: By this reclaim function can get page_size as argument. So...there may be something should be improvoed.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

From: KAMEZAWA Hiroyuki <[email protected]>

Current memory cgroup's code tends to assume page_size == PAGE_SIZE but THP does HPAGE_SIZE charge.

This is one of fixes for supporing THP. This modifies mem_cgroup_check_under_limit to take page_size into account.

Total fixes for do_charge()/reclaim memory will follow this patch.

TODO: By this reclaim function can get page_size as argument. So...there may be something should be improvoed.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

---
 include/linux/res_counter.h |   11 +++++++++++
 mm/memcontrol.c             |   27 ++++++++++++++-------------
 2 files changed, 25 insertions(+), 13 deletions(-)

Index: mmotm-0107/include/linux/res_counter.h
===================================================================

include/linux/res_counter.h +11 -0

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

Index: mmotm-0107/mm/memcontrol.c
===================================================================

mm/memcontrol.c +8 -7

@@ -1099,14 +1099,14 @@ unsigned long mem_cgroup_isolate_pages(u
 #define mem_cgroup_from_res_counter(counter, member)	\
 	container_of(counter, struct mem_cgroup, member)
 
-static bool mem_cgroup_check_under_limit(struct mem_cgroup *mem)
+static bool mem_cgroup_check_under_limit(struct mem_cgroup *mem, int page_size)
 {
 	if (do_swap_account) {
-		if (res_counter_check_under_limit(&mem->res) &&
-			res_counter_check_under_limit(&mem->memsw))
+		if (res_counter_check_margin(&mem->res) >= page_size &&
+			res_counter_check_margin(&mem->memsw) >= page_size)
 			return true;
 	} else
-		if (res_counter_check_under_limit(&mem->res))
+		if (res_counter_check_margin(&mem->res) >= page_size)
 			return true;
 	return false;
 }

@@ -1367,7 +1367,8 @@ mem_cgroup_select_victim(struct mem_cgro
 static int mem_cgroup_hierarchical_reclaim(struct mem_cgroup *root_mem,
 						struct zone *zone,
 						gfp_t gfp_mask,
-						unsigned long reclaim_options)
+						unsigned long reclaim_options,
+						int page_size)
 {
 	struct mem_cgroup *victim;
 	int ret, total = 0;

@@ -1434,7 +1435,7 @@ static int mem_cgroup_hierarchical_recla
 		if (check_soft) {
 			if (res_counter_check_under_soft_limit(&root_mem->res))
 				return total;
-		} else if (mem_cgroup_check_under_limit(root_mem))
+		} else if (mem_cgroup_check_under_limit(root_mem, page_size))
 			return 1 + total;
 	}
 	return total;

@@ -1844,7 +1845,7 @@ static int __mem_cgroup_do_charge(struct
 		return CHARGE_WOULDBLOCK;
 
 	ret = mem_cgroup_hierarchical_reclaim(mem_over_limit, NULL,
-					gfp_mask, flags);
+					gfp_mask, flags, csize);
 	/*
 	 * try_to_free_mem_cgroup_pages() might not give us a full
 	 * picture of reclaim. Some pages are reclaimed and might be

Quoted 7 lines

mm/memcontrol.c +1 -1

 	ret = mem_cgroup_hierarchical_reclaim(mem_over_limit, NULL,
-					gfp_mask, flags);
+					gfp_mask, flags, csize);
 	/*
 	 * try_to_free_mem_cgroup_pages() might not give us a full
 	 * picture of reclaim. Some pages are reclaimed and might be

@@ -1852,7 +1853,7 @@ static int __mem_cgroup_do_charge(struct

mm/memcontrol.c static int __mem_cgroup_do_charge(struct · line 1857

Quoted 6 lines

mm/memcontrol.c +1 -1

 	 * Check the limit again to see if the reclaim reduced the
 	 * current usage of the cgroup before giving up
 	 */
-	if (ret || mem_cgroup_check_under_limit(mem_over_limit))
+	if (ret || mem_cgroup_check_under_limit(mem_over_limit, csize))
 		return CHARGE_RETRY;

This is the only site that is really involved with THP.

But you need to touch every site because you change mem_cgroup_check_under_limit() instead of adding a new function.

I would suggest just adding another function for checking available space explicitely and only changing this single call site to use it.

Just ignore the return value of mem_cgroup_hierarchical_reclaim() and check for enough space unconditionally.

Everybody else is happy with PAGE_SIZE pages.

From: KAMEZAWA Hiroyuki <[email protected]>

When THP is used, Hugepage size charge can happen. It's not handled correctly in mem_cgroup_do_charge(). For example, THP can fallback to small page allocation when HUGEPAGE allocation seems difficult or busy, but memory cgroup doesn't understand it and continue to try HUGEPAGE charging. And the worst thing is memory cgroup believes 'memory reclaim succeeded' if limit - usage > PAGE_SIZE.

By this, khugepaged etc...can goes into inifinite reclaim loop if tasks in memcg are busy.

After this patch
- Hugepage allocation will fail if 1st trial of page reclaim fails.
- distinguish THP allocaton from Bached allocation.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

---
 mm/memcontrol.c |   51 +++++++++++++++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 16 deletions(-)

Index: mmotm-0107/mm/memcontrol.c
===================================================================

mm/memcontrol.c +0 -0

@@ -1812,24 +1812,25 @@ enum {
 	CHARGE_OK,		/* success */

mm/memcontrol.c +3 -3

 {
 	struct mem_cgroup *mem_over_limit;
 	struct res_counter *fail_res;
 	unsigned long flags = 0;
 	int ret;
 
-	ret = res_counter_charge(&mem->res, csize, &fail_res);
+	ret = res_counter_charge(&mem->res, page_size, &fail_res);
 
 	if (likely(!ret)) {
 		if (!do_swap_account)
 			return CHARGE_OK;
-		ret = res_counter_charge(&mem->memsw, csize, &fail_res);
+		ret = res_counter_charge(&mem->memsw, page_size, &fail_res);
 		if (likely(!ret))
 			return CHARGE_OK;
 

@@ -1838,14 +1839,14 @@ static int __mem_cgroup_do_charge(struct
 	} else
 		mem_over_limit = mem_cgroup_from_res_counter(fail_res, res);
 
-	if (csize > PAGE_SIZE) /* change csize and retry */
+	if (!do_reclaim)
 		return CHARGE_RETRY;
 
 

From the very beginning, do we need this "CHARGE_RETRY" ?

mm/memcontrol.c +15 -6

 	if (!(gfp_mask & __GFP_WAIT))
 		return CHARGE_WOULDBLOCK;
 
 	ret = mem_cgroup_hierarchical_reclaim(mem_over_limit, NULL,
-					gfp_mask, flags, csize);
+					gfp_mask, flags, page_size);
 	/*
 	 * try_to_free_mem_cgroup_pages() might not give us a full
 	 * picture of reclaim. Some pages are reclaimed and might be

@@ -1853,19 +1854,28 @@ static int __mem_cgroup_do_charge(struct
 	 * Check the limit again to see if the reclaim reduced the
 	 * current usage of the cgroup before giving up
 	 */
-	if (ret || mem_cgroup_check_under_limit(mem_over_limit, csize))
+	if (ret || mem_cgroup_check_under_limit(mem_over_limit, page_size))
 		return CHARGE_RETRY;
 
 	/*
+	 * When page_size > PAGE_SIZE, THP calls this function and it's
+	 * ok to tell 'there are not enough pages for hugepage'. THP will
+	 * fallback into PAGE_SIZE allocation. If we do reclaim eagerly,
+	 * page splitting will occur and it seems much worse.
+	 */
+	if (page_size > PAGE_SIZE)
+		return CHARGE_NEED_BREAK;
+
+	/*
 	 * At task move, charge accounts can be doubly counted. So, it's
 	 * better to wait until the end of task_move if something is going on.
 	 */
 	if (mem_cgroup_wait_acct_move(mem_over_limit))
 		return CHARGE_RETRY;
-
 	/* If we don't need to call oom-killer at el, return immediately */
 	if (!oom_check)
 		return CHARGE_NOMEM;
+
 	/* check OOM */
 	if (!mem_cgroup_handle_oom(mem_over_limit, gfp_mask))
 		return CHARGE_OOM_DIE;

@@ -1885,7 +1895,7 @@ static int __mem_cgroup_try_charge(struc
 	int nr_oom_retries = MEM_CGROUP_RECLAIM_RETRIES;
 	struct mem_cgroup *mem = NULL;
 	int ret;
-	int csize = max(CHARGE_SIZE, (unsigned long) page_size);
+	bool use_pcp_cache = (page_size == PAGE_SIZE);
 
 	/*
 	 * Unlike gloval-vm's OOM-kill, we're not in memory shortage

@@ -1910,7 +1920,7 @@ again:
 		VM_BUG_ON(css_is_removed(&mem->css));
 		if (mem_cgroup_is_root(mem))
 			goto done;
-		if (page_size == PAGE_SIZE && consume_stock(mem))
+		if (use_pcp_cache && consume_stock(mem))
 			goto done;
 		css_get(&mem->css);
 	} else {

@@ -1933,7 +1943,7 @@ again:
 			rcu_read_unlock();
 			goto done;
 		}
-		if (page_size == PAGE_SIZE && consume_stock(mem)) {
+		if (use_pcp_cache && consume_stock(mem)) {
 			/*
 			 * It seems dagerous to access memcg without css_get().
 			 * But considering how consume_stok works, it's not

@@ -1967,17 +1977,26 @@ again:
 			oom_check = true;
 			nr_oom_retries = MEM_CGROUP_RECLAIM_RETRIES;

mm/memcontrol.c +0 -0

 		switch (ret) {
 		case CHARGE_OK:
 			break;
 		case CHARGE_RETRY: /* not in OOM situation but retry */

Quoted 27 lines · On Fri, 21 Jan 2011 15:44:30 +0900 KAMEZAWA Hiroyuki…

From: KAMEZAWA Hiroyuki <[email protected]>

When THP is used, Hugepage size charge can happen. It's not handled correctly in mem_cgroup_do_charge(). For example, THP can fallback to small page allocation when HUGEPAGE allocation seems difficult or busy, but memory cgroup doesn't understand it and continue to try HUGEPAGE charging. And the worst thing is memory cgroup believes 'memory reclaim succeeded' if limit - usage > PAGE_SIZE.

By this, khugepaged etc...can goes into inifinite reclaim loop if tasks in memcg are busy.

After this patch
- Hugepage allocation will fail if 1st trial of page reclaim fails.
- distinguish THP allocaton from Bached allocation.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

---
 mm/memcontrol.c |   51 +++++++++++++++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 16 deletions(-)

Index: mmotm-0107/mm/memcontrol.c
===================================================================

mm/memcontrol.c +0 -0

@@ -1812,24 +1812,25 @@ enum {
 	CHARGE_OK,		/* success */

mm/memcontrol.c enum { · line 1821

Quoted 10 lines

mm/memcontrol.c +2 -1

 	CHARGE_RETRY,		/* need to retry but retry is not bad */
 	CHARGE_NOMEM,		/* we can't do more. return -ENOMEM */
+	CHARGE_NEED_BREAK,	/* big size allocation failure */
 	CHARGE_WOULDBLOCK,	/* GFP_WAIT wasn't set and no enough res. */
 	CHARGE_OOM_DIE,		/* the current is killed because of OOM */
 };
 
 static int __mem_cgroup_do_charge(struct mem_cgroup *mem, gfp_t gfp_mask,
-				int csize, bool oom_check)
+			int page_size, bool do_reclaim, bool oom_check)

I'm sorry, I can't understand why we need 'do_reclaim'. See below.

Quoted 21 lines

mm/memcontrol.c +2 -2

 {
 	struct mem_cgroup *mem_over_limit;
 	struct res_counter *fail_res;
 	unsigned long flags = 0;
 	int ret;
 
-	ret = res_counter_charge(&mem->res, csize, &fail_res);
+	ret = res_counter_charge(&mem->res, page_size, &fail_res);
 
 	if (likely(!ret)) {
 		if (!do_swap_account)
 			return CHARGE_OK;
-		ret = res_counter_charge(&mem->memsw, csize, &fail_res);
+		ret = res_counter_charge(&mem->memsw, page_size, &fail_res);
 		if (likely(!ret))
 			return CHARGE_OK;
 

@@ -1838,14 +1839,14 @@ static int __mem_cgroup_do_charge(struct
 	} else
 		mem_over_limit = mem_cgroup_from_res_counter(fail_res, res);
 

mm/memcontrol.c static int __mem_cgroup_do_charge(struct · line 1844

Quoted 4 lines

mm/memcontrol.c +1 -1

-	if (csize > PAGE_SIZE) /* change csize and retry */
+	if (!do_reclaim)
 		return CHARGE_RETRY;
 

From the very beginning, do we need this "CHARGE_RETRY" ?

Quoted 77 lines

mm/memcontrol.c +19 -8

 	if (!(gfp_mask & __GFP_WAIT))
 		return CHARGE_WOULDBLOCK;
 
 	ret = mem_cgroup_hierarchical_reclaim(mem_over_limit, NULL,
-					gfp_mask, flags, csize);
+					gfp_mask, flags, page_size);
 	/*
 	 * try_to_free_mem_cgroup_pages() might not give us a full
 	 * picture of reclaim. Some pages are reclaimed and might be

@@ -1853,19 +1854,28 @@ static int __mem_cgroup_do_charge(struct
 	 * Check the limit again to see if the reclaim reduced the
 	 * current usage of the cgroup before giving up
 	 */
-	if (ret || mem_cgroup_check_under_limit(mem_over_limit, csize))
+	if (ret || mem_cgroup_check_under_limit(mem_over_limit, page_size))
 		return CHARGE_RETRY;
 
 	/*
+	 * When page_size > PAGE_SIZE, THP calls this function and it's
+	 * ok to tell 'there are not enough pages for hugepage'. THP will
+	 * fallback into PAGE_SIZE allocation. If we do reclaim eagerly,
+	 * page splitting will occur and it seems much worse.
+	 */
+	if (page_size > PAGE_SIZE)
+		return CHARGE_NEED_BREAK;
+
+	/*
 	 * At task move, charge accounts can be doubly counted. So, it's
 	 * better to wait until the end of task_move if something is going on.
 	 */
 	if (mem_cgroup_wait_acct_move(mem_over_limit))
 		return CHARGE_RETRY;
-
 	/* If we don't need to call oom-killer at el, return immediately */
 	if (!oom_check)
 		return CHARGE_NOMEM;
+
 	/* check OOM */
 	if (!mem_cgroup_handle_oom(mem_over_limit, gfp_mask))
 		return CHARGE_OOM_DIE;

@@ -1885,7 +1895,7 @@ static int __mem_cgroup_try_charge(struc
 	int nr_oom_retries = MEM_CGROUP_RECLAIM_RETRIES;
 	struct mem_cgroup *mem = NULL;
 	int ret;
-	int csize = max(CHARGE_SIZE, (unsigned long) page_size);
+	bool use_pcp_cache = (page_size == PAGE_SIZE);
 
 	/*
 	 * Unlike gloval-vm's OOM-kill, we're not in memory shortage

@@ -1910,7 +1920,7 @@ again:
 		VM_BUG_ON(css_is_removed(&mem->css));
 		if (mem_cgroup_is_root(mem))
 			goto done;
-		if (page_size == PAGE_SIZE && consume_stock(mem))
+		if (use_pcp_cache && consume_stock(mem))
 			goto done;
 		css_get(&mem->css);
 	} else {

@@ -1933,7 +1943,7 @@ again:
 			rcu_read_unlock();
 			goto done;
 		}
-		if (page_size == PAGE_SIZE && consume_stock(mem)) {
+		if (use_pcp_cache && consume_stock(mem)) {
 			/*
 			 * It seems dagerous to access memcg without css_get().
 			 * But considering how consume_stok works, it's not

@@ -1967,17 +1977,26 @@ again:
 			oom_check = true;
 			nr_oom_retries = MEM_CGROUP_RECLAIM_RETRIES;
 		}
-
-		ret = __mem_cgroup_do_charge(mem, gfp_mask, csize, oom_check);
+		if (use_pcp_cache)
+			ret = __mem_cgroup_do_charge(mem, gfp_mask,
+					CHARGE_SIZE, false, oom_check);
+		else

mm/memcontrol.c again: · line 1986

Quoted 3 lines

mm/memcontrol.c +2 -0

+			ret = __mem_cgroup_do_charge(mem, gfp_mask,
+					page_size, true, oom_check);
 

hmm, this confuses me. I think 'use_pcp_cache' will be used to decide whether we should do consume_stock() or not, but why we change charge size and reclaim behavior depending on it ? I think this code itself is right, but using 'use_pcp_cache' confused me.

Quoted 5 lines

mm/memcontrol.c +0 -0

 		switch (ret) {
 		case CHARGE_OK:
 			break;
 		case CHARGE_RETRY: /* not in OOM situation but retry */

mm/memcontrol.c again: · line 1999

Quoted 10 lines

mm/memcontrol.c +6 -1

-			csize = page_size;
+			if (use_pcp_cache)/* need to reclaim pages */
+				use_pcp_cache = false;
 			css_put(&mem->css);
 			mem = NULL;
 			goto again;
+		case CHARGE_NEED_BREAK: /* page_size > PAGE_SIZE */
+			css_put(&mem->css);
+			/* returning faiulre doesn't mean OOM for hugepages */
+			goto nomem;

I like this change.

mm/memcontrol.c again: · line 2018

Quoted 13 lines

mm/memcontrol.c +3 -3

 		case CHARGE_WOULDBLOCK: /* !__GFP_WAIT */
 			css_put(&mem->css);
 			goto nomem;

@@ -1994,9 +2013,9 @@ again:
 			goto bypass;
 		}
 	} while (ret != CHARGE_OK);
-
-	if (csize > page_size)
-		refill_stock(mem, csize - page_size);
+	/* This flag is cleared when we fail CHAEGE_SIZE charge. */
+	if (use_pcp_cache)
+		refill_stock(mem, CHARGE_SIZE - page_size);

Ditto. can't we keep 'csize' and old code here ?

From: KAMEZAWA Hiroyuki <[email protected]>

When THP is used, Hugepage size charge can happen. It's not handled correctly in mem_cgroup_do_charge(). For example, THP can fallback to small page allocation when HUGEPAGE allocation seems difficult or busy, but memory cgroup doesn't understand it and continue to try HUGEPAGE charging. And the worst thing is memory cgroup believes 'memory reclaim succeeded' if limit - usage > PAGE_SIZE.

By this, khugepaged etc...can goes into inifinite reclaim loop if tasks in memcg are busy.

After this patch
- Hugepage allocation will fail if 1st trial of page reclaim fails.
- distinguish THP allocaton from Bached allocation.

This is a patch I sent to Andrea ages ago in response to a RHEL bugzilla. Not sure why it did not reach mainline... But it fixes one issue you described in 4/7, namely looping around a not exceeded limit with a huge page that won't fit anymore.

---
From: Johannes Weiner <[email protected]>
Subject: [patch] memcg: prevent endless loop with huge pages and near-limit group

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]>

---
 include/linux/res_counter.h |   12 ++++++++++++
 mm/memcontrol.c             |   20 +++++++++++---------
 2 files changed, 23 insertions(+), 9 deletions(-)

include/linux/res_counter.h +12 -0

index fcb9884..03212e4 100644
@@ -182,6 +182,18 @@ static inline bool res_counter_check_under_limit(struct res_counter *cnt)
 	return ret;
 }
 
+static inline bool res_counter_check_room(struct res_counter *cnt,
+					  unsigned long room)
+{
+	bool ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cnt->lock, flags);
+	ret = cnt->limit - cnt->usage >= room;
+	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 +11 -9

index d572102..8fa4be3 100644
@@ -1111,6 +1111,15 @@ static bool mem_cgroup_check_under_limit(struct mem_cgroup *mem)
 	return false;
 }
 
+static bool mem_cgroup_check_room(struct mem_cgroup *mem, unsigned long room)
+{
+	if (!res_counter_check_room(&mem->res, room))
+		return false;
+	if (!do_swap_account)
+		return true;
+	return res_counter_check_room(&mem->memsw, room);
+}
+
 static unsigned int get_swappiness(struct mem_cgroup *memcg)
 {
 	struct cgroup *cgrp = memcg->css.cgroup;

@@ -1844,16 +1853,9 @@ static int __mem_cgroup_do_charge(struct mem_cgroup *mem, gfp_t gfp_mask,
 	if (!(gfp_mask & __GFP_WAIT))
 		return CHARGE_WOULDBLOCK;
 
-	ret = mem_cgroup_hierarchical_reclaim(mem_over_limit, NULL,
+	mem_cgroup_hierarchical_reclaim(mem_over_limit, NULL,
 					gfp_mask, flags);
-	/*
-	 * try_to_free_mem_cgroup_pages() might not give us a full
-	 * picture of reclaim. Some pages are reclaimed and might be
-	 * moved to swap cache or just unmapped from the cgroup.
-	 * Check the limit again to see if the reclaim reduced the
-	 * current usage of the cgroup before giving up
-	 */
-	if (ret || mem_cgroup_check_under_limit(mem_over_limit))
+	if (mem_cgroup_check_room(mem_over_limit, csize))
 		return CHARGE_RETRY;
 
 	/*

Signature · 1 line

1.7.3.5

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 |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

mm/memcontrol.c +1 -1

index 8fa4be3..17c4e36 100644
@@ -1847,7 +1847,7 @@ static int __mem_cgroup_do_charge(struct mem_cgroup *mem, gfp_t gfp_mask,
 	} else
 		mem_over_limit = mem_cgroup_from_res_counter(fail_res, res);
 
-	if (csize > PAGE_SIZE) /* change csize and retry */
+	if (csize == CHARGE_SIZE) /* retry without batching */
 		return CHARGE_RETRY;
 
 	if (!(gfp_mask & __GFP_WAIT))

Signature · 8 lines

1.7.3.5

--
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>

Quoted 11 lines · Johannes Weiner

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.

Quoted 5 lines

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.

Yeah, that is exactly the case I am debugging right now. Came up with different solution: pass page_size to __mem_cgroup_do_charge() and compare csize with page_size (not CHARGE_SIZE). Not sure which solution it more correct.

Hi Gleb,

Quoted 17 lines · Gleb Natapov

Quoted 16 lines · Johannes Weiner

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.

Quoted 4 lines

Yeah, that is exactly the case I am debugging right now. Came up with different solution: pass page_size to __mem_cgroup_do_charge() and compare csize with page_size (not CHARGE_SIZE). Not sure which solution it more correct.

I guess it makes no difference, but using CHARGE_SIZE gets away without adding another parameter to __mem_cgroup_do_charge().

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]>

---
 mm/memcontrol.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

mm/memcontrol.c +7 -0

index 17c4e36..2945649 100644
@@ -1890,6 +1890,13 @@ static int __mem_cgroup_try_charge(struct mm_struct *mm,
 	int csize = max(CHARGE_SIZE, (unsigned long) page_size);
 
 	/*
+	 * Do not OOM on huge pages.  Fall back to regular pages after
+	 * the first failed reclaim attempt.
+	 */
+	if (page_size > PAGE_SIZE)
+		oom = false;
+
+	/*
 	 * Unlike gloval-vm's OOM-kill, we're not in memory shortage
 	 * in system level. So, allow to go ahead dying process in addition to
 	 * MEMDIE process.

Signature · 1 line

1.7.3.5

Quoted 19 lines · On Thu, 27 Jan 2011 14:47:03 +0100 Johannes Weiner <[email protected]> wrote:

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]>

---
 mm/memcontrol.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

mm/memcontrol.c +0 -0

index 17c4e36..2945649 100644
@@ -1890,6 +1890,13 @@ static int __mem_cgroup_try_charge(struct mm_struct *mm,
 	int csize = max(CHARGE_SIZE, (unsigned long) page_size);
 

Quoted 14 lines

mm/memcontrol.c +7 -0

 	/*
+	 * Do not OOM on huge pages.  Fall back to regular pages after
+	 * the first failed reclaim attempt.
+	 */
+	if (page_size > PAGE_SIZE)
+		oom = false;
+
+	/*
 	 * Unlike gloval-vm's OOM-kill, we're not in memory shortage
 	 * in system level. So, allow to go ahead dying process in addition to
 	 * MEMDIE process.

Signature · 1 line

1.7.3.5

__mem_cgroup_try_charge() has "oom" switch already, so I prefer making callers use the switch properly by themselves.

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]>

---
 mm/memcontrol.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

mm/memcontrol.c +7 -0

index 17c4e36..2945649 100644
@@ -1890,6 +1890,13 @@ static int __mem_cgroup_try_charge(struct mm_struct *mm,
 	int csize = max(CHARGE_SIZE, (unsigned long) page_size);
 
 	/*
+	 * Do not OOM on huge pages.  Fall back to regular pages after
+	 * the first failed reclaim attempt.
+	 */
+	if (page_size > PAGE_SIZE)
+		oom = false;
+
+	/*
 	 * Unlike gloval-vm's OOM-kill, we're not in memory shortage
 	 * in system level. So, allow to go ahead dying process in addition to
 	 * MEMDIE process.

Signature · 7 lines

1.7.3.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 12 lines · KAMEZAWA Hiroyuki

From: KAMEZAWA Hiroyuki <[email protected]>

When THP is used, Hugepage size charge can happen. It's not handled correctly in mem_cgroup_do_charge(). For example, THP can fallback to small page allocation when HUGEPAGE allocation seems difficult or busy, but memory cgroup doesn't understand it and continue to try HUGEPAGE charging. And the worst thing is memory cgroup believes 'memory reclaim succeeded' if limit - usage > PAGE_SIZE.

By this, khugepaged etc...can goes into inifinite reclaim loop if tasks in memcg are busy.

Quoted 3 lines

After this patch
- Hugepage allocation will fail if 1st trial of page reclaim fails.
- distinguish THP allocaton from Bached allocation.

This does too many things at once. Can you split this into more patches where each one has a single objective? Thanks.

Quoted 12 lines · KAMEZAWA Hiroyuki

From: KAMEZAWA Hiroyuki <[email protected]>

When THP is used, Hugepage size charge can happen. It's not handled correctly in mem_cgroup_do_charge(). For example, THP can fallback to small page allocation when HUGEPAGE allocation seems difficult or busy, but memory cgroup doesn't understand it and continue to try HUGEPAGE charging. And the worst thing is memory cgroup believes 'memory reclaim succeeded' if limit - usage > PAGE_SIZE.

By this, khugepaged etc...can goes into inifinite reclaim loop if tasks in memcg are busy.

Quoted 3 lines

After this patch
- Hugepage allocation will fail if 1st trial of page reclaim fails.
- distinguish THP allocaton from Bached allocation.

This does too many things at once. Can you split this into more patches where each one has a single objective? Thanks.

This is a promised one.
==
From: KAMEZAWA Hiroyuki <[email protected]>

This patch is for removing initialization in caller of memory cgroup function. Some memory cgroup uses following style to bring the result of start function to the end function for avoiding races.

   mem_cgroup_start_A(&(*ptr))
   /* Something very complicated can happen here. */
   mem_cgroup_end_A(*ptr)

In some calls, *ptr should be initialized to NULL be caller. But it's ugly. This patch fixes that *ptr is initialized by _start function.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

This is a promised one.
==
From: KAMEZAWA Hiroyuki <[email protected]>

This patch is for removing initialization in caller of memory cgroup function. Some memory cgroup uses following style to bring the result of start function to the end function for avoiding races.

   mem_cgroup_start_A(&(*ptr))
   /* Something very complicated can happen here. */
   mem_cgroup_end_A(*ptr)

In some calls, *ptr should be initialized to NULL be caller. But it's ugly. This patch fixes that *ptr is initialized by _start function.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

This is a set of patches which I'm now testing, and it seems it passed small test. So I post this.

Some are bug fixes and other are clean ups but I think these are for 2.6.38.

Brief decription

[1/7] remove buggy comment and use better name for mem_cgroup_move_parent()
The fixes for mem_cgroup_move_parent() is already in mainline, this is an add-on.

[2/7] a bug fix for a new function mem_cgroup_split_huge_fixup(),
which was recently merged.

[3/7] prepare for fixes in [4/7],[5/7]. This is an enhancement of function
which is used now.

[4/7] fix mem_cgroup_charge() for THP. By this, memory cgroup's charge function
will handle THP request in sane way.

[5/7] fix khugepaged scan condition for memcg.
This is a fix for hang of processes under small/buzy memory cgroup.

[6/7] rename vairable names to be page_size, nr_pages, bytes rather than
ambiguous names.

[7/7] some memcg function requires the caller to initialize variable
before call. It's ugly and fix it.

I think patch 1,2,3,4,5 is urgent ones. But I think patch "5" needs some good review. But without "5", stress-test on small memory cgroup will not run succesfully.