[RFC] memcg/cgroup: do not fail fail on pre_destroy callbacks

[RFC] memcg/cgroup: do not fail fail on pre_destroy callbacks

[Michal Hocko]

Hi,
memcg is the only controller which might fail in its pre_destroy
callback which makes the cgroup core more complicated for no good
reason. This is an attempt to change this unfortunate state. 

I am sending this a RFC because I would like to hear back whether the
approach is correct. I thought that the changes would be more invasive
but it seems that the current code was mostly prepared for this and it
needs just some small tweaks (so I might be missing something important
here).

The first two patches are just clean ups. They could be merged even
without the rest.

The real change, although the code is not changed that much, is the 3rd
patch. It changes the way how we handle mem_cgroup_move_parent failures.
We have to realize that all those failures are *temporal*. Because we
are either racing with the page removal or the page is temporarily off
the LRU because of migration resp. global reclaim. As a result we do
not fail mem_cgroup_force_empty_list if the page cannot be moved to the
parent and rather retry until the LRU is empty.

The 4th patch is for cgroup core. I have moved cgroup_call_pre_destroy
inside the cgroup_lock which is not very nice because the callbacks
can take some time. Maybe we can move this call at the very end of the
function?
All I need for memcg is that cgroup_call_pre_destroy has been called and
that no new cgroups can be attached to the group. The cgroup_lock is
necessary for the later condition but if we move after CGRP_REMOVED flag
is set then we are safe as well.

The last two patches are trivial follow ups for the cgroups core change
because now we know that nobody will interfere with us so we can drop
those empty && no child condition.

Comments, thoughts?

Michal Hocko (6):
      memcg: split mem_cgroup_force_empty into reclaiming and reparenting parts
      memcg: root_cgroup cannot reach mem_cgroup_move_parent
      memcg: Simplify mem_cgroup_force_empty_list error handling
      cgroups: forbid pre_destroy callback to fail
      memcg: make mem_cgroup_reparent_charges non failing
      hugetlb: do not fail in hugetlb_cgroup_pre_destroy

Cumulative diffstat:
 kernel/cgroup.c     |   30 ++++---------
 mm/hugetlb_cgroup.c |   11 ++---
 mm/memcontrol.c     |  124 +++++++++++++++++++++++++++------------------------
 3 files changed, 78 insertions(+), 87 deletions(-)

[PATCH 1/6] memcg: split mem_cgroup_force_empty into reclaiming and reparenting parts

[Michal Hocko]

mem_cgroup_force_empty did two separate things depending on free_all
parameter from the very beginning. It either reclaimed as many pages as
possible and moved the rest to the parent or just moved charges to the
parent. The first variant is used as memory.force_empty callback while
the later is used from the mem_cgroup_pre_destroy.

The whole games around gotos are far from being nice and there is no
reason to keep those two functions inside one. Let's split them and
also move the responsibility for css reference counting to their callers
to make to code easier.

This patch doesn't have any functional changes.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/memcontrol.c |   72 ++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 42 insertions(+), 30 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index e4e9b18..f25e9c0 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3733,27 +3733,21 @@ static bool mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
 }
 
 /*
- * make mem_cgroup's charge to be 0 if there is no task.
+ * make mem_cgroup's charge to be 0 if there is no task by moving
+ * all the charges and pages to the parent.
  * This enables deleting this mem_cgroup.
+ *
+ * Caller is responsible for holding css reference on the memcg.
  */
