[PATCH] memcg: deprecate memory.force_empty knob

[PATCH] memcg: deprecate memory.force_empty knob

[Michal Hocko]

force_empty has been introduced primarily to drop memory before it gets
reparented on the group removal. This alone doesn't sound fully
justified because reparented pages which are not in use can be reclaimed
also later when there is a memory pressure on the parent level.

Mark the knob CFTYPE_INSANE which tells the cgroup core that it
shouldn't create the knob with the experimental sane_behavior. Other
users will get informed about the deprecation and asked to tell us more
because I do not expect most users will use sane_behavior cgroups mode
very soon.
Anyway I expect that most users will be simply cgroup remove handlers
which do that since ever without having any good reason for it.

If somebody really cares because reparented pages, which would be
dropped otherwise, push out more important ones then we should fix the
reparenting code and put pages to the tail.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
---

Hi,
This patch has been created based on http://marc.info/?l=linux-kernel&m=139967135405272

 Documentation/cgroups/memory.txt | 3 +++
 mm/memcontrol.c                  | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
index f0f67b44ea07..fc9fad984bfb 100644
--- a/Documentation/cgroups/memory.txt
+++ b/Documentation/cgroups/memory.txt
@@ -477,6 +477,9 @@ About use_hierarchy, see Section 6.
   write will still return success. In this case, it is expected that
   memory.kmem.usage_in_bytes == memory.usage_in_bytes.
 
+  Please note that this knob is considered deprecated and will be removed
+  in future.
+
   About use_hierarchy, see Section 6.
 
 5.2 stat file
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b030b15b626a..ee123f3d40d5 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4793,6 +4793,10 @@ static int mem_cgroup_force_empty_write(struct cgroup_subsys_state *css,
 
 	if (mem_cgroup_is_root(memcg))
 		return -EINVAL;
+	pr_info("%s (%d): memory.force_empty is deprecated and will be removed.",
+			current->comm, task_pid_nr(current));
+	pr_cont(" Let us know if you know if it needed in your usecase at");
+	pr_cont(" linux-mm@kvack.org\n");
 	return mem_cgroup_force_empty(memcg);
 }
 
