memcg: prevent failure in pre_destroy()

Maybe Aneesh will post his own version. This is just for my work.

EBUSY should be returned.

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

---
 mm/hugetlb.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

mm/hugetlb.c +0 -0

index 17ae2e4..4dd6b39 100644
@@ -1922,8 +1922,11 @@ int hugetlb_force_memcg_empty(struct cgroup *cgroup)
 	int ret = 0, idx = 0;
 

From bb0168d5c85f62f36434956e4728a67d0cc41e55 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki<[email protected]>
Date: Thu, 26 Apr 2012 18:48:07 +0900
Subject: [PATCH 3/9] memcg: add res_counter_uncharge_until()

At killing res_counter which is a child of other counter, we need to do
res_counter_uncharge(child, xxx) res_counter_charge(parent, xxx)

This is not atomic and wasting cpu. This patch adds res_counter_uncharge_until(). This function's uncharge propagates to ancestors until specified res_counter.

res_counter_uncharge_until(child, parent, xxx)

This ops is atomic and more efficient.

Originaly-written-by Frederic Weisbecker<[email protected]>

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

Quoted 16 lines · KAMEZAWA Hiroyuki

 From bb0168d5c85f62f36434956e4728a67d0cc41e55 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki<[email protected]>
Date: Thu, 26 Apr 2012 18:48:07 +0900
Subject: [PATCH 3/9] memcg: add res_counter_uncharge_until()

At killing res_counter which is a child of other counter, we need to do
      res_counter_uncharge(child, xxx) res_counter_charge(parent, xxx)

This is not atomic and wasting cpu. This patch adds res_counter_uncharge_until(). This function's uncharge propagates to ancestors until specified res_counter.

      res_counter_uncharge_until(child, parent, xxx)

Quoted 4 lines

This ops is atomic and more efficient.

Originaly-written-by Frederic Weisbecker<[email protected]>

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

I have been carrying Frederic's patch itself in my code for a while now.

Why not just use it? What are you doing differently to justify writing a patch yourself? It's a bit of credit giving as well

Quoted 8 lines · KAMEZAWA Hiroyuki

kernel/res_counter.c +0 -0

index d508363..f4ec411 100644
@@ -66,6 +66,8 @@ done:
       return ret;
  }
 

kernel/res_counter.c done: · line 70

Quoted 2 lines

kernel/res_counter.c +2 -0

+
+

Contamination?

By using res_counter_uncharge_until(), we can avoid unnecessary charging.

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

---
  mm/memcontrol.c |   63 ++++++++++++++++++++++++++++++++++++------------------
  1 files changed, 42 insertions(+), 21 deletions(-)

mm/memcontrol.c +6 -0