-static int mem_cgroup_force_empty(struct mem_cgroup *memcg, bool free_all)
+static int mem_cgroup_reparent_charges(struct mem_cgroup *memcg)
 {
-	int ret;
-	int node, zid, shrink;
-	int nr_retries = MEM_CGROUP_RECLAIM_RETRIES;
 	struct cgroup *cgrp = memcg->css.cgroup;
+	int node, zid;
+	int ret;
 
-	css_get(&memcg->css);
-
-	shrink = 0;
-	/* should free all ? */
-	if (free_all)
-		goto try_to_free;
-move_account:
 	do {
-		ret = -EBUSY;
 		if (cgroup_task_count(cgrp) || !list_empty(&cgrp->children))
-			goto out;
+			return -EBUSY;
 		/* This is for making all *used* pages to be on LRU. */
 		lru_add_drain_all();
 		drain_all_stock_sync(memcg);
@@ -3777,27 +3771,34 @@ move_account:
 		cond_resched();
 	/* "ret" should also be checked to ensure all lists are empty. */
 	} while (res_counter_read_u64(&memcg->res, RES_USAGE) > 0 || ret);
-out:
-	css_put(&memcg->css);
+
 	return ret;
+}
+
+/*
+ * Reclaims as many pages from the given memcg as possible and moves
+ * the rest to the parent.
+ *
+ * Caller is responsible for holding css reference for memcg.
+ */
+static int mem_cgroup_force_empty(struct mem_cgroup *memcg)
+{
+	int nr_retries = MEM_CGROUP_RECLAIM_RETRIES;
+	struct cgroup *cgrp = memcg->css.cgroup;
 
-try_to_free:
 	/* returns EBUSY if there is a task or if we come here twice. */
-	if (cgroup_task_count(cgrp) || !list_empty(&cgrp->children) || shrink) {
-		ret = -EBUSY;
-		goto out;
-	}
+	if (cgroup_task_count(cgrp) || !list_empty(&cgrp->children))
+		return -EBUSY;
+
 	/* we call try-to-free pages for make this cgroup empty */
 	lru_add_drain_all();
 	/* try to free all pages in this cgroup */
-	shrink = 1;
 	while (nr_retries && res_counter_read_u64(&memcg->res, RES_USAGE) > 0) {
 		int progress;
 
-		if (signal_pending(current)) {
-			ret = -EINTR;
-			goto out;
-		}
+		if (signal_pending(current))
+			return -EINTR;
+
 		progress = try_to_free_mem_cgroup_pages(memcg, GFP_KERNEL,
 						false);
 		if (!progress) {
@@ -3808,13 +3809,19 @@ try_to_free:
 
 	}
 	lru_add_drain();
-	/* try move_account...there may be some *locked* pages. */
-	goto move_account;
+	return mem_cgroup_reparent_charges(memcg);
 }
 
 static int mem_cgroup_force_empty_write(struct cgroup *cont, unsigned int event)
 {
-	return mem_cgroup_force_empty(mem_cgroup_from_cont(cont), true);
+	struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
+	int ret;
+
+	css_get(&memcg->css);
+	ret = mem_cgroup_force_empty(memcg);
+	css_put(&memcg->css);
+
+	return ret;
 }
 
 
@@ -5004,8 +5011,13 @@ free_out:
 static int mem_cgroup_pre_destroy(struct cgroup *cont)
 {
 	struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
+	int ret;
 
-	return mem_cgroup_force_empty(memcg, false);
+	css_get(&memcg->css);
+	ret = mem_cgroup_reparent_charges(memcg);
+	css_put(&memcg->css);
+
+	return ret;
 }
 
 static void mem_cgroup_destroy(struct cgroup *cont)
-- 
1.7.10.4

Re: [PATCH 1/6] memcg: split mem_cgroup_force_empty into reclaiming and reparenting parts

[Tejun Heo]

On Wed, Oct 17, 2012 at 03:30:43PM +0200, Michal Hocko wrote:
> mem_cgroup_force_empty did two separate things depending on free_all
> parameter from the very beginning. It either reclaimed as many pages as
> possible and moved the rest to the parent or just moved charges to the
> parent. The first variant is used as memory.force_empty callback while
> the later is used from the mem_cgroup_pre_destroy.
> 
> The whole games around gotos are far from being nice and there is no
> reason to keep those two functions inside one. Let's split them and
> also move the responsibility for css reference counting to their callers
> to make to code easier.
> 
> This patch doesn't have any functional changes.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

Reviewed-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

[PATCH 2/6] memcg: root_cgroup cannot reach mem_cgroup_move_parent

[Michal Hocko]

The root cgroup cannot be destroyed so we never hit it idown the
mem_cgroup_pre_destroy path and mem_cgroup_force_empty_write shouldn't
even try to do anything if called for the root.

This means that mem_cgroup_move_parent doesn't have to bother with the
root cgroup and it can assume it can always move charges upwards.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/memcontrol.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index f25e9c0..9ce24b7 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2709,9 +2709,7 @@ static int mem_cgroup_move_parent(struct page *page,
 	unsigned long uninitialized_var(flags);
 	int ret;
 
-	/* Is ROOT ? */
-	if (mem_cgroup_is_root(child))
-		return -EINVAL;
+	VM_BUG_ON(mem_cgroup_is_root(child));
 
 	ret = -EBUSY;
 	if (!get_page_unless_zero(page))
@@ -3817,6 +3815,8 @@ static int mem_cgroup_force_empty_write(struct cgroup *cont, unsigned int event)
 	struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
 	int ret;
 
+	if (mem_cgroup_is_root(memcg))
+		return -EINVAL;
 	css_get(&memcg->css);
 	ret = mem_cgroup_force_empty(memcg);
 	css_put(&memcg->css);
-- 
1.7.10.4

Re: [PATCH 2/6] memcg: root_cgroup cannot reach mem_cgroup_move_parent

[Tejun Heo]

On Wed, Oct 17, 2012 at 03:30:44PM +0200, Michal Hocko wrote:
> The root cgroup cannot be destroyed so we never hit it idown the
> mem_cgroup_pre_destroy path and mem_cgroup_force_empty_write shouldn't
> even try to do anything if called for the root.
> 
> This means that mem_cgroup_move_parent doesn't have to bother with the
> root cgroup and it can assume it can always move charges upwards.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

Reviewed-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

[PATCH 3/6] memcg: Simplify mem_cgroup_force_empty_list error handling

[Michal Hocko]

mem_cgroup_force_empty_list currently tries to remove all pages from
the given LRU. To prevent from temoporary failures (EBUSY returned by
mem_cgroup_move_parent) it uses a margin to the current LRU pages and
returns the true if there are still some pages left on the list.

If we consider that mem_cgroup_move_parent fails only when we are racing
with somebody else removing the page (resp. uncharging it) or when the
page is migrated then it is obvious that all those failures are only
temporal and so we can safely retry later.
Let's get rid of the safety margin and make the loop really wait for the
empty LRU. The caller should still make sure that all charges have been
removed from the res_counter because mem_cgroup_replace_page_cache might
add a page to the LRU after the check (it doesn't touch res_counter
though).
This catches most of the cases except for shmem which might call
mem_cgroup_replace_page_cache with a page which is not charged and on
the LRU yet but this was the case also without this patch. In order to
fix this we need a guarantee that try_get_mem_cgroup_from_page falls
back to the current mm's cgroup so it needs css_tryget to fail. This
will be fixed up in a later patch because it nees a help from cgroup
core.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/memcontrol.c |   52 +++++++++++++++++++++++++++-------------------------
 1 file changed, 27 insertions(+), 25 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 9ce24b7..f57ba4c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2697,9 +2697,13 @@ out:
 }
 
 /*
- * move charges to its parent.
+ * move charges to its parent or the root cgroup if the group
+ * has no parent (aka use_hierarchy==0).
+ * Although this might fail the failure is always temporary and it
+ * signals a race with a page removal/uncharge or migration. In the
+ * first case the page will vanish from the LRU on the next attempt
+ * and the call should be retried later.
  */
-
 static int mem_cgroup_move_parent(struct page *page,
 				  struct page_cgroup *pc,
 				  struct mem_cgroup *child)
@@ -2726,8 +2730,10 @@ static int mem_cgroup_move_parent(struct page *page,
 	if (!parent)
 		parent = root_mem_cgroup;
 
-	if (nr_pages > 1)
+	if (nr_pages > 1) {
+		VM_BUG_ON(!PageTransHuge(page));
 		flags = compound_lock_irqsave(page);
+	}
 
 	ret = mem_cgroup_move_account(page, nr_pages,
 				pc, child, parent);
@@ -3679,15 +3685,14 @@ unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order,
 
 /*
  * 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.
+ * reclaim the pages page themselves - pages are moved to the parent (or root)
+ * group.
  */
-static bool mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
+static void mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
 				int node, int zid, enum lru_list lru)
 {
 	struct mem_cgroup_per_zone *mz;
-	unsigned long flags, loop;
+	unsigned long flags;
 	struct list_head *list;
 	struct page *busy;
 	struct zone *zone;
@@ -3696,11 +3701,8 @@ static bool mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
 	mz = mem_cgroup_zoneinfo(memcg, node, zid);
 	list = &mz->lruvec.lists[lru];
 
-	loop = mz->lru_size[lru];
-	/* give some margin against EBUSY etc...*/
-	loop += 256;
 	busy = NULL;
-	while (loop--) {
+	do {
 		struct page_cgroup *pc;
 		struct page *page;
 
@@ -3726,8 +3728,7 @@ static bool mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
 			cond_resched();
 		} else
 			busy = NULL;
-	}
-	return !list_empty(list);
+	} while (!list_empty(list));
 }
 
 /*
@@ -3741,7 +3742,6 @@ static int mem_cgroup_reparent_charges(struct mem_cgroup *memcg)
 {
 	struct cgroup *cgrp = memcg->css.cgroup;
 	int node, zid;
-	int ret;
 
 	do {
 		if (cgroup_task_count(cgrp) || !list_empty(&cgrp->children))
@@ -3749,28 +3749,30 @@ static int mem_cgroup_reparent_charges(struct mem_cgroup *memcg)
 		/* This is for making all *used* pages to be on LRU. */
 		lru_add_drain_all();
 		drain_all_stock_sync(memcg);
-		ret = 0;
 		mem_cgroup_start_move(memcg);
 		for_each_node_state(node, N_HIGH_MEMORY) {
-			for (zid = 0; !ret && zid < MAX_NR_ZONES; zid++) {
+			for (zid = 0; zid < MAX_NR_ZONES; zid++) {
 				enum lru_list lru;
 				for_each_lru(lru) {
-					ret = mem_cgroup_force_empty_list(memcg,
+					mem_cgroup_force_empty_list(memcg,
 							node, zid, lru);
-					if (ret)
-						break;
 				}
 			}
-			if (ret)
-				break;
 		}
 		mem_cgroup_end_move(memcg);
 		memcg_oom_recover(memcg);
 		cond_resched();
-	/* "ret" should also be checked to ensure all lists are empty. */
-	} while (res_counter_read_u64(&memcg->res, RES_USAGE) > 0 || ret);
 
-	return ret;
+		/*
+		 * This is a safety check because mem_cgroup_force_empty_list
+		 * could have raced with mem_cgroup_replace_page_cache callers
+		 * so the lru seemed empty but the page could have been added
+		 * right after the check. RES_USAGE should be safe as we always
+		 * charge before adding to the LRU.
+		 */
+	} while (res_counter_read_u64(&memcg->res, RES_USAGE) > 0);
+
+	return 0;
 }
 
 /*
-- 
1.7.10.4

Re: [PATCH 3/6] memcg: Simplify mem_cgroup_force_empty_list error handling

[Tejun Heo]

Hello, Michal.

On Wed, Oct 17, 2012 at 03:30:45PM +0200, Michal Hocko wrote:
> mem_cgroup_force_empty_list currently tries to remove all pages from
> the given LRU. To prevent from temoporary failures (EBUSY returned by
> mem_cgroup_move_parent) it uses a margin to the current LRU pages and
> returns the true if there are still some pages left on the list.
> 
> If we consider that mem_cgroup_move_parent fails only when we are racing
> with somebody else removing the page (resp. uncharging it) or when the
> page is migrated then it is obvious that all those failures are only
> temporal and so we can safely retry later.
> Let's get rid of the safety margin and make the loop really wait for the
> empty LRU. The caller should still make sure that all charges have been
> removed from the res_counter because mem_cgroup_replace_page_cache might
> add a page to the LRU after the check (it doesn't touch res_counter
> though).
> This catches most of the cases except for shmem which might call
> mem_cgroup_replace_page_cache with a page which is not charged and on
> the LRU yet but this was the case also without this patch. In order to
> fix this we need a guarantee that try_get_mem_cgroup_from_page falls
> back to the current mm's cgroup so it needs css_tryget to fail. This
> will be fixed up in a later patch because it nees a help from cgroup
> core.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

In the sense that "I looked at it and nothing seemed too scary".

 Reviewed-by: Tejun Heo <tj@kernel.org>

Some nitpicks below.

>  /*
> - * move charges to its parent.
> + * move charges to its parent or the root cgroup if the group
> + * has no parent (aka use_hierarchy==0).
> + * Although this might fail the failure is always temporary and it
> + * signals a race with a page removal/uncharge or migration. In the
> + * first case the page will vanish from the LRU on the next attempt
> + * and the call should be retried later.
>   */
> -