@@ -6037,6 +6041,7 @@ static struct cftype mem_cgroup_files[] = {
 	},
 	{
 		.name = "force_empty",
+		.flags = CFTYPE_INSANE,
 		.trigger = mem_cgroup_force_empty_write,
 	},
 	{
-- 
2.0.0.rc0

Re: [PATCH] memcg: deprecate memory.force_empty knob

[Andrew Morton]

On Tue, 13 May 2014 17:29:16 +0200 Michal Hocko <mhocko@suse.cz> wrote:

> force_empty has been introduced primarily to drop memory before it gets
> reparented on the group removal. This alone doesn't sound fully
> justified because reparented pages which are not in use can be reclaimed
> also later when there is a memory pressure on the parent level.
> 
> Mark the knob CFTYPE_INSANE which tells the cgroup core that it
> shouldn't create the knob with the experimental sane_behavior. Other
> users will get informed about the deprecation and asked to tell us more
> because I do not expect most users will use sane_behavior cgroups mode
> very soon.
> Anyway I expect that most users will be simply cgroup remove handlers
> which do that since ever without having any good reason for it.
> 
> If somebody really cares because reparented pages, which would be
> dropped otherwise, push out more important ones then we should fix the
> reparenting code and put pages to the tail.
> 
> ...
>
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4793,6 +4793,10 @@ static int mem_cgroup_force_empty_write(struct cgroup_subsys_state *css,
>  
>  	if (mem_cgroup_is_root(memcg))
>  		return -EINVAL;
> +	pr_info("%s (%d): memory.force_empty is deprecated and will be removed.",
> +			current->comm, task_pid_nr(current));
> +	pr_cont(" Let us know if you know if it needed in your usecase at");
> +	pr_cont(" linux-mm@kvack.org\n");
>  	return mem_cgroup_force_empty(memcg);
>  }
>  

Do we really want to spam the poor user each and every time they use
this?  Using pr_info_once() is kinder and gentler?


From: Andrew Morton <akpm@linux-foundation.org>
Subject: memcg-deprecate-memoryforce_empty-knob-fix

- s/pr_info/pr_info_once/
- fix garbled printk text

Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@suse.cz>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 Documentation/cgroups/memory.txt |    2 +-
 mm/memcontrol.c                  |    8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff -puN Documentation/cgroups/memory.txt~memcg-deprecate-memoryforce_empty-knob-fix Documentation/cgroups/memory.txt
--- a/Documentation/cgroups/memory.txt~memcg-deprecate-memoryforce_empty-knob-fix
+++ a/Documentation/cgroups/memory.txt
@@ -482,7 +482,7 @@ About use_hierarchy, see Section 6.
   memory.kmem.usage_in_bytes == memory.usage_in_bytes.
 
   Please note that this knob is considered deprecated and will be removed
-  in future.
+  in the future.
 
   About use_hierarchy, see Section 6.
 
diff -puN mm/memcontrol.c~memcg-deprecate-memoryforce_empty-knob-fix mm/memcontrol.c
--- a/mm/memcontrol.c~memcg-deprecate-memoryforce_empty-knob-fix
+++ a/mm/memcontrol.c
@@ -4799,10 +4799,10 @@ static int mem_cgroup_force_empty_write(
 
 	if (mem_cgroup_is_root(memcg))
 		return -EINVAL;
-	pr_info("%s (%d): memory.force_empty is deprecated and will be removed.",
-			current->comm, task_pid_nr(current));
-	pr_cont(" Let us know if you know if it needed in your usecase at");
-	pr_cont(" linux-mm@kvack.org\n");
+	pr_info_once("%s (%d): memory.force_empty is deprecated and will be "
+		     "removed.  Let us know if it is needed in your usecase at "
+		     "linux-mm@kvack.org\n",
+		     current->comm, task_pid_nr(current));
 	return mem_cgroup_force_empty(memcg);
 }
 
_

Re: [PATCH] memcg: deprecate memory.force_empty knob

[Michal Hocko]

On Tue 13-05-14 14:39:53, Andrew Morton wrote:
> On Tue, 13 May 2014 17:29:16 +0200 Michal Hocko <mhocko@suse.cz> wrote:
> 
> > force_empty has been introduced primarily to drop memory before it gets
> > reparented on the group removal. This alone doesn't sound fully
> > justified because reparented pages which are not in use can be reclaimed
> > also later when there is a memory pressure on the parent level.
> > 
> > Mark the knob CFTYPE_INSANE which tells the cgroup core that it
> > shouldn't create the knob with the experimental sane_behavior. Other
> > users will get informed about the deprecation and asked to tell us more
> > because I do not expect most users will use sane_behavior cgroups mode
> > very soon.
> > Anyway I expect that most users will be simply cgroup remove handlers
> > which do that since ever without having any good reason for it.
> > 
> > If somebody really cares because reparented pages, which would be
> > dropped otherwise, push out more important ones then we should fix the
> > reparenting code and put pages to the tail.
> > 
> > ...
> >
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -4793,6 +4793,10 @@ static int mem_cgroup_force_empty_write(struct cgroup_subsys_state *css,
> >  
> >  	if (mem_cgroup_is_root(memcg))
> >  		return -EINVAL;
> > +	pr_info("%s (%d): memory.force_empty is deprecated and will be removed.",
> > +			current->comm, task_pid_nr(current));
> > +	pr_cont(" Let us know if you know if it needed in your usecase at");
> > +	pr_cont(" linux-mm@kvack.org\n");
> >  	return mem_cgroup_force_empty(memcg);
> >  }
> >  
> 
> Do we really want to spam the poor user each and every time they use
> this?  Using pr_info_once() is kinder and gentler?

We do not catch all potential callers but it is true that some
configurations might have thousands of cgroups and the notify_on_release
handler will spam the log.

> From: Andrew Morton <akpm@linux-foundation.org>
> Subject: memcg-deprecate-memoryforce_empty-knob-fix
> 
> - s/pr_info/pr_info_once/
> - fix garbled printk text
> 
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@suse.cz>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Acked-by: Michal Hocko <mhocko@suse.cz>

> ---
> 
>  Documentation/cgroups/memory.txt |    2 +-
>  mm/memcontrol.c                  |    8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff -puN Documentation/cgroups/memory.txt~memcg-deprecate-memoryforce_empty-knob-fix Documentation/cgroups/memory.txt
> --- a/Documentation/cgroups/memory.txt~memcg-deprecate-memoryforce_empty-knob-fix
> +++ a/Documentation/cgroups/memory.txt
> @@ -482,7 +482,7 @@ About use_hierarchy, see Section 6.
>    memory.kmem.usage_in_bytes == memory.usage_in_bytes.
>  
>    Please note that this knob is considered deprecated and will be removed
> -  in future.
> +  in the future.
>  
>    About use_hierarchy, see Section 6.
>  
> diff -puN mm/memcontrol.c~memcg-deprecate-memoryforce_empty-knob-fix mm/memcontrol.c
> --- a/mm/memcontrol.c~memcg-deprecate-memoryforce_empty-knob-fix
> +++ a/mm/memcontrol.c
> @@ -4799,10 +4799,10 @@ static int mem_cgroup_force_empty_write(
>  
>  	if (mem_cgroup_is_root(memcg))
>  		return -EINVAL;
> -	pr_info("%s (%d): memory.force_empty is deprecated and will be removed.",
> -			current->comm, task_pid_nr(current));
> -	pr_cont(" Let us know if you know if it needed in your usecase at");
> -	pr_cont(" linux-mm@kvack.org\n");
> +	pr_info_once("%s (%d): memory.force_empty is deprecated and will be "
> +		     "removed.  Let us know if it is needed in your usecase at "
> +		     "linux-mm@kvack.org\n",
> +		     current->comm, task_pid_nr(current));
>  	return mem_cgroup_force_empty(memcg);
>  }
>  
> _
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] memcg: deprecate memory.force_empty knob

[Greg Thelen]

On Tue, May 13 2014, Michal Hocko <mhocko@suse.cz> wrote:

> force_empty has been introduced primarily to drop memory before it gets
> reparented on the group removal. This alone doesn't sound fully
> justified because reparented pages which are not in use can be reclaimed
> also later when there is a memory pressure on the parent level.
>
> Mark the knob CFTYPE_INSANE which tells the cgroup core that it
> shouldn't create the knob with the experimental sane_behavior. Other
> users will get informed about the deprecation and asked to tell us more
> because I do not expect most users will use sane_behavior cgroups mode
> very soon.
> Anyway I expect that most users will be simply cgroup remove handlers
> which do that since ever without having any good reason for it.
>
> If somebody really cares because reparented pages, which would be
> dropped otherwise, push out more important ones then we should fix the
> reparenting code and put pages to the tail.

