[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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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);
 }
 
_

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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 )

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>