Maybe convert to proper /** function comment while at it?  I also
think it would be helpful to actually comment on each possible failure
case explaining why the failure condition is temporal.

>  /*
>   * 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.
> + * reclaim the pages page themselves - pages are moved to the parent (or root)
> + * group.
>   */

Ditto.

> -static bool mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
> +static void mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
>  				int node, int zid, enum lru_list lru)
>  {
>  	struct mem_cgroup_per_zone *mz;
> -	unsigned long flags, loop;
> +	unsigned long flags;
>  	struct list_head *list;
>  	struct page *busy;
>  	struct zone *zone;
> @@ -3696,11 +3701,8 @@ static bool mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
>  	mz = mem_cgroup_zoneinfo(memcg, node, zid);
>  	list = &mz->lruvec.lists[lru];
>  
> -	loop = mz->lru_size[lru];
> -	/* give some margin against EBUSY etc...*/
> -	loop += 256;
>  	busy = NULL;
> -	while (loop--) {
> +	do {
>  		struct page_cgroup *pc;
>  		struct page *page;
>  
> @@ -3726,8 +3728,7 @@ static bool mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
>  			cond_resched();
>  		} else
>  			busy = NULL;
> -	}
> -	return !list_empty(list);
> +	} while (!list_empty(list));
>  }

Is there anything which can keep failing until migration to another
cgroup is complete?  I think there is, e.g., if mmap_sem is busy or
memcg is co-mounted with other controllers and another controller's
->attach() is blocking on something.

If so, busy-looping blindly probably isn't a good idea and we would
want at least msleep between retries (e.g. have two lists, throw
failed ones to the other and sleep shortly when switching the front
and back lists).

> +		/*
> +		 * This is a safety check because mem_cgroup_force_empty_list
> +		 * could have raced with mem_cgroup_replace_page_cache callers
> +		 * so the lru seemed empty but the page could have been added
> +		 * right after the check. RES_USAGE should be safe as we always
> +		 * charge before adding to the LRU.
> +		 */
> +	} while (res_counter_read_u64(&memcg->res, RES_USAGE) > 0);

Maybe we want to trigger some warning if retry count gets too high?
At least for now?

Thanks.

-- 
tejun

Re: [PATCH 3/6] memcg: Simplify mem_cgroup_force_empty_list error handling

[Michal Hocko]

On Thu 18-10-12 15:16:54, Tejun Heo wrote:
> Hello, Michal.
> 
> On Wed, Oct 17, 2012 at 03:30:45PM +0200, Michal Hocko wrote:
> > mem_cgroup_force_empty_list currently tries to remove all pages from
> > the given LRU. To prevent from temoporary failures (EBUSY returned by
> > mem_cgroup_move_parent) it uses a margin to the current LRU pages and
> > returns the true if there are still some pages left on the list.
> > 
> > If we consider that mem_cgroup_move_parent fails only when we are racing
> > with somebody else removing the page (resp. uncharging it) or when the
> > page is migrated then it is obvious that all those failures are only
> > temporal and so we can safely retry later.
> > Let's get rid of the safety margin and make the loop really wait for the
> > empty LRU. The caller should still make sure that all charges have been
> > removed from the res_counter because mem_cgroup_replace_page_cache might
> > add a page to the LRU after the check (it doesn't touch res_counter
> > though).
> > This catches most of the cases except for shmem which might call
> > mem_cgroup_replace_page_cache with a page which is not charged and on
> > the LRU yet but this was the case also without this patch. In order to
> > fix this we need a guarantee that try_get_mem_cgroup_from_page falls
> > back to the current mm's cgroup so it needs css_tryget to fail. This
> > will be fixed up in a later patch because it nees a help from cgroup
> > core.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> 
> In the sense that "I looked at it and nothing seemed too scary".
> 
>  Reviewed-by: Tejun Heo <tj@kernel.org>

Thanks

> 
> Some nitpicks below.
> 
> >  /*
> > - * move charges to its parent.
> > + * move charges to its parent or the root cgroup if the group
> > + * has no parent (aka use_hierarchy==0).
> > + * Although this might fail the failure is always temporary and it
> > + * signals a race with a page removal/uncharge or migration. In the
> > + * first case the page will vanish from the LRU on the next attempt
> > + * and the call should be retried later.
> >   */
> > -
> 
> Maybe convert to proper /** function comment while at it?  

these are internal functions and we usually do not create kerneldoc for
them. But I can surely change it - it would deserve a bigger clean up
then.

> I also think it would be helpful to actually comment on each possible
> failure case explaining why the failure condition is temporal.

What about:
"
 * Although this might fail (get_page_unless_zero, isolate_lru_page or
 * mem_cgroup_move_account fails) the failure is always temporary and
 * it signals a race with a page removal/uncharge or migration. In the
 * first case the page is on the way out and it will vanish from the LRU
 * on the next attempt and the call should be retried later.
 * Isolation from the LRU fails only if page has been isolated from
 * the LRU since we looked at it and that usually means either global
 * reclaim or migration going on. The page will either get back to the
 * LRU or vanish.
 * Finaly mem_cgroup_move_account fails only if the page got uncharged
 * (!PageCgroupUsed) or moved to a different group. The page will
 * disappear in the next attempt.
"

Better? Or should it rather be in the changelog?

> 
> >  /*
> >   * 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.
> > + * reclaim the pages page themselves - pages are moved to the parent (or root)
> > + * group.
> >   */
> 
> Ditto.
> 
> > -static bool mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
> > +static void mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
> >  				int node, int zid, enum lru_list lru)
> >  {
> >  	struct mem_cgroup_per_zone *mz;
> > -	unsigned long flags, loop;
> > +	unsigned long flags;
> >  	struct list_head *list;
> >  	struct page *busy;
> >  	struct zone *zone;
> > @@ -3696,11 +3701,8 @@ static bool mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
> >  	mz = mem_cgroup_zoneinfo(memcg, node, zid);
> >  	list = &mz->lruvec.lists[lru];
> >  
> > -	loop = mz->lru_size[lru];
> > -	/* give some margin against EBUSY etc...*/
> > -	loop += 256;
> >  	busy = NULL;
> > -	while (loop--) {
> > +	do {
> >  		struct page_cgroup *pc;
> >  		struct page *page;
> >  
> > @@ -3726,8 +3728,7 @@ static bool mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
> >  			cond_resched();
> >  		} else
> >  			busy = NULL;
> > -	}
> > -	return !list_empty(list);
> > +	} while (!list_empty(list));
> >  }
> 
> Is there anything which can keep failing until migration to another
> cgroup is complete?  

This is not about migration to another cgroup. Remember there are no
tasks in the group so we have no origin for the migration. I was talking
about migrate_pages.

> I think there is, e.g., if mmap_sem is busy or memcg is co-mounted
> with other controllers and another controller's ->attach() is blocking
> on something.

I am not sure I understand your concern. There are no tasks and we will
break out the loop if some appear. And yes we can retry a lot in
pathological cases. But this is a group removal path which is not hot.

> If so, busy-looping blindly probably isn't a good idea and we would
> want at least msleep between retries (e.g. have two lists, throw
> failed ones to the other and sleep shortly when switching the front
> and back lists).

we do cond_resched if we fail.

> > +		/*
> > +		 * This is a safety check because mem_cgroup_force_empty_list
> > +		 * could have raced with mem_cgroup_replace_page_cache callers
> > +		 * so the lru seemed empty but the page could have been added
> > +		 * right after the check. RES_USAGE should be safe as we always
> > +		 * charge before adding to the LRU.
> > +		 */
> > +	} while (res_counter_read_u64(&memcg->res, RES_USAGE) > 0);
> 
> Maybe we want to trigger some warning if retry count gets too high?
> At least for now?

We can but is this really worth it?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 3/6] memcg: Simplify mem_cgroup_force_empty_list error handling

[Tejun Heo]

Hello, Michal.