I should mention a case where I've needed to use memory.force_empty: to
synchronously flush stats from child to parent.  Without force_empty
memory.stat is temporarily inconsistent until async css_offline
reparents charges.  Here is an example on v3.14 showing that
parent/memory.stat contents are in-flux immediately after rmdir of
parent/child.

$ cat /test
#!/bin/bash

# Create parent and child.  Add some non-reclaimable anon rss to child,
# then move running task to parent.
mkdir p p/c
(echo $BASHPID > p/c/cgroup.procs && exec sleep 1d) &
pid=$!
sleep 1
echo $pid > p/cgroup.procs 

grep 'rss ' {p,p/c}/memory.stat
if [[ $1 == force ]]; then
  echo 1 > p/c/memory.force_empty
fi
rmdir p/c

echo 'For a small time the p/c memory has not been reparented to p.'
grep 'rss ' {p,p/c}/memory.stat

sleep 1
echo 'After waiting all memory has been reparented'
grep 'rss ' {p,p/c}/memory.stat

kill $pid
rmdir p


-- First, demonstrate that just rmdir, without memory.force_empty,
   temporarily hides reparented child memory stats.

$ /test
p/memory.stat:rss 0
p/memory.stat:total_rss 69632
p/c/memory.stat:rss 69632
p/c/memory.stat:total_rss 69632
For a small time the p/c memory has not been reparented to p.
p/memory.stat:rss 0
p/memory.stat:total_rss 0
grep: p/c/memory.stat: No such file or directory
After waiting all memory has been reparented
p/memory.stat:rss 69632
p/memory.stat:total_rss 69632
grep: p/c/memory.stat: No such file or directory
/test: Terminated              ( echo $BASHPID > p/c/cgroup.procs && exec sleep 1d )

-- Demonstrate that using memory.force_empty before rmdir, behaves more
   sensibly.  Stats for reparented child memory are not hidden.

$ /test force
p/memory.stat:rss 0
p/memory.stat:total_rss 69632
p/c/memory.stat:rss 69632
p/c/memory.stat:total_rss 69632
For a small time the p/c memory has not been reparented to p.
p/memory.stat:rss 69632
p/memory.stat:total_rss 69632
grep: p/c/memory.stat: No such file or directory
After waiting all memory has been reparented
p/memory.stat:rss 69632
p/memory.stat:total_rss 69632
grep: p/c/memory.stat: No such file or directory
/test: Terminated              ( echo $BASHPID > p/c/cgroup.procs && exec sleep 1d )

Re: [PATCH] memcg: deprecate memory.force_empty knob

[Michal Hocko]

On Fri 16-05-14 15:00:16, Greg Thelen wrote:
> On Tue, May 13 2014, Michal Hocko <mhocko@suse.cz> wrote:
[...]
> > If somebody really cares because reparented pages, which would be
> > dropped otherwise, push out more important ones then we should fix the
> > reparenting code and put pages to the tail.
> 
> I should mention a case where I've needed to use memory.force_empty: to
> synchronously flush stats from child to parent.  Without force_empty
> memory.stat is temporarily inconsistent until async css_offline
> reparents charges.  Here is an example on v3.14 showing that
> parent/memory.stat contents are in-flux immediately after rmdir of
> parent/child.

OK, it is true that the delayed offlining makes this little bit
complicated because there is no direct user visible relation between
rmdir and css_offline.

> $ cat /test
> #!/bin/bash
> 
> # Create parent and child.  Add some non-reclaimable anon rss to child,
> # then move running task to parent.
> mkdir p p/c
> (echo $BASHPID > p/c/cgroup.procs && exec sleep 1d) &
> pid=$!
> sleep 1
> echo $pid > p/cgroup.procs 
> 
> grep 'rss ' {p,p/c}/memory.stat
> if [[ $1 == force ]]; then
>   echo 1 > p/c/memory.force_empty
> fi
> rmdir p/c
> 
> echo 'For a small time the p/c memory has not been reparented to p.'
> grep 'rss ' {p,p/c}/memory.stat
> 
> sleep 1
> echo 'After waiting all memory has been reparented'
> grep 'rss ' {p,p/c}/memory.stat
> 
> kill $pid
> rmdir p
> 
> 
> -- First, demonstrate that just rmdir, without memory.force_empty,
>    temporarily hides reparented child memory stats.
> 
> $ /test
> p/memory.stat:rss 0
> p/memory.stat:total_rss 69632
> p/c/memory.stat:rss 69632
> p/c/memory.stat:total_rss 69632
> For a small time the p/c memory has not been reparented to p.
> p/memory.stat:rss 0
> p/memory.stat:total_rss 0

OK, this is a bug. Our iterators skip the children because css_tryget
fails on it but css_offline still not done. This is fixable, though,
and force_empty is just a workaround so I wouldn't see this as a proper
justification to keep it alive.

One possible way to fix this is to iterate children even when css_tryget
fails for them if they haven't finished css_offline yet.
There are some changes in the cgroups core which should make this easier
and Johannes claimed he has some work in that area.

