[PATCH v2] memcg: first step towards hierarchical controller

[PATCH v2] memcg: first step towards hierarchical controller

[Glauber Costa]

Here is a new attempt to lay down a path that will allow us to deprecate
the non-hierarchical mode of operation from memcg.  Unlike what I posted
before, I am making this behavior conditional on a Kconfig option.
Vanilla users will see no change in behavior unless they don't
explicitly set this option to on.

Distributions, however, are encouraged to set it.  In that case,
hierarchy will still be there. We'll just default to true in the root
cgroup, and print a warning once if you try to set it back to 0.

After a grace period, we should be able to gauge if anyone actually
relies on it and get rid of the hierarchy file, or at least of its
behavior.

[ v2: make it dependent on a Kconfig option ]

Signed-off-by: Glauber Costa <glommer@parallels.com>
CC: Dave Jones <davej@redhat.com>
CC: Ben Hutchings <ben@decadent.org.uk>
CC: Peter Zijlstra <a.p.zijlstra@chello.nl>
CC: Paul Turner <pjt@google.com>
CC: Lennart Poettering <lennart@poettering.net>
CC: Kay Sievers <kay.sievers@vrfy.org>
CC: Michal Hocko <mhocko@suse.cz>
CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
CC: Johannes Weiner <hannes@cmpxchg.org>
CC: Tejun Heo <tj@kernel.org>
---
 init/Kconfig    | 18 ++++++++++++++++++
 mm/memcontrol.c |  9 +++++++++
 2 files changed, 27 insertions(+)

diff --git a/init/Kconfig b/init/Kconfig
index 707d015..f64f888 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -726,6 +726,24 @@ config MEMCG_SWAP
 	  if boot option "swapaccount=0" is set, swap will not be accounted.
 	  Now, memory usage of swap_cgroup is 2 bytes per entry. If swap page
 	  size is 4096bytes, 512k per 1Gbytes of swap.