On Fri, Oct 19, 2012 at 03:24:38PM +0200, Michal Hocko wrote:
> > Maybe convert to proper /** function comment while at it?  
> 
> these are internal functions and we usually do not create kerneldoc for
> them. But I can surely change it - it would deserve a bigger clean up
> then.

Yeah, I got into the habit of making function comments kerneldoc if
the function is important / scary enough.  It's upto you but I think
that would be an improvement here.

> What about:
> "
>  * Although this might fail (get_page_unless_zero, isolate_lru_page or
>  * mem_cgroup_move_account fails) the failure is always temporary and
>  * it signals a race with a page removal/uncharge or migration. In the
>  * first case the page is on the way out and it will vanish from the LRU
>  * on the next attempt and the call should be retried later.
>  * Isolation from the LRU fails only if page has been isolated from
>  * the LRU since we looked at it and that usually means either global
>  * reclaim or migration going on. The page will either get back to the
>  * LRU or vanish.
>  * Finaly mem_cgroup_move_account fails only if the page got uncharged
>  * (!PageCgroupUsed) or moved to a different group. The page will
>  * disappear in the next attempt.
> "
> 
> Better? Or should it rather be in the changelog?

Looks good to me and I personally think it deserves to be a comment.

> > Is there anything which can keep failing until migration to another
> > cgroup is complete?  
> 
> This is not about migration to another cgroup. Remember there are no
> tasks in the group so we have no origin for the migration. I was talking
> about migrate_pages.
> 
> > I think there is, e.g., if mmap_sem is busy or memcg is co-mounted
> > with other controllers and another controller's ->attach() is blocking
> > on something.
> 
> I am not sure I understand your concern. There are no tasks and we will
> break out the loop if some appear. And yes we can retry a lot in
> pathological cases. But this is a group removal path which is not hot.

Ah, okay, I misunderstood that it could wait for task cgroup
migration.

> > If so, busy-looping blindly probably isn't a good idea and we would
> > want at least msleep between retries (e.g. have two lists, throw
> > failed ones to the other and sleep shortly when switching the front
> > and back lists).
> 
> we do cond_resched if we fail.

If it won't ever spin for someone else sleeping, I think it should be
fine.

> > Maybe we want to trigger some warning if retry count gets too high?
> > At least for now?
> 
> We can but is this really worth it?

I don't know.  My sense of danger here is likely to be way off
compared to yours so if you think it's a fairly safe loop, it probably
is.

It just reminds me of the busy looping we had in freezer.  It was
correct but actually manifested as a problem - when a system was going
down for emergency hibernation from low battery, that busy loop not
too rarely drained the small reserve making the machine lose power
before completing hibernation.  So, it could be that I'm a bit
paranoid here.

Thanks.

-- 
tejun

[PATCH 4/6] cgroups: forbid pre_destroy callback to fail

[Michal Hocko]

Now that mem_cgroup_pre_destroy callback doesn't fail finally we can
safely move on and forbit all the callbacks to fail. The last missing
piece is moving cgroup_call_pre_destroy after cgroup_clear_css_refs so
that css_tryget fails so no new charges for the memcg can happen.
The callbacks are also called from within cgroup_lock to guarantee that
no new tasks show up. We could theoretically call them outside of the
lock but then we have to move after CGRP_REMOVED flag is set.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 kernel/cgroup.c |   30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index b7d9606..00729c1 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -855,7 +855,7 @@ static struct inode *cgroup_new_inode(umode_t mode, struct super_block *sb)
  * Call subsys's pre_destroy handler.
  * This is called before css refcnt check.
  */
-static int cgroup_call_pre_destroy(struct cgroup *cgrp)
+static void cgroup_call_pre_destroy(struct cgroup *cgrp)
 {
 	struct cgroup_subsys *ss;
 	int ret = 0;
@@ -864,15 +864,8 @@ static int cgroup_call_pre_destroy(struct cgroup *cgrp)
 		if (!ss->pre_destroy)
 			continue;
 
-		ret = ss->pre_destroy(cgrp);
-		if (ret) {
-			/* ->pre_destroy() failure is being deprecated */
-			WARN_ON_ONCE(!ss->__DEPRECATED_clear_css_refs);
-			break;
-		}
+		BUG_ON(ss->pre_destroy(cgrp));
 	}
-
-	return ret;
 }
 
 static void cgroup_diput(struct dentry *dentry, struct inode *inode)
@@ -4161,7 +4154,6 @@ again:
 		mutex_unlock(&cgroup_mutex);
 		return -EBUSY;
 	}
-	mutex_unlock(&cgroup_mutex);
 
 	/*
 	 * In general, subsystem has no css->refcnt after pre_destroy(). But
@@ -4174,17 +4166,6 @@ again:
 	 */
 	set_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
 
-	/*
-	 * Call pre_destroy handlers of subsys. Notify subsystems
-	 * that rmdir() request comes.
-	 */
-	ret = cgroup_call_pre_destroy(cgrp);
-	if (ret) {
-		clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
-		return ret;
-	}
-
-	mutex_lock(&cgroup_mutex);
 	parent = cgrp->parent;
 	if (atomic_read(&cgrp->count) || !list_empty(&cgrp->children)) {
 		clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
@@ -4206,6 +4187,13 @@ again:
 			return -EINTR;
 		goto again;
 	}
+
+	/*
+	 * Call pre_destroy handlers of subsys. Notify subsystems
+	 * that rmdir() request comes.
+	 */
+	cgroup_call_pre_destroy(cgrp);
+
 	/* NO css_tryget() can success after here. */
 	finish_wait(&cgroup_rmdir_waitq, &wait);
 	clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
-- 
1.7.10.4

Re: [PATCH 4/6] cgroups: forbid pre_destroy callback to fail

[Tejun Heo]

Hello, Michal.

On Wed, Oct 17, 2012 at 03:30:46PM +0200, Michal Hocko wrote:
> Now that mem_cgroup_pre_destroy callback doesn't fail finally we can
> safely move on and forbit all the callbacks to fail. The last missing
> piece is moving cgroup_call_pre_destroy after cgroup_clear_css_refs so
> that css_tryget fails so no new charges for the memcg can happen.
> The callbacks are also called from within cgroup_lock to guarantee that
> no new tasks show up. We could theoretically call them outside of the
> lock but then we have to move after CGRP_REMOVED flag is set.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

So, the plan is to do something like the following once memcg is
ready.

  http://thread.gmane.org/gmane.linux.kernel.containers/22559/focus=75251

Note that the patch is broken in a couple places but it does show the
general direction.  I'd prefer if patch #3 simply makes pre_destroy()
return 0 and drop __DEPRECATED_clear_css_refs from mem_cgroup_subsys.
Then, I can pull the branch in and drop all the unnecessary cruft.

Thanks.

-- 
tejun

Re: [PATCH 4/6] cgroups: forbid pre_destroy callback to fail

[Tejun Heo]

On Thu, Oct 18, 2012 at 03:41:48PM -0700, Tejun Heo wrote:
> Note that the patch is broken in a couple places but it does show the
> general direction.  I'd prefer if patch #3 simply makes pre_destroy()
> return 0 and drop __DEPRECATED_clear_css_refs from mem_cgroup_subsys.
> Then, I can pull the branch in and drop all the unnecessary cruft.

But you need the locking change for further memcg cleanup.  To avoid
interlocked pulls from both sides, I think it's okay to push this one
with the rest of memcg changes.  I can do the cleanup on top of this
whole series, but please do drop .__DEPRECATED_clear_css_refs from
memcg.

 Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH 4/6] cgroups: forbid pre_destroy callback to fail

[Michal Hocko]

On Thu 18-10-12 15:46:06, Tejun Heo wrote:
> On Thu, Oct 18, 2012 at 03:41:48PM -0700, Tejun Heo wrote:
> > Note that the patch is broken in a couple places but it does show the
> > general direction.  I'd prefer if patch #3 simply makes pre_destroy()
> > return 0 and drop __DEPRECATED_clear_css_refs from mem_cgroup_subsys.
> > Then, I can pull the branch in and drop all the unnecessary cruft.
> 
> But you need the locking change for further memcg cleanup.  To avoid
> interlocked pulls from both sides, I think it's okay to push this one
> with the rest of memcg changes.  I can do the cleanup on top of this
> whole series, but please do drop .__DEPRECATED_clear_css_refs from
> memcg.

OK I will drop that one.

>  Acked-by: Tejun Heo <tj@kernel.org>

Do you still agree with the v2 based on Li's feedback?

Thanks
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/6] cgroups: forbid pre_destroy callback to fail

[Michal Hocko]

On Thu 18-10-12 15:41:48, Tejun Heo wrote:
> Hello, Michal.
> 
> On Wed, Oct 17, 2012 at 03:30:46PM +0200, Michal Hocko wrote:
> > Now that mem_cgroup_pre_destroy callback doesn't fail finally we can
> > safely move on and forbit all the callbacks to fail. The last missing
> > piece is moving cgroup_call_pre_destroy after cgroup_clear_css_refs so
> > that css_tryget fails so no new charges for the memcg can happen.
> > The callbacks are also called from within cgroup_lock to guarantee that
> > no new tasks show up. We could theoretically call them outside of the
> > lock but then we have to move after CGRP_REMOVED flag is set.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> 
> So, the plan is to do something like the following once memcg is
> ready.
> 
>   http://thread.gmane.org/gmane.linux.kernel.containers/22559/focus=75251
> 
> Note that the patch is broken in a couple places but it does show the
> general direction.  I'd prefer if patch #3 simply makes pre_destroy()
> return 0 and drop __DEPRECATED_clear_css_refs from mem_cgroup_subsys.