index 613bb15..ed53d64 100644
@@ -2420,6 +2420,24 @@ static void __mem_cgroup_cancel_charge(struct mem_cgroup *memcg,
  }
 
  /*
+ * Cancel chages in this cgroup....doesn't propagates to parent cgroup.
+ * This is useful when moving usage to parent cgroup.
+ */
+static void __mem_cgroup_cancel_local_charge(struct mem_cgroup *memcg,
+					unsigned int nr_pages)
+{

mm/memcontrol.c +2 -3

  	if (nr_pages>  1)
  		flags = compound_lock_irqsave(page);
 
-	ret = mem_cgroup_move_account(page, nr_pages, pc, child, parent, true);
-	if (ret)
-		__mem_cgroup_cancel_charge(parent, nr_pages);
+	if (parent->use_hierarchy) {
+		ret = mem_cgroup_move_account(page, nr_pages,

Quoted 22 lines · KAMEZAWA Hiroyuki

By using res_counter_uncharge_until(), we can avoid unnecessary charging.

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

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

  1 files changed, 42 insertions(+), 21 deletions(-)

mm/memcontrol.c +6 -0

index 613bb15..ed53d64 100644
@@ -2420,6 +2420,24 @@ static void __mem_cgroup_cancel_charge(struct mem_cgroup *memcg,
   }
 
   /*
+ * Cancel chages in this cgroup....doesn't propagates to parent cgroup.
+ * This is useful when moving usage to parent cgroup.
+ */
+static void __mem_cgroup_cancel_local_charge(struct mem_cgroup *memcg,
+                                     unsigned int nr_pages)
+{

mm/memcontrol.c static void __mem_cgroup_cancel_charge(struct mem_cgroup *memcg, · line 2438

Quoted 10 lines

mm/memcontrol.c +10 -0

+     if (!mem_cgroup_is_root(memcg)) {
+             unsigned long bytes = nr_pages * PAGE_SIZE;
+
+             res_counter_uncharge_until(&memcg->res,
+                                     memcg->res.parent, bytes);
+             if (do_swap_account)
+                     res_counter_uncharge_until(&memcg->memsw,
+                                             memcg->memsw.parent, bytes);
+     }
+}

Kame, this is a nitpick, but I usually prefer to write this like:

if (mem_cgroup_is_root(memcg))
  return;

res_counter...

Specially with memcg, where function names are bigger than average, in comparison.

the code itself seems fine.

mm/memcontrol.c static int mem_cgroup_move_parent(struct page *page, · line 2698

Quoted 12 lines

mm/memcontrol.c +2 -3

+/*
    * A helper function to get mem_cgroup from ID. must be called under
    * rcu_read_lock(). The caller must check css_is_removed() or some if
    * it's concern. (dropping refcnt from swap can be called against removed

@@ -2677,16 +2695,28 @@ static int mem_cgroup_move_parent(struct page *page,
       nr_pages = hpage_nr_pages(page);
 
       parent = mem_cgroup_from_cont(pcg);
-     ret = __mem_cgroup_try_charge(NULL, gfp_mask, nr_pages,&parent, false);
-     if (ret)
-             goto put_back;
+     if (!parent->use_hierarchy) {

Can we avoid testing for use hierarchy ?
Specially given this might go away.

parent_mem_cgroup() already bundles this information. So maybe we can test for parent_mem_cgroup(parent) == NULL. It is the same thing after all.

mm/memcontrol.c static int mem_cgroup_move_parent(struct page *page, · line 2703

Quoted 5 lines

mm/memcontrol.c +5 -0

+             ret = __mem_cgroup_try_charge(NULL,
+                                     gfp_mask, nr_pages,&parent, false);
+             if (ret)
+                     goto put_back;
+     }

Why? If we are not hierarchical, we should not charge the parent, right?

Quoted 9 lines

mm/memcontrol.c +2 -3

       if (nr_pages>  1)
               flags = compound_lock_irqsave(page);
 
-     ret = mem_cgroup_move_account(page, nr_pages, pc, child, parent, true);
-     if (ret)
-             __mem_cgroup_cancel_charge(parent, nr_pages);
+     if (parent->use_hierarchy) {
+             ret = mem_cgroup_move_account(page, nr_pages,

mm/memcontrol.c static int mem_cgroup_move_parent(struct page *page, · line 2719

Quoted 10 lines

mm/memcontrol.c +10 -0

+                                     pc, child, parent, false);
+             if (!ret)
+                     __mem_cgroup_cancel_local_charge(child, nr_pages);
+     } else {
+             ret = mem_cgroup_move_account(page, nr_pages,
+                                     pc, child, parent, true);
+
+             if (ret)
+                     __mem_cgroup_cancel_charge(parent, nr_pages);
+     }

Calling move account also seems not necessary to me. If we are not uncharging + charging, we won't even touch the parent.

Quoted 25 lines

Quoted 7 lines

mm/memcontrol.c +1 -0

+/*
    * A helper function to get mem_cgroup from ID. must be called under
    * rcu_read_lock(). The caller must check css_is_removed() or some if
    * it's concern. (dropping refcnt from swap can be called against removed

mm/memcontrol.c static int mem_cgroup_move_parent(struct page *page, · line 2698

Quoted 7 lines

mm/memcontrol.c +1 -3

@@ -2677,16 +2695,28 @@ static int mem_cgroup_move_parent(struct page *page,
       nr_pages = hpage_nr_pages(page);
 
       parent = mem_cgroup_from_cont(pcg);
-     ret = __mem_cgroup_try_charge(NULL, gfp_mask, nr_pages,&parent, false);
-     if (ret)
-             goto put_back;
+     if (!parent->use_hierarchy) {

Can we avoid testing for use hierarchy ?
Specially given this might go away.

parent_mem_cgroup() already bundles this information. So maybe we can test for parent_mem_cgroup(parent) == NULL. It is the same thing after all.

Quoted 5 lines

mm/memcontrol.c +5 -0

+             ret = __mem_cgroup_try_charge(NULL,
+                                     gfp_mask, nr_pages,&parent, false);
+             if (ret)
+                     goto put_back;
+     }

Quoted 1 line

Why? If we are not hierarchical, we should not charge the parent, right?

This is how it is implemented today and I think he changed that to move to root on the next patch.

Quoted 22 lines · KAMEZAWA Hiroyuki

By using res_counter_uncharge_until(), we can avoid unnecessary charging.

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

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

  1 files changed, 42 insertions(+), 21 deletions(-)

mm/memcontrol.c +6 -0

index 613bb15..ed53d64 100644
@@ -2420,6 +2420,24 @@ static void __mem_cgroup_cancel_charge(struct mem_cgroup *memcg,
   }
 
   /*
+ * Cancel chages in this cgroup....doesn't propagates to parent cgroup.
+ * This is useful when moving usage to parent cgroup.
+ */
+static void __mem_cgroup_cancel_local_charge(struct mem_cgroup *memcg,
+                                     unsigned int nr_pages)
+{

mm/memcontrol.c static void __mem_cgroup_cancel_charge(struct mem_cgroup *memcg, · line 2438

Quoted 10 lines

mm/memcontrol.c +10 -0

+     if (!mem_cgroup_is_root(memcg)) {
+             unsigned long bytes = nr_pages * PAGE_SIZE;
+
+             res_counter_uncharge_until(&memcg->res,
+                                     memcg->res.parent, bytes);
+             if (do_swap_account)
+                     res_counter_uncharge_until(&memcg->memsw,
+                                             memcg->memsw.parent, bytes);
+     }
+}

Kame, this is a nitpick, but I usually prefer to write this like:

if (mem_cgroup_is_root(memcg))
  return;

res_counter...

Specially with memcg, where function names are bigger than average, in comparison.

the code itself seems fine.

mm/memcontrol.c static int mem_cgroup_move_parent(struct page *page, · line 2698

Quoted 12 lines

mm/memcontrol.c +2 -3

+/*
    * A helper function to get mem_cgroup from ID. must be called under
    * rcu_read_lock(). The caller must check css_is_removed() or some if
    * it's concern. (dropping refcnt from swap can be called against removed

@@ -2677,16 +2695,28 @@ static int mem_cgroup_move_parent(struct page *page,
       nr_pages = hpage_nr_pages(page);
 
       parent = mem_cgroup_from_cont(pcg);
-     ret = __mem_cgroup_try_charge(NULL, gfp_mask, nr_pages,&parent, false);
-     if (ret)
-             goto put_back;
+     if (!parent->use_hierarchy) {

Can we avoid testing for use hierarchy ?
Specially given this might go away.

parent_mem_cgroup() already bundles this information. So maybe we can test for parent_mem_cgroup(parent) == NULL. It is the same thing after all.

mm/memcontrol.c static int mem_cgroup_move_parent(struct page *page, · line 2703

Quoted 5 lines

mm/memcontrol.c +5 -0

+             ret = __mem_cgroup_try_charge(NULL,
+                                     gfp_mask, nr_pages,&parent, false);
+             if (ret)
+                     goto put_back;
+     }

Why? If we are not hierarchical, we should not charge the parent, right?

Quoted 9 lines

mm/memcontrol.c +2 -3

       if (nr_pages>  1)
               flags = compound_lock_irqsave(page);
 
-     ret = mem_cgroup_move_account(page, nr_pages, pc, child, parent, true);
-     if (ret)
-             __mem_cgroup_cancel_charge(parent, nr_pages);
+     if (parent->use_hierarchy) {
+             ret = mem_cgroup_move_account(page, nr_pages,

mm/memcontrol.c static int mem_cgroup_move_parent(struct page *page, · line 2719

Quoted 10 lines

mm/memcontrol.c +10 -0

+                                     pc, child, parent, false);
+             if (!ret)
+                     __mem_cgroup_cancel_local_charge(child, nr_pages);
+     } else {
+             ret = mem_cgroup_move_account(page, nr_pages,
+                                     pc, child, parent, true);
+
+             if (ret)
+                     __mem_cgroup_cancel_charge(parent, nr_pages);
+     }

Calling move account also seems not necessary to me. If we are not uncharging + charging, we won't even touch the parent.

By using res_counter_uncharge_until(), we can avoid unnecessary charging.

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

---
 mm/memcontrol.c |   63 ++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 42 insertions(+), 21 deletions(-)

mm/memcontrol.c +0 -0

index 613bb15..ed53d64 100644

Quoted 15 lines · KAMEZAWA Hiroyuki

By using res_counter_uncharge_until(), we can avoid unnecessary charging.

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

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

 1 files changed, 42 insertions(+), 21 deletions(-)

mm/memcontrol.c +0 -0

index 613bb15..ed53d64 100644
@@ -2420,6 +2420,24 @@ static void __mem_cgroup_cancel_charge(struct mem_cgroup *memcg,
  }
 

mm/memcontrol.c static void __mem_cgroup_cancel_charge(struct mem_cgroup *memcg, · line 2423

Quoted 2 lines

mm/memcontrol.c +1 -0

  /*
+ * Cancel chages in this cgroup....doesn't propagates to parent cgroup.

            ^typo                                     ^ unnecessary s

By using res_counter_uncharge_until(), we can avoid unnecessary charging.

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

---
 mm/memcontrol.c |   63 ++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 42 insertions(+), 21 deletions(-)

mm/memcontrol.c +27 -6

index 613bb15..ed53d64 100644
@@ -2420,6 +2420,24 @@ static void __mem_cgroup_cancel_charge(struct mem_cgroup *memcg,
 }
 
 /*
+ * Cancel chages in this cgroup....doesn't propagates to parent cgroup.
+ * This is useful when moving usage to parent cgroup.
+ */
+static void __mem_cgroup_cancel_local_charge(struct mem_cgroup *memcg,
+					unsigned int nr_pages)
+{
+	if (!mem_cgroup_is_root(memcg)) {
+		unsigned long bytes = nr_pages * PAGE_SIZE;
+
+		res_counter_uncharge_until(&memcg->res,
+					memcg->res.parent, bytes);
+		if (do_swap_account)
+			res_counter_uncharge_until(&memcg->memsw,
+						memcg->memsw.parent, bytes);
+	}
+}
+
+/*
  * A helper function to get mem_cgroup from ID. must be called under
  * rcu_read_lock(). The caller must check css_is_removed() or some if
  * it's concern. (dropping refcnt from swap can be called against removed

@@ -2677,16 +2695,28 @@ static int mem_cgroup_move_parent(struct page *page,
 	nr_pages = hpage_nr_pages(page);
 
 	parent = mem_cgroup_from_cont(pcg);
-	ret = __mem_cgroup_try_charge(NULL, gfp_mask, nr_pages, &parent, false);
-	if (ret)
-		goto put_back;
+	if (!parent->use_hierarchy) {
+		ret = __mem_cgroup_try_charge(NULL,
+					gfp_mask, nr_pages, &parent, false);
+		if (ret)
+			goto put_back;
+	}
 
 	if (nr_pages > 1)
 		flags = compound_lock_irqsave(page);
 
-	ret = mem_cgroup_move_account(page, nr_pages, pc, child, parent, true);
-	if (ret)
-		__mem_cgroup_cancel_charge(parent, nr_pages);
+	if (parent->use_hierarchy) {
+		ret = mem_cgroup_move_account(page, nr_pages,
+					pc, child, parent, false);

mm/memcontrol.c +3 -14

 	if (nr_pages > 1)
 		compound_unlock_irqrestore(page, flags);

@@ -3295,6 +3325,7 @@ int mem_cgroup_move_hugetlb_parent(int idx, struct cgroup *cgroup,
 	struct cgroup *pcgrp = cgroup->parent;
 	struct mem_cgroup *parent = mem_cgroup_from_cont(pcgrp);
 	struct mem_cgroup *memcg  = mem_cgroup_from_cont(cgroup);
+	struct res_counter *counter;
 
 	if (!get_page_unless_zero(page))
 		goto out;

@@ -3305,28 +3336,18 @@ int mem_cgroup_move_hugetlb_parent(int idx, struct cgroup *cgroup,
 		goto err_out;
 
 	csize = PAGE_SIZE << compound_order(page);
-	/*
-	 * If we have use_hierarchy set we can never fail here. So instead of
-	 * using res_counter_uncharge use the open-coded variant which just
-	 * uncharge the child res_counter. The parent will retain the charge.
-	 */
-	if (parent->use_hierarchy) {
-		unsigned long flags;
-		struct res_counter *counter;
-
-		counter = &memcg->hugepage[idx];
-		spin_lock_irqsave(&counter->lock, flags);
-		res_counter_uncharge_locked(counter, csize);
-		spin_unlock_irqrestore(&counter->lock, flags);
-	} else {
+	/* If parent->use_hierarchy == 0, we need to charge parent */
+	if (!parent->use_hierarchy) {
 		ret = res_counter_charge(&parent->hugepage[idx],
 					 csize, &fail_res);
 		if (ret) {
 			ret = -EBUSY;

Now, at removal of cgroup, ->pre_destroy() is called and move charges to the parent cgroup. A major reason of -EBUSY returned by ->pre_destroy() is that the 'moving' hits parent's resource limitation. It happens only when use_hierarchy=0. This was a mistake of original design.(it's me...)

Considering use_hierarchy=0, all cgroups are treated as flat. So, no one cannot justify moving charges to parent...parent and children are in flat configuration, not hierarchical.

This patch modifes to move charges to root cgroup at rmdir/force_empty if use_hierarchy==0. This will much simplify rmdir() and reduce error in ->pre_destroy.

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

---
 Documentation/cgroups/memory.txt |   12 ++++++---- mm/memcontrol.c                  |   39 +++++++++++++------------------------

 2 files changed, 21 insertions(+), 30 deletions(-)

Documentation/cgroups/memory.txt +0 -0

index 54c338d..82ce1ef 100644
@@ -393,14 +393,14 @@ cgroup might have some charge associated with it, even though all

 tasks have migrated away from it. (because we charge against pages, not against tasks.)

 Charges recorded in swap information is not updated at removal of cgroup. Recorded information is discarded and a cgroup which uses swap (swapcache) will be charged as a new owner of it.

+About use_hierarchy, see Section 6.

 5. Misc. interfaces.

Documentation/cgroups/memory.txt +0 -0

@@ -413,13 +413,15 @@ will be charged as a new owner of it.
 

  Almost all pages tracked by this memory cgroup will be unmapped and freed. Some pages cannot be freed because they are locked or in-use. Such pages are
-  moved to parent and this cgroup will be empty. This may return -EBUSY if
-  VM is too busy to free/move all pages immediately.
+  moved to parent(if use_hierarchy==1) or root (if use_hierarchy==0) and this
+  cgroup will be empty.

  Typical use case of this interface is that calling this before rmdir(). Because rmdir() moves all pages to parent, some out-of-use page caches can be moved to the parent. If you want to avoid that, force_empty will be useful.

+  About use_hierarchy, see Section 6.
+
 5.2 stat file

 memory.stat file includes following statistics

mm/memcontrol.c +10 -19

index ed53d64..62200f1 100644
@@ -2695,32 +2695,23 @@ static int mem_cgroup_move_parent(struct page *page,
        nr_pages = hpage_nr_pages(page);
 
        parent = mem_cgroup_from_cont(pcg);
-       if (!parent->use_hierarchy) {
-               ret = __mem_cgroup_try_charge(NULL,
-                                       gfp_mask, nr_pages, &parent, false);
-               if (ret)
-                       goto put_back;
-       }
+       /*
+        * if use_hierarchy==0, move charges to root cgroup.
+        * in root cgroup, we don't touch res_counter
+        */
+       if (!parent->use_hierarchy)
+               parent = root_mem_cgroup;
 
        if (nr_pages > 1)
                flags = compound_lock_irqsave(page);
 
-       if (parent->use_hierarchy) {
-               ret = mem_cgroup_move_account(page, nr_pages,
-                                       pc, child, parent, false);
-               if (!ret)
-                       __mem_cgroup_cancel_local_charge(child, nr_pages);
-       } else {
-               ret = mem_cgroup_move_account(page, nr_pages,
-                                       pc, child, parent, true);
-
-               if (ret)
-                       __mem_cgroup_cancel_charge(parent, nr_pages);
-       }
+       ret = mem_cgroup_move_account(page, nr_pages,
+                               pc, child, parent, false);
+       if (!ret)
+               __mem_cgroup_cancel_local_charge(child, nr_pages);
 
        if (nr_pages > 1)
                compound_unlock_irqrestore(page, flags);
-put_back:
        putback_lru_page(page);

 put:
       put_page(page);

mm/memcontrol.c +4 -7

@@ -3338,12 +3329,10 @@ int mem_cgroup_move_hugetlb_parent(int idx, struct cgroup *cgroup,
        csize = PAGE_SIZE << compound_order(page);
        /* If parent->use_hierarchy == 0, we need to charge parent */
        if (!parent->use_hierarchy) {
-               ret = res_counter_charge(&parent->hugepage[idx],
-                                        csize, &fail_res);
-               if (ret) {
-                       ret = -EBUSY;
-                       goto err_out;
-               }
+               parent = root_mem_cgroup;
+               /* root has no limit */
+               res_counter_charge_nofail(&parent->hugepage[idx],
+                                csize, &fail_res);
        }
        counter = &memcg->hugepage[idx];
        res_counter_uncharge_until(counter, counter->parent, csize);
--

1.7.4.1

Quoted 4 lines · KAMEZAWA Hiroyuki

Now, at removal of cgroup, ->pre_destroy() is called and move charges to the parent cgroup. A major reason of -EBUSY returned by ->pre_destroy() is that the 'moving' hits parent's resource limitation. It happens only when use_hierarchy=0. This was a mistake of original design.(it's me...)

Nice patch, i can see how broken it is now with use_hierarchy=0...

nitpick on the documentation below:

Quoted 23 lines

Considering use_hierarchy=0, all cgroups are treated as flat. So, no one cannot justify moving charges to parent...parent and children are in flat configuration, not hierarchical.

This patch modifes to move charges to root cgroup at rmdir/force_empty if use_hierarchy==0. This will much simplify rmdir() and reduce error in ->pre_destroy.

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

---
 Documentation/cgroups/memory.txt |   12 ++++++---- mm/memcontrol.c                  |   39 +++++++++++++------------------------

 2 files changed, 21 insertions(+), 30 deletions(-)

Documentation/cgroups/memory.txt +0 -0

index 54c338d..82ce1ef 100644
@@ -393,14 +393,14 @@ cgroup might have some charge associated with it, even though all

 tasks have migrated away from it. (because we charge against pages, not against tasks.)

Documentation/cgroups/memory.txt cgroup might have some charge associated with it, even though all · line 397

Quoted 5 lines

Documentation/cgroups/memory.txt +2 -3

-Such charges are freed or moved to their parent. At moving, both of RSS
-and CACHES are moved to parent.
-rmdir() may return -EBUSY if freeing/moving fails. See 5.1 also.
+Such charges are freed or moved to their parent if use_hierarchy=1.
+if use_hierarchy=0, the charges will be moved to root cgroup.

It is more clear that we move the stats to root (if use_hierarchy==0) or parent (if use_hierarchy==1), and no change on the charge except uncharging from the child.

Now, at removal of cgroup, ->pre_destroy() is called and move charges to the parent cgroup. A major reason of -EBUSY returned by ->pre_destroy() is that the 'moving' hits parent's resource limitation. It happens only when use_hierarchy=0. This was a mistake of original design.(it's me...)

Considering use_hierarchy=0, all cgroups are treated as flat. So, no one cannot justify moving charges to parent...parent and children are in flat configuration, not hierarchical.

This patch modifes to move charges to root cgroup at rmdir/force_empty if use_hierarchy==0. This will much simplify rmdir() and reduce error in ->pre_destroy.

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

---
 Documentation/cgroups/memory.txt |   12 ++++++----
 mm/memcontrol.c                  |   39 +++++++++++++------------------------
 2 files changed, 21 insertions(+), 30 deletions(-)

Documentation/cgroups/memory.txt +7 -5

index 54c338d..82ce1ef 100644
@@ -393,14 +393,14 @@ cgroup might have some charge associated with it, even though all
 tasks have migrated away from it. (because we charge against pages, not
 against tasks.)
 
-Such charges are freed or moved to their parent. At moving, both of RSS
-and CACHES are moved to parent.
-rmdir() may return -EBUSY if freeing/moving fails. See 5.1 also.
+Such charges are freed or moved to their parent if use_hierarchy=1.
+if use_hierarchy=0, the charges will be moved to root cgroup.
 
 Charges recorded in swap information is not updated at removal of cgroup.
 Recorded information is discarded and a cgroup which uses swap (swapcache)
 will be charged as a new owner of it.
 
+About use_hierarchy, see Section 6.
 
 5. Misc. interfaces.
 

@@ -413,13 +413,15 @@ will be charged as a new owner of it.
 
   Almost all pages tracked by this memory cgroup will be unmapped and freed.
   Some pages cannot be freed because they are locked or in-use. Such pages are
-  moved to parent and this cgroup will be empty. This may return -EBUSY if
-  VM is too busy to free/move all pages immediately.
+  moved to parent(if use_hierarchy==1) or root (if use_hierarchy==0) and this
+  cgroup will be empty.
 
   Typical use case of this interface is that calling this before rmdir().
   Because rmdir() moves all pages to parent, some out-of-use page caches can be
   moved to the parent. If you want to avoid that, force_empty will be useful.
 
+  About use_hierarchy, see Section 6.
+
 5.2 stat file
 
 memory.stat file includes following statistics

mm/memcontrol.c +10 -22

index ed53d64..62200f1 100644
@@ -2695,32 +2695,23 @@ static int mem_cgroup_move_parent(struct page *page,
 	nr_pages = hpage_nr_pages(page);
 
 	parent = mem_cgroup_from_cont(pcg);
-	if (!parent->use_hierarchy) {
-		ret = __mem_cgroup_try_charge(NULL,
-					gfp_mask, nr_pages, &parent, false);
-		if (ret)
-			goto put_back;
-	}
+	/*
+	 * if use_hierarchy==0, move charges to root cgroup.
+	 * in root cgroup, we don't touch res_counter
+	 */
+	if (!parent->use_hierarchy)
+		parent = root_mem_cgroup;
 
 	if (nr_pages > 1)
 		flags = compound_lock_irqsave(page);
 
-	if (parent->use_hierarchy) {
-		ret = mem_cgroup_move_account(page, nr_pages,
-					pc, child, parent, false);
-		if (!ret)
-			__mem_cgroup_cancel_local_charge(child, nr_pages);
-	} else {
-		ret = mem_cgroup_move_account(page, nr_pages,
-					pc, child, parent, true);
-
-		if (ret)
-			__mem_cgroup_cancel_charge(parent, nr_pages);
-	}
+	ret = mem_cgroup_move_account(page, nr_pages,
+				pc, child, parent, false);
+	if (!ret)
+		__mem_cgroup_cancel_local_charge(child, nr_pages);
 
 	if (nr_pages > 1)
 		compound_unlock_irqrestore(page, flags);
-put_back:
 	putback_lru_page(page);
 put:
 	put_page(page);

@@ -3338,12 +3329,10 @@ int mem_cgroup_move_hugetlb_parent(int idx, struct cgroup *cgroup,
 	csize = PAGE_SIZE << compound_order(page);
 	/* If parent->use_hierarchy == 0, we need to charge parent */
 	if (!parent->use_hierarchy) {
-		ret = res_counter_charge(&parent->hugepage[idx],
-					 csize, &fail_res);
-		if (ret) {

attach_task() is done under cgroup_mutex() but ->pre_destroy() callback in rmdir() isn't called under cgroup_mutex().

It's better to avoid attaching a task to a cgroup which is under pre_destroy(). Considering memcg, the attached task may increase resource usage after memcg's pre_destroy() confirms that memcg is empty. This is not good.

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

---
 kernel/cgroup.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

kernel/cgroup.c +0 -0

index ad8eae5..7a3076b 100644

kernel/cgroup.c +1 -1

 	tset.single.task = tsk;
 	tset.single.cgrp = oldcgrp;
 

@@ -4181,7 +4184,6 @@ again:
 		mutex_unlock(&cgroup_mutex);
 		return -EBUSY;
 	}
-	mutex_unlock(&cgroup_mutex);
 
 	/*
 	 * In general, subsystem has no css->refcnt after pre_destroy(). But

@@ -4193,6 +4195,7 @@ again:
 	 * and css_tryget() and cgroup_wakeup_rmdir_waiter() implementation.
 	 */
 	set_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
+	mutex_unlock(&cgroup_mutex);
 
 	/*
 	 * Call pre_destroy handlers of subsys. Notify subsystems

Signature · 1 line

1.7.4.1

Quoted 21 lines · KAMEZAWA Hiroyuki

attach_task() is done under cgroup_mutex() but ->pre_destroy() callback in rmdir() isn't called under cgroup_mutex().

It's better to avoid attaching a task to a cgroup which is under pre_destroy(). Considering memcg, the attached task may increase resource usage after memcg's pre_destroy() confirms that memcg is empty. This is not good.

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

---
 kernel/cgroup.c |    5 ++++-

 1 files changed, 4 insertions(+), 1 deletions(-)

kernel/cgroup.c +0 -0

index ad8eae5..7a3076b 100644
@@ -1953,6 +1953,9 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
       if (cgrp == oldcgrp)
               return 0;
 

kernel/cgroup.c int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk) · line 1958

Quoted 3 lines

kernel/cgroup.c +3 -0

+     if (test_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags))
+             return -EBUSY;
+

You probably need to update cgroup_attach_proc() as well?

attach_task() is done under cgroup_mutex() but ->pre_destroy() callback in rmdir() isn't called under cgroup_mutex().

It's better to avoid attaching a task to a cgroup which is under pre_destroy(). Considering memcg, the attached task may increase resource usage after memcg's pre_destroy() confirms that memcg is empty. This is not good.

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

Quoted 8 lines · KAMEZAWA Hiroyuki

attach_task() is done under cgroup_mutex() but ->pre_destroy() callback in rmdir() isn't called under cgroup_mutex().

It's better to avoid attaching a task to a cgroup which is under pre_destroy(). Considering memcg, the attached task may increase resource usage after memcg's pre_destroy() confirms that memcg is empty. This is not good.

Quoted 1 line

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

Hmm... once memcg's pre_destroy() can't fail, I think what we should do is marking a cgroup DEAD before calling pre_destroy() and the existing cgroup_is_removed() check should be enough. Patches upto this point already make ->pre_destroy() not fail, right?

When ->pre_destroy() is called, it should be guaranteed that new child cgroup is not created under a cgroup, where pre_destroy() is running. If not, ->pre_destroy() must check children and return -EBUSY, which causes warning.

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

When ->pre_destroy() is called, it should be guaranteed that new child cgroup is not created under a cgroup, where pre_destroy() is running. If not, ->pre_destroy() must check children and return -EBUSY, which causes warning.

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

stands. The only change necessary is memcg's pre_destroy() not returning zero.

Quoted 6 lines · KAMEZAWA Hiroyuki

When ->pre_destroy() is called, it should be guaranteed that new child cgroup is not created under a cgroup, where pre_destroy() is running. If not, ->pre_destroy() must check children and return -EBUSY, which causes warning.

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

Hmm... I'm getting confused more.  Why do we need these cgroup changes at all?  cgroup still has cgrp->count check and cgroup_clear_css_refs() after pre_destroy() calls.  The order of changes should be,

* Make memcg pre_destroy() not fail; however, pre_destroy() should still be ready to be retried.  That's the defined interface.

* cgroup core updated to drop pre_destroy() retrying and guarantee that pre_destroy() invocation will happen only once.

* memcg and other cgroups can update their pre_destroy() if the "won't be retried" part can simplify their implementations.

What I thought was...
Assume a memory cgoup A, with use_hierarchy==1.

1.  thread:0   start calling pre->destroy of cgroup A
2.  thread:0   it sometimes calls cond_resched or other sleep functions.
3.  thread:1   create a cgroup B under "A"
4.  thread:1   attach a thread X to cgroup A/B
5.  res_counter of A charged up. but pre_destroy() can't find what happens

because it scans LRU of A.

So, we have -EBUSY now. I considered some options to fix this.

option 1) just return 0 instead of -EBUSY when pre_destroy() finds a task or a child.

There is a race....even if we return 0 here and expects cgroup code can catch it,
the thread or a child we found may be moved to other cgroup before we check it in cgroup's final check.
In that case, the cgroup will be freed before full-ack of pre_destory() and the charges
will be lost.

Hi, KAME.

Quoted 15 lines · Hiroyuki Kamezawa

What I thought was...
Assume a memory cgoup A, with use_hierarchy==1.

1.  thread:0   start calling pre->destroy of cgroup A
2.  thread:0   it sometimes calls cond_resched or other sleep functions.
3.  thread:1   create a cgroup B under "A"
4.  thread:1   attach a thread X to cgroup A/B
5.  res_counter of A charged up. but pre_destroy() can't find what happens because it scans LRU of A.

So, we have -EBUSY now. I considered some options to fix this.

option 1) just return 0 instead of -EBUSY when pre_destroy() finds a task or a child.

Quoted 7 lines

There is a race....even if we return 0 here and expects cgroup code can catch it,
the thread or a child we found may be moved to other cgroup before we check it in cgroup's final check.
In that case, the cgroup will be freed before full-ack of pre_destory() and the charges
will be lost.

So, cgroup code won't proceed with rmdir if children are created inbetween and note that the race condition of lost charge you described above existed before this change - ie. new cgroup could be created after pre_destroy() is complete.

The current cgroup rmdir code is transitional.  It has to support both retrying and non-retrying pre_destroy()s and that means we can't mark the cgroup DEAD before starting invoking pre_destroy(); however, we can do that once memcg's pre_destroy() is converted which will also remove all the WAIT_ON_RMDIR mechanism and the above described race.

There really isn't much point in trying to make the current cgroup rmdir behave perfectly when the next step is removing all the fixed up parts.

So, IMHO, just making pre_destroy() clean up its own charges and always returning 0 is enough.  There's no need to fix up old non-critical race condition at this point in the patch stream.  cgroup rmdir simplification will make them disappear anyway.

Quoted 4 lines

So, IMHO, just making pre_destroy() clean up its own charges and always returning 0 is enough.  There's no need to fix up old non-critical race condition at this point in the patch stream.  cgroup rmdir simplification will make them disappear anyway.

So, hmm, ok. I'll drop patch 7 & 8. memcg may return -EBUSY in very very race case but users will not see it in the most case.
I'll fix limit, move-charge and use_hierarchy problem first.

When force_empty() called by ->pre_destroy(), no memory reclaim happens and it doesn't take very long time which requires signal_pending() check. And if we return -EINTR from pre_destroy(), cgroup.c show warning.

This patch removes signal check in force_empty(). By this, ->pre_destroy() returns success always.

Note: check for 'cgroup is empty' remains for force_empty interface.

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

---
 mm/hugetlb.c    |   10 +---------
 mm/memcontrol.c |   14 +++++---------

 2 files changed, 6 insertions(+), 18 deletions(-)

mm/hugetlb.c +1 -9

index 4dd6b39..770f1642 100644
@@ -1922,20 +1922,12 @@ int hugetlb_force_memcg_empty(struct cgroup *cgroup)
        int ret = 0, idx = 0;
 
        do {
+               /* see memcontrol.c::mem_cgroup_force_empty() */
                if (cgroup_task_count(cgroup)
                        || !list_empty(&cgroup->children)) {
                        ret = -EBUSY;
                        goto out;
                }
-               /*
-                * If the task doing the cgroup_rmdir got a signal
-                * we don't really need to loop till the hugetlb resource
-                * usage become zero.
-                */
-               if (signal_pending(current)) {
-                       ret = -EINTR;
-                       goto out;
-               }
                for_each_hstate(h) {
                        spin_lock(&hugetlb_lock);
                        list_for_each_entry(page, &h->hugepage_activelist, lru) {

mm/memcontrol.c +0 -2

index 2715223..ee350c5 100644
@@ -3852,8 +3852,6 @@ static int mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
                pc = lookup_page_cgroup(page);
 
                ret = mem_cgroup_move_parent(page, pc, memcg, GFP_KERNEL);
-               if (ret == -ENOMEM || ret == -EINTR)
-                       break;
 
                if (ret == -EBUSY || ret == -EINVAL) {
                        /* found lock contention or "pc" is obsolete. */

mm/memcontrol.c +0 -0

                return -EBUSY;
        return ret;
  }

@@ -3893,11 +3891,12 @@ static int mem_cgroup_force_empty(struct mem_cgroup *memcg, bool free_all)

 move_account:
       do {
               ret = -EBUSY;
+               /*
+                * This never happens when this is called by ->pre_destroy().
+                * But we need to take care of force_empty interface.
+                */
               if (cgroup_task_count(cgrp) || !list_empty(&cgrp->children)) goto out;
-               ret = -EINTR;
-               if (signal_pending(current))
-                       goto out;
               /* This is for making all *used* pages to be on LRU. */ lru_add_drain_all();
               drain_all_stock_sync(memcg);

mm/memcontrol.c +0 -4

@@ -3918,9 +3917,6 @@ move_account:
                }
                mem_cgroup_end_move(memcg);
                memcg_oom_recover(memcg);
-               /* it seems parent cgroup doesn't have enough mem */
-               if (ret == -ENOMEM)
-                       goto try_to_free;
                cond_resched();
        /* "ret" should also be checked to ensure all lists are empty. */
        } while (res_counter_read_u64(&memcg->res, RES_USAGE) > 0 || ret);
--

1.7.4.1

Quoted 58 lines · KAMEZAWA Hiroyuki

When force_empty() called by ->pre_destroy(), no memory reclaim happens and it doesn't take very long time which requires signal_pending() check. And if we return -EINTR from pre_destroy(), cgroup.c show warning.

This patch removes signal check in force_empty(). By this, ->pre_destroy() returns success always.

Note: check for 'cgroup is empty' remains for force_empty interface.

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

---
 mm/hugetlb.c    |   10 +---------
 mm/memcontrol.c |   14 +++++---------

 2 files changed, 6 insertions(+), 18 deletions(-)

mm/hugetlb.c +1 -9

index 4dd6b39..770f1642 100644
@@ -1922,20 +1922,12 @@ int hugetlb_force_memcg_empty(struct cgroup *cgroup)
        int ret = 0, idx = 0;
 
        do {
+               /* see memcontrol.c::mem_cgroup_force_empty() */
                if (cgroup_task_count(cgroup)
                        || !list_empty(&cgroup->children)) {
                        ret = -EBUSY;
                        goto out;
                }
-               /*
-                * If the task doing the cgroup_rmdir got a signal
-                * we don't really need to loop till the hugetlb resource
-                * usage become zero.
-                */
-               if (signal_pending(current)) {
-                       ret = -EINTR;
-                       goto out;
-               }
                for_each_hstate(h) {
                        spin_lock(&hugetlb_lock);
                        list_for_each_entry(page, &h->hugepage_activelist, lru) {

mm/memcontrol.c +0 -2

index 2715223..ee350c5 100644
@@ -3852,8 +3852,6 @@ static int mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
                pc = lookup_page_cgroup(page);
 
                ret = mem_cgroup_move_parent(page, pc, memcg, GFP_KERNEL);
-               if (ret == -ENOMEM || ret == -EINTR)
-                       break;
 
                if (ret == -EBUSY || ret == -EINVAL) {
                        /* found lock contention or "pc" is obsolete. */

@@ -3863,7 +3861,7 @@ static int mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
                        busy = NULL;
        }
 

mm/memcontrol.c static int mem_cgroup_force_empty_list(struct mem_cgroup *memcg, · line 3864

Quoted 2 lines

mm/memcontrol.c +1 -1

-       if (!ret && !list_empty(list))
+       if (!loop)

This looks a bit strange to me... why we make the change ?

Quoted 60 lines · Ying Han

Quoted 58 lines · KAMEZAWA Hiroyuki

When force_empty() called by ->pre_destroy(), no memory reclaim happens and it doesn't take very long time which requires signal_pending() check. And if we return -EINTR from pre_destroy(), cgroup.c show warning.

This patch removes signal check in force_empty(). By this, ->pre_destroy() returns success always.

Note: check for 'cgroup is empty' remains for force_empty interface.

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

---
 mm/hugetlb.c    |   10 +---------
 mm/memcontrol.c |   14 +++++---------

 2 files changed, 6 insertions(+), 18 deletions(-)

mm/hugetlb.c +1 -9

index 4dd6b39..770f1642 100644
@@ -1922,20 +1922,12 @@ int hugetlb_force_memcg_empty(struct cgroup *cgroup)
        int ret = 0, idx = 0;
 
        do {
+               /* see memcontrol.c::mem_cgroup_force_empty() */
                if (cgroup_task_count(cgroup)
                        || !list_empty(&cgroup->children)) {
                        ret = -EBUSY;
                        goto out;
                }
-               /*
-                * If the task doing the cgroup_rmdir got a signal
-                * we don't really need to loop till the hugetlb resource
-                * usage become zero.
-                */
-               if (signal_pending(current)) {
-                       ret = -EINTR;
-                       goto out;
-               }
                for_each_hstate(h) {
                        spin_lock(&hugetlb_lock);
                        list_for_each_entry(page, &h->hugepage_activelist, lru) {

mm/memcontrol.c +0 -2

index 2715223..ee350c5 100644
@@ -3852,8 +3852,6 @@ static int mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
                pc = lookup_page_cgroup(page);
 
                ret = mem_cgroup_move_parent(page, pc, memcg, GFP_KERNEL);
-               if (ret == -ENOMEM || ret == -EINTR)
-                       break;
 
                if (ret == -EBUSY || ret == -EINVAL) {
                        /* found lock contention or "pc" is obsolete. */

@@ -3863,7 +3861,7 @@ static int mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
                        busy = NULL;
        }
 

Quoted 5 lines

mm/memcontrol.c static int mem_cgroup_force_empty_list(struct mem_cgroup *memcg, · line 3864

Quoted 2 lines

mm/memcontrol.c +1 -1

-       if (!ret && !list_empty(list))
+       if (!loop)

This looks a bit strange to me... why we make the change ?

Ah, I should this move to an independet patch.
Because we don't have -ENOMEM path to exit loop, the return value of this function
is
 0 (if loop !=0 this means lru is empty under the lru lock ) -EBUSY (if loop== 0)

I'll move this part out as an independent clean up patch

thanks,
-kame

When force_empty() called by ->pre_destroy(), no memory reclaim happens and it doesn't take very long time which requires signal_pending() check. And if we return -EINTR from pre_destroy(), cgroup.c show warning.

This patch removes signal check in force_empty(). By this, ->pre_destroy() returns success always.

Note: check for 'cgroup is empty' remains for force_empty interface.

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

---
 mm/hugetlb.c    |   10 +---------
 mm/memcontrol.c |   14 +++++---------

 2 files changed, 6 insertions(+), 18 deletions(-)

mm/hugetlb.c +1 -9

index 4dd6b39..770f1642 100644
@@ -1922,20 +1922,12 @@ int hugetlb_force_memcg_empty(struct cgroup *cgroup)
        int ret = 0, idx = 0;
 
        do {
+               /* see memcontrol.c::mem_cgroup_force_empty() */
                if (cgroup_task_count(cgroup)
                        || !list_empty(&cgroup->children)) {
                        ret = -EBUSY;
                        goto out;
                }
-               /*
-                * If the task doing the cgroup_rmdir got a signal
-                * we don't really need to loop till the hugetlb resource
-                * usage become zero.
-                */
-               if (signal_pending(current)) {
-                       ret = -EINTR;
-                       goto out;
-               }
                for_each_hstate(h) {
                        spin_lock(&hugetlb_lock);
                        list_for_each_entry(page, &h->hugepage_activelist, lru) {

mm/memcontrol.c +1 -3

index 2715223..ee350c5 100644
@@ -3852,8 +3852,6 @@ static int mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
                pc = lookup_page_cgroup(page);
 
                ret = mem_cgroup_move_parent(page, pc, memcg, GFP_KERNEL);
-               if (ret == -ENOMEM || ret == -EINTR)
-                       break;
 
                if (ret == -EBUSY || ret == -EINVAL) {
                        /* found lock contention or "pc" is obsolete. */

@@ -3863,7 +3861,7 @@ static int mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
                        busy = NULL;
        }
 
-       if (!ret && !list_empty(list))
+       if (!loop)
                return -EBUSY;
        return ret;
  }

@@ -3893,11 +3891,12 @@ static int mem_cgroup_force_empty(struct mem_cgroup *memcg, bool free_all)

 move_account:

       do {
               ret = -EBUSY;
+               /*
+                * This never happens when this is called by ->pre_destroy().
+                * But we need to take care of force_empty interface.
+                */
               if (cgroup_task_count(cgrp) || !list_empty(&cgrp->children)) goto out;

2012/4/26 KAMEZAWA Hiroyuki <[email protected]>:

Quoted 68 lines

When force_empty() called by ->pre_destroy(), no memory reclaim happens and it doesn't take very long time which requires signal_pending() check. And if we return -EINTR from pre_destroy(), cgroup.c show warning.

This patch removes signal check in force_empty(). By this, ->pre_destroy() returns success always.

Note: check for 'cgroup is empty' remains for force_empty interface.

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

---
 mm/hugetlb.c    |   10 +---------
 mm/memcontrol.c |   14 +++++---------

 2 files changed, 6 insertions(+), 18 deletions(-)

mm/hugetlb.c +1 -9

index 4dd6b39..770f1642 100644
@@ -1922,20 +1922,12 @@ int hugetlb_force_memcg_empty(struct cgroup *cgroup)
        int ret = 0, idx = 0;
 
        do {
+               /* see memcontrol.c::mem_cgroup_force_empty() */
                if (cgroup_task_count(cgroup)
                        || !list_empty(&cgroup->children)) {
                        ret = -EBUSY;
                        goto out;
                }
-               /*
-                * If the task doing the cgroup_rmdir got a signal
-                * we don't really need to loop till the hugetlb resource
-                * usage become zero.
-                */
-               if (signal_pending(current)) {
-                       ret = -EINTR;
-                       goto out;
-               }
                for_each_hstate(h) {
                        spin_lock(&hugetlb_lock);
                        list_for_each_entry(page, &h->hugepage_activelist, lru) {

mm/memcontrol.c +1 -3

index 2715223..ee350c5 100644
@@ -3852,8 +3852,6 @@ static int mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
                pc = lookup_page_cgroup(page);
 
                ret = mem_cgroup_move_parent(page, pc, memcg, GFP_KERNEL);
-               if (ret == -ENOMEM || ret == -EINTR)
-                       break;
 
                if (ret == -EBUSY || ret == -EINVAL) {
                        /* found lock contention or "pc" is obsolete. */

@@ -3863,7 +3861,7 @@ static int mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
                        busy = NULL;
        }
 
-       if (!ret && !list_empty(list))
+       if (!loop)
                return -EBUSY;
        return ret;
  }

@@ -3893,11 +3891,12 @@ static int mem_cgroup_force_empty(struct mem_cgroup *memcg, bool free_all)

 move_account:
       do {
               ret = -EBUSY;
+               /*

Quoted 5 lines

+                * This never happens when this is called by ->pre_destroy().
+                * But we need to take care of force_empty interface.
+                */
               if (cgroup_task_count(cgrp) || !list_empty(&cgrp->children)) goto out;

Are you sure this never happens when called by ->pre_destroy()? Can't a task still get attached to the cgroup while ->pre_destroy() is running?

This is a v2 patch for preventing failure in memcg->pre_destroy(). With this patch, ->pre_destroy() will never return error code and users will not see warning at rmdir(). And this work will simplify memcg->pre_destroy(), largely.

This patch is based on linux-next + hugetlb memory control patches.

Hello,

Quoted 5 lines · KAMEZAWA Hiroyuki

This is a v2 patch for preventing failure in memcg->pre_destroy(). With this patch, ->pre_destroy() will never return error code and users will not see warning at rmdir(). And this work will simplify memcg->pre_destroy(), largely.

Quoted 1 line

This patch is based on linux-next + hugetlb memory control patches.

Ergh... can you please set up a git branch somewhere for review purposes?

Quoted 8 lines · Tejun Heo

Hello,

Quoted 5 lines · KAMEZAWA Hiroyuki

This is a v2 patch for preventing failure in memcg->pre_destroy(). With this patch, ->pre_destroy() will never return error code and users will not see warning at rmdir(). And this work will simplify memcg->pre_destroy(), largely.

Quoted 5 lines

Quoted 1 line

This patch is based on linux-next + hugetlb memory control patches.

Ergh... can you please set up a git branch somewhere for review purposes?

I'm sorry...I can't. (To do that, I need to pass many my company's check.) I'll repost all a week later, hugetlb tree will be seen in memcg-devel or linux-next.

Thanks,
-Kame

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Quoted 11 lines · Hiroyuki Kamezawa

Quoted 10 lines · Tejun Heo

Hello,

Quoted 6 lines · KAMEZAWA Hiroyuki

This is a v2 patch for preventing failure in memcg->pre_destroy(). With this patch, ->pre_destroy() will never return error code and users will not see warning at rmdir(). And this work will simplify memcg->pre_destroy(), largely.

This patch is based on linux-next + hugetlb memory control patches.

Quoted 6 lines

Quoted 3 lines

Ergh... can you please set up a git branch somewhere for review purposes?

I'm sorry...I can't. (To do that, I need to pass many my company's check.) I'll repost all a week later, hugetlb tree will be seen in memcg-devel or linux-next.

I can push it to memcg-devel tree if you want.