memcg: remove -EINTR at rmdir()

By commit "memcg: move charges to root cgroup if use_hierarchy=0" mem_cgroup_move_parent() only returns -EBUSY, -EINVAL. So, we can remove -ENOMEM and -EINTR checks.

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

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

mm/memcontrol.c +0 -5

index cf8a0f6..726b7c6 100644
@@ -3847,8 +3847,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. */

@@ -3910,9 +3908,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);

Signature · 7 lines

1.7.4.1


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

By commit "memcg: move charges to root cgroup if use_hierarchy=0" mem_cgroup_move_parent() only returns -EBUSY, -EINVAL. So, we can remove -ENOMEM and -EINTR checks.

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

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

mm/memcontrol.c +0 -0

index cf8a0f6..726b7c6 100644

Quoted 21 lines · On Mon, 18 Jun 2012 20:59:44 +0900 Kamezawa…

By commit "memcg: move charges to root cgroup if use_hierarchy=0" mem_cgroup_move_parent() only returns -EBUSY, -EINVAL. So, we can remove -ENOMEM and -EINTR checks.

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

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

mm/memcontrol.c +0 -2

index cf8a0f6..726b7c6 100644
@@ -3847,8 +3847,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;
 

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

Quoted 1 line

mm/memcontrol.c +0 -0

  		if (ret == -EBUSY || ret == -EINVAL) {

This looks a bit fragile - if mem_cgroup_move_parent() is later changed (intentionally or otherwise!) to return -Esomethingelse then mem_cgroup_force_empty_list() will subtly break. Why not just do

		if (ret<  0)

Now, mem_cgroup_force_empty_list() just returns 0 or -EBUSY and -EBUSY is just indicating 'you need to retry.'.
This patch makes mem_cgroup_force_empty_list() as boolean function and make the logic simpler.

Quoted 5 lines · On Thu, 21 Jun 2012 17:17:01 +0900 Kamezawa…

Now, mem_cgroup_force_empty_list() just returns 0 or -EBUSY and -EBUSY is just indicating 'you need to retry.'.
This patch makes mem_cgroup_force_empty_list() as boolean function and make the logic simpler.

For some reason I'm having trouble applying these patches - many rejects, need to massage it in by hand.

mm/memcontrol.c unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order, · line 3801

Quoted 9 lines

mm/memcontrol.c +1 -1

@@ -3797,7 +3797,7 @@ unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order,
    * This routine traverse page_cgroup in given list and drop them all.
    * *And* this routine doesn't reclaim page itself, just removes page_cgroup.
    */
-static int mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
+static bool mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
   				int node, int zid, enum lru_list lru)

Let's document the return value. The mem_cgroup_force_empty_list() comment is a mess so I tried to help it a bit. How does this look?

mm/memcontrol.c renamed from mm/memcontrol.c~memcg-make-mem_cgroup_force_empty_list-return-bool-fix +4 -2

@@ -3609,8 +3609,10 @@ unsigned long mem_cgroup_soft_limit_recl
  }
 
  /*
- * This routine traverse page_cgroup in given list and drop them all.
- * *And* this routine doesn't reclaim page itself, just removes page_cgroup.
+ * Traverse a specified page_cgroup list and try to drop them all.  This doesn't
+ * reclaim the pages page themselves - it just removes the page_cgroups.
+ * Returns true if some page_cgroups were not freed, indicating that the caller
+ * must retry this operation.
   */
  static bool mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
  				int node, int zid, enum lru_list lru)

_

2 follow-up patches for "memcg: move charges to root cgroup if use_hierarchy=0", developped/tested onto memcg-devel tree. Maybe no HUNK with -next and -mm.... -Kame
==
memcg: remove -EINTR at rmdir()

By commit "memcg: move charges to root cgroup if use_hierarchy=0", no memory reclaiming will occur at removing memory cgroup.

So, we don't need to take care of user interrupt by signal. This patch removes it.
(*) If -EINTR is returned here, cgroup will show WARNING.

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

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

mm/memcontrol.c +0 -3

index 0623300..cf8a0f6 100644
@@ -3890,9 +3890,6 @@ move_account:
 		ret = -EBUSY;
 		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);

Signature · 7 lines

1.7.4.1


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

Quoted 8 lines · KAMEZAWA Hiroyuki

2 follow-up patches for "memcg: move charges to root cgroup if use_hierarchy=0", developped/tested onto memcg-devel tree. Maybe no HUNK with -next and -mm.... -Kame
==
memcg: remove -EINTR at rmdir()

By commit "memcg: move charges to root cgroup if use_hierarchy=0", no memory reclaiming will occur at removing memory cgroup.

OK, so the there are only 2 reasons why move_parent could fail in this path. 1) it races with somebody else who is uncharging or moving the charge and 2) THP split.
1) works for us and 2) doens't seem to be serious enough to expect that it would stall rmdir on the group for unbound amount of time so the change is safe (can we make this into the changelog please?).

Quoted 10 lines · Michal Hocko

Quoted 8 lines · KAMEZAWA Hiroyuki

2 follow-up patches for "memcg: move charges to root cgroup if use_hierarchy=0", developped/tested onto memcg-devel tree. Maybe no HUNK with -next and -mm.... -Kame
==
memcg: remove -EINTR at rmdir()

By commit "memcg: move charges to root cgroup if use_hierarchy=0", no memory reclaiming will occur at removing memory cgroup.

Quoted 7 lines

OK, so the there are only 2 reasons why move_parent could fail in this path. 1) it races with somebody else who is uncharging or moving the charge and 2) THP split.
1) works for us and 2) doens't seem to be serious enough to expect that it would stall rmdir on the group for unbound amount of time so the change is safe (can we make this into the changelog please?).

Yes. But the failure of move_parent() (-EBUSY) will be retried.

Remaining problems are
- attaching task while pre_destroy() is called.
- creating child cgroup while pre_destroy() is called.

I think I need to make a patch for cgroup layer as I previously posted. I'd like to try again.

Thanks,
-Kame

Quoted 21 lines · KAMEZAWA Hiroyuki

Quoted 10 lines · Michal Hocko

Quoted 8 lines · KAMEZAWA Hiroyuki

2 follow-up patches for "memcg: move charges to root cgroup if use_hierarchy=0", developped/tested onto memcg-devel tree. Maybe no HUNK with -next and -mm.... -Kame
==
memcg: remove -EINTR at rmdir()

By commit "memcg: move charges to root cgroup if use_hierarchy=0", no memory reclaiming will occur at removing memory cgroup.

Quoted 7 lines

OK, so the there are only 2 reasons why move_parent could fail in this path. 1) it races with somebody else who is uncharging or moving the charge and 2) THP split.
1) works for us and 2) doens't seem to be serious enough to expect that it would stall rmdir on the group for unbound amount of time so the change is safe (can we make this into the changelog please?).

Yes. But the failure of move_parent() (-EBUSY) will be retried.

Quoted 3 lines

Remaining problems are
- attaching task while pre_destroy() is called.
- creating child cgroup while pre_destroy() is called.

I don't know why but I thought that tasks and subgroups are not alowed when pre_destroy is called. If this is possible then we probably want to check for pending signals or at least add cond_resched.