We can still fail inn #3 without this patch becasuse there are is no
guarantee that a new task is attached to the group. And I wanted to keep
memcg and generic cgroup parts separated.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/6] cgroups: forbid pre_destroy callback to fail

[Tejun Heo]

Hello, Michal.

On Fri, Oct 19, 2012 at 03:32:45PM +0200, Michal Hocko wrote:
> On Thu 18-10-12 15:41:48, Tejun Heo wrote:
> > Hello, Michal.
> > 
> > On Wed, Oct 17, 2012 at 03:30:46PM +0200, Michal Hocko wrote:
> > > Now that mem_cgroup_pre_destroy callback doesn't fail finally we can
> > > safely move on and forbit all the callbacks to fail. The last missing
> > > piece is moving cgroup_call_pre_destroy after cgroup_clear_css_refs so
> > > that css_tryget fails so no new charges for the memcg can happen.
> > > The callbacks are also called from within cgroup_lock to guarantee that
> > > no new tasks show up. We could theoretically call them outside of the
> > > lock but then we have to move after CGRP_REMOVED flag is set.
> > > 
> > > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> > 
> > So, the plan is to do something like the following once memcg is
> > ready.
> > 
> >   http://thread.gmane.org/gmane.linux.kernel.containers/22559/focus=75251
> > 
> > Note that the patch is broken in a couple places but it does show the
> > general direction.  I'd prefer if patch #3 simply makes pre_destroy()
> > return 0 and drop __DEPRECATED_clear_css_refs from mem_cgroup_subsys.
> 
> We can still fail inn #3 without this patch becasuse there are is no
> guarantee that a new task is attached to the group. And I wanted to keep
> memcg and generic cgroup parts separated.

Yes, but all other controllers are broken that way too and the worst
thing which will hapen is triggering WARN_ON_ONCE().  Let's note the
failure in the commit and remove DEPREDATED_clear_css_refs in the
previous patch.  Then, I can pull from you, clean up pre_destroy mess
and then you can pull back for further cleanups.

Thanks.

-- 
tejun

Re: [PATCH 4/6] cgroups: forbid pre_destroy callback to fail

[Michal Hocko]

On Fri 19-10-12 13:24:05, Tejun Heo wrote:
> Hello, Michal.
> 
> On Fri, Oct 19, 2012 at 03:32:45PM +0200, Michal Hocko wrote:
> > On Thu 18-10-12 15:41:48, Tejun Heo wrote:
> > > Hello, Michal.
> > > 
> > > On Wed, Oct 17, 2012 at 03:30:46PM +0200, Michal Hocko wrote:
> > > > Now that mem_cgroup_pre_destroy callback doesn't fail finally we can
> > > > safely move on and forbit all the callbacks to fail. The last missing
> > > > piece is moving cgroup_call_pre_destroy after cgroup_clear_css_refs so
> > > > that css_tryget fails so no new charges for the memcg can happen.
> > > > The callbacks are also called from within cgroup_lock to guarantee that
> > > > no new tasks show up. We could theoretically call them outside of the
> > > > lock but then we have to move after CGRP_REMOVED flag is set.
> > > > 
> > > > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> > > 
> > > So, the plan is to do something like the following once memcg is
> > > ready.
> > > 
> > >   http://thread.gmane.org/gmane.linux.kernel.containers/22559/focus=75251
> > > 
> > > Note that the patch is broken in a couple places but it does show the
> > > general direction.  I'd prefer if patch #3 simply makes pre_destroy()
> > > return 0 and drop __DEPRECATED_clear_css_refs from mem_cgroup_subsys.
> > 
> > We can still fail inn #3 without this patch becasuse there are is no
> > guarantee that a new task is attached to the group. And I wanted to keep
> > memcg and generic cgroup parts separated.
> 
> Yes, but all other controllers are broken that way too

It's just hugetlb and memcg that have pre_destroy.

> and the worst thing which will hapen is triggering WARN_ON_ONCE().

The patch does BUG_ON(ss->pre_destroy(cgrp)). I am not sure WARN_ON_ONCE is
appropriate here because we would like to have it at least per
controller warning. I do not see any reason why to make this more
complicated but I am open to suggestions.

> Let's note the failure in the commit and remove
> DEPREDATED_clear_css_refs in the previous patch.  Then, I can pull
> from you, clean up pre_destroy mess and then you can pull back for
> further cleanups.

Well this will get complicated as there are dependencies between memcg
parts (based on Andrew's tree) and your tree. My tree is not pullable as
all the patches go via Andrew. I am not sure how to get out of this.
There is only one cgroup patch so what about pushing all of this via
Andrew and do the follow up cleanups once they get merged? We are not in
hurry, are we?

Anyway does it really make sense to drop DEPREDATED_clear_css_refs
already in the previous patch when it is _not_ guaranteed that
pre_destroy succeeds?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/6] cgroups: forbid pre_destroy callback to fail

[Tejun Heo]

Hello, Michal.

On Mon, Oct 22, 2012 at 12:30:21PM +0200, Michal Hocko wrote:
> > > We can still fail inn #3 without this patch becasuse there are is no
> > > guarantee that a new task is attached to the group. And I wanted to keep
> > > memcg and generic cgroup parts separated.
> > 
> > Yes, but all other controllers are broken that way too
> 
> It's just hugetlb and memcg that have pre_destroy.
> 
> > and the worst thing which will hapen is triggering WARN_ON_ONCE().
> 
> The patch does BUG_ON(ss->pre_destroy(cgrp)). I am not sure WARN_ON_ONCE is
> appropriate here because we would like to have it at least per
> controller warning. I do not see any reason why to make this more
> complicated but I am open to suggestions.

Once it's dropped from memcg, the next patch can update cgroup core
accordingly and the bug will exist for a single commit and the failure
mode would be triggering of WARN_ON_ONCE().  Seems pretty simple to
me.

