[PATCH] memcg: first step towards hierarchical controller

[PATCH] memcg: first step towards hierarchical controller

[Glauber Costa]

Okay, so after recent discussions, I am proposing the following
patch. It won't remove hierarchy, or anything like that. Just default
to true in the root cgroup, and print a warning once if you try
to set it back to 0.

I am not adding it to feature-removal-schedule.txt because I don't
view it as a consensus. Rather, changing the default would allow us
to give it a time around in the open, and see if people complain
and what we can learn about that.

Signed-off-by: Glauber Costa <glommer@parallels.com>
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>
---
 mm/memcontrol.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 9e710bc..037ddd4 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3949,6 +3949,8 @@ static int mem_cgroup_hierarchy_write(struct cgroup *cont, struct cftype *cft,
 	if (memcg->use_hierarchy == val)
 		goto out;
 
+	WARN_ONCE((!parent_memcg && memcg->use_hierarchy && val == false),
+		"Non-hierarchical memcg is considered for deprecation");
 	/*
 	 * 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
@@ -5175,6 +5177,7 @@ mem_cgroup_create(struct cgroup *cont)
 			INIT_WORK(&stock->work, drain_local_stock);
 		}
 		hotcpu_notifier(memcg_cpu_hotplug_callback, 0);
+		memcg->use_hierarchy = true;
 	} else {
 		parent = mem_cgroup_from_cont(cont->parent);
 		memcg->use_hierarchy = parent->use_hierarchy;
-- 
1.7.10.2

--
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: first step towards hierarchical controller

[Johannes Weiner]

On Tue, Jun 26, 2012 at 05:30:28PM +0400, Glauber Costa wrote:
> Okay, so after recent discussions, I am proposing the following
> patch. It won't remove hierarchy, or anything like that. Just default
> to true in the root cgroup, and print a warning once if you try
> to set it back to 0.
> 
> I am not adding it to feature-removal-schedule.txt because I don't
> view it as a consensus. Rather, changing the default would allow us
> to give it a time around in the open, and see if people complain
> and what we can learn about that.
> 
> Signed-off-by: Glauber Costa <glommer@parallels.com>
> 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>
> ---
>  mm/memcontrol.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 9e710bc..037ddd4 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3949,6 +3949,8 @@ static int mem_cgroup_hierarchy_write(struct cgroup *cont, struct cftype *cft,
>  	if (memcg->use_hierarchy == val)
>  		goto out;
>  
> +	WARN_ONCE((!parent_memcg && memcg->use_hierarchy && val == false),
> +		"Non-hierarchical memcg is considered for deprecation");

Agreed, this is a much better first step than the global switch.

Nitpicks:

Should the warning be emitted for any memcg, not just the parent?  If
somebody takes notice of the changed semantics, it's better to print
the warning on the first try to disable hierarchies instead of holding
back until they walk up the tree and try to change it in the root.
Still forbid disabling at lower levels, just be more eager to inform
the people trying it.

The memcg->use_hierarchy check should not be needed as you make sure
it's different from val, so checking val == false should suffice?

Also, why the extra parens around the condition?

I find the warning message a bit terse.  Maybe include something like
"restructure the cgroup directory structure to match your accounting
requirements or complain to (linux-mm, cgroups list etc.)  if not
possible"

> @@ -5175,6 +5177,7 @@ mem_cgroup_create(struct cgroup *cont)
>  			INIT_WORK(&stock->work, drain_local_stock);
>  		}
>  		hotcpu_notifier(memcg_cpu_hotplug_callback, 0);
> +		memcg->use_hierarchy = true;

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

Let's try this.  We have crappy semantics all over the place and no
evidence, only fear, that someone may rely on them.  Pushing and
watching for fallout seems like the most sensible solution to get us
anywhere at this point.

--
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: first step towards hierarchical controller

[Peter Zijlstra]

On Tue, 2012-06-26 at 16:11 +0200, Johannes Weiner wrote:
> 
> Should the warning be emitted for any memcg, not just the parent?  If
> somebody takes notice of the changed semantics, it's better to print
> the warning on the first try to disable hierarchies instead of holding
> back until they walk up the tree and try to change it in the root.
> Still forbid disabling at lower levels, just be more eager to inform
> the people trying it. 

*blink* You mean you can mix-and-match use_hierarchy over the hierarchy?
Can I have some of those drugs? It must be strong and powerful stuff
that.

--
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: first step towards hierarchical controller

[Johannes Weiner]

On Tue, Jun 26, 2012 at 04:19:26PM +0200, Peter Zijlstra wrote:
> On Tue, 2012-06-26 at 16:11 +0200, Johannes Weiner wrote:
> > 
> > Should the warning be emitted for any memcg, not just the parent?  If
> > somebody takes notice of the changed semantics, it's better to print
> > the warning on the first try to disable hierarchies instead of holding
> > back until they walk up the tree and try to change it in the root.
> > Still forbid disabling at lower levels, just be more eager to inform
> > the people trying it. 
> 
> *blink* You mean you can mix-and-match use_hierarchy over the hierarchy?
> Can I have some of those drugs? It must be strong and powerful stuff
> that.

You can create root/a/b/c/d/e and enable hierarchy in b, which ends up
treating (a) and (b+children) like siblings even though they nest in
the cgroup fs.

Yes, drugs.

But you can't disable the hierarchy if you have a hierarchy-enabled
parent, which we try to make the new default.  So in case somebody has
an existing setup that happened to nest group directories without
hierarchy and so never used memory.use_hierarchy before, they'll
probably try to disable it where it bothers them, below the root
group, which will get them -EBUSY, nothing else.  I'm just asking to
warn in that case as well and suggest they get their directory
structure in order, to save them some time wondering wtf changed.

--
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: first step towards hierarchical controller

[Glauber Costa]

On 06/26/2012 06:38 PM, Johannes Weiner wrote:
> On Tue, Jun 26, 2012 at 04:19:26PM +0200, Peter Zijlstra wrote:
>> On Tue, 2012-06-26 at 16:11 +0200, Johannes Weiner wrote:
>>>
>>> Should the warning be emitted for any memcg, not just the parent?  If
>>> somebody takes notice of the changed semantics, it's better to print
>>> the warning on the first try to disable hierarchies instead of holding
>>> back until they walk up the tree and try to change it in the root.
>>> Still forbid disabling at lower levels, just be more eager to inform
>>> the people trying it.
>>
>> *blink* You mean you can mix-and-match use_hierarchy over the hierarchy?
>> Can I have some of those drugs? It must be strong and powerful stuff
>> that.
>
> You can create root/a/b/c/d/e and enable hierarchy in b, which ends up
> treating (a) and (b+children) like siblings even though they nest in
> the cgroup fs.
>
> Yes, drugs.
>
> But you can't disable the hierarchy if you have a hierarchy-enabled
> parent, which we try to make the new default.  So in case somebody has
> an existing setup that happened to nest group directories without
> hierarchy and so never used memory.use_hierarchy before, they'll
> probably try to disable it where it bothers them,

The only problem is that it was already disallowed way before, since as
you said yourself, you can't disable hierarchy if you have a hierarchy
enabled parent.


--
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: first step towards hierarchical controller

[Peter Zijlstra]

On Tue, 2012-06-26 at 16:38 +0200, Johannes Weiner wrote:
> But you can't disable the hierarchy if you have a hierarchy-enabled
> parent, which we try to make the new default. 

Ah ok.. so still crazy, but slightly less insane ;-)

--
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: first step towards hierarchical controller

[Glauber Costa]

On 06/26/2012 06:11 PM, Johannes Weiner wrote:
> On Tue, Jun 26, 2012 at 05:30:28PM +0400, Glauber Costa wrote:
>> Okay, so after recent discussions, I am proposing the following
>> patch. It won't remove hierarchy, or anything like that. Just default
>> to true in the root cgroup, and print a warning once if you try
>> to set it back to 0.
>>
>> I am not adding it to feature-removal-schedule.txt because I don't
>> view it as a consensus. Rather, changing the default would allow us
>> to give it a time around in the open, and see if people complain
>> and what we can learn about that.
>>
>> Signed-off-by: Glauber Costa <glommer@parallels.com>
>> 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>
>> ---
>>   mm/memcontrol.c |    3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 9e710bc..037ddd4 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -3949,6 +3949,8 @@ static int mem_cgroup_hierarchy_write(struct cgroup *cont, struct cftype *cft,
>>   	if (memcg->use_hierarchy == val)
>>   		goto out;
>>
>> +	WARN_ONCE((!parent_memcg && memcg->use_hierarchy && val == false),
>> +		"Non-hierarchical memcg is considered for deprecation");
>
> Agreed, this is a much better first step than the global switch.
>
> Nitpicks:
>
> Should the warning be emitted for any memcg, not just the parent?

It doesn't matter. Only the parent can be set to 0. It is impossible to 
set the others to 0 if the parent is set to 1 (only vice-versa)

> If
> somebody takes notice of the changed semantics, it's better to print
> the warning on the first try to disable hierarchies instead of holding
> back until they walk up the tree and try to change it in the root.

That is precisely what is done here. Again, since you can only 
effectively change in the root, that will be equivalent to first try.

> Still forbid disabling at lower levels, just be more eager to inform
> the people trying it.
>
> The memcg->use_hierarchy check should not be needed as you make sure
> it's different from val, so checking val == false should suffice?

Yes, you are right here.

> Also, why the extra parens around the condition?

I will remove them.

> I find the warning message a bit terse.  Maybe include something like
> "restructure the cgroup directory structure to match your accounting
> requirements or complain to (linux-mm, cgroups list etc.)  if not
> possible"

Since I was expecting a "get out of here, you moron!" kind of reception,
I didn't worried too much about the message =p

I do agree that pointing to a mailing list address is a good thing to do.

>> @@ -5175,6 +5177,7 @@ mem_cgroup_create(struct cgroup *cont)
>>   			INIT_WORK(&stock->work, drain_local_stock);
>>   		}
>>   		hotcpu_notifier(memcg_cpu_hotplug_callback, 0);
>> +		memcg->use_hierarchy = true;
>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
>
> Let's try this.  We have crappy semantics all over the place and no
> evidence, only fear, that someone may rely on them.

beautiful words.

--
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: first step towards hierarchical controller

[Glauber Costa]

On 06/26/2012 06:11 PM, Johannes Weiner wrote:
> I find the warning message a bit terse.  Maybe include something like
> "restructure the cgroup directory structure to match your accounting
> requirements or complain to (linux-mm, cgroups list etc.)  if not
> possible"

How about:

WARN_ONCE(!parent_memcg && memcg->use_hierarchy,
    "Non-hierarchical memcg is considered for deprecation\n"
    "Please consider reorganizing your tree to work with hierarchical 
accounting\n"
    "If you have any reason not to, let us know at 
cgroups@vger.kernel.org\n");


--
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: first step towards hierarchical controller

[Johannes Weiner]

On Tue, Jun 26, 2012 at 07:31:21PM +0400, Glauber Costa wrote:
> On 06/26/2012 06:11 PM, Johannes Weiner wrote:
> >I find the warning message a bit terse.  Maybe include something like
> >"restructure the cgroup directory structure to match your accounting
> >requirements or complain to (linux-mm, cgroups list etc.)  if not
> >possible"
> 
> How about:
> 
> WARN_ONCE(!parent_memcg && memcg->use_hierarchy,
>    "Non-hierarchical memcg is considered for deprecation\n"
>    "Please consider reorganizing your tree to work with hierarchical
> accounting\n"
>    "If you have any reason not to, let us know at
> cgroups@vger.kernel.org\n");

Sounds good to me, thanks!

--
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: first step towards hierarchical controller

[Michal Hocko]

On Tue 26-06-12 17:30:28, Glauber Costa wrote:
> Okay, so after recent discussions, I am proposing the following
> patch. It won't remove hierarchy, or anything like that. Just default
> to true in the root cgroup, and print a warning once if you try
> to set it back to 0.
> 
> I am not adding it to feature-removal-schedule.txt because I don't
> view it as a consensus. Rather, changing the default would allow us
> to give it a time around in the open, and see if people complain
> and what we can learn about that.
> 
> Signed-off-by: Glauber Costa <glommer@parallels.com>
> 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>
> ---
>  mm/memcontrol.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 9e710bc..037ddd4 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3949,6 +3949,8 @@ static int mem_cgroup_hierarchy_write(struct cgroup *cont, struct cftype *cft,
>  	if (memcg->use_hierarchy == val)
>  		goto out;
>  
> +	WARN_ONCE((!parent_memcg && memcg->use_hierarchy && val == false),
> +		"Non-hierarchical memcg is considered for deprecation");
>  	/*
>  	 * 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
> @@ -5175,6 +5177,7 @@ mem_cgroup_create(struct cgroup *cont)
>  			INIT_WORK(&stock->work, drain_local_stock);
>  		}
>  		hotcpu_notifier(memcg_cpu_hotplug_callback, 0);
> +		memcg->use_hierarchy = true;

So the only way to disable hierarchies is to do it on the root first
(before any children exist) and then start creating your groups?

I think it will be much safer if we could enable it to the first floor
under the root - I know hackish - but I guess that most users don't set
anything in the root cgroup (most of the time it's EINVAL anyway) and
only set up groups they are creating.

Anyway, I guess we can give this approach a try.

>  	} else {
>  		parent = mem_cgroup_from_cont(cont->parent);
>  		memcg->use_hierarchy = parent->use_hierarchy;

Thanks
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
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: first step towards hierarchical controller

[Glauber Costa]

On 06/26/2012 07:27 PM, Michal Hocko wrote:
> On Tue 26-06-12 17:30:28, Glauber Costa wrote:
>> Okay, so after recent discussions, I am proposing the following
>> patch. It won't remove hierarchy, or anything like that. Just default
>> to true in the root cgroup, and print a warning once if you try
>> to set it back to 0.
>>
>> I am not adding it to feature-removal-schedule.txt because I don't
>> view it as a consensus. Rather, changing the default would allow us
>> to give it a time around in the open, and see if people complain
>> and what we can learn about that.
>>
>> Signed-off-by: Glauber Costa <glommer@parallels.com>
>> 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>
>> ---
>>   mm/memcontrol.c |    3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 9e710bc..037ddd4 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -3949,6 +3949,8 @@ static int mem_cgroup_hierarchy_write(struct cgroup *cont, struct cftype *cft,
>>   	if (memcg->use_hierarchy == val)
>>   		goto out;
>>
>> +	WARN_ONCE((!parent_memcg && memcg->use_hierarchy && val == false),
>> +		"Non-hierarchical memcg is considered for deprecation");
>>   	/*
>>   	 * 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
>> @@ -5175,6 +5177,7 @@ mem_cgroup_create(struct cgroup *cont)
>>   			INIT_WORK(&stock->work, drain_local_stock);
>>   		}
>>   		hotcpu_notifier(memcg_cpu_hotplug_callback, 0);
>> +		memcg->use_hierarchy = true;
>
> So the only way to disable hierarchies is to do it on the root first
> (before any children exist) and then start creating your groups?

Yes.

This is true after my patch.
This is also true before my patch, if you set it to 1 in the root, and 
then tries to flip it back.

> I think it will be much safer if we could enable it to the first floor
> under the root - I know hackish - but I guess that most users don't set
> anything in the root cgroup (most of the time it's EINVAL anyway) and
> only set up groups they are creating.

Well, I think it is fair to say that the reasonable expectation of this,
is that they will still not set anything, and still things will work
(we are assuming very few people actually care about this)

So I don't see a reason for it.
> Anyway, I guess we can give this approach a try.
>
>>   	} else {
>>   		parent = mem_cgroup_from_cont(cont->parent);
>>   		memcg->use_hierarchy = parent->use_hierarchy;
>
> Thanks
>


--
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: first step towards hierarchical controller

[Michal Hocko]

On Tue 26-06-12 19:28:01, Glauber Costa wrote:
[...]
> >>diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> >>index 9e710bc..037ddd4 100644
> >>--- a/mm/memcontrol.c
> >>+++ b/mm/memcontrol.c
> >>@@ -3949,6 +3949,8 @@ static int mem_cgroup_hierarchy_write(struct cgroup *cont, struct cftype *cft,
> >>  	if (memcg->use_hierarchy == val)
> >>  		goto out;
> >>
> >>+	WARN_ONCE((!parent_memcg && memcg->use_hierarchy && val == false),
> >>+		"Non-hierarchical memcg is considered for deprecation");
> >>  	/*
> >>  	 * 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
> >>@@ -5175,6 +5177,7 @@ mem_cgroup_create(struct cgroup *cont)
> >>  			INIT_WORK(&stock->work, drain_local_stock);
> >>  		}
> >>  		hotcpu_notifier(memcg_cpu_hotplug_callback, 0);
> >>+		memcg->use_hierarchy = true;
> >
> >So the only way to disable hierarchies is to do it on the root first
> >(before any children exist) and then start creating your groups?
> 
> Yes.
> 
> This is true after my patch.
> This is also true before my patch, if you set it to 1 in the root,
> and then tries to flip it back.

OK, fair enough. Having another tweaks for the 1st level cgroups would
change the behavior from the current state which is bad (so it wouldn't
be just hackish but stupid as well...).

Just make sure we are warning also deeper in the hierarchy (for cgconfig
cases) suggested by Johannes and you can add my Acked-by.

Please also mention that if anybody is really interested in the original
behavior, for what ever reasons, then the following cgconfig.conf should
do the trick

group . {
	memory {
		memory.use_hierarchy = 0;
	}
}

Thanks
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
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: first step towards hierarchical controller

[Tejun Heo]

On Tue, Jun 26, 2012 at 05:30:28PM +0400, Glauber Costa wrote:
> Okay, so after recent discussions, I am proposing the following
> patch. It won't remove hierarchy, or anything like that. Just default
> to true in the root cgroup, and print a warning once if you try
> to set it back to 0.
> 
> I am not adding it to feature-removal-schedule.txt because I don't
> view it as a consensus. Rather, changing the default would allow us
> to give it a time around in the open, and see if people complain
> and what we can learn about that.
> 
> Signed-off-by: Glauber Costa <glommer@parallels.com>
> 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>

Just in case it wasn't clear in the other posting.

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

You can't change the default behavior silently.  Not in this scale.

Thanks.

-- 
tejun

--
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: first step towards hierarchical controller

[Glauber Costa]

On 06/26/2012 10:12 PM, Tejun Heo wrote:
> On Tue, Jun 26, 2012 at 05:30:28PM +0400, Glauber Costa wrote:
>> Okay, so after recent discussions, I am proposing the following
>> patch. It won't remove hierarchy, or anything like that. Just default
>> to true in the root cgroup, and print a warning once if you try
>> to set it back to 0.
>>
>> I am not adding it to feature-removal-schedule.txt because I don't
>> view it as a consensus. Rather, changing the default would allow us
>> to give it a time around in the open, and see if people complain
>> and what we can learn about that.
>>
>> Signed-off-by: Glauber Costa <glommer@parallels.com>
>> 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>
>
> Just in case it wasn't clear in the other posting.
>
>   Nacked-by: Tejun Heo <tj@kernel.org>
>
> You can't change the default behavior silently.  Not in this scale.
>
> Thanks.
>
I certainly don't share your views of the matter here.

I would agree with you if we were changing a fundamental algorithm,
with no way to resort back to a default setup. We are not removing any
functionality whatsoever here.

I would agree with you if we were actually documenting explicitly
that this is an expected default behavior.

But we never made the claim that use_hierarchy would default to 0.

Well, we seldom make claims about default values of any tunables. We 
just expect them to be reasonable values, and we seem to agree that this 
is, indeed, reasonable.

I personally consider this even better than a mount option. It doesn't 
add or remove any new interface, since use_hierarchy was already there.

It doesn't change the behavior of any interface. What would happen for 
instance if I rely on a multitude of use_hierarchy = 0 and 1 and 
suddenly a mount option would override that?

--
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: first step towards hierarchical controller

[Tejun Heo]

On Tue, Jun 26, 2012 at 10:22:04PM +0400, Glauber Costa wrote:
> I would agree with you if we were changing a fundamental algorithm,
> with no way to resort back to a default setup. We are not removing any
> functionality whatsoever here.
> 
> I would agree with you if we were actually documenting explicitly
> that this is an expected default behavior.
> 
> But we never made the claim that use_hierarchy would default to 0.
> 
> Well, we seldom make claims about default values of any tunables. We
> just expect them to be reasonable values, and we seem to agree that
> this is, indeed, reasonable.

No, we can't change behavior in this major way silently.  Any change
of this scale must be explicit.  The behavior change is not only major
but also subtle at the same time.  If a user is using flat hierarchy
and then boot a new kernel, the user suddenly gets hierarchical
accounting which may or may not cause noticeable problem immediately
and would be difficult like hell to chase down and diagnose.

I don't mind how global switch is implemented but the flip should be
explicit no matter what.  This is something we simply CAN NOT do.

Thanks.

-- 
tejun

--
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: first step towards hierarchical controller

[Michal Hocko]

On Tue 26-06-12 22:22:04, Glauber Costa wrote:
> On 06/26/2012 10:12 PM, Tejun Heo wrote:
> >On Tue, Jun 26, 2012 at 05:30:28PM +0400, Glauber Costa wrote:
> >>Okay, so after recent discussions, I am proposing the following
> >>patch. It won't remove hierarchy, or anything like that. Just default
> >>to true in the root cgroup, and print a warning once if you try
> >>to set it back to 0.
> >>
> >>I am not adding it to feature-removal-schedule.txt because I don't
> >>view it as a consensus. Rather, changing the default would allow us
> >>to give it a time around in the open, and see if people complain
> >>and what we can learn about that.
> >>
> >>Signed-off-by: Glauber Costa <glommer@parallels.com>
> >>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>
> >
> >Just in case it wasn't clear in the other posting.
> >
> >  Nacked-by: Tejun Heo <tj@kernel.org>
> >
> >You can't change the default behavior silently.  Not in this scale.
> >
> >Thanks.
> >
> I certainly don't share your views of the matter here.
> 
> I would agree with you if we were changing a fundamental algorithm,
> with no way to resort back to a default setup. We are not removing any
> functionality whatsoever here.
> 
> I would agree with you if we were actually documenting explicitly
> that this is an expected default behavior.

Actually we did:
Documentation/cgroups/memory.txt
"
6.1 Enabling hierarchical accounting and reclaim

A memory cgroup by default disables the hierarchy feature. Support
can be enabled by writing 1 to memory.use_hierarchy file of the root
cgroup
"

But I do not think this is really that important. We are still
interested in making the thing sane. Flat_hierarchical trees just don't
seem right... Generic? Sure. Sane? Really?

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
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>