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@xxxxxxxxxxxxx>
CC: Michal Hocko <mhocko@xxxxxxx>
CC: Kamezawa Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
CC: Johannes Weiner <hannes@xxxxxxxxxxx>
CC: Tejun Heo <tj@xxxxxxxxxx>
---
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@xxxxxxxxxxx>
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@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>