> > Let's note the failure in the commit and remove
> > DEPREDATED_clear_css_refs in the previous patch.  Then, I can pull
> > from you, clean up pre_destroy mess and then you can pull back for
> > further cleanups.
> 
> Well this will get complicated as there are dependencies between memcg
> parts (based on Andrew's tree) and your tree. My tree is not pullable as
> all the patches go via Andrew. I am not sure how to get out of this.
> There is only one cgroup patch so what about pushing all of this via
> Andrew and do the follow up cleanups once they get merged? We are not in
> hurry, are we?

Let's create a cgroup branch and build things there.  I don't think
cgroup changes are gonna be a single patch and expect to see at least
some bug fixes afterwards and don't wanna keep them floating separate
from other cgroup changes.  mm being based on top of -next, that
should work, right?

> Anyway does it really make sense to drop DEPREDATED_clear_css_refs
> already in the previous patch when it is _not_ guaranteed that
> pre_destroy succeeds?

It makes things simpler here by decoupling memcg change with core
cgroup changes and the introduced bug isn't too easy to trigger and
even when triggered the failure mode isn't critical.  It's not gonna
break normal common operations or bisection.  As long as the issue is
clearly documented, I think it should be fine.  Just note that this
opens up a race window from deficient cgroup API and the following
commits will address it.

Thanks.

--
tejun

Re: [PATCH 4/6] cgroups: forbid pre_destroy callback to fail

[Michal Hocko]

On Wed 24-10-12 12:25:35, Tejun Heo wrote:
> Hello, Michal.
> 
> On Mon, Oct 22, 2012 at 12:30:21PM +0200, Michal Hocko wrote:
> > > > We can still fail inn #3 without this patch becasuse there are is no
> > > > guarantee that a new task is attached to the group. And I wanted to keep
> > > > memcg and generic cgroup parts separated.
> > > 
> > > Yes, but all other controllers are broken that way too
> > 
> > It's just hugetlb and memcg that have pre_destroy.
> > 
> > > and the worst thing which will hapen is triggering WARN_ON_ONCE().
> > 
> > The patch does BUG_ON(ss->pre_destroy(cgrp)). I am not sure WARN_ON_ONCE is
> > appropriate here because we would like to have it at least per
> > controller warning. I do not see any reason why to make this more
> > complicated but I am open to suggestions.
> 
> Once it's dropped from memcg, the next patch can update cgroup core
> accordingly and the bug will exist for a single commit and the failure
> mode would be triggering of WARN_ON_ONCE().  Seems pretty simple to
> me.

I am not sure I understand you here. So are you suggesting
s/BUG_ON/WARN_ON_ONCE/ in this patch?
It is true that this will not break bisectability but it is still not
correct (strictly speaking because any load that can race group removal
with new tasks addition would hit BUG/WARN and we will remove a group
with a task inside).
The patchset as posted makes sure that none of the stages adds a
regression and I would like to stick with that as much as possible if it
doesn't cause too much of a hassle.

> > > Let's note the failure in the commit and remove
> > > DEPREDATED_clear_css_refs in the previous patch.  Then, I can pull
> > > from you, clean up pre_destroy mess and then you can pull back for
> > > further cleanups.
> > 
> > Well this will get complicated as there are dependencies between memcg
> > parts (based on Andrew's tree) and your tree. My tree is not pullable as
> > all the patches go via Andrew. I am not sure how to get out of this.
> > There is only one cgroup patch so what about pushing all of this via
> > Andrew and do the follow up cleanups once they get merged? We are not in
> > hurry, are we?
> 
> Let's create a cgroup branch and build things there.  I don't think
> cgroup changes are gonna be a single patch and expect to see at least
> some bug fixes afterwards and don't wanna keep them floating separate
> from other cgroup changes.  

> mm being based on top of -next, that should work, right?

Well, a tree based on -next is, ehm, impractical. I can create a bug on
top of my -mm git branch (where I merge your cgroup common changes) for
development and then when we are ready we can send it as a series and
push it via Andrew. Would that work for you?
Or we can push the core part via Andrew, wait for the merge and work on
the follow up cleanups later?
It is not like the follow up part is really urgent, isn't it? I would
just like the memcg part settled first because this can potentially
conflict with other memcg work.

[...]
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/6] cgroups: forbid pre_destroy callback to fail

[Tejun Heo]

Hey, Michal.

On Thu, Oct 25, 2012 at 04:37:56PM +0200, Michal Hocko wrote:
> I am not sure I understand you here. So are you suggesting
> s/BUG_ON/WARN_ON_ONCE/ in this patch?

Oh, no, I meant that we can do upto patch 3 of this series and then
follow up with proper cgroup core update and then stack further
memcg cleanups on top.

> > Let's create a cgroup branch and build things there.  I don't think
> > cgroup changes are gonna be a single patch and expect to see at least
> > some bug fixes afterwards and don't wanna keep them floating separate
> > from other cgroup changes.  
> 
> > mm being based on top of -next, that should work, right?
> 
> Well, a tree based on -next is, ehm, impractical. I can create a bug on
> top of my -mm git branch (where I merge your cgroup common changes) for
> development and then when we are ready we can send it as a series and
> push it via Andrew. Would that work for you?
> Or we can push the core part via Andrew, wait for the merge and work on
> the follow up cleanups later?
> It is not like the follow up part is really urgent, isn't it? I would
> just like the memcg part settled first because this can potentially
> conflict with other memcg work.

Argh... can we pretty *please* just do a plain git branch?  I don't
care where it is but I want to be able to pull it into cgroup core and
yes I do wanna make this happen in this devel cycle.  We've been
sitting on it far too long waiting for memcg.

Thanks.

-- 
tejun

Re: [PATCH 4/6] cgroups: forbid pre_destroy callback to fail

[Michal Hocko]

On Thu 25-10-12 10:42:20, Tejun Heo wrote:
> Hey, Michal.
> 
> On Thu, Oct 25, 2012 at 04:37:56PM +0200, Michal Hocko wrote:
> > I am not sure I understand you here. So are you suggesting
> > s/BUG_ON/WARN_ON_ONCE/ in this patch?
> 
> Oh, no, I meant that we can do upto patch 3 of this series and then
> follow up with proper cgroup core update and then stack further
> memcg cleanups on top.

I thought the later cleanups would be on top of the series.

> > > Let's create a cgroup branch and build things there.  I don't think
> > > cgroup changes are gonna be a single patch and expect to see at least
> > > some bug fixes afterwards and don't wanna keep them floating separate
> > > from other cgroup changes.  
> > 
> > > mm being based on top of -next, that should work, right?
> > 
> > Well, a tree based on -next is, ehm, impractical. I can create a bug on
> > top of my -mm git branch (where I merge your cgroup common changes) for
> > development and then when we are ready we can send it as a series and
> > push it via Andrew. Would that work for you?
> > Or we can push the core part via Andrew, wait for the merge and work on
> > the follow up cleanups later?
> > It is not like the follow up part is really urgent, isn't it? I would
> > just like the memcg part settled first because this can potentially
> > conflict with other memcg work.
> 
> Argh... can we pretty *please* just do a plain git branch?  I don't
> care where it is but I want to be able to pull it into cgroup core and

Hohumm, I have tried to apply the series on top of Linus' 3.6 and there
were no conflicts so I can create a branch which you can pull into your
cgroup branch (which I can then merge into -mm git tree).
This would however mean that those patches wouldn't fly through Andrew's
tree. Is this really what we want and what does it give to us?

> yes I do wanna make this happen in this devel cycle.  We've been
> sitting on it far too long waiting for memcg.

I can surely imagine that (for the memcg part) but it needs throughout
review.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/6] cgroups: forbid pre_destroy callback to fail

[Li Zefan]

On 2012/10/17 21:30, Michal Hocko wrote:
> Now that mem_cgroup_pre_destroy callback doesn't fail finally we can
> safely move on and forbit all the callbacks to fail. The last missing
> piece is moving cgroup_call_pre_destroy after cgroup_clear_css_refs so
> that css_tryget fails so no new charges for the memcg can happen.

> The callbacks are also called from within cgroup_lock to guarantee that
> no new tasks show up. 

I'm afraid this won't work. See commit 3fa59dfbc3b223f02c26593be69ce6fc9a940405
("cgroup: fix potential deadlock in pre_destroy")

> We could theoretically call them outside of the
> lock but then we have to move after CGRP_REMOVED flag is set.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> ---
>  kernel/cgroup.c |   30 +++++++++---------------------
>  1 file changed, 9 insertions(+), 21 deletions(-)

Re: [PATCH 4/6] cgroups: forbid pre_destroy callback to fail

[Michal Hocko]

On Fri 19-10-12 17:33:18, Li Zefan wrote:
> On 2012/10/17 21:30, Michal Hocko wrote:
> > Now that mem_cgroup_pre_destroy callback doesn't fail finally we can
> > safely move on and forbit all the callbacks to fail. The last missing
> > piece is moving cgroup_call_pre_destroy after cgroup_clear_css_refs so
> > that css_tryget fails so no new charges for the memcg can happen.
> 
> > The callbacks are also called from within cgroup_lock to guarantee that
> > no new tasks show up. 
> 
> I'm afraid this won't work. See commit 3fa59dfbc3b223f02c26593be69ce6fc9a940405
> ("cgroup: fix potential deadlock in pre_destroy")

Very good point. Thanks for poiting this out. So we should call
pre_destroy at the very end? What about the following?
Or should be rather drop the lock after check_for_release(parent) or
sooner but after CGRP_REMOVED is set?
---
>From 70ea8718aba1c1784b94bfb26aa2307195c07c0b Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Wed, 17 Oct 2012 13:42:06 +0200
Subject: [PATCH] cgroups: forbid pre_destroy callback to fail

Now that mem_cgroup_pre_destroy callback doesn't fail finally we can
safely move on and forbit all the callbacks to fail. The last missing
piece is moving cgroup_call_pre_destroy after cgroup_clear_css_refs so
that css_tryget fails so no new charges for the memcg can happen.
We cannot, however, move cgroup_call_pre_destroy right after because we
cannot call mem_cgroup_pre_destroy with the cgroup_lock held (see
3fa59dfb cgroup: fix potential deadlock in pre_destroy) so we have to
move it after the lock is released.

Changes since v1
- Li Zefan pointed out that mem_cgroup_pre_destroy cannot be called with
  cgroup_lock held

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 kernel/cgroup.c |   30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index b7d9606..4c6adbd 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -855,7 +855,7 @@ static struct inode *cgroup_new_inode(umode_t mode, struct super_block *sb)
  * Call subsys's pre_destroy handler.
  * This is called before css refcnt check.
  */