+
+config MEMCG_HIERARCHY_DEFAULT
+	bool "Hierarchical memcg"
+	depends on MEMCG
+	default n
+	help
+	  The memory controller has two modes of accounting: hierarchical and
+	  flat. Hierarchical accounting will charge pages all the way towards a
+	  group's parent while flat hierarchy will threat all groups as children
+	  of the root memcg, regardless of their positioning in the tree.
+
+	  Use of flat hierarchies is highly discouraged, but has been the
+	  default for performance reasons for quite some time. Setting this flag
+	  to on will make hierarchical accounting the default. It is still
+	  possible to set it back to flat by writing 0 to the file
+	  memory.use_hierarchy, albeit discouraged. Distributors are encouraged
+	  to set this option.
+
 config MEMCG_SWAP_ENABLED
 	bool "Memory Resource Controller Swap Extension enabled by default"
 	depends on MEMCG_SWAP
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 61831c33..ab79746 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4073,6 +4073,12 @@ static int mem_cgroup_hierarchy_write(struct cgroup *cont, struct cftype *cft,
 	if (memcg->use_hierarchy == val)
 		goto out;
 
+#ifdef CONFIG_MEMCG_HIERARCHY_DEFAULT
+	WARN_ONCE((!parent_memcg && memcg->use_hierarchy && val == false),
+	"Setting this file to 0 (flat hierarchy) is considered deprecated.\n"
+	"If you believe you have a valid use case for that, we kindly ask you to contact linux-mm@kvack.org and let us know");
+#endif
+
 	/*
 	 * If parent's use_hierarchy is set, we can't make any modifications
 	 * in the child subtrees. If it is unset, then the change can
@@ -5325,6 +5331,9 @@ mem_cgroup_create(struct cgroup *cont)
 			INIT_WORK(&stock->work, drain_local_stock);
 		}
 		hotcpu_notifier(memcg_cpu_hotplug_callback, 0);
+#ifdef CONFIG_MEMCG_HIERARCHY_DEFAULT
+		memcg->use_hierarchy = true;
+#endif
 	} else {
 		parent = mem_cgroup_from_cont(cont->parent);
 		memcg->use_hierarchy = parent->use_hierarchy;
-- 
1.7.11.4

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Ben Hutchings]

On Mon, Sep 03, 2012 at 07:46:51PM +0400, Glauber Costa wrote:
> Here is a new attempt to lay down a path that will allow us to deprecate
> the non-hierarchical mode of operation from memcg.  Unlike what I posted
> before, I am making this behavior conditional on a Kconfig option.
> Vanilla users will see no change in behavior unless they don't
> explicitly set this option to on.

There are too many negatives in this sentence - it is not only
unclear, but appears to be incorrect.  I think you should delete
'don't'.

[...]
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -726,6 +726,24 @@ config MEMCG_SWAP
>  	  if boot option "swapaccount=0" is set, swap will not be accounted.
>  	  Now, memory usage of swap_cgroup is 2 bytes per entry. If swap page
>  	  size is 4096bytes, 512k per 1Gbytes of swap.
> +
> +config MEMCG_HIERARCHY_DEFAULT
> +	bool "Hierarchical memcg"
> +	depends on MEMCG
> +	default n
> +	help
> +	  The memory controller has two modes of accounting: hierarchical and
> +	  flat. Hierarchical accounting will charge pages all the way towards a
> +	  group's parent while flat hierarchy will threat all groups as children

typo: 'threat' should be 'treat'

> +	  of the root memcg, regardless of their positioning in the tree.
> +
> +	  Use of flat hierarchies is highly discouraged, but has been the
> +	  default for performance reasons for quite some time. Setting this flag
> +	  to on will make hierarchical accounting the default. It is still
> +	  possible to set it back to flat by writing 0 to the file
> +	  memory.use_hierarchy, albeit discouraged. Distributors are encouraged
> +	  to set this option.
[...]

I don't think that 'default n' is effective encouragement!

Ben.

-- 
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
                                                              - Albert Camus

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Michal Hocko]

On Mon 03-09-12 19:46:51, Glauber Costa wrote:
> Here is a new attempt to lay down a path that will allow us to deprecate
> the non-hierarchical mode of operation from memcg.  Unlike what I posted
> before, I am making this behavior conditional on a Kconfig option.
> Vanilla users will see no change in behavior unless they don't
> explicitly set this option to on.

Which is the reason why I don't like this approach. Why would you enable
the option in the first place? If you know the default should be 1 then
you would already do that via cgconfig or directly, right?
I think we should either change the default (which I am planning to do
for the next OpenSUSE) or do it slow way suggested by Tejun.
We really want to have as big testing coverage as possible for the
default change and config option is IMHO not a way to accomplish this.

> Distributions, however, are encouraged to set it.  

As I said, I plan to change the default with WARN_ONCE for both first
cgroup created and default changed. It would be nice if other
distributions could do the same but this might be tricky as nobody wants
to regress and there are certain usecases which could really suffer
(most of them fixable easily but there still might be some where
use_hierarchy=0 is valid).

> In that case, hierarchy will still be there. We'll just default to
> true in the root cgroup, and print a warning once if you try to set it
> back to 0.
> 
> After a grace period, we should be able to gauge if anyone actually
> relies on it and get rid of the hierarchy file, or at least of its
> behavior.
> 
> [ v2: make it dependent on a Kconfig option ]
> 
> Signed-off-by: Glauber Costa <glommer@parallels.com>
> CC: Dave Jones <davej@redhat.com>
> CC: Ben Hutchings <ben@decadent.org.uk>
> CC: Peter Zijlstra <a.p.zijlstra@chello.nl>
> CC: Paul Turner <pjt@google.com>
> CC: Lennart Poettering <lennart@poettering.net>
> CC: Kay Sievers <kay.sievers@vrfy.org>
> CC: Michal Hocko <mhocko@suse.cz>
> CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> CC: Johannes Weiner <hannes@cmpxchg.org>
> CC: Tejun Heo <tj@kernel.org>
> ---
>  init/Kconfig    | 18 ++++++++++++++++++
>  mm/memcontrol.c |  9 +++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/init/Kconfig b/init/Kconfig
> index 707d015..f64f888 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -726,6 +726,24 @@ config MEMCG_SWAP
>  	  if boot option "swapaccount=0" is set, swap will not be accounted.
>  	  Now, memory usage of swap_cgroup is 2 bytes per entry. If swap page
>  	  size is 4096bytes, 512k per 1Gbytes of swap.
> +
> +config MEMCG_HIERARCHY_DEFAULT
> +	bool "Hierarchical memcg"
> +	depends on MEMCG
> +	default n
> +	help
> +	  The memory controller has two modes of accounting: hierarchical and
> +	  flat. Hierarchical accounting will charge pages all the way towards a
> +	  group's parent while flat hierarchy will threat all groups as children
> +	  of the root memcg, regardless of their positioning in the tree.
> +
> +	  Use of flat hierarchies is highly discouraged, but has been the
> +	  default for performance reasons for quite some time. Setting this flag
> +	  to on will make hierarchical accounting the default. It is still
> +	  possible to set it back to flat by writing 0 to the file
> +	  memory.use_hierarchy, albeit discouraged. Distributors are encouraged
> +	  to set this option.
> +
>  config MEMCG_SWAP_ENABLED
>  	bool "Memory Resource Controller Swap Extension enabled by default"
>  	depends on MEMCG_SWAP
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 61831c33..ab79746 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4073,6 +4073,12 @@ static int mem_cgroup_hierarchy_write(struct cgroup *cont, struct cftype *cft,
>  	if (memcg->use_hierarchy == val)
>  		goto out;
>  
> +#ifdef CONFIG_MEMCG_HIERARCHY_DEFAULT
> +	WARN_ONCE((!parent_memcg && memcg->use_hierarchy && val == false),
> +	"Setting this file to 0 (flat hierarchy) is considered deprecated.\n"
> +	"If you believe you have a valid use case for that, we kindly ask you to contact linux-mm@kvack.org and let us know");
> +#endif
> +
>  	/*
>  	 * If parent's use_hierarchy is set, we can't make any modifications
>  	 * in the child subtrees. If it is unset, then the change can
> @@ -5325,6 +5331,9 @@ mem_cgroup_create(struct cgroup *cont)
>  			INIT_WORK(&stock->work, drain_local_stock);
>  		}
>  		hotcpu_notifier(memcg_cpu_hotplug_callback, 0);
> +#ifdef CONFIG_MEMCG_HIERARCHY_DEFAULT
> +		memcg->use_hierarchy = true;
> +#endif
>  	} else {
>  		parent = mem_cgroup_from_cont(cont->parent);
>  		memcg->use_hierarchy = parent->use_hierarchy;
> -- 
> 1.7.11.4
> 
> --
> 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>

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Glauber Costa]

> 
>> +	  of the root memcg, regardless of their positioning in the tree.
>> +
>> +	  Use of flat hierarchies is highly discouraged, but has been the
>> +	  default for performance reasons for quite some time. Setting this flag
>> +	  to on will make hierarchical accounting the default. It is still
>> +	  possible to set it back to flat by writing 0 to the file
>> +	  memory.use_hierarchy, albeit discouraged. Distributors are encouraged
>> +	  to set this option.
> [...]
> 
> I don't think that 'default n' is effective encouragement!
> 
> Ben.
> 
If it were up to me, I would just flip it to 1. No option.
A bit of history here, is that people have a - quite valid - concern
that this will disrupt users using their own kernel, should they decide
to update, recompile and run.

Conditional on a Kconfig option, people reusing their .config will see
no change. Distros, otoh, are versioned. It is not unreasonable to
expect a behavior change when a major version flips.

The encouragement here comes not from the default, but from the
acknowledgment that his thing is totally broken, and we need to act to
fix it in a compatible way.

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Glauber Costa]

On 09/03/2012 09:08 PM, Michal Hocko wrote:
> On Mon 03-09-12 19:46:51, Glauber Costa wrote:
>> Here is a new attempt to lay down a path that will allow us to deprecate
>> the non-hierarchical mode of operation from memcg.  Unlike what I posted
>> before, I am making this behavior conditional on a Kconfig option.
>> Vanilla users will see no change in behavior unless they don't
>> explicitly set this option to on.
> 
> Which is the reason why I don't like this approach. Why would you enable
> the option in the first place? If you know the default should be 1 then
> you would already do that via cgconfig or directly, right?
> I think we should either change the default (which I am planning to do
> for the next OpenSUSE) or do it slow way suggested by Tejun.
> We really want to have as big testing coverage as possible for the
> default change and config option is IMHO not a way to accomplish this.
> 

Not sure you realize, Michal, but you actually agree with me and my
patch, given your reasoning.

If you plan to change it in OpenSUSE, you have two ways of doing so:
You either carry a patch, which as simple as this is, is always
undesirable, or you add one line to your distro config. Pick my patch,
and do the later.

This patch does exactly the "do it slowly" thing, but without
introducing more churn, like mount options. Keep in mind that since
there is the concern that direct upstream users won't see a sudden
change in behavior, *every* way we choose to do it will raise the same
question you posed: "Why would you enable this in the first place?" Be
it a Kconfig, mount option, etc. The solution here is: Direct users of
upstream kernels won't see a behavior change - as requested - but
distributors will have a way to flip it without carrying a non-upstream
patch.


>> Distributions, however, are encouraged to set it.  
> 
> As I said, I plan to change the default with WARN_ONCE for both first
> cgroup created and default changed. It would be nice if other
> distributions could do the same but this might be tricky as nobody wants
> to regress and there are certain usecases which could really suffer
> (most of them fixable easily but there still might be some where
> use_hierarchy=0 is valid).
> 

tip: They can do the same without applying a non-upstream patch by using
this patch and just changing their default config.

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Michal Hocko]

On Tue 04-09-12 12:34:45, Glauber Costa wrote:
> On 09/03/2012 09:08 PM, Michal Hocko wrote:
> > On Mon 03-09-12 19:46:51, Glauber Costa wrote:
> >> Here is a new attempt to lay down a path that will allow us to deprecate
> >> the non-hierarchical mode of operation from memcg.  Unlike what I posted
> >> before, I am making this behavior conditional on a Kconfig option.
> >> Vanilla users will see no change in behavior unless they don't
> >> explicitly set this option to on.
> > 
> > Which is the reason why I don't like this approach. Why would you enable
> > the option in the first place? If you know the default should be 1 then
> > you would already do that via cgconfig or directly, right?
> > I think we should either change the default (which I am planning to do
> > for the next OpenSUSE) or do it slow way suggested by Tejun.
> > We really want to have as big testing coverage as possible for the
> > default change and config option is IMHO not a way to accomplish this.
> > 
> 
> Not sure you realize, Michal, but you actually agree with me and my
> patch, given your reasoning.

I do agree with the default change, all right, but I really don't like
the config option because that one will not help us that much.

> If you plan to change it in OpenSUSE, you have two ways of doing so:
> You either carry a patch, which as simple as this is, is always
> undesirable, or you add one line to your distro config. Pick my patch,
> and do the later.

I would have to care the patch anyway until the distro kernel moves to
a kernel which has the patch which won't happen anytime soon (at least
from distro POV) and I guess we want the testing coverage as long as
possible.

> This patch does exactly the "do it slowly" thing, but without
> introducing more churn, like mount options.

Not really. Do it slowly means that somebody actually _notices_ that
something is about to change and they have a lot of time for that. This
will be really hard with the config option saying N by default.  People
will ignore that until it's too late.
We are interested in those users who would keep the config default N and
they are (ab)using use_hierarchy=0 in a way which is hard/impossible to
fix. This is where distributions might help and they should IMHO but why
to put an additional code into upstream? Isn't it sufficient that those
who would like to help (and take the risk) would just take the patch?

> Keep in mind that since
> there is the concern that direct upstream users won't see a sudden
> change in behavior, *every* way we choose to do it will raise the same
> question you posed: "Why would you enable this in the first place?" Be
> it a Kconfig, mount option, etc. The solution here is: Direct users of
> upstream kernels won't see a behavior change - as requested - but
> distributors will have a way to flip it without carrying a non-upstream
> patch.

The patch is so small that I do not care having it without being
upstream. Do others care that much?
The consequences of the semantic change is what matters much more to me.

[...]
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Glauber Costa]

On 09/04/2012 05:09 PM, Michal Hocko wrote:
> Not really. Do it slowly means that somebody actually _notices_ that
> something is about to change and they have a lot of time for that. This
> will be really hard with the config option saying N by default.  People
> will ignore that until it's too late.
> We are interested in those users who would keep the config default N and
> they are (ab)using use_hierarchy=0 in a way which is hard/impossible to
> fix. This is where distributions might help and they should IMHO but why
> to put an additional code into upstream? Isn't it sufficient that those
> who would like to help (and take the risk) would just take the patch?

At least Fedora, seem to frown upon heavily at non-upstream patches.
To follow up with what you say, what would you say if we would WARN_ON()
unconditionally even if this switch is turned off?

a warn on dmesg is almost impossible not to be seen by anyone who cares.
That warning would tell people to flip the Kconfig option for the
warning will disappear. But ultimately, we are still keeping the
behavior intact.

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Michal Hocko]

On Tue 04-09-12 17:27:20, Glauber Costa wrote:
> On 09/04/2012 05:09 PM, Michal Hocko wrote:
> > Not really. Do it slowly means that somebody actually _notices_ that
> > something is about to change and they have a lot of time for that. This
> > will be really hard with the config option saying N by default.  People
> > will ignore that until it's too late.
> > We are interested in those users who would keep the config default N and
> > they are (ab)using use_hierarchy=0 in a way which is hard/impossible to
> > fix. This is where distributions might help and they should IMHO but why
> > to put an additional code into upstream? Isn't it sufficient that those
> > who would like to help (and take the risk) would just take the patch?
> 
> At least Fedora, seem to frown upon heavily at non-upstream patches.

OK, so what about the following approach instead? We won't change the
default but rather shout at people when they actually create subtrees
with use_hierarchy==0. This shouldn't make pointless noise. I do not
remember whether we have considered this previously so sorry if this was
shot down as well.
---
>From 5e6e9dbe09dc6144d84a76d936327d17bbad9ecb Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Tue, 4 Sep 2012 15:55:03 +0200
Subject: [PATCH] memcg: warn on deeper hierarchies with use_hierarchy==0

The memory controller supports both hierarchical and non-hierarchical
behavior which is controlled by use_hierarchy knob (0 by default).
The primary motivation for this distinction was an ineffectiveness
of hierarchical accounting. This has improved a lot since it was
introduced.

This schizophrenia makes the code and integration with other controllers
more complicated (e.g. mounting it with fully hierarchical one could
have an unexpected side effects) for no good reason so it would be good
to make the memory controller behave only hierarchically.

It seems that there is no good reasons for deep cgroup hierarchies which
are not truly hierarchical so we could set the default to 1. This might,
however, lead to unexpected regressions when somebody relies on the
current default behavior. For example, consider the following setup:
		 Root[cpuset,memory]
		  |
		  A (use_hierarchy=0)
		 / \
		B  C

All three A, B, C have some tasks and their memory limits. The hierarchy
is created only because of the cpuset and its configuration.
Say the default is changed. Then a memory pressure in C could influence
both A and B which wouldn't happen before. The problem might be really
hard to notice (unexpected slowdown).
This configuration could be fixed up easily by reorganization, though:
		 Root
		  |
		  A' (use_hierarchy=1, limit=unlimited, no tasks)
		 /|\
		A B C

The problem is that we don't know whether somebody has an use case which
cannot be transformed like that. Therefore this patch starts the slow
transition to hierarchical only memory controller by warning users who
are using flat hierarchies. The warning triggers only if a subgroup of
non-root group is created with use_hierarchy==0.

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

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 795e525..87cb83f 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4958,6 +4958,11 @@ mem_cgroup_create(struct cgroup *cont)
 		parent = mem_cgroup_from_cont(cont->parent);
 		memcg->use_hierarchy = parent->use_hierarchy;
 		memcg->oom_kill_disable = parent->oom_kill_disable;
+		WARN_ONCE(!memcg->use_hierarchy && parent != root_mem_cgroup,
+				"Creating hierarchies with use_hierarchy==0 "
+				"(flat hierarchy) is considered deprecated. "
+				"If you believe that your setup is correct, "
+				"we kindly ask you to contact linux-mm@kvack.org and let us");
 	}
 
 	if (parent && parent->use_hierarchy) {
-- 
1.7.10.4


-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Glauber Costa]

On 09/04/2012 06:35 PM, Michal Hocko wrote:
> On Tue 04-09-12 17:27:20, Glauber Costa wrote:
>> On 09/04/2012 05:09 PM, Michal Hocko wrote:
>>> Not really. Do it slowly means that somebody actually _notices_ that
>>> something is about to change and they have a lot of time for that. This
>>> will be really hard with the config option saying N by default.  People
>>> will ignore that until it's too late.
>>> We are interested in those users who would keep the config default N and
>>> they are (ab)using use_hierarchy=0 in a way which is hard/impossible to
>>> fix. This is where distributions might help and they should IMHO but why
>>> to put an additional code into upstream? Isn't it sufficient that those
>>> who would like to help (and take the risk) would just take the patch?
>>
>> At least Fedora, seem to frown upon heavily at non-upstream patches.
> 
> OK, so what about the following approach instead? We won't change the
> default but rather shout at people when they actually create subtrees
> with use_hierarchy==0. This shouldn't make pointless noise. I do not
> remember whether we have considered this previously so sorry if this was
> shot down as well.

The warning is fine, but just shouting won't achieve nothing. I believe
it would be really great to have a way to turn the default to 1 - and
stop the shouting.

Even if you are doing it in OpenSUSE as a patch, an upstream patch means
at least that every distribution is using the same patch, and those who
rebase will just flip the config.

I'd personally believe merging both our patches together would achieve a
good result.

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Glauber Costa]

>> I believe it would be really great to have a way to turn the default
>> to 1 - and stop the shouting.
> 
> We already can. You can use /etc/cgconfig (if you are using libcgroup)
> or do it manually.
> 
>> Even if you are doing it in OpenSUSE as a patch, an upstream patch means
>> at least that every distribution is using the same patch, and those who
>> rebase will just flip the config.
>>
>> I'd personally believe merging both our patches together would achieve a
>> good result.
> 
> I am still not sure we want to add a config option for something that is
> meant to go away. But let's see what others think.
> 

So what you propose in the end is that we add a userspace tweak for
something that could go away, instead of a Kconfig for something that go
away.

Way I see it, Kconfig is better because it is totally transparent, under
the hood, and will give us a single location to unpatch in case/when it
really goes away.

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Michal Hocko]

On Tue 04-09-12 18:37:53, Glauber Costa wrote:
> On 09/04/2012 06:35 PM, Michal Hocko wrote:
> > On Tue 04-09-12 17:27:20, Glauber Costa wrote:
> >> On 09/04/2012 05:09 PM, Michal Hocko wrote:
> >>> Not really. Do it slowly means that somebody actually _notices_ that
> >>> something is about to change and they have a lot of time for that. This
> >>> will be really hard with the config option saying N by default.  People
> >>> will ignore that until it's too late.
> >>> We are interested in those users who would keep the config default N and
> >>> they are (ab)using use_hierarchy=0 in a way which is hard/impossible to
> >>> fix. This is where distributions might help and they should IMHO but why
> >>> to put an additional code into upstream? Isn't it sufficient that those
> >>> who would like to help (and take the risk) would just take the patch?
> >>
> >> At least Fedora, seem to frown upon heavily at non-upstream patches.
> > 
> > OK, so what about the following approach instead? We won't change the
> > default but rather shout at people when they actually create subtrees
> > with use_hierarchy==0. This shouldn't make pointless noise. I do not
> > remember whether we have considered this previously so sorry if this was
> > shot down as well.
> 
> The warning is fine, but just shouting won't achieve nothing. 

I am not so sure about that. Users are usually quite sensitive to WARN
messages and I can put this kind of patch into older code bases as
well because it cannot introduce any regression. This could produce
a much bigger testing base. All we want to achieve at this stage is
to find out whether we can get rid of the knob and help people to use
use_hierarchy=1, right?

> I believe it would be really great to have a way to turn the default
> to 1 - and stop the shouting.

We already can. You can use /etc/cgconfig (if you are using libcgroup)
or do it manually.

> Even if you are doing it in OpenSUSE as a patch, an upstream patch means
> at least that every distribution is using the same patch, and those who
> rebase will just flip the config.
> 
> I'd personally believe merging both our patches together would achieve a
> good result.

I am still not sure we want to add a config option for something that is
meant to go away. But let's see what others think.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Michal Hocko]

On Tue 04-09-12 18:54:08, Glauber Costa wrote:
[...]
> >> I'd personally believe merging both our patches together would achieve a
> >> good result.
> > 
> > I am still not sure we want to add a config option for something that is
> > meant to go away. But let's see what others think.
> > 
> 
> So what you propose in the end is that we add a userspace tweak for
> something that could go away, instead of a Kconfig for something that go
> away.

The tweak is necessary only if you want to have use_hierarchy=1 for all
cgroups without taking care about that (aka setting the attribute for
the first level under the root). All the users that use only one level
bellow root don't have to do anything at all.

> Way I see it, Kconfig is better because it is totally transparent, under
> the hood, and will give us a single location to unpatch in case/when it
> really goes away.

I guess that by the single location you mean that no other user space
changes would have to be done, right? If yes then this is not true
because there will be a lot of configurations setting this up already
(either by cgconfig or by other scripts). All of them will have to be
fixed some day.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Tejun Heo]

Hello,

On Tue, Sep 04, 2012 at 04:35:52PM +0200, Michal Hocko wrote:
...
> The problem is that we don't know whether somebody has an use case which
> cannot be transformed like that. Therefore this patch starts the slow
> transition to hierarchical only memory controller by warning users who
> are using flat hierarchies. The warning triggers only if a subgroup of
> non-root group is created with use_hierarchy==0.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

I think this could work as the first step.  Regardless of the involved
steps, the goal is 1. finding out whether there are use cases or users
of flat hierarchy (ugh... even the name is stupid :) and 2. if so,
push them to stop doing that and give them time to do so.  While
userland growing "echo 1" to use_hierarchy isn't optimal, it isn't the
end of the world and something which can be taken care of by the
distros.

That said, I don't see how different this is from the staged way I
suggested other than requiring "echo 1" instead of a mount option.  At
any rate, the two aren't mutually exclusive and this looks good to me.

Thanks.

-- 
tejun

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Glauber Costa]

On 09/04/2012 08:25 PM, Michal Hocko wrote:
> On Tue 04-09-12 18:54:08, Glauber Costa wrote:
> [...]
>>>> I'd personally believe merging both our patches together would achieve a
>>>> good result.
>>>
>>> I am still not sure we want to add a config option for something that is
>>> meant to go away. But let's see what others think.
>>>
>>
>> So what you propose in the end is that we add a userspace tweak for
>> something that could go away, instead of a Kconfig for something that go
>> away.
> 
> The tweak is necessary only if you want to have use_hierarchy=1 for all
> cgroups without taking care about that (aka setting the attribute for
> the first level under the root). All the users that use only one level
> bellow root don't have to do anything at all.
> 
>> Way I see it, Kconfig is better because it is totally transparent, under
>> the hood, and will give us a single location to unpatch in case/when it
>> really goes away.
> 
> I guess that by the single location you mean that no other user space
> changes would have to be done, right? If yes then this is not true
> because there will be a lot of configurations setting this up already
> (either by cgconfig or by other scripts). All of them will have to be
> fixed some day.
> 

Some userspaces, not all. And the ones who set:

They are either explicitly setting to 0, and those are the ones we need
to find out, or they are setting to 1, which will be harmless. If they
were all mandated to do it, fine. But they are not everywhere, and much
many other exists that don't touch it at all. What you are proposing is
that *all* userspace tools that use it go flip it, instead of doing it
in the kernel.

As I've said before, distributions have lifecycles where changes in
behavior like this are tolerated. Some of those lifecycles are
incredibly long, in the 5+ years range. It could be really nice if they
would never see use_hierarchy=0 *at all*, which is much better
accomplished by a kernel-side switch. A Kconfig option is the choice
between carrying either an upstream patch or no patch at all (Depending
on timing), and carrying a non-standard patch.

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Michal Hocko]

On Wed 05-09-12 12:14:12, Glauber Costa wrote:
> On 09/04/2012 08:25 PM, Michal Hocko wrote:
> > On Tue 04-09-12 18:54:08, Glauber Costa wrote:
> > [...]
> >>>> I'd personally believe merging both our patches together would achieve a
> >>>> good result.
> >>>
> >>> I am still not sure we want to add a config option for something that is
> >>> meant to go away. But let's see what others think.
> >>>
> >>
> >> So what you propose in the end is that we add a userspace tweak for
> >> something that could go away, instead of a Kconfig for something that go
> >> away.
> > 
> > The tweak is necessary only if you want to have use_hierarchy=1 for all
> > cgroups without taking care about that (aka setting the attribute for
> > the first level under the root). All the users that use only one level
> > bellow root don't have to do anything at all.
> > 
> >> Way I see it, Kconfig is better because it is totally transparent, under
> >> the hood, and will give us a single location to unpatch in case/when it
> >> really goes away.
> > 
> > I guess that by the single location you mean that no other user space
> > changes would have to be done, right? If yes then this is not true
> > because there will be a lot of configurations setting this up already
> > (either by cgconfig or by other scripts). All of them will have to be
> > fixed some day.
> > 
> 
> Some userspaces, not all. And the ones who set:
> 
> They are either explicitly setting to 0, and those are the ones we need
> to find out, or they are setting to 1, which will be harmless. If they
> were all mandated to do it, fine. But they are not everywhere, and much
> many other exists that don't touch it at all. What you are proposing is
> that *all* userspace tools that use it go flip it, instead of doing it
> in the kernel.

If we want to have a big coverage then older kernels (aka distributions)
have to help here and their users cannot simply change the config. So
distributions would need to enable the config by default and we are
back...

> As I've said before, distributions have lifecycles where changes in
> behavior like this are tolerated. 

We can do that only when a new codestream is released. Config changes
are not allowed otherwise - at least here in Suse.

> Some of those lifecycles are incredibly long, in the 5+ years
> range. It could be really nice if they would never see use_hierarchy=0
> *at all*, which is much better accomplished by a kernel-side switch. A
> Kconfig option is the choice between carrying either an upstream patch
> or no patch at all (Depending on timing), and carrying a non-standard
> patch.

Can we settle on the following 3 steps?
1) warn about "flat" hierarchies (give it X releases) - I will push it
   to as many Suse code streams as possible (hope other distributions
   could do the same)
2) flip the default on the root cgroup & warn when somebody tries to
   change it to 0 (give it another X releases) that the knob will be
   removed
3) remove the knob and the whole nonsese
4) revert 3 if somebody really objects
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Tejun Heo]

Hello, Michal.

On Wed, Sep 05, 2012 at 04:49:42PM +0200, Michal Hocko wrote:
> Can we settle on the following 3 steps?
> 1) warn about "flat" hierarchies (give it X releases) - I will push it
>    to as many Suse code streams as possible (hope other distributions
>    could do the same)

I think I'm just gonna trigger WARN from cgroup core if anyone tries
to create hierarchy with a controller which doesn't support full
hierarchy.  WARN_ON_ONCE() at first and then WARN_ON() on each
creation later on.

> 2) flip the default on the root cgroup & warn when somebody tries to
>    change it to 0 (give it another X releases) that the knob will be
>    removed
> 3) remove the knob and the whole nonsese
> 4) revert 3 if somebody really objects

If we can get to 3, I don't think 4 would be a problem.

Thanks.

-- 
tejun

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Michal Hocko]

On Wed 05-09-12 13:12:38, Tejun Heo wrote:
> Hello, Michal.
> 
> On Wed, Sep 05, 2012 at 04:49:42PM +0200, Michal Hocko wrote:
> > Can we settle on the following 3 steps?
> > 1) warn about "flat" hierarchies (give it X releases) - I will push it
> >    to as many Suse code streams as possible (hope other distributions
> >    could do the same)
> 
> I think I'm just gonna trigger WARN from cgroup core if anyone tries
> to create hierarchy with a controller which doesn't support full
> hierarchy.  WARN_ON_ONCE() at first and then WARN_ON() on each
> creation later on.

How do you find out that a controller is not fully hierarchical? Memory
controller can be both.

> > 2) flip the default on the root cgroup & warn when somebody tries to
> >    change it to 0 (give it another X releases) that the knob will be
> >    removed
> > 3) remove the knob and the whole nonsese
> > 4) revert 3 if somebody really objects
> 
> If we can get to 3, I don't think 4 would be a problem.

Agreed.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Glauber Costa]

On 09/06/2012 04:06 PM, Michal Hocko wrote:
> On Wed 05-09-12 13:12:38, Tejun Heo wrote:
>> Hello, Michal.
>>
>> On Wed, Sep 05, 2012 at 04:49:42PM +0200, Michal Hocko wrote:
>>> Can we settle on the following 3 steps?
>>> 1) warn about "flat" hierarchies (give it X releases) - I will push it
>>>    to as many Suse code streams as possible (hope other distributions
>>>    could do the same)
>>
>> I think I'm just gonna trigger WARN from cgroup core if anyone tries
>> to create hierarchy with a controller which doesn't support full
>> hierarchy.  WARN_ON_ONCE() at first and then WARN_ON() on each
>> creation later on.
> 
> How do you find out that a controller is not fully hierarchical? Memory
> controller can be both.
> 
>>> 2) flip the default on the root cgroup & warn when somebody tries to
>>>    change it to 0 (give it another X releases) that the knob will be
>>>    removed
>>> 3) remove the knob and the whole nonsese
>>> 4) revert 3 if somebody really objects
>>
>> If we can get to 3, I don't think 4 would be a problem.
> 
> Agreed.
> 
Just so I understand it:

Michal clearly objected before folding his patch with my Kconfig patch.
But is there still opposition to merge both?

By having it default-n, only people that are either sure that this is
safe for them, or have more clearly defined lifecycles could set it.

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Michal Hocko]

On Thu 06-09-12 16:09:20, Glauber Costa wrote:
> On 09/06/2012 04:06 PM, Michal Hocko wrote:
> > On Wed 05-09-12 13:12:38, Tejun Heo wrote:
> >> Hello, Michal.
> >>
> >> On Wed, Sep 05, 2012 at 04:49:42PM +0200, Michal Hocko wrote:
> >>> Can we settle on the following 3 steps?
> >>> 1) warn about "flat" hierarchies (give it X releases) - I will push it
> >>>    to as many Suse code streams as possible (hope other distributions
> >>>    could do the same)
> >>
> >> I think I'm just gonna trigger WARN from cgroup core if anyone tries
> >> to create hierarchy with a controller which doesn't support full
> >> hierarchy.  WARN_ON_ONCE() at first and then WARN_ON() on each
> >> creation later on.
> > 
> > How do you find out that a controller is not fully hierarchical? Memory
> > controller can be both.
> > 
> >>> 2) flip the default on the root cgroup & warn when somebody tries to
> >>>    change it to 0 (give it another X releases) that the knob will be
> >>>    removed
> >>> 3) remove the knob and the whole nonsese
> >>> 4) revert 3 if somebody really objects
> >>
> >> If we can get to 3, I don't think 4 would be a problem.
> > 
> > Agreed.
> > 
> Just so I understand it:
> 
> Michal clearly objected before folding his patch with my Kconfig patch.
> But is there still opposition to merge both?

I do not find the config option very much useful but if others feel it
really is I won't block it.

> By having it default-n, only people that are either sure that this is
> safe for them, or have more clearly defined lifecycles could set it.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] memcg: first step towards hierarchical controller

[Glauber Costa]

On 09/06/2012 04:18 PM, Michal Hocko wrote:
>> Just so I understand it:
>> > 
>> > Michal clearly objected before folding his patch with my Kconfig patch.
>> > But is there still opposition to merge both?
> I do not find the config option very much useful but if others feel it
> really is I won't block it.
> 
Tejun, what is your take on this?