Anyway this is a useful testcase. Thanks Greg!

> grep: p/c/memory.stat: No such file or directory
> After waiting all memory has been reparented
> p/memory.stat:rss 69632
> p/memory.stat:total_rss 69632
> grep: p/c/memory.stat: No such file or directory
> /test: Terminated              ( echo $BASHPID > p/c/cgroup.procs && exec sleep 1d )
> 
> -- Demonstrate that using memory.force_empty before rmdir, behaves more
>    sensibly.  Stats for reparented child memory are not hidden.
> 
> $ /test force
> p/memory.stat:rss 0
> p/memory.stat:total_rss 69632
> p/c/memory.stat:rss 69632
> p/c/memory.stat:total_rss 69632
> For a small time the p/c memory has not been reparented to p.
> p/memory.stat:rss 69632
> p/memory.stat:total_rss 69632
> grep: p/c/memory.stat: No such file or directory
> After waiting all memory has been reparented
> p/memory.stat:rss 69632
> p/memory.stat:total_rss 69632
> grep: p/c/memory.stat: No such file or directory
> /test: Terminated              ( echo $BASHPID > p/c/cgroup.procs && exec sleep 1d )

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] memcg: deprecate memory.force_empty knob

[Michal Hocko]

On Mon 19-05-14 16:02:48, Michal Hocko wrote:
> On Fri 16-05-14 15:00:16, Greg Thelen wrote:
[...]
> > -- First, demonstrate that just rmdir, without memory.force_empty,
> >    temporarily hides reparented child memory stats.
> > 
> > $ /test
> > p/memory.stat:rss 0
> > p/memory.stat:total_rss 69632
> > p/c/memory.stat:rss 69632
> > p/c/memory.stat:total_rss 69632
> > For a small time the p/c memory has not been reparented to p.
> > p/memory.stat:rss 0
> > p/memory.stat:total_rss 0
> 
> OK, this is a bug. Our iterators skip the children because css_tryget
> fails on it but css_offline still not done.

Or use the cgroups iterator directly. Which would be even easier to fix.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] memcg: deprecate memory.force_empty knob

[Michal Hocko]

On Mon 19-05-14 17:50:18, Michal Hocko wrote:
> On Mon 19-05-14 16:02:48, Michal Hocko wrote:
> > On Fri 16-05-14 15:00:16, Greg Thelen wrote:
> [...]
> > > -- First, demonstrate that just rmdir, without memory.force_empty,
> > >    temporarily hides reparented child memory stats.
> > > 
> > > $ /test
> > > p/memory.stat:rss 0
> > > p/memory.stat:total_rss 69632
> > > p/c/memory.stat:rss 69632
> > > p/c/memory.stat:total_rss 69632
> > > For a small time the p/c memory has not been reparented to p.
> > > p/memory.stat:rss 0
> > > p/memory.stat:total_rss 0
> > 
> > OK, this is a bug. Our iterators skip the children because css_tryget
> > fails on it but css_offline still not done.

Recent cgroup changes distinguish css_tryget and css_tryget_online
(http://marc.info/?l=linux-kernel&m=140025648704805). So we will only
need to use css_tryget rather than the _online variant in
__mem_cgroup_iter_next. I guess this is what Johannes was talking about.

-- 
Michal Hocko
SUSE Labs

[PATCHSET cgroup/for-3.16] cgroup: iterate cgroup_subsys_states directly

[Tejun Heo]

Hello,

Currently, while csses (cgroup_subsys_states) have ->parent linkage
too, only cgroups form full tree through their ->children and
->sibling fields and css iterations naturally is implemented by
iterating cgroups and then dereferencing the css for the specified
subsystem.

There are now use cases where controllers need to iterate through
csses regardless of their online state as long as they have positive
reference.  This can't easily be achieved by iterating cgroups because
its css pointer array needs to be cleared on offline and there may be
multiple dying csses for a cgroup for the same subsystem and there's
only one pointer per cgroup-subsystem pair.

This patchset moves ->children and ->sibling from cgroup to css and
link all csses in proper trees and then make css iterators walk csses
directly instead of going through cgroups.  This achieves iteration of
all non-released csses while also simplifying the iteration
implementation.  This is also in line with the general direction of
using csses as the primary structural component.

This patchset contains the following fourteen patches.

 0001-cgroup-remove-css_parent.patch
 0002-cgroup-remove-pointless-has-tasks-children-test-from.patch
 0003-memcg-update-memcg_has_children-to-use-css_next_chil.patch
 0004-device_cgroup-remove-direct-access-to-cgroup-childre.patch
 0005-cgroup-remove-cgroup-parent.patch
 0006-cgroup-move-cgroup-sibling-and-children-into-cgroup_.patch
 0007-cgroup-link-all-cgroup_subsys_states-in-their-siblin.patch
 0008-cgroup-move-cgroup-serial_nr-into-cgroup_subsys_stat.patch
 0009-cgroup-introduce-CSS_RELEASED-and-reduce-css-iterati.patch
 0010-cgroup-iterate-cgroup_subsys_states-directly.patch
 0011-cgroup-use-CSS_ONLINE-instead-of-CGRP_DEAD.patch
 0012-cgroup-convert-cgroup_has_live_children-into-css_has.patch
 0013-device_cgroup-use-css_has_online_children-instead-of.patch
 0014-cgroup-implement-css_tryget.patch

0001-0004 are prep patches.

0005-0008 move fields from cgroup to css and link csses in tree
structure instead of cgroups.

0009-0010 implement direct css iteration.

0011-0013 convert a cgroup based interface to a css one, which is now
possible as both are the same in terms of the tree structure, and fix
devcg brekage using it.

0014 implements css_tryget() which is to be used to gain access to
offline but not-yet-released csses.

This pachset is on top of

 b9a63d0116e8 ("Merge branch 'for-3.16' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/percpu into for-3.16")
 + [1] [PATCHSET v2 cgroup/for-3.16] cgroup: post unified hierarchy fixes and updates
 + [2] (REFRESHED) [PATCHSET cgroup/for-3.16] cgroup: implement cftype->write()
 + [3] (REFRESHED) [PATCHSET cgroup/for-3.16] cgroup: remove cgroup_tree_mutex
 + [4] [PATCHSET cgroup/for-3.16] cgroup: use css->refcnt for cgroup reference counting

and available in the following git branch.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git review-direct-css-iteration

diffstat follows.  Thanks.

 block/blk-cgroup.h           |    2 
 include/linux/cgroup.h       |  122 +++++++++++---------
 kernel/cgroup.c              |  257 ++++++++++++++++++++++++-------------------
 kernel/cgroup_freezer.c      |    2 
 kernel/cpuset.c              |    2 
 kernel/sched/core.c          |    2 
 kernel/sched/cpuacct.c       |    2 
 mm/hugetlb_cgroup.c          |    2 
 mm/memcontrol.c              |   45 +++----
 net/core/netclassid_cgroup.c |    2 
 net/core/netprio_cgroup.c    |    2 
 security/device_cgroup.c     |   17 --
 12 files changed, 251 insertions(+), 206 deletions(-)

--
tejun

[1] http://lkml.kernel.org/g/1399663975-315-1-git-send-email-tj@kernel.org
[2] http://lkml.kernel.org/g/20140509195059.GE4486@htj.dyndns.org
[3] http://lkml.kernel.org/g/20140509195827.GG4486@htj.dyndns.org
[4] http://lkml.kernel.org/g/1399670015-23463-1-git-send-email-tj@kernel.org

[PATCH 02/14] cgroup: remove pointless has tasks/children test from mem_cgroup_force_empty()

[Tejun Heo]

mem_cgroup_force_empty() is used only from
mem_cgroup_force_empty_write() and tests whether the target memcg has
any tasks or children without any synchronization and then returns
-EBUSY if so.

This is just weird.  The tests don't really mean anything as tasks and
children may be added after the tests and it also makes the behavior
of the knob arbitrary because there may be lingering offline and
removed children on the children list for extended period of time -
writes to the knob can return -EBUSY for reasons completely invisible
to userland.

The knob is best-effort anyway and the broken business test doesn't
affect its operation.  Remove it.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@suse.cz>
---
 mm/memcontrol.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index a5e0417..036453a 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4857,11 +4857,6 @@ static inline bool memcg_has_children(struct mem_cgroup *memcg)
 static int mem_cgroup_force_empty(struct mem_cgroup *memcg)
 {
 	int nr_retries = MEM_CGROUP_RECLAIM_RETRIES;
-	struct cgroup *cgrp = memcg->css.cgroup;
-
-	/* returns EBUSY if there is a task or if we come here twice. */
-	if (cgroup_has_tasks(cgrp) || !list_empty(&cgrp->children))
-		return -EBUSY;
 
 	/* we call try-to-free pages for make this cgroup empty */
 	lru_add_drain_all();
-- 
1.9.0

Re: [PATCH 02/14] cgroup: remove pointless has tasks/children test from mem_cgroup_force_empty()

[Michal Hocko]

On Fri 09-05-14 17:31:19, Tejun Heo wrote:
> mem_cgroup_force_empty() is used only from
> mem_cgroup_force_empty_write() and tests whether the target memcg has
> any tasks or children without any synchronization and then returns
> -EBUSY if so.
> 
> This is just weird.  The tests don't really mean anything as tasks and
> children may be added after the tests and it also makes the behavior
> of the knob arbitrary because there may be lingering offline and
> removed children on the children list for extended period of time -
> writes to the knob can return -EBUSY for reasons completely invisible
> to userland.

Agreed.

> The knob is best-effort anyway and the broken business test doesn't
> affect its operation.  Remove it.

But I do not think that removing just the test is the right way to go.
It is mem_cgroup_reparent_charges which bothers me because it loops
until the current counter falls down to 0 and it also feels strange that
a group can hand over pages up the hierarchy (or even to an unlimitted
root if this is a top of a hierarchy).

So I think that we want to get rid of reparenting as well.  The main use
case as described by the documentation is:
"
  The typical use case for this interface is before calling 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.
"

rmdir will reparent pages implicitly so the reclaim part should be
sufficient and it would be OK even with existing tasks. Subgroups would
be little bit more tricky because the user doesn't have any control over
which group of the hierarchy will get reclaimed.

Anyway, the knob sucks - for the similar reasons why drop_cache sucks -
especially when the check would be gone and it would be another way how
to trigger reclaim anytime. Having the check doesn't prevent from races
but it at least prevents abuse.

So I would be rather for dropping the knob altogether, but that seems to
be a long term thing. So let's start with deprecating it + remove the
check with dropping reparenting part.

What do you think about the following patch instead:
---
>From 03f8cb2e1fd2636d859c54df9b58719fe96e0e54 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Mon, 12 May 2014 16:34:17 +0200
Subject: [PATCH] memcg: remove tasks/children test from from
 mem_cgroup_force_empty

Tejun has correctly pointed out that tasks/children test in
mem_cgroup_force_empty is not correct because there is no other locking
which preserves this state throughout the rest of the function so both
new tasks can join the group or new children groups can be added while
somebody is writing to memory.force_empty. A new task would break
mem_cgroup_reparent_charges expectation that all failures as described
by mem_cgroup_force_empty_list are temporal and there is no way out.

The main use case for the knob as described by
Documentation/cgroups/memory.txt is to:
"
  The typical use case for this interface is before calling 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.
"

This means that reparenting is not really required as rmdir will
reparent pages implicitly from the safe context. If we remove it from
mem_cgroup_force_empty then we are safe even with existing tasks because
the number of reclaim attempts is bounded. Moreover the knob still does
what the documentation claims (modulo reparenting which doesn't make any
difference) and users might expect. Longterm we want to deprecate the
whole knob and put the reparented pages to the tail of parent LRU during
cgroup removal.

Signed-off-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 Documentation/cgroups/memory.txt | 6 +-----
 mm/memcontrol.c                  | 6 ------
 2 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
index cfcf14de2598..fc9fad984bfb 100644
--- a/Documentation/cgroups/memory.txt
+++ b/Documentation/cgroups/memory.txt
@@ -462,15 +462,11 @@ About use_hierarchy, see Section 6.
 
 5.1 force_empty
   memory.force_empty interface is provided to make cgroup's memory usage empty.
-  You can use this interface only when the cgroup has no tasks.
   When writing anything to this
 
   # echo 0 > memory.force_empty
 
-  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 (if use_hierarchy==1) or root (if use_hierarchy==0) and this
-  cgroup will be empty.
+  the cgroup will be reclaimed and as many pages reclaimed as possible.
 
   The typical use case for this interface is before calling rmdir().
   Because rmdir() moves all pages to parent, some out-of-use page caches can be
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index f9b84fc2e9fe..912104d6d2a9 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4764,10 +4764,6 @@ static int mem_cgroup_force_empty(struct mem_cgroup *memcg)
 	int nr_retries = MEM_CGROUP_RECLAIM_RETRIES;
 	struct cgroup *cgrp = memcg->css.cgroup;
 
-	/* returns EBUSY if there is a task or if we come here twice. */
-	if (cgroup_has_tasks(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 */
@@ -4786,8 +4782,6 @@ static int mem_cgroup_force_empty(struct mem_cgroup *memcg)
 		}
 
 	}
-	lru_add_drain();
-	mem_cgroup_reparent_charges(memcg);
 
 	return 0;
 }
-- 
2.0.0.rc0

-- 
Michal Hocko
SUSE Labs

[PATCH] memcg: deprecate memory.force_empty knob

[Michal Hocko]

And this one for deprecating force_empty.
---
>From 9bb3119900baa07b92fac932991cf94dd930f907 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Mon, 12 May 2014 16:20:46 +0200
Subject: [PATCH] memcg: deprecate memory.force_empty knob

force_empty has been introduced primarily to drop memory before it gets
reparented on the group removal. This alone doesn't sound fully
justified because reparented pages which are not in use can reclaimed
also later when there is a memory pressure on the parent level.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 Documentation/cgroups/memory.txt | 3 +++
 mm/memcontrol.c                  | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
index f0f67b44ea07..fc9fad984bfb 100644
--- a/Documentation/cgroups/memory.txt
+++ b/Documentation/cgroups/memory.txt
@@ -477,6 +477,9 @@ About use_hierarchy, see Section 6.
   write will still return success. In this case, it is expected that
   memory.kmem.usage_in_bytes == memory.usage_in_bytes.
 
+  Please note that this knob is considered deprecated and will be removed
+  in future.
+
   About use_hierarchy, see Section 6.
 
 5.2 stat file
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b030b15b626a..912104d6d2a9 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4793,6 +4793,10 @@ static int mem_cgroup_force_empty_write(struct cgroup_subsys_state *css,
 
 	if (mem_cgroup_is_root(memcg))
 		return -EINVAL;
+	pr_info("%s (%d): memory.force_empty is deprecated and will be removed.",
+			current->comm, task_pid_nr(current));
+	pr_cont(" Let us know if you know if it needed in your usecase at");
+	pr_cont(" linux-mm@kvack.org\n");
 	return mem_cgroup_force_empty(memcg);
 }
 
-- 
2.0.0.rc0

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] memcg: deprecate memory.force_empty knob

[Tejun Heo]

On Mon, May 12, 2014 at 04:58:03PM +0200, Michal Hocko wrote:
> @@ -4793,6 +4793,10 @@ static int mem_cgroup_force_empty_write(struct cgroup_subsys_state *css,
>  
>  	if (mem_cgroup_is_root(memcg))
>  		return -EINVAL;
> +	pr_info("%s (%d): memory.force_empty is deprecated and will be removed.",
> +			current->comm, task_pid_nr(current));
> +	pr_cont(" Let us know if you know if it needed in your usecase at");
> +	pr_cont(" linux-mm@kvack.org\n");
>  	return mem_cgroup_force_empty(memcg);

It probably would be way easier to just mark the knob with
CFTYPE_INSANE.

Thanks.

-- 
tejun

Re: [PATCH] memcg: deprecate memory.force_empty knob

[Michal Hocko]

On Mon 12-05-14 11:00:14, Tejun Heo wrote:
> On Mon, May 12, 2014 at 04:58:03PM +0200, Michal Hocko wrote:
> > @@ -4793,6 +4793,10 @@ static int mem_cgroup_force_empty_write(struct cgroup_subsys_state *css,
> >  
> >  	if (mem_cgroup_is_root(memcg))
> >  		return -EINVAL;
> > +	pr_info("%s (%d): memory.force_empty is deprecated and will be removed.",
> > +			current->comm, task_pid_nr(current));
> > +	pr_cont(" Let us know if you know if it needed in your usecase at");
> > +	pr_cont(" linux-mm@kvack.org\n");
> >  	return mem_cgroup_force_empty(memcg);
> 
> It probably would be way easier to just mark the knob with
> CFTYPE_INSANE.

That would prevent from creating the file, right? I do not mind that but
I would like to see people complaining before.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] memcg: deprecate memory.force_empty knob

[Tejun Heo]

On Mon, May 12, 2014 at 05:20:15PM +0200, Michal Hocko wrote:
> On Mon 12-05-14 11:00:14, Tejun Heo wrote:
> > On Mon, May 12, 2014 at 04:58:03PM +0200, Michal Hocko wrote:
> > > @@ -4793,6 +4793,10 @@ static int mem_cgroup_force_empty_write(struct cgroup_subsys_state *css,
> > >  
> > >  	if (mem_cgroup_is_root(memcg))
> > >  		return -EINVAL;
> > > +	pr_info("%s (%d): memory.force_empty is deprecated and will be removed.",
> > > +			current->comm, task_pid_nr(current));
> > > +	pr_cont(" Let us know if you know if it needed in your usecase at");
> > > +	pr_cont(" linux-mm@kvack.org\n");
> > >  	return mem_cgroup_force_empty(memcg);
> > 
> > It probably would be way easier to just mark the knob with
> > CFTYPE_INSANE.
> 
> That would prevent from creating the file, right? I do not mind that but
> I would like to see people complaining before.

Oh sure, if you wanna see people complaining before the roll out of
unified hierarchy, but let's make sure it's also marked with
CFTYPE_INSANE.  It's easy to remove the flag afterwards.  The other
way isn't, so...

Thanks.

-- 
tejun

Re: [PATCH] memcg: deprecate memory.force_empty knob

[Michal Hocko]

On Mon 12-05-14 11:25:07, Tejun Heo wrote:
> On Mon, May 12, 2014 at 05:20:15PM +0200, Michal Hocko wrote:
> > On Mon 12-05-14 11:00:14, Tejun Heo wrote:
> > > On Mon, May 12, 2014 at 04:58:03PM +0200, Michal Hocko wrote:
> > > > @@ -4793,6 +4793,10 @@ static int mem_cgroup_force_empty_write(struct cgroup_subsys_state *css,
> > > >  
> > > >  	if (mem_cgroup_is_root(memcg))
> > > >  		return -EINVAL;
> > > > +	pr_info("%s (%d): memory.force_empty is deprecated and will be removed.",
> > > > +			current->comm, task_pid_nr(current));
> > > > +	pr_cont(" Let us know if you know if it needed in your usecase at");
> > > > +	pr_cont(" linux-mm@kvack.org\n");
> > > >  	return mem_cgroup_force_empty(memcg);
> > > 
> > > It probably would be way easier to just mark the knob with
> > > CFTYPE_INSANE.
> > 
> > That would prevent from creating the file, right? I do not mind that but
> > I would like to see people complaining before.
> 
> Oh sure, if you wanna see people complaining before the roll out of
> unified hierarchy, but let's make sure it's also marked with
> CFTYPE_INSANE.  It's easy to remove the flag afterwards.  The other
> way isn't, so...
---
>From 6f2a33df7750f0794b03f7a85aba02a4e631f2a0 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Mon, 12 May 2014 16:20:46 +0200
Subject: [PATCH] memcg: deprecate memory.force_empty knob

force_empty has been introduced primarily to drop memory before it gets
reparented on the group removal. This alone doesn't sound fully
justified because reparented pages which are not in use can be reclaimed
also later when there is a memory pressure on the parent level.

Mark the knob CFTYPE_INSANE which tells the cgroup core that it
shouldn't create the knob with the experimental sane_behavior. Other
users will get informed about the deprecation and asked to tell us more.
But I expect that most users will be simply cgroup remove handlers
which do that since ever without having any good reason for it.

If somebody really cares and the reparented pages, which would be dropped
otherwise, push out more important ones then we should fix the
reparenting code and put pages to the tail.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 Documentation/cgroups/memory.txt | 3 +++
 mm/memcontrol.c                  | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
index f0f67b44ea07..fc9fad984bfb 100644
--- a/Documentation/cgroups/memory.txt
+++ b/Documentation/cgroups/memory.txt
@@ -477,6 +477,9 @@ About use_hierarchy, see Section 6.
   write will still return success. In this case, it is expected that
   memory.kmem.usage_in_bytes == memory.usage_in_bytes.
 
+  Please note that this knob is considered deprecated and will be removed
+  in future.
+
   About use_hierarchy, see Section 6.
 
 5.2 stat file
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b030b15b626a..ee123f3d40d5 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4793,6 +4793,10 @@ static int mem_cgroup_force_empty_write(struct cgroup_subsys_state *css,
 
 	if (mem_cgroup_is_root(memcg))
 		return -EINVAL;
+	pr_info("%s (%d): memory.force_empty is deprecated and will be removed.",
+			current->comm, task_pid_nr(current));
+	pr_cont(" Let us know if you know if it needed in your usecase at");
+	pr_cont(" linux-mm@kvack.org\n");
 	return mem_cgroup_force_empty(memcg);
 }
 
@@ -6037,6 +6041,7 @@ static struct cftype mem_cgroup_files[] = {
 	},
 	{
 		.name = "force_empty",
+		.flags = CFTYPE_INSANE,
 		.trigger = mem_cgroup_force_empty_write,
 	},
 	{
-- 
2.0.0.rc0

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] memcg: deprecate memory.force_empty knob

[Johannes Weiner]

On Mon, May 12, 2014 at 05:34:58PM +0200, Michal Hocko wrote:
> On Mon 12-05-14 11:25:07, Tejun Heo wrote:
> > On Mon, May 12, 2014 at 05:20:15PM +0200, Michal Hocko wrote:
> > > On Mon 12-05-14 11:00:14, Tejun Heo wrote:
> > > > On Mon, May 12, 2014 at 04:58:03PM +0200, Michal Hocko wrote:
> > > > > @@ -4793,6 +4793,10 @@ static int mem_cgroup_force_empty_write(struct cgroup_subsys_state *css,
> > > > >  
> > > > >  	if (mem_cgroup_is_root(memcg))
> > > > >  		return -EINVAL;
> > > > > +	pr_info("%s (%d): memory.force_empty is deprecated and will be removed.",
> > > > > +			current->comm, task_pid_nr(current));
> > > > > +	pr_cont(" Let us know if you know if it needed in your usecase at");
> > > > > +	pr_cont(" linux-mm@kvack.org\n");
> > > > >  	return mem_cgroup_force_empty(memcg);
> > > > 
> > > > It probably would be way easier to just mark the knob with
> > > > CFTYPE_INSANE.
> > > 
> > > That would prevent from creating the file, right? I do not mind that but
> > > I would like to see people complaining before.
> > 
> > Oh sure, if you wanna see people complaining before the roll out of
> > unified hierarchy, but let's make sure it's also marked with
> > CFTYPE_INSANE.  It's easy to remove the flag afterwards.  The other
> > way isn't, so...
> ---
> >From 6f2a33df7750f0794b03f7a85aba02a4e631f2a0 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.cz>
> Date: Mon, 12 May 2014 16:20:46 +0200
> Subject: [PATCH] memcg: deprecate memory.force_empty knob
> 
> force_empty has been introduced primarily to drop memory before it gets
> reparented on the group removal. This alone doesn't sound fully
> justified because reparented pages which are not in use can be reclaimed
> also later when there is a memory pressure on the parent level.
> 
> Mark the knob CFTYPE_INSANE which tells the cgroup core that it
> shouldn't create the knob with the experimental sane_behavior. Other
> users will get informed about the deprecation and asked to tell us more.
> But I expect that most users will be simply cgroup remove handlers
> which do that since ever without having any good reason for it.
> 
> If somebody really cares and the reparented pages, which would be dropped
> otherwise, push out more important ones then we should fix the
> reparenting code and put pages to the tail.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

I'm skeptical the printk will do anything useful, but you marked the
knob insane and that's the most important change.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH] memcg: deprecate memory.force_empty knob

[Michal Hocko]

On Tue 13-05-14 09:16:56, Johannes Weiner wrote:
> On Mon, May 12, 2014 at 05:34:58PM +0200, Michal Hocko wrote:
[...]
> > >From 6f2a33df7750f0794b03f7a85aba02a4e631f2a0 Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.cz>
> > Date: Mon, 12 May 2014 16:20:46 +0200
> > Subject: [PATCH] memcg: deprecate memory.force_empty knob
> > 
> > force_empty has been introduced primarily to drop memory before it gets
> > reparented on the group removal. This alone doesn't sound fully
> > justified because reparented pages which are not in use can be reclaimed
> > also later when there is a memory pressure on the parent level.
> > 
> > Mark the knob CFTYPE_INSANE which tells the cgroup core that it
> > shouldn't create the knob with the experimental sane_behavior. Other
> > users will get informed about the deprecation and asked to tell us more.
> > But I expect that most users will be simply cgroup remove handlers
> > which do that since ever without having any good reason for it.
> > 
> > If somebody really cares and the reparented pages, which would be dropped
> > otherwise, push out more important ones then we should fix the
> > reparenting code and put pages to the tail.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> 
> I'm skeptical the printk will do anything useful, but you marked the
> knob insane and that's the most important change.

Well, I suspect that most users will try the new semantic at the latest
possible moment and then it can come up as a surprise. I would prefer to
catch those as soon as possible. I am even thinking to push this to SLES
to catch possible enterprise users.

> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Thanks. OK, I will post it to Andrew. I guess he will want to have some
rate-limiting or print-once semantic...
-- 
Michal Hocko
SUSE Labs