-static int cgroup_call_pre_destroy(struct cgroup *cgrp)
+static void cgroup_call_pre_destroy(struct cgroup *cgrp)
 {
 	struct cgroup_subsys *ss;
 	int ret = 0;
@@ -864,15 +864,8 @@ static int cgroup_call_pre_destroy(struct cgroup *cgrp)
 		if (!ss->pre_destroy)
 			continue;
 
-		ret = ss->pre_destroy(cgrp);
-		if (ret) {
-			/* ->pre_destroy() failure is being deprecated */
-			WARN_ON_ONCE(!ss->__DEPRECATED_clear_css_refs);
-			break;
-		}
+		BUG_ON(ss->pre_destroy(cgrp));
 	}
-
-	return ret;
 }
 
 static void cgroup_diput(struct dentry *dentry, struct inode *inode)
@@ -4161,7 +4154,6 @@ again:
 		mutex_unlock(&cgroup_mutex);
 		return -EBUSY;
 	}
-	mutex_unlock(&cgroup_mutex);
 
 	/*
 	 * In general, subsystem has no css->refcnt after pre_destroy(). But
@@ -4174,17 +4166,6 @@ again:
 	 */
 	set_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
 
-	/*
-	 * Call pre_destroy handlers of subsys. Notify subsystems
-	 * that rmdir() request comes.
-	 */
-	ret = cgroup_call_pre_destroy(cgrp);
-	if (ret) {
-		clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
-		return ret;
-	}
-
-	mutex_lock(&cgroup_mutex);
 	parent = cgrp->parent;
 	if (atomic_read(&cgrp->count) || !list_empty(&cgrp->children)) {
 		clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
@@ -4206,6 +4187,7 @@ again:
 			return -EINTR;
 		goto again;
 	}
+
 	/* NO css_tryget() can success after here. */
 	finish_wait(&cgroup_rmdir_waitq, &wait);
 	clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
@@ -4244,6 +4226,12 @@ again:
 	spin_unlock(&cgrp->event_list_lock);
 
 	mutex_unlock(&cgroup_mutex);
+
+	/*
+	 * Call pre_destroy handlers of subsys. Notify subsystems
+	 * that rmdir() request comes.
+	 */
+	cgroup_call_pre_destroy(cgrp);
 	return 0;
 }
 
-- 
1.7.10.4


-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/6] cgroups: forbid pre_destroy callback to fail

[Tejun Heo]

On Fri, Oct 19, 2012 at 05:33:18PM +0800, Li Zefan wrote:
> On 2012/10/17 21:30, Michal Hocko wrote:
> > Now that mem_cgroup_pre_destroy callback doesn't fail finally we can
> > safely move on and forbit all the callbacks to fail. The last missing
> > piece is moving cgroup_call_pre_destroy after cgroup_clear_css_refs so
> > that css_tryget fails so no new charges for the memcg can happen.
> 
> > The callbacks are also called from within cgroup_lock to guarantee that
> > no new tasks show up. 
> 
> I'm afraid this won't work. See commit 3fa59dfbc3b223f02c26593be69ce6fc9a940405
> ("cgroup: fix potential deadlock in pre_destroy")

Yeah, you're right.  Argh... we really should unexport cgroup_lock
soon.

Thanks.

-- 
tejun

[PATCH 5/6] memcg: make mem_cgroup_reparent_charges non failing

[Michal Hocko]

Now that pre_destroy callbacks are called from within cgroup_lock and
the cgroup has been checked to be empty without any children then there
is no other way to fail.
mem_cgroup_pre_destroy doesn't have to take a reference to memcg's css
because all css' are marked dead already.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/memcontrol.c |   18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index f57ba4c..7c75da3 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3738,14 +3738,12 @@ static void mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
  *
  * Caller is responsible for holding css reference on the memcg.
  */
-static int mem_cgroup_reparent_charges(struct mem_cgroup *memcg)
+static void mem_cgroup_reparent_charges(struct mem_cgroup *memcg)
 {
 	struct cgroup *cgrp = memcg->css.cgroup;
 	int node, zid;
 
 	do {
-		if (cgroup_task_count(cgrp) || !list_empty(&cgrp->children))
-			return -EBUSY;
 		/* This is for making all *used* pages to be on LRU. */
 		lru_add_drain_all();
 		drain_all_stock_sync(memcg);
@@ -3771,8 +3769,6 @@ static int mem_cgroup_reparent_charges(struct mem_cgroup *memcg)
 		 * charge before adding to the LRU.
 		 */
 	} while (res_counter_read_u64(&memcg->res, RES_USAGE) > 0);
-
-	return 0;
 }
 
 /*
@@ -3809,7 +3805,9 @@ static int mem_cgroup_force_empty(struct mem_cgroup *memcg)
 
 	}
 	lru_add_drain();
-	return mem_cgroup_reparent_charges(memcg);
+	mem_cgroup_reparent_charges(memcg);
+
+	return 0;
 }
 
 static int mem_cgroup_force_empty_write(struct cgroup *cont, unsigned int event)
@@ -5013,13 +5011,9 @@ free_out:
 static int mem_cgroup_pre_destroy(struct cgroup *cont)
 {
 	struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
-	int ret;
 
-	css_get(&memcg->css);
-	ret = mem_cgroup_reparent_charges(memcg);
-	css_put(&memcg->css);
-
-	return ret;
+	mem_cgroup_reparent_charges(memcg);
+	return 0;
 }
 
 static void mem_cgroup_destroy(struct cgroup *cont)
-- 
1.7.10.4

Re: [PATCH 5/6] memcg: make mem_cgroup_reparent_charges non failing

[Li Zefan]

>  static int mem_cgroup_force_empty_write(struct cgroup *cont, unsigned int event)
> @@ -5013,13 +5011,9 @@ free_out:
>  static int mem_cgroup_pre_destroy(struct cgroup *cont)
>  {
>  	struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
> -	int ret;
>  
> -	css_get(&memcg->css);
> -	ret = mem_cgroup_reparent_charges(memcg);
> -	css_put(&memcg->css);
> -
> -	return ret;
> +	mem_cgroup_reparent_charges(memcg);
> +	return 0;
>  }
>  

Why don't you make pre_destroy() return void?

Re: [PATCH 5/6] memcg: make mem_cgroup_reparent_charges non failing

[Michal Hocko]

On Thu 18-10-12 16:30:19, Li Zefan wrote:
> >  static int mem_cgroup_force_empty_write(struct cgroup *cont, unsigned int event)
> > @@ -5013,13 +5011,9 @@ free_out:
> >  static int mem_cgroup_pre_destroy(struct cgroup *cont)
> >  {
> >  	struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
> > -	int ret;
> >  
> > -	css_get(&memcg->css);
> > -	ret = mem_cgroup_reparent_charges(memcg);
> > -	css_put(&memcg->css);
> > -
> > -	return ret;
> > +	mem_cgroup_reparent_charges(memcg);
> > +	return 0;
> >  }
> >  
> 
> Why don't you make pre_destroy() return void?

Yes I plan to do that later after I have feedback for this RFC.  I am
especially interested whether the cgroup core patch is OK, resp. has to
be reworked to pull pre_destroy outside of cgroup_lock

Thanks
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 5/6] memcg: make mem_cgroup_reparent_charges non failing

[Tejun Heo]

On Wed, Oct 17, 2012 at 03:30:47PM +0200, Michal Hocko wrote:
> Now that pre_destroy callbacks are called from within cgroup_lock and
> the cgroup has been checked to be empty without any children then there
> is no other way to fail.
> mem_cgroup_pre_destroy doesn't have to take a reference to memcg's css
> because all css' are marked dead already.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

Reviewed-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH 5/6] memcg: make mem_cgroup_reparent_charges non failing

[Michal Hocko]

This is an updated version of the patch. I have dropped
.__DEPRECATED_clear_css_refs in this one as it makes the best sense to
me. I didn't add Tejun's Reviewed-by because of this change. Could you
recheck, please?
---
>From 6c1f2e76e254e7638ad8cc87f319e3492ac80c5b Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Wed, 17 Oct 2012 14:15:09 +0200
Subject: [PATCH] memcg: make mem_cgroup_reparent_charges non failing

Now that pre_destroy callbacks are called from within cgroup_lock and
the cgroup has been checked to be empty without any children then there
is no other way to fail.
mem_cgroup_pre_destroy doesn't have to take a reference to memcg's css
because all css' are marked dead already.

mem_cgroup_subsys.__DEPRECATED_clear_css_refs can be dropped as
mem_cgroup_pre_destroy cannot fail now.

Changes since v1
- drop __DEPRECATED_clear_css_refs

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/memcontrol.c |   19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index f57ba4c..b4d854e 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3738,14 +3738,12 @@ static void mem_cgroup_force_empty_list(struct mem_cgroup *memcg,
  *
  * Caller is responsible for holding css reference on the memcg.
  */
-static int mem_cgroup_reparent_charges(struct mem_cgroup *memcg)
+static void mem_cgroup_reparent_charges(struct mem_cgroup *memcg)
 {
 	struct cgroup *cgrp = memcg->css.cgroup;
 	int node, zid;
 
 	do {
-		if (cgroup_task_count(cgrp) || !list_empty(&cgrp->children))
-			return -EBUSY;
 		/* This is for making all *used* pages to be on LRU. */
 		lru_add_drain_all();
 		drain_all_stock_sync(memcg);
@@ -3771,8 +3769,6 @@ static int mem_cgroup_reparent_charges(struct mem_cgroup *memcg)
 		 * charge before adding to the LRU.
 		 */
 	} while (res_counter_read_u64(&memcg->res, RES_USAGE) > 0);
-
-	return 0;
 }
 
 /*
@@ -3809,7 +3805,9 @@ static int mem_cgroup_force_empty(struct mem_cgroup *memcg)
 
 	}
 	lru_add_drain();
-	return mem_cgroup_reparent_charges(memcg);
+	mem_cgroup_reparent_charges(memcg);
+
+	return 0;
 }
 
 static int mem_cgroup_force_empty_write(struct cgroup *cont, unsigned int event)
@@ -5013,13 +5011,9 @@ free_out:
 static int mem_cgroup_pre_destroy(struct cgroup *cont)
 {
 	struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
-	int ret;
 
-	css_get(&memcg->css);
-	ret = mem_cgroup_reparent_charges(memcg);
-	css_put(&memcg->css);
-
-	return ret;
+	mem_cgroup_reparent_charges(memcg);
+	return 0;
 }
 
 static void mem_cgroup_destroy(struct cgroup *cont)
@@ -5621,7 +5615,6 @@ struct cgroup_subsys mem_cgroup_subsys = {
 	.base_cftypes = mem_cgroup_files,
 	.early_init = 0,
 	.use_id = 1,
-	.__DEPRECATED_clear_css_refs = true,
 };
 
 #ifdef CONFIG_MEMCG_SWAP
-- 
1.7.10.4

-- 
Michal Hocko
SUSE Labs

[PATCH 6/6] hugetlb: do not fail in hugetlb_cgroup_pre_destroy

[Michal Hocko]

Now that pre_destroy callbacks are called from within cgroup_lock and
the cgroup has been checked to be empty without any children then there
is no other way to fail.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/hugetlb_cgroup.c |   11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
index a3f358f..dc595c6 100644
--- a/mm/hugetlb_cgroup.c
+++ b/mm/hugetlb_cgroup.c
@@ -159,14 +159,9 @@ static int hugetlb_cgroup_pre_destroy(struct cgroup *cgroup)
 {
 	struct hstate *h;
 	struct page *page;
-	int ret = 0, idx = 0;
+	int idx = 0;
 
 	do {
-		if (cgroup_task_count(cgroup) ||
-		    !list_empty(&cgroup->children)) {
-			ret = -EBUSY;
-			goto out;
-		}
 		for_each_hstate(h) {
 			spin_lock(&hugetlb_lock);
 			list_for_each_entry(page, &h->hugepage_activelist, lru)
@@ -177,8 +172,8 @@ static int hugetlb_cgroup_pre_destroy(struct cgroup *cgroup)
 		}
 		cond_resched();
 	} while (hugetlb_cgroup_have_usage(cgroup));
-out:
-	return ret;
+
+	return 0;
 }
 
 int hugetlb_cgroup_charge_cgroup(int idx, unsigned long nr_pages,
-- 
1.7.10.4

Re: [PATCH 6/6] hugetlb: do not fail in hugetlb_cgroup_pre_destroy

[Tejun Heo]

On Wed, Oct 17, 2012 at 03:30:48PM +0200, Michal Hocko wrote:
> Now that pre_destroy callbacks are called from within cgroup_lock and
> the cgroup has been checked to be empty without any children then there
> is no other way to fail.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

Reviewed-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [RFC] memcg/cgroup: do not fail fail on pre_destroy callbacks

[Glauber Costa]

On 10/17/2012 05:30 PM, Michal Hocko wrote:
> Hi,
> memcg is the only controller which might fail in its pre_destroy
> callback which makes the cgroup core more complicated for no good
> reason. This is an attempt to change this unfortunate state. 
> 
> I am sending this a RFC because I would like to hear back whether the
> approach is correct. I thought that the changes would be more invasive
> but it seems that the current code was mostly prepared for this and it
> needs just some small tweaks (so I might be missing something important
> here).
> 
> The first two patches are just clean ups. They could be merged even
> without the rest.
> 
> The real change, although the code is not changed that much, is the 3rd
> patch. It changes the way how we handle mem_cgroup_move_parent failures.
> We have to realize that all those failures are *temporal*. Because we
> are either racing with the page removal or the page is temporarily off
> the LRU because of migration resp. global reclaim. As a result we do
> not fail mem_cgroup_force_empty_list if the page cannot be moved to the
> parent and rather retry until the LRU is empty.
> 
> The 4th patch is for cgroup core. I have moved cgroup_call_pre_destroy
> inside the cgroup_lock which is not very nice because the callbacks
> can take some time. Maybe we can move this call at the very end of the
> function?
> All I need for memcg is that cgroup_call_pre_destroy has been called and
> that no new cgroups can be attached to the group. The cgroup_lock is
> necessary for the later condition but if we move after CGRP_REMOVED flag
> is set then we are safe as well.
> 
> The last two patches are trivial follow ups for the cgroups core change
> because now we know that nobody will interfere with us so we can drop
> those empty && no child condition.
> 
> Comments, thoughts?
> 

I personally don't see anything fundamentally wrong with this.

Re: [RFC] memcg/cgroup: do not fail fail on pre_destroy callbacks

[Kamezawa Hiroyuki]

(2012/10/17 22:30), Michal Hocko wrote:
> Hi,
> memcg is the only controller which might fail in its pre_destroy
> callback which makes the cgroup core more complicated for no good
> reason. This is an attempt to change this unfortunate state.
> 
> I am sending this a RFC because I would like to hear back whether the
> approach is correct. I thought that the changes would be more invasive
> but it seems that the current code was mostly prepared for this and it
> needs just some small tweaks (so I might be missing something important
> here).
> 
> The first two patches are just clean ups. They could be merged even
> without the rest.
> 
> The real change, although the code is not changed that much, is the 3rd
> patch. It changes the way how we handle mem_cgroup_move_parent failures.
> We have to realize that all those failures are *temporal*. Because we
> are either racing with the page removal or the page is temporarily off
> the LRU because of migration resp. global reclaim. As a result we do
> not fail mem_cgroup_force_empty_list if the page cannot be moved to the
> parent and rather retry until the LRU is empty.
> 
> The 4th patch is for cgroup core. I have moved cgroup_call_pre_destroy
> inside the cgroup_lock which is not very nice because the callbacks
> can take some time. Maybe we can move this call at the very end of the
> function?
> All I need for memcg is that cgroup_call_pre_destroy has been called and
> that no new cgroups can be attached to the group. The cgroup_lock is
> necessary for the later condition but if we move after CGRP_REMOVED flag
> is set then we are safe as well.
> 
> The last two patches are trivial follow ups for the cgroups core change
> because now we know that nobody will interfere with us so we can drop
> those empty && no child condition.
> 
> Comments, thoughts?
> 
> Michal Hocko (6):
>        memcg: split mem_cgroup_force_empty into reclaiming and reparenting parts
>        memcg: root_cgroup cannot reach mem_cgroup_move_parent
>        memcg: Simplify mem_cgroup_force_empty_list error handling
>        cgroups: forbid pre_destroy callback to fail
>        memcg: make mem_cgroup_reparent_charges non failing
>        hugetlb: do not fail in hugetlb_cgroup_pre_destroy
> 
> Cumulative diffstat:
>   kernel/cgroup.c     |   30 ++++---------
>   mm/hugetlb_cgroup.c |   11 ++---
>   mm/memcontrol.c     |  124 +++++++++++++++++++++++++++------------------------
>   3 files changed, 78 insertions(+), 87 deletions(-)

Thank you very much ! The whole patch seems good to me and I like this approach.

Thanks,
-Kame