memcg, cgroup: kill css_id
Subsystems (3)
cover memcg, cgroup: kill css_id
(This patchset depends on "memcg: make memcg's life cycle the same as cgroup")
This patchset converts memcg to always use cgroup->id, and then kills css_id.
As we've removed memcg's own refcnt, converting memcg to use cgroup->id is very straight-forward.
Li Zefan (8):
cgroup: implement cgroup_is_ancestor()
cgroup: implement cgroup_from_id()
memcg: convert to use cgroup_is_ancestor()
memcg: convert to use cgroup_from_id()
memcg: convert to use cgroup->id
memcg: fail to create cgroup if the cgroup id is too big
memcg: don't use css_id any more
cgroup: kill css_id
--
include/linux/cgroup.h | 44 ++------- kernel/cgroup.c | 302 +++++++++----------------------------------------------------- mm/memcontrol.c | 53 ++++++----- 3 files changed, 77 insertions(+), 322 deletions(-)
PATCH 1/8 cgroup: implement cgroup_is_ancestor()
This will be used as a replacement for css_is_ancestor().
--- include/linux/cgroup.h | 3 +++ kernel/cgroup.c | 21 +++++++++++++++++++++ 2 files changed, 24 insertions(+)
include/linux/cgroup.h +3 -0
@@ -177,6 +177,7 @@ struct cgroup {atomic_t count;int id;/* ida allocated in-hierarchy ID */ +int depth;/* the depth of the cgroup */ /* * We link our 'sibling' struct into our parent's 'children'.
@@ -730,6 +731,8 @@ unsigned short css_id(struct cgroup_subsys_state *css);unsigned short css_depth (struct cgroup_subsys_state *css);struct cgroup_subsys_state *cgroup_css_from_dir (struct file *f,int id); +bool cgroup_is_ancestor (struct cgroup *child,struct cgroup *root); +#else /* !CONFIG_CGROUPS */ static inline int cgroup_init_early (void ) {return 0 ; }
kernel/cgroup.c +21 -0
@@ -4133,6 +4133,7 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry, cgrp->dentry = dentry; cgrp->parent = parent; + cgrp->depth = parent->depth +1 ; cgrp->root = parent->root; cgrp->top_cgroup = parent->top_cgroup;
@@ -5299,6 +5300,26 @@ struct cgroup_subsys_state *cgroup_css_from_dir(struct file *f, int id)return css ? css :ERR_PTR (-ENOENT ); } +/** +* cgroup_is_ancestor - test "root" cgroup is an ancestor of "child" +* @child: the cgroup to be tested. +* @root: the cgroup supposed to be an ancestor of the child. +* +* Returns true if "root" is an ancestor of "child" in its hierarchy. +*/ +bool cgroup_is_ancestor (struct cgroup *child,struct cgroup *root) +{ +int depth = child->depth; + +if (depth < root->depth) +return false; + +while (depth-- != root->depth) + child = child->parent; + +return (child == root); +} +#ifdef CONFIG_CGROUP_DEBUG static struct cgroup_subsys_state *debug_css_alloc (struct cgroup *cont) {
Signature · 1 line
1.8.0.2
Re: [PATCH 1/8] cgroup: implement cgroup_is_ancestor()
On Mon 08-04-13 16:20:11, Li Zefan wrote:
[...]
Quoted 4 lines
(quoted diff) +0
-0@@ -5299,6 +5300,26 @@ struct cgroup_subsys_state *cgroup_css_from_dir(struct file *f, int id) return css ? css : ERR_PTR(-ENOENT); }
Review struct cgroup_subsys_state *cgroup_css_from_dir(struct file *f, int id) · line 5312
Quoted 10 lines
(quoted diff) +10
-0+/** + * cgroup_is_ancestor - test "root" cgroup is an ancestor of "child" + * @child: the cgroup to be tested. + * @root: the cgroup supposed to be an ancestor of the child. + * + * Returns true if "root" is an ancestor of "child" in its hierarchy. + */ +bool cgroup_is_ancestor(struct cgroup *child, struct cgroup *root) +{ + int depth = child->depth;
Is this functionality helpful for other controllers but memcg? css_is_ancestor is currently used only by memcg code AFAICS and we can get the same functionality easily by using something like:
mm/memcontrol.c +7 -5
@@ -1472,11 +1472,13 @@ void mem_cgroup_update_lru_size(struct lruvec *lruvec, enum lru_list lru,bool __mem_cgroup_same_or_subtree (const struct mem_cgroup *root_memcg,struct mem_cgroup *memcg) { -if (root_memcg == memcg) -return true; -if (!root_memcg->use_hierarchy || !memcg) -return false; -return css_is_ancestor (&memcg->css, &root_memcg->css); +struct mem_cgroup *parent = memcg; +do { +if (parent == root_memcg) +return true; + }while ((parent =parent_mem_cgroup (parent))); + +return false; } static bool mem_cgroup_same_or_subtree(const struct mem_cgroup *root_memcg,
In both cases we have to go up the hierarchy.
Quoted 19 lines
(quoted diff) +10
-0+ + if (depth < root->depth) + return false; + + while (depth-- != root->depth) + child = child->parent; + + return (child == root); +} + #ifdef CONFIG_CGROUP_DEBUG static struct cgroup_subsys_state *debug_css_alloc(struct cgroup *cont) {Signature · 5 lines
1.8.0.2 -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 2 lines
Michal Hocko SUSE Labs
Tejun Heo
maintainerRe: [PATCH 1/8] cgroup: implement cgroup_is_ancestor()
Hello,
Quoted 6 lines · Michal Hocko
On Mon 08-04-13 16:20:11, Li Zefan wrote:
[...]Quoted 4 lines
(quoted diff) +0
-0@@ -5299,6 +5300,26 @@ struct cgroup_subsys_state *cgroup_css_from_dir(struct file *f, int id) return css ? css : ERR_PTR(-ENOENT); }
Quoted 14 lines
Review struct cgroup_subsys_state *cgroup_css_from_dir(struct file *f, int id) · line 5312
Quoted 10 lines
(quoted diff) +10
-0+/** + * cgroup_is_ancestor - test "root" cgroup is an ancestor of "child" + * @child: the cgroup to be tested. + * @root: the cgroup supposed to be an ancestor of the child. + * + * Returns true if "root" is an ancestor of "child" in its hierarchy. + */ +bool cgroup_is_ancestor(struct cgroup *child, struct cgroup *root) +{ + int depth = child->depth;Is this functionality helpful for other controllers but memcg?
css_is_ancestoris currently used only by memcg code AFAICS and we can get the same functionality easily by using something like:
It's a basic hierarchy operation. I'd prefer it to be in cgroup and in general let's try to avoid memcg-specific infrastructure. It doesn't seem to end well.
Thanks.
Signature · 7 lines
tejun -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 1/8] cgroup: implement cgroup_is_ancestor()
Quoted 24 lines · Tejun Heo
Hello,
Quoted 6 lines · Michal Hocko
On Mon 08-04-13 16:20:11, Li Zefan wrote:
[...]Quoted 4 lines
(quoted diff) +0
-0@@ -5299,6 +5300,26 @@ struct cgroup_subsys_state *cgroup_css_from_dir(struct file *f, int id) return css ? css : ERR_PTR(-ENOENT); }Quoted 14 lines
Review struct cgroup_subsys_state *cgroup_css_from_dir(struct file *f, int id) · line 5312
Quoted 10 lines
(quoted diff) +10
-0+/** + * cgroup_is_ancestor - test "root" cgroup is an ancestor of "child" + * @child: the cgroup to be tested. + * @root: the cgroup supposed to be an ancestor of the child. + * + * Returns true if "root" is an ancestor of "child" in its hierarchy. + */ +bool cgroup_is_ancestor(struct cgroup *child, struct cgroup *root) +{ + int depth = child->depth;Is this functionality helpful for other controllers but memcg?
css_is_ancestoris currently used only by memcg code AFAICS and we can get the same functionality easily by using something like:
Quoted 1 line
It's a basic hierarchy operation.
Which nobody seem to need ATM so it could be added later when an user emerges.
Quoted 2 lines
I'd prefer it to be in cgroup and in general let's try to avoid memcg-specific infrastructure.
Now that I am thinking about that some more css_is_ancestor is even not correct. Consider this for example
root
\
A (use_hierarchy = 0)
\
B (use_hierarchy = 1)
\
C
__mem_cgroup_same_or_subtree(A, C) would happily return true even though this is not correct because A is not a part of the same hierarchy. So it seems that memcg will need a special treatment here anyway because cgroup core doesn't know about use_hierarchy thingy which is still with us...
I will post a patch.
Quoted 10 lines
It doesn't seem to end well.
Thanks.
Signature · 5 lines
tejun -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 8 lines
Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 1/8] cgroup: implement cgroup_is_ancestor()
In reply to Michal HockoQuoted 6 lines · Michal Hocko
On Mon 08-04-13 16:20:11, Li Zefan wrote:
[...]Quoted 4 lines
(quoted diff) +0
-0@@ -5299,6 +5300,26 @@ struct cgroup_subsys_state *cgroup_css_from_dir(struct file *f, int id) return css ? css : ERR_PTR(-ENOENT); }
Quoted 14 lines
Review struct cgroup_subsys_state *cgroup_css_from_dir(struct file *f, int id) · line 5312
Quoted 10 lines
(quoted diff) +10
-0+/** + * cgroup_is_ancestor - test "root" cgroup is an ancestor of "child" + * @child: the cgroup to be tested. + * @root: the cgroup supposed to be an ancestor of the child. + * + * Returns true if "root" is an ancestor of "child" in its hierarchy. + */ +bool cgroup_is_ancestor(struct cgroup *child, struct cgroup *root) +{ + int depth = child->depth;Is this functionality helpful for other controllers but memcg?
css_is_ancestoris currently used only by memcg code AFAICS and we can get the same functionality easily by using something like:
And as it turned out using css_is_ancestor is not correct. Here is a patch to fix the issue. I will leave the decision whether cgroup_is_ancestor makes sense even without users to you. Would you be willing to take this into your current series so that we to not clash over that code?
---
From 684e90bf2fcd5e0f376a3c7bb0176c1267add7df Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Mon, 8 Apr 2013 19:46:34 +0200
Subject: [PATCH] memcg: fix __mem_cgroup_same_or_subtree
__mem_cgroup_same_or_subtree relies on css_is_ancestor if hierarchy is enabled for ages. This, however, is not correct because use_hierarchy doesn't need to be true all the way up the cgroup hierarchy. Consider the following example:
root (use_hierarchy=0)
\
A (use_hierarchy=0)
\
B (use_hierarchy=1)
\
C (use_hierarchy=1)__mem_cgroup_same_or_subtree(A, C) would return true even though C is not from the same hierarchy subtree. The bug shouldn't be critical but at least dump_tasks might print unrelated tasks (via task_in_mem_cgroup).
--- mm/memcontrol.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
mm/memcontrol.c +5 -2
@@ -1470,9 +1470,12 @@ bool __mem_cgroup_same_or_subtree(const struct mem_cgroup *root_memcg, {if (root_memcg == memcg)return true; -if (!root_memcg->use_hierarchy || !memcg) +if (!memcg)return false; -return css_is_ancestor (&memcg->css, &root_memcg->css); +while ((memcg =parent_mem_cgroup (memcg))) +if (memcg == root_memcg) +return true; +return false; } static bool mem_cgroup_same_or_subtree(const struct mem_cgroup *root_memcg,
Signature · 11 lines
1.7.10.4 -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Tejun Heo
maintainerRe: [PATCH 1/8] cgroup: implement cgroup_is_ancestor()
Quoted 12 lines · Michal Hocko
__mem_cgroup_same_or_subtreerelies oncss_is_ancestorif hierarchy is enabled for ages. This, however, is not correct becauseuse_hierarchydoesn't need to be true all the way up the cgroup hierarchy. Consider the following example:
root (use_hierarchy=0)
\
A (use_hierarchy=0)\ B (use_hierarchy=1) \ C (use_hierarchy=1)
Quoted 4 lines
__mem_cgroup_same_or_subtree(A, C)would return true even though C is not from the same hierarchy subtree. The bug shouldn't be critical but at leastdump_tasksmight print unrelated tasks (viatask_in_mem_cgroup).
Huh? Isn't that avoided by the !root_memcg->use_hierarchy test?
mm/memcontrol.c bool __mem_cgroup_same_or_subtree(const struct mem_cgroup *root_memcg, · line 1473
Quoted 5 lines
mm/memcontrol.c +0
-1@@ -1470,9 +1470,12 @@ bool __mem_cgroup_same_or_subtree(const struct mem_cgroup *root_memcg, {if (root_memcg == memcg)return true; -if (!root_memcg->use_hierarchy || !memcg)
^^^^^^^^^^^^^^^^^^^^^^^^^^
Quoted 16 lines
mm/memcontrol.c +5
-1+if (!memcg)return false; -return css_is_ancestor (&memcg->css, &root_memcg->css); +while ((memcg =parent_mem_cgroup (memcg))) +if (memcg == root_memcg) +return true; +return false; } static bool mem_cgroup_same_or_subtree(const struct mem_cgroup *root_memcg,Signature · 5 lines
1.7.10.4 -- Michal Hocko SUSE Labs
Signature · 7 lines
tejun -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 1/8] cgroup: implement cgroup_is_ancestor()
Quoted 13 lines · Tejun Heo
Quoted 12 lines · Michal Hocko
__mem_cgroup_same_or_subtreerelies oncss_is_ancestorif hierarchy is enabled for ages. This, however, is not correct becauseuse_hierarchydoesn't need to be true all the way up the cgroup hierarchy. Consider the following example:
root (use_hierarchy=0)
\
A (use_hierarchy=0)\ B (use_hierarchy=1) \ C (use_hierarchy=1)
Quoted 6 lines
Quoted 4 lines
__mem_cgroup_same_or_subtree(A, C)would return true even though C is not from the same hierarchy subtree. The bug shouldn't be critical but at leastdump_tasksmight print unrelated tasks (viatask_in_mem_cgroup).Huh? Isn't that avoided by the !root_memcg->use_hierarchy test?
Yes, it is. My selective blindness strikes again :/ I was convinced that it was memcg we tested use_hierarchy for...
Sorry about all the churn.
Quoted 15 lines
mm/memcontrol.c bool __mem_cgroup_same_or_subtree(const struct mem_cgroup *root_memcg, · line 1473
Quoted 5 lines
mm/memcontrol.c +0
-1@@ -1470,9 +1470,12 @@ bool __mem_cgroup_same_or_subtree(const struct mem_cgroup *root_memcg, {if (root_memcg == memcg)return true; -if (!root_memcg->use_hierarchy || !memcg)^^^^^^^^^^^^^^^^^^^^^^^^^^Quoted 9 lines
mm/memcontrol.c +5
-1+if (!memcg)return false; -return css_is_ancestor (&memcg->css, &root_memcg->css); +while ((memcg =parent_mem_cgroup (memcg))) +if (memcg == root_memcg) +return true; +return false; }
Signature · 2 lines
Michal Hocko SUSE Labs
Tejun Heo
maintainerRe: [PATCH 1/8] cgroup: implement cgroup_is_ancestor()
In reply to Li ZefanHello, Li.
Quoted 4 lines · Li Zefan
+/**
+ *cgroup_is_ancestor- test "root" cgroup is an ancestor of "child"
+ * @child: the cgroup to be tested.
+ * @root: the cgroup supposed to be an ancestor of the child.
Please explain locking in the comment. (I know it only requires both cgroups to be accessible but being explicit is nice.)
Quoted 3 lines
+ * Returns true if "root" is an ancestor of "child" in its hierarchy.
+ */
+boolcgroup_is_ancestor(struct cgroup *child, struct cgroup *root)
s/root/ancestor/
kernel/cgroup.c struct cgroup_subsys_state *cgroup_css_from_dir(struct file *f, int id) · line 5318
Quoted 8 lines
kernel/cgroup.c +8
-0+{ +int depth = child->depth; + +if (depth < root->depth) +return false; + +while (depth-- != root->depth) + child = child->parent;
Just walk up till it meets the ancestor or reaches root. Why bother with depth?
Thanks.
Signature · 1 line
tejun
Re: [PATCH 1/8] cgroup: implement cgroup_is_ancestor()
In reply to Li ZefanQuoted 3 lines · Li Zefan
This will be used as a replacement for
css_is_ancestor().Signed-off-by Li Zefan <[email protected]>
Hmm....but do we need "depth" ?
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 1/8] cgroup: implement cgroup_is_ancestor()
Quoted 9 lines · Kamezawa Hiroyuki
Quoted 3 lines · Li Zefan
This will be used as a replacement for
css_is_ancestor().Signed-off-by Li Zefan <[email protected]>Hmm....but do we need "depth" ?
which was removed in Tejun's
"[PATCHSET] perf, cgroup: implement hierarchy support for perf_event controller"
PATCH 2/8 cgroup: implement cgroup_from_id()
In reply to Li ZefanThis will be used as a replacement for css_lookup().
--- include/linux/cgroup.h | 1 + kernel/cgroup.c | 31 +++++++++++++++++++++++++------ 2 files changed, 26 insertions(+), 6 deletions(-)
include/linux/cgroup.h +1 -0
@@ -732,6 +732,7 @@ unsigned short css_depth(struct cgroup_subsys_state *css);struct cgroup_subsys_state *cgroup_css_from_dir (struct file *f,int id);bool cgroup_is_ancestor (struct cgroup *child,struct cgroup *root); +struct cgroup *cgroup_from_id (struct cgroup_subsys *ss,int id);#else /* !CONFIG_CGROUPS */
kernel/cgroup.c +25 -6
@@ -139,7 +139,7 @@ struct cgroupfs_root {unsigned long flags;/* IDs for cgroups in this hierarchy */ -struct ida cgroup_ida; +struct idr cgroup_idr;/* The path to use for release notifications. */ char release_agent_path[PATH_MAX ];
@@ -908,7 +908,7 @@ static void cgroup_free_fn(struct work_struct *work)simple_xattrs_free (&cgrp->xattrs); -ida_simple_remove (&cgrp->root->cgroup_ida, cgrp->id); +idr_remove (&cgrp->root->cgroup_idr, cgrp->id);kfree (rcu_dereference_raw (cgrp->name));kfree (cgrp); }
@@ -1512,7 +1512,8 @@ static struct cgroupfs_root *cgroup_root_from_opts(struct cgroup_sb_opts *opts) root->subsys_mask = opts->subsys_mask; root->flags = opts->flags; -ida_init (&root->cgroup_ida); +idr_init (&root->cgroup_idr); +if (opts->release_agent)strcpy (root->release_agent_path, opts->release_agent);if (opts->name)
@@ -1531,7 +1532,7 @@ static void cgroup_drop_root(struct cgroupfs_root *root)spin_lock (&hierarchy_id_lock);ida_remove (&hierarchy_ida, root->hierarchy_id);spin_unlock (&hierarchy_id_lock); -ida_destroy (&root->cgroup_ida); +idr_destroy (&root->cgroup_idr);kfree (root); }
@@ -1645,6 +1646,11 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,mutex_lock (&cgroup_mutex);mutex_lock (&cgroup_root_mutex); + root_cgrp->id =idr_alloc (&root->cgroup_idr, root_cgrp, +0 ,0 ,GFP_KERNEL ); +if (root_cgrp->id <0 ) + goto unlock_drop; +/* Check for name clashes with existing mounts */ ret = -EBUSY ;if (strlen (root->name))
@@ -4104,7 +4110,7 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry, goto err_free_cgrp;rcu_assign_pointer (cgrp->name, name); - cgrp->id =ida_simple_get (&root->cgroup_ida,1 ,0 ,GFP_KERNEL ); + cgrp->id =idr_alloc (&root->cgroup_idr, cgrp,1 ,0 ,GFP_KERNEL );if (cgrp->id <0 ) goto err_free_name;
@@ -4215,7 +4221,7 @@ err_free_all:/* Release the reference count that we took on the superblock */ deactivate_super (sb); err_free_id: -ida_simple_remove (&root->cgroup_ida, cgrp->id); +idr_remove (&root->cgroup_idr, cgrp->id); err_free_name:kfree (rcu_dereference_raw (cgrp->name)); err_free_cgrp:
@@ -5320,6 +5326,19 @@ bool cgroup_is_ancestor(struct cgroup *child, struct cgroup *root)return (child == root); } +/** +* cgroup_from_id - lookup cgroup by id +* @ss: cgroup subsys to be looked into. +* @id: the id +* +* Returns pointer to cgroup if there is valid one with id. +* NULL if not. Should be called under rcu_read_lock() +*/ +struct cgroup *cgroup_from_id (struct cgroup_subsys *ss,int id) +{ +return idr_find (&ss->root->cgroup_idr, id); +} +#ifdef CONFIG_CGROUP_DEBUG static struct cgroup_subsys_state *debug_css_alloc (struct cgroup *cont) {
Signature · 1 line
1.8.0.2
Tejun Heo
maintainerRe: [PATCH 2/8] cgroup: implement cgroup_from_id()
kernel/cgroup.c bool cgroup_is_ancestor(struct cgroup *child, struct cgroup *root) · line 5338
Quoted 10 lines · Li Zefan
kernel/cgroup.c +10
-0+/** +* cgroup_from_id - lookup cgroup by id +* @ss: cgroup subsys to be looked into. +* @id: the id +* +* Returns pointer to cgroup if there is valid one with id. +* NULL if not. Should be called under rcu_read_lock() +*/ +struct cgroup *cgroup_from_id (struct cgroup_subsys *ss,int id) +{
rcu_lockdep_assert(rcu_read_lock_held(), ...);
kernel/cgroup.c bool cgroup_is_ancestor(struct cgroup *child, struct cgroup *root) · line 5339
Quoted 1 line
kernel/cgroup.c +1
-0+return idr_find (&ss->root->cgroup_idr, id);
Thanks.
Signature · 1 line
tejun
Re: [PATCH 2/8] cgroup: implement cgroup_from_id()
Quoted 13 lines · Tejun Heo
kernel/cgroup.c bool cgroup_is_ancestor(struct cgroup *child, struct cgroup *root) · line 5338
Quoted 10 lines · Li Zefan
kernel/cgroup.c +10
-0+/** +* cgroup_from_id - lookup cgroup by id +* @ss: cgroup subsys to be looked into. +* @id: the id +* +* Returns pointer to cgroup if there is valid one with id. +* NULL if not. Should be called under rcu_read_lock() +*/ +struct cgroup *cgroup_from_id (struct cgroup_subsys *ss,int id) +{rcu_lockdep_assert(rcu_read_lock_held(), ..
.);
will update
Quoted 2 lines
Quoted 2 lines
kernel/cgroup.c +1
-0+return idr_find (&ss->root->cgroup_idr, id);
Tejun Heo
maintainerRe: [PATCH 2/8] cgroup: implement cgroup_from_id()
In reply to Li ZefanOops, one more thing.
kernel/cgroup.c static long cgroup_create(struct cgroup *parent, struct dentry *dentry, · line 4113
Quoted 2 lines · Li Zefan
kernel/cgroup.c +1
-1- cgrp->id =ida_simple_get (&root->cgroup_ida,1 ,0 ,GFP_KERNEL ); + cgrp->id =idr_alloc (&root->cgroup_idr, cgrp,1 ,0 ,GFP_KERNEL );
This will allow lookups to return half-initialized cgroup, which shouldn't happen. Either idr_alloc() should be moved to after initialization of other fields are finished, or it should be called with NULL @ptr with idr_replace() added at the end to install @cgrp.
Similarly, the removal path should guarantee that the object is removed from idr *before* its grace period starts.
Thanks.
Signature · 7 lines
tejun -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 2/8] cgroup: implement cgroup_from_id()
In reply to Li ZefanQuoted 98 lines · Li Zefan
This will be used as a replacement for
css_lookup().Signed-off-by Li Zefan <[email protected]>--- include/linux/cgroup.h | 1 + kernel/cgroup.c | 31 +++++++++++++++++++++++++------ 2 files changed, 26 insertions(+), 6 deletions(-)include/linux/cgroup.h +1
-0@@ -732,6 +732,7 @@ unsigned short css_depth(struct cgroup_subsys_state *css);struct cgroup_subsys_state *cgroup_css_from_dir (struct file *f,int id);bool cgroup_is_ancestor (struct cgroup *child,struct cgroup *root); +struct cgroup *cgroup_from_id (struct cgroup_subsys *ss,int id);#else /* !CONFIG_CGROUPS */ kernel/cgroup.c +15
-6@@ -139,7 +139,7 @@ struct cgroupfs_root {unsigned long flags;/* IDs for cgroups in this hierarchy */ -struct ida cgroup_ida; +struct idr cgroup_idr;/* The path to use for release notifications. */ char release_agent_path[PATH_MAX ];@@ -908,7 +908,7 @@ static void cgroup_free_fn(struct work_struct *work)simple_xattrs_free (&cgrp->xattrs); -ida_simple_remove (&cgrp->root->cgroup_ida, cgrp->id); +idr_remove (&cgrp->root->cgroup_idr, cgrp->id);kfree (rcu_dereference_raw (cgrp->name));kfree (cgrp); }@@ -1512,7 +1512,8 @@ static struct cgroupfs_root *cgroup_root_from_opts(struct cgroup_sb_opts *opts) root->subsys_mask = opts->subsys_mask; root->flags = opts->flags; -ida_init (&root->cgroup_ida); +idr_init (&root->cgroup_idr); +if (opts->release_agent)strcpy (root->release_agent_path, opts->release_agent);if (opts->name)@@ -1531,7 +1532,7 @@ static void cgroup_drop_root(struct cgroupfs_root *root)spin_lock (&hierarchy_id_lock);ida_remove (&hierarchy_ida, root->hierarchy_id);spin_unlock (&hierarchy_id_lock); -ida_destroy (&root->cgroup_ida); +idr_destroy (&root->cgroup_idr);kfree (root); }@@ -1645,6 +1646,11 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,mutex_lock (&cgroup_mutex);mutex_lock (&cgroup_root_mutex); + root_cgrp->id =idr_alloc (&root->cgroup_idr, root_cgrp, +0 ,0 ,GFP_KERNEL ); +if (root_cgrp->id <0 ) + goto unlock_drop; +/* Check for name clashes with existing mounts */ ret = -EBUSY ;if (strlen (root->name))@@ -4104,7 +4110,7 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry, goto err_free_cgrp;rcu_assign_pointer (cgrp->name, name); - cgrp->id =ida_simple_get (&root->cgroup_ida,1 ,0 ,GFP_KERNEL ); + cgrp->id =idr_alloc (&root->cgroup_idr, cgrp,1 ,0 ,GFP_KERNEL );if (cgrp->id <0 ) goto err_free_name;@@ -4215,7 +4221,7 @@ err_free_all:/* Release the reference count that we took on the superblock */ deactivate_super (sb); err_free_id: -ida_simple_remove (&root->cgroup_ida, cgrp->id); +idr_remove (&root->cgroup_idr, cgrp->id); err_free_name:kfree (rcu_dereference_raw (cgrp->name)); err_free_cgrp:@@ -5320,6 +5326,19 @@ bool cgroup_is_ancestor(struct cgroup *child, struct cgroup *root)return (child == root); } +/** +* cgroup_from_id - lookup cgroup by id +* @ss: cgroup subsys to be looked into.
kernel/cgroup.c bool cgroup_is_ancestor(struct cgroup *child, struct cgroup *root) · line 5341
Quoted 10 lines
kernel/cgroup.c +10
-0+* @id: the id +* +* Returns pointer to cgroup if there is valid one with id. +* NULL if not. Should be called under rcu_read_lock() +*/ +struct cgroup *cgroup_from_id (struct cgroup_subsys *ss,int id) +{ +return idr_find (&ss->root->cgroup_idr, id); +} +
How about checking cgroup_populated_dir() have been called once ? If not, returning NULL.
Thanks,
-Kame
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 2/8] cgroup: implement cgroup_from_id()
In reply to Li ZefanQuoted 62 lines · Li Zefan
This will be used as a replacement for
css_lookup().Signed-off-by Li Zefan <[email protected]>--- include/linux/cgroup.h | 1 + kernel/cgroup.c | 31 +++++++++++++++++++++++++------ 2 files changed, 26 insertions(+), 6 deletions(-)include/linux/cgroup.h +1
-0@@ -732,6 +732,7 @@ unsigned short css_depth(struct cgroup_subsys_state *css);struct cgroup_subsys_state *cgroup_css_from_dir (struct file *f,int id);bool cgroup_is_ancestor (struct cgroup *child,struct cgroup *root); +struct cgroup *cgroup_from_id (struct cgroup_subsys *ss,int id);#else /* !CONFIG_CGROUPS */ kernel/cgroup.c +5
-4@@ -139,7 +139,7 @@ struct cgroupfs_root {unsigned long flags;/* IDs for cgroups in this hierarchy */ -struct ida cgroup_ida; +struct idr cgroup_idr;/* The path to use for release notifications. */ char release_agent_path[PATH_MAX ];@@ -908,7 +908,7 @@ static void cgroup_free_fn(struct work_struct *work)simple_xattrs_free (&cgrp->xattrs); -ida_simple_remove (&cgrp->root->cgroup_ida, cgrp->id); +idr_remove (&cgrp->root->cgroup_idr, cgrp->id);kfree (rcu_dereference_raw (cgrp->name));kfree (cgrp); }@@ -1512,7 +1512,8 @@ static struct cgroupfs_root *cgroup_root_from_opts(struct cgroup_sb_opts *opts) root->subsys_mask = opts->subsys_mask; root->flags = opts->flags; -ida_init (&root->cgroup_ida); +idr_init (&root->cgroup_idr); +if (opts->release_agent)strcpy (root->release_agent_path, opts->release_agent);if (opts->name)@@ -1531,7 +1532,7 @@ static void cgroup_drop_root(struct cgroupfs_root *root)spin_lock (&hierarchy_id_lock);ida_remove (&hierarchy_ida, root->hierarchy_id);spin_unlock (&hierarchy_id_lock); -ida_destroy (&root->cgroup_ida); +idr_destroy (&root->cgroup_idr);kfree (root); }
kernel/cgroup.c static struct dentry *cgroup_mount(struct file_system_type *fs_type, · line 1652
Quoted 7 lines
kernel/cgroup.c +4
-0@@ -1645,6 +1646,11 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,mutex_lock (&cgroup_mutex);mutex_lock (&cgroup_root_mutex); + root_cgrp->id =idr_alloc (&root->cgroup_idr, root_cgrp, +0 ,0 ,GFP_KERNEL ); +if (root_cgrp->id <0 ) + goto unlock_drop;
Ah. hmm, I'm sorry but css ID is allocated from "1" and 0 was an invalid number. With this change, root cgroup will have 0.
If this change is intentional, please add comments in Changelog. IIRC, swap_cgroup treats 0 as "unused" (root_memcg doesn't account anything... so, I guess we'll not see troubles.) we need double check.
Thanks,
-Kame
PATCH 3/8 memcg: convert to use cgroup_is_ancestor()
In reply to Li ZefanThis is a preparation to kill css_id.
--- mm/memcontrol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
mm/memcontrol.c +1 -1
@@ -1383,7 +1383,7 @@ bool __mem_cgroup_same_or_subtree(const struct mem_cgroup *root_memcg,return true;if (!root_memcg->use_hierarchy || !memcg)return false; -return css_is_ancestor (&memcg->css, &root_memcg->css); +return cgroup_is_ancestor (memcg->css.cgroup, root_memcg->css.cgroup); } static bool mem_cgroup_same_or_subtree(const struct mem_cgroup *root_memcg,
Signature · 1 line
1.8.0.2
Re: [PATCH 3/8] memcg: convert to use cgroup_is_ancestor()
Quoted 13 lines · Li Zefan
This is a preparation to kill
css_id.Signed-off-by Li Zefan <[email protected]>--- mm/memcontrol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)mm/memcontrol.c +0
-0
mm/memcontrol.c bool __mem_cgroup_same_or_subtree(const struct mem_cgroup *root_memcg, · line 1389
Quoted 8 lines
mm/memcontrol.c +1
-1@@ -1383,7 +1383,7 @@ bool __mem_cgroup_same_or_subtree(const struct mem_cgroup *root_memcg,return true;if (!root_memcg->use_hierarchy || !memcg)return false; -return css_is_ancestor (&memcg->css, &root_memcg->css); +return cgroup_is_ancestor (memcg->css.cgroup, root_memcg->css.cgroup); } static bool mem_cgroup_same_or_subtree(const struct mem_cgroup *root_memcg,
Re: [PATCH 3/8] memcg: convert to use cgroup_is_ancestor()
In reply to Li ZefanQuoted 3 lines · Li Zefan
This is a preparation to kill
css_id.Signed-off-by Li Zefan <[email protected]>
Quoted 24 lines
--- mm/memcontrol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)mm/memcontrol.c +1
-1@@ -1383,7 +1383,7 @@ bool __mem_cgroup_same_or_subtree(const struct mem_cgroup *root_memcg,return true;if (!root_memcg->use_hierarchy || !memcg)return false; -return css_is_ancestor (&memcg->css, &root_memcg->css); +return cgroup_is_ancestor (memcg->css.cgroup, root_memcg->css.cgroup); } static bool mem_cgroup_same_or_subtree(const struct mem_cgroup *root_memcg,Signature · 6 lines
1.8.0.2 -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 2 lines
Michal Hocko SUSE Labs
PATCH 4/8 memcg: convert to use cgroup_from_id()
In reply to Li ZefanThis is a preparation to kill css_id.
--- mm/memcontrol.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
mm/memcontrol.c +4 -4
@@ -2769,15 +2769,15 @@ static void __mem_cgroup_cancel_local_charge(struct mem_cgroup *memcg,*/ static struct mem_cgroup *mem_cgroup_lookup (unsigned short id) { -struct cgroup_subsys_state *css; +struct cgroup *cgrp;/* ID 0 is unused ID */ if (!id)return NULL ; - css =css_lookup (&mem_cgroup_subsys, id); -if (!css) + cgrp =cgroup_from_id (&mem_cgroup_subsys, id); +if (!cgrp)return NULL ; -return mem_cgroup_from_css (css); +return mem_cgroup_from_cont (cgrp); }struct mem_cgroup *try_get_mem_cgroup_from_page (struct page *page)
Signature · 1 line
1.8.0.2
Re: [PATCH 4/8] memcg: convert to use cgroup_from_id()
Quoted 3 lines · Li Zefan
This is a preparation to kill
css_id.Signed-off-by Li Zefan <[email protected]>
I would be tempted to stuff this into the same patch which introduces cgroup_from_id but this is just a minor thing.
Quoted 36 lines
--- mm/memcontrol.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)mm/memcontrol.c +4
-4@@ -2769,15 +2769,15 @@ static void __mem_cgroup_cancel_local_charge(struct mem_cgroup *memcg,*/ static struct mem_cgroup *mem_cgroup_lookup (unsigned short id) { -struct cgroup_subsys_state *css; +struct cgroup *cgrp;/* ID 0 is unused ID */ if (!id)return NULL ; - css =css_lookup (&mem_cgroup_subsys, id); -if (!css) + cgrp =cgroup_from_id (&mem_cgroup_subsys, id); +if (!cgrp)return NULL ; -return mem_cgroup_from_css (css); +return mem_cgroup_from_cont (cgrp); }struct mem_cgroup *try_get_mem_cgroup_from_page (struct page *page)Signature · 7 lines
1.8.0.2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH 4/8] memcg: convert to use cgroup_from_id()
Quoted 8 lines · Michal Hocko
Quoted 3 lines · Li Zefan
This is a preparation to kill
css_id.Signed-off-by Li Zefan <[email protected]>I would be tempted to stuff this into the same patch which introduces
cgroup_from_idbut this is just a minor thing.
yeah it's not a big deal, just want to separate changes to cgroup and memcg.
Quoted 32 lines
Acked-by Michal Hocko <[email protected]>Quoted 30 lines
--- mm/memcontrol.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)mm/memcontrol.c +4
-4@@ -2769,15 +2769,15 @@ static void __mem_cgroup_cancel_local_charge(struct mem_cgroup *memcg,*/ static struct mem_cgroup *mem_cgroup_lookup (unsigned short id) { -struct cgroup_subsys_state *css; +struct cgroup *cgrp;/* ID 0 is unused ID */ if (!id)return NULL ; - css =css_lookup (&mem_cgroup_subsys, id); -if (!css) + cgrp =cgroup_from_id (&mem_cgroup_subsys, id); +if (!cgrp)return NULL ; -return mem_cgroup_from_css (css); +return mem_cgroup_from_cont (cgrp); }struct mem_cgroup *try_get_mem_cgroup_from_page (struct page *page)Signature · 1 line
1.8.0.2
Re: [PATCH 4/8] memcg: convert to use cgroup_from_id()
In reply to Li ZefanQuoted 6 lines · Li Zefan
This is a preparation to kill
css_id.Signed-off-by Li Zefan <[email protected]>--- mm/memcontrol.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
PATCH 5/8 memcg: convert to use cgroup->id
In reply to Li ZefanThis is a preparation to kill css_id.
--- mm/memcontrol.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
mm/memcontrol.c +9 -4
@@ -492,6 +492,11 @@ static inline bool mem_cgroup_is_root(struct mem_cgroup *memcg)return (memcg == root_mem_cgroup); } +static inline unsigned short mem_cgroup_id (struct mem_cgroup *memcg) +{ +return memcg->css.cgroup->id; +} +/* Writing them here to avoid exposing memcg's inner layout */ #if defined(CONFIG_INET ) && defined(CONFIG_MEMCG_KMEM )
@@ -4234,7 +4239,7 @@ mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout)* css_get() was called in uncharge(). */ if (do_swap_account && swapout && memcg) - swap_cgroup_record(ent, css_id(&memcg->css)); + swap_cgroup_record(ent, mem_cgroup_id(memcg)); }#endif
@@ -4286,8 +4291,8 @@ static int mem_cgroup_move_swap_account(swp_entry_t entry, {unsigned short old_id, new_id; - old_id =css_id (&from->css); - new_id =css_id (&to->css); + old_id =mem_cgroup_id (from); + new_id =mem_cgroup_id (to);if (swap_cgroup_cmpxchg (entry, old_id, new_id) == old_id) {mem_cgroup_swap_statistics (from, false);
@@ -6428,7 +6433,7 @@ static enum mc_target_type get_mctgt_type(struct vm_area_struct *vma, }/* There is a swap entry and a page doesn't exist or isn't charged */ if (ent.val && !ret && -css_id (&mc.from->css) ==lookup_swap_cgroup_id (ent)) { +mem_cgroup_id (mc.from) ==lookup_swap_cgroup_id (ent)) { ret =MC_TARGET_SWAP ;if (target) target->ent = ent;
Signature · 1 line
1.8.0.2
Re: [PATCH 5/8] memcg: convert to use cgroup->id
Quoted 3 lines · Li Zefan
This is a preparation to kill
css_id.Signed-off-by Li Zefan <[email protected]>
This patch depends on the following patch, doesn't it? There is no guarantee that id fits into short right now. Not such a big deal but would be nicer to have that guarantee for bisectability.
The patch on its own looks good.
Quoted 56 lines
--- mm/memcontrol.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)mm/memcontrol.c +9
-4@@ -492,6 +492,11 @@ static inline bool mem_cgroup_is_root(struct mem_cgroup *memcg)return (memcg == root_mem_cgroup); } +static inline unsigned short mem_cgroup_id (struct mem_cgroup *memcg) +{ +return memcg->css.cgroup->id; +} +/* Writing them here to avoid exposing memcg's inner layout */ #if defined(CONFIG_INET ) && defined(CONFIG_MEMCG_KMEM )@@ -4234,7 +4239,7 @@ mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout)* css_get() was called in uncharge(). */ if (do_swap_account && swapout && memcg) - swap_cgroup_record(ent, css_id(&memcg->css)); + swap_cgroup_record(ent, mem_cgroup_id(memcg)); }#endif @@ -4286,8 +4291,8 @@ static int mem_cgroup_move_swap_account(swp_entry_t entry, {unsigned short old_id, new_id; - old_id =css_id (&from->css); - new_id =css_id (&to->css); + old_id =mem_cgroup_id (from); + new_id =mem_cgroup_id (to);if (swap_cgroup_cmpxchg (entry, old_id, new_id) == old_id) {mem_cgroup_swap_statistics (from, false);@@ -6428,7 +6433,7 @@ static enum mc_target_type get_mctgt_type(struct vm_area_struct *vma, }/* There is a swap entry and a page doesn't exist or isn't charged */ if (ent.val && !ret && -css_id (&mc.from->css) ==lookup_swap_cgroup_id (ent)) { +mem_cgroup_id (mc.from) ==lookup_swap_cgroup_id (ent)) { ret =MC_TARGET_SWAP ;if (target) target->ent = ent;Signature · 6 lines
1.8.0.2 -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH 5/8] memcg: convert to use cgroup->id
Quoted 9 lines · Michal Hocko
Quoted 3 lines · Li Zefan
This is a preparation to kill
css_id.Signed-off-by Li Zefan <[email protected]>This patch depends on the following patch, doesn't it? There is no guarantee that id fits into short right now. Not such a big deal but would be nicer to have that guarantee for bisectability.
Not necessary, because css_id still prevents us from creating too many cgroups.
Quoted 3 lines
The patch on its own looks good.
Acked-by Michal Hocko <[email protected]>
Re: [PATCH 5/8] memcg: convert to use cgroup->id
Quoted 6 lines · Li Zefan
Quoted 5 lines · Michal Hocko
Quoted 3 lines · Li Zefan
This is a preparation to kill
css_id.Signed-off-by Li Zefan <[email protected]>
Quoted 7 lines
Quoted 4 lines
This patch depends on the following patch, doesn't it? There is no guarantee that id fits into short right now. Not such a big deal but would be nicer to have that guarantee for bisectability.
Not necessary, because
css_idstill prevents us from creating too many cgroups.
Right you are.
Thanks
Signature · 8 lines
Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 5/8] memcg: convert to use cgroup->id
In reply to Li ZefanQuoted 3 lines · Li Zefan
This is a preparation to kill
css_id.Signed-off-by Li Zefan <[email protected]>
PATCH 6/8 memcg: fail to create cgroup if the cgroup id is too big
In reply to Li Zefanmemcg requires the cgroup id to be smaller than 65536.
--- mm/memcontrol.c | 9 +++++++++ 1 file changed, 9 insertions(+)
mm/memcontrol.c +9 -0
@@ -492,6 +492,12 @@ static inline bool mem_cgroup_is_root(struct mem_cgroup *memcg)return (memcg == root_mem_cgroup); } +/* +* We restrict the id in the range of [0, 65535], so it can fit into +* an unsigned short. +*/ +#define MEM_CGROUP_ID_MAX (65535) +static inline unsigned short mem_cgroup_id (struct mem_cgroup *memcg) {return memcg->css.cgroup->id;
@@ -6125,6 +6131,9 @@ mem_cgroup_css_alloc(struct cgroup *cont)long error = -ENOMEM ;int node; +if (cont->id >MEM_CGROUP_ID_MAX ) +return ERR_PTR (-ENOSPC ); + memcg =mem_cgroup_alloc ();if (!memcg)return ERR_PTR (error);
Signature · 1 line
1.8.0.2
Re: [PATCH 6/8] memcg: fail to create cgroup if the cgroup id is too big
Quoted 3 lines · Li Zefan
memcg requires the cgroup id to be smaller than 65536.
Signed-off-by Li Zefan <[email protected]>
But this should be moved up the patch stack as mentioned in the previous email.
Minor nit bellow.
Quoted 9 lines
--- mm/memcontrol.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/memcontrol.c +0
-0
mm/memcontrol.c static inline bool mem_cgroup_is_root(struct mem_cgroup *memcg) · line 499
Quoted 8 lines
mm/memcontrol.c +5
-0@@ -492,6 +492,12 @@ static inline bool mem_cgroup_is_root(struct mem_cgroup *memcg)return (memcg == root_mem_cgroup); } +/* +* We restrict the id in the range of [0, 65535], so it can fit into +* an unsigned short. +*/ +#define MEM_CGROUP_ID_MAX (65535)
USHRT_MAX ?
Quoted 4 lines
mm/memcontrol.c +1
-0+static inline unsigned short mem_cgroup_id (struct mem_cgroup *memcg) {return memcg->css.cgroup->id;
Signature · 8 lines
Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
PATCH 7/8 memcg: don't use css_id any more
In reply to Li ZefanNow memcg uses cgroup->id instead of css_id. Update some comments and set mem_cgroup_subsys->use_id to 0.
--- mm/memcontrol.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)
mm/memcontrol.c +7 -14
@@ -574,16 +574,11 @@ static void disarm_sock_keys(struct mem_cgroup *memcg)#ifdef CONFIG_MEMCG_KMEM /* * This will be the memcg's index in each cache's ->memcg_params->memcg_caches. -* There are two main reasons for not using the css_id for this: -* 1) this works better in sparse environments, where we have a lot of memcgs, -* but only a few kmem-limited. Or also, if we have, for instance, 200 -* memcgs, and none but the 200th is kmem-limited, we'd have to have a -* 200 entry array for that. -* -* 2) In order not to violate the cgroup API, we would like to do all memory -* allocation in ->create(). At that point, we haven't yet allocated the -* css_id. Having a separate index prevents us from messing with the cgroup -* core for this +* The main reason for not using cgrp_id for this: +* this works better in sparse environments, where we have a lot of memcgs, +* but only a few kmem-limited. Or also, if we have, for instance, 200 +* memcgs, and none but the 200th is kmem-limited, we'd have to have a +* 200 entry array for that. * * The current size of the caches array is stored in * memcg_limited_groups_array_size. It will double each time we have to
@@ -598,10 +593,10 @@ int memcg_limited_groups_array_size;* cgroups is a reasonable guess. In the future, it could be a parameter or * tunable, but that is strictly not necessary. * -* MAX_SIZE should be as large as the number of css_ids. Ideally, we could get +* MAX_SIZE should be as large as the number of cgrp_ids. Ideally, we could get * this constant directly from cgroup, but it is understandable that this is * better kept as an internal representation in cgroup.c. In any case, the -* css_id space is not getting any smaller, and we don't have to necessarily +* cgrp_id space is not getting any smaller, and we don't have to necessarily * increase ours as well if it increases. */ #define MEMCG_CACHES_MIN_SIZE 4
@@ -6065,7 +6060,6 @@ static void __mem_cgroup_free(struct mem_cgroup *memcg)size_t size =memcg_size ();mem_cgroup_remove_from_trees (memcg); -free_css_id (&mem_cgroup_subsys, &memcg->css);for_each_node (node)free_mem_cgroup_per_zone_info (memcg, node);
@@ -6846,7 +6840,6 @@ struct cgroup_subsys mem_cgroup_subsys = { .attach = mem_cgroup_move_task, .base_cftypes = mem_cgroup_files, .early_init = 0, - .use_id = 1, };#ifdef CONFIG_MEMCG_SWAP
Signature · 1 line
1.8.0.2
Re: [PATCH 7/8] memcg: don't use css_id any more
Quoted 4 lines · Li Zefan
Now memcg uses
cgroup->idinstead ofcss_id. Update some comments and setmem_cgroup_subsys->use_idto 0.Signed-off-by Li Zefan <[email protected]>
Quoted 62 lines
--- mm/memcontrol.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)mm/memcontrol.c +7
-14@@ -574,16 +574,11 @@ static void disarm_sock_keys(struct mem_cgroup *memcg)#ifdef CONFIG_MEMCG_KMEM /* * This will be the memcg's index in each cache's ->memcg_params->memcg_caches. -* There are two main reasons for not using the css_id for this: -* 1) this works better in sparse environments, where we have a lot of memcgs, -* but only a few kmem-limited. Or also, if we have, for instance, 200 -* memcgs, and none but the 200th is kmem-limited, we'd have to have a -* 200 entry array for that. -* -* 2) In order not to violate the cgroup API, we would like to do all memory -* allocation in ->create(). At that point, we haven't yet allocated the -* css_id. Having a separate index prevents us from messing with the cgroup -* core for this +* The main reason for not using cgrp_id for this: +* this works better in sparse environments, where we have a lot of memcgs, +* but only a few kmem-limited. Or also, if we have, for instance, 200 +* memcgs, and none but the 200th is kmem-limited, we'd have to have a +* 200 entry array for that. * * The current size of the caches array is stored in * memcg_limited_groups_array_size. It will double each time we have to @@ -598,10 +593,10 @@ int memcg_limited_groups_array_size;* cgroups is a reasonable guess. In the future, it could be a parameter or * tunable, but that is strictly not necessary. * -* MAX_SIZE should be as large as the number of css_ids. Ideally, we could get +* MAX_SIZE should be as large as the number of cgrp_ids. Ideally, we could get * this constant directly from cgroup, but it is understandable that this is * better kept as an internal representation in cgroup.c. In any case, the -* css_id space is not getting any smaller, and we don't have to necessarily +* cgrp_id space is not getting any smaller, and we don't have to necessarily * increase ours as well if it increases. */ #define MEMCG_CACHES_MIN_SIZE 4@@ -6065,7 +6060,6 @@ static void __mem_cgroup_free(struct mem_cgroup *memcg)size_t size =memcg_size ();mem_cgroup_remove_from_trees (memcg); -free_css_id (&mem_cgroup_subsys, &memcg->css);for_each_node (node)free_mem_cgroup_per_zone_info (memcg, node);@@ -6846,7 +6840,6 @@ struct cgroup_subsys mem_cgroup_subsys = { .attach = mem_cgroup_move_task, .base_cftypes = mem_cgroup_files, .early_init = 0, - .use_id = 1, };#ifdef CONFIG_MEMCG_SWAP Signature · 1 line
1.8.0.2
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH 7/8] memcg: don't use css_id any more
In reply to Li ZefanQuoted 4 lines · Li Zefan
Now memcg uses
cgroup->idinstead ofcss_id. Update some comments and setmem_cgroup_subsys->use_idto 0.Signed-off-by Li Zefan <[email protected]>
PATCH 8/8 cgroup: kill css_id
In reply to Li ZefanThe only user of css_id was memcg, and it has been converted to use cgroup->id, so kill css_id.
--- include/linux/cgroup.h | 38 -------- kernel/cgroup.c | 258 ------------------------------------------------- 2 files changed, 296 deletions(-)
include/linux/cgroup.h +0 -38
@@ -495,11 +495,6 @@ struct cgroup_subsys {int active;int disabled;int early_init; -/* -* True if this subsys uses ID. ID is not available before cgroup_init() -* (not available in early_init time.) -*/ -bool use_id;/* * If %false, this subsystem is properly hierarchical -
@@ -525,9 +520,6 @@ struct cgroup_subsys {*/ struct cgroupfs_root *root;struct list_head sibling; -/* used when use_id == true */ -struct idr idr; -spinlock_t id_lock;/* list of cftype_sets */ struct list_head cftsets;
@@ -699,36 +691,6 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk, bool threadgroup);int cgroup_attach_task_all (struct task_struct *from,struct task_struct *); -/* -* CSS ID is ID for cgroup_subsys_state structs under subsys. This only works -* if cgroup_subsys.use_id == true. It can be used for looking up and scanning. -* CSS ID is assigned at cgroup allocation (create) automatically -* and removed when subsys calls free_css_id() function. This is because -* the lifetime of cgroup_subsys_state is subsys's matter. -* -* Looking up and scanning function should be called under rcu_read_lock(). -* Taking cgroup_mutex is not necessary for following calls. -* But the css returned by this routine can be "not populated yet" or "being -* destroyed". The caller should check css and cgroup's status. -*/ - -/* -* Typically Called at ->destroy(), or somewhere the subsys frees -* cgroup_subsys_state. -*/ -void free_css_id (struct cgroup_subsys *ss,struct cgroup_subsys_state *css); - -/* Find a cgroup_subsys_state which has given ID */ - -struct cgroup_subsys_state *css_lookup (struct cgroup_subsys *ss,int id); - -/* Returns true if root is ancestor of cg */ -bool css_is_ancestor (struct cgroup_subsys_state *cg, -const struct cgroup_subsys_state *root); - -/* Get id and depth of css */ -unsigned short css_id (struct cgroup_subsys_state *css); -unsigned short css_depth (struct cgroup_subsys_state *css);struct cgroup_subsys_state *cgroup_css_from_dir (struct file *f,int id);bool cgroup_is_ancestor (struct cgroup *child,struct cgroup *root);
kernel/cgroup.c +0 -258
312 lines not on this page · raw message
Signature · 1 line
1.8.0.2
Tejun Heo
maintainerRe: [PATCH 8/8] cgroup: kill css_id
Quoted 4 lines · Li Zefan
The only user of
css_idwas memcg, and it has been converted to usecgroup->id, so killcss_id.Signed-off-by Li Zefan <[email protected]>
Thanks a lot for killing this abomination.
Signature · 7 lines
tejun -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
On Mon 08-04-13 16:19:53, Li Zefan wrote:
[...]
Quoted 4 lines
include/linux/cgroup.h | 44 ++------- kernel/cgroup.c | 302 +++++++++----------------------------------------------------- mm/memcontrol.c | 53 ++++++----- 3 files changed, 77 insertions(+), 322 deletions(-)
Nice and thanks a lot Li!
Signature · 2 lines
Michal Hocko SUSE Labs