(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(-)
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(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 5aa6e91..14f1375 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -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,
--
1.8.0.2
This will be used as a replacement for css_is_ancestor().
Signed-off-by: Li Zefan <[email protected]>
---
include/linux/cgroup.h | 3 +++
kernel/cgroup.c | 21 +++++++++++++++++++++
2 files changed, 24 insertions(+)
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 2eaedc1..96072e4 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -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; }
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 7ee3bdf..e87872c 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -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)
{
--
1.8.0.2
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(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 14f1375..3561d0b 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -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)
--
1.8.0.2
This is a preparation to kill css_id.
Signed-off-by: Li Zefan <[email protected]>
---
mm/memcontrol.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 3561d0b..c4e0173 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -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;
--
1.8.0.2
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(-)
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 96072e4..6ae8ae1 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -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 */
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index e87872c..5ae1e87 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -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)
{
--
1.8.0.2
memcg requires the cgroup id to be smaller than 65536.
Signed-off-by: Li Zefan <[email protected]>
---
mm/memcontrol.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index c4e0173..947dff1 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -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);
--
1.8.0.2
Now memcg uses cgroup->id instead of css_id. Update some comments and
set mem_cgroup_subsys->use_id to 0.
Signed-off-by: Li Zefan <[email protected]>
---
mm/memcontrol.c | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 947dff1..26ee672 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -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
--
1.8.0.2
The only user of css_id was memcg, and it has been converted to
use cgroup->id, so kill css_id.
Signed-off-by: Li Zefan <[email protected]>
---
include/linux/cgroup.h | 38 --------
kernel/cgroup.c | 258 -------------------------------------------------
2 files changed, 296 deletions(-)
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 6ae8ae1..d2c06db 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -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);
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 5ae1e87..2389484 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -165,38 +165,6 @@ struct cfent {
};
/*
- * CSS ID -- ID per subsys's Cgroup Subsys State(CSS). used only when
- * cgroup_subsys->use_id != 0.
- */
-#define CSS_ID_MAX (65535)
-struct css_id {
- /*
- * The css to which this ID points. This pointer is set to valid value
- * after cgroup is populated. If cgroup is removed, this will be NULL.
- * This pointer is expected to be RCU-safe because destroy()
- * is called after synchronize_rcu(). But for safe use, css_tryget()
- * should be used for avoiding race.
- */
- struct cgroup_subsys_state __rcu *css;
- /*
- * ID of this css.
- */
- unsigned short id;
- /*
- * Depth in hierarchy which this ID belongs to.
- */
- unsigned short depth;
- /*
- * ID is freed by RCU. (and lookup routine is RCU safe.)
- */
- struct rcu_head rcu_head;
- /*
- * Hierarchy of CSS ID belongs to.
- */
- unsigned short stack[0]; /* Array of Length (depth+1) */
-};
-
-/*
* cgroup_event represents events which userspace want to receive.
*/
struct cgroup_event {
@@ -363,9 +331,6 @@ struct cg_cgroup_link {
static struct css_set init_css_set;
static struct cg_cgroup_link init_css_set_link;
-static int cgroup_init_idr(struct cgroup_subsys *ss,
- struct cgroup_subsys_state *css);
-
/* css_set_lock protects the list of css_set objects, and the
* chain of tasks off each css_set. Nests outside task->alloc_lock
* due to cgroup_iter_start() */
@@ -843,9 +808,6 @@ static struct backing_dev_info cgroup_backing_dev_info = {
.capabilities = BDI_CAP_NO_ACCT_AND_WRITEBACK,
};
-static int alloc_css_id(struct cgroup_subsys *ss,
- struct cgroup *parent, struct cgroup *child);
-
static struct inode *cgroup_new_inode(umode_t mode, struct super_block *sb)
{
struct inode *inode = new_inode(sb);
@@ -4002,18 +3964,6 @@ static int cgroup_populate_dir(struct cgroup *cgrp, bool base_files,
cgroup_addrm_files(cgrp, ss, set->cfts, true);
}
- /* This cgroup is ready now */
- for_each_subsys(cgrp->root, ss) {
- struct cgroup_subsys_state *css = cgrp->subsys[ss->subsys_id];
- /*
- * Update id->css pointer and make this css visible from
- * CSS ID functions. This pointer will be dereferened
- * from RCU-read-side without locks.
- */
- if (css->id)
- rcu_assign_pointer(css->id->css, css);
- }
-
return 0;
}
@@ -4158,11 +4108,6 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry,
goto err_free_all;
}
init_cgroup_css(css, ss, cgrp);
- if (ss->use_id) {
- err = alloc_css_id(ss, parent, cgrp);
- if (err)
- goto err_free_all;
- }
}
/*
@@ -4448,12 +4393,6 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss)
/* our new subsystem will be attached to the dummy hierarchy. */
init_cgroup_css(css, ss, dummytop);
- /* init_idr must be after init_cgroup_css because it sets css->id. */
- if (ss->use_id) {
- ret = cgroup_init_idr(ss, css);
- if (ret)
- goto err_unload;
- }
/*
* Now we need to entangle the css into the existing css_sets. unlike
@@ -4521,9 +4460,6 @@ void cgroup_unload_subsys(struct cgroup_subsys *ss)
offline_css(ss, dummytop);
ss->active = 0;
- if (ss->use_id)
- idr_destroy(&ss->idr);
-
/* deassign the subsys_id */
subsys[ss->subsys_id] = NULL;
@@ -4631,8 +4567,6 @@ int __init cgroup_init(void)
continue;
if (!ss->early_init)
cgroup_init_subsys(ss);
- if (ss->use_id)
- cgroup_init_idr(ss, init_css_set.subsys[ss->subsys_id]);
}
/* Add init_css_set to the hash table */
@@ -5092,198 +5026,6 @@ static int __init cgroup_disable(char *str)
__setup("cgroup_disable=", cgroup_disable);
/*
- * Functons for CSS ID.
- */
-
-/*
- *To get ID other than 0, this should be called when !cgroup_is_removed().
- */
-unsigned short css_id(struct cgroup_subsys_state *css)
-{
- struct css_id *cssid;
-
- /*
- * This css_id() can return correct value when somone has refcnt
- * on this or this is under rcu_read_lock(). Once css->id is allocated,
- * it's unchanged until freed.
- */
- cssid = rcu_dereference_check(css->id, css_refcnt(css));
-
- if (cssid)
- return cssid->id;
- return 0;
-}
-EXPORT_SYMBOL_GPL(css_id);
-
-unsigned short css_depth(struct cgroup_subsys_state *css)
-{
- struct css_id *cssid;
-
- cssid = rcu_dereference_check(css->id, css_refcnt(css));
-
- if (cssid)
- return cssid->depth;
- return 0;
-}
-EXPORT_SYMBOL_GPL(css_depth);
-
-/**
- * css_is_ancestor - test "root" css is an ancestor of "child"
- * @child: the css to be tested.
- * @root: the css supporsed to be an ancestor of the child.
- *
- * Returns true if "root" is an ancestor of "child" in its hierarchy. Because
- * this function reads css->id, the caller must hold rcu_read_lock().
- * But, considering usual usage, the csses should be valid objects after test.
- * Assuming that the caller will do some action to the child if this returns
- * returns true, the caller must take "child";s reference count.
- * If "child" is valid object and this returns true, "root" is valid, too.
- */
-
-bool css_is_ancestor(struct cgroup_subsys_state *child,
- const struct cgroup_subsys_state *root)
-{
- struct css_id *child_id;
- struct css_id *root_id;
-
- child_id = rcu_dereference(child->id);
- if (!child_id)
- return false;
- root_id = rcu_dereference(root->id);
- if (!root_id)
- return false;
- if (child_id->depth < root_id->depth)
- return false;
- if (child_id->stack[root_id->depth] != root_id->id)
- return false;
- return true;
-}
-
-void free_css_id(struct cgroup_subsys *ss, struct cgroup_subsys_state *css)
-{
- struct css_id *id = css->id;
- /* When this is called before css_id initialization, id can be NULL */
- if (!id)
- return;
-
- BUG_ON(!ss->use_id);
-
- rcu_assign_pointer(id->css, NULL);
- rcu_assign_pointer(css->id, NULL);
- spin_lock(&ss->id_lock);
- idr_remove(&ss->idr, id->id);
- spin_unlock(&ss->id_lock);
- kfree_rcu(id, rcu_head);
-}
-EXPORT_SYMBOL_GPL(free_css_id);
-
-/*
- * This is called by init or create(). Then, calls to this function are
- * always serialized (By cgroup_mutex() at create()).
- */
-
-static struct css_id *get_new_cssid(struct cgroup_subsys *ss, int depth)
-{
- struct css_id *newid;
- int ret, size;
-
- BUG_ON(!ss->use_id);
-
- size = sizeof(*newid) + sizeof(unsigned short) * (depth + 1);
- newid = kzalloc(size, GFP_KERNEL);
- if (!newid)
- return ERR_PTR(-ENOMEM);
-
- idr_preload(GFP_KERNEL);
- spin_lock(&ss->id_lock);
- /* Don't use 0. allocates an ID of 1-65535 */
- ret = idr_alloc(&ss->idr, newid, 1, CSS_ID_MAX + 1, GFP_NOWAIT);
- spin_unlock(&ss->id_lock);
- idr_preload_end();
-
- /* Returns error when there are no free spaces for new ID.*/
- if (ret < 0)
- goto err_out;
-
- newid->id = ret;
- newid->depth = depth;
- return newid;
-err_out:
- kfree(newid);
- return ERR_PTR(ret);
-
-}
-
-static int __init_or_module cgroup_init_idr(struct cgroup_subsys *ss,
- struct cgroup_subsys_state *rootcss)
-{
- struct css_id *newid;
-
- spin_lock_init(&ss->id_lock);
- idr_init(&ss->idr);
-
- newid = get_new_cssid(ss, 0);
- if (IS_ERR(newid))
- return PTR_ERR(newid);
-
- newid->stack[0] = newid->id;
- newid->css = rootcss;
- rootcss->id = newid;
- return 0;
-}
-
-static int alloc_css_id(struct cgroup_subsys *ss, struct cgroup *parent,
- struct cgroup *child)
-{
- int subsys_id, i, depth = 0;
- struct cgroup_subsys_state *parent_css, *child_css;
- struct css_id *child_id, *parent_id;
-
- subsys_id = ss->subsys_id;
- parent_css = parent->subsys[subsys_id];
- child_css = child->subsys[subsys_id];
- parent_id = parent_css->id;
- depth = parent_id->depth + 1;
-
- child_id = get_new_cssid(ss, depth);
- if (IS_ERR(child_id))
- return PTR_ERR(child_id);
-
- for (i = 0; i < depth; i++)
- child_id->stack[i] = parent_id->stack[i];
- child_id->stack[depth] = child_id->id;
- /*
- * child_id->css pointer will be set after this cgroup is available
- * see cgroup_populate_dir()
- */
- rcu_assign_pointer(child_css->id, child_id);
-
- return 0;
-}
-
-/**
- * css_lookup - lookup css by id
- * @ss: cgroup subsys to be looked into.
- * @id: the id
- *
- * Returns pointer to cgroup_subsys_state if there is valid one with id.
- * NULL if not. Should be called under rcu_read_lock()
- */
-struct cgroup_subsys_state *css_lookup(struct cgroup_subsys *ss, int id)
-{
- struct css_id *cssid = NULL;
-
- BUG_ON(!ss->use_id);
- cssid = idr_find(&ss->idr, id);
-
- if (unlikely(!cssid))
- return NULL;
-
- return rcu_dereference(cssid->css);
-}
-EXPORT_SYMBOL_GPL(css_lookup);
-
-/*
* get corresponding css from file open on cgroupfs directory
*/
struct cgroup_subsys_state *cgroup_css_from_dir(struct file *f, int id)
--
1.8.0.2
On Mon 08-04-13 16:19:53, Li Zefan wrote:
[...]
> 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!
--
Michal Hocko
SUSE Labs
On Mon 08-04-13 16:20:11, Li Zefan wrote:
[...]
> @@ -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;
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:
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index d195f40..37bbbff 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -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.
> +
> + 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)
> {
> --
> 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
--
Michal Hocko
SUSE Labs
On Mon 08-04-13 16:21:29, Li Zefan wrote:
> 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.
Acked-by: Michal Hocko <[email protected]>
> ---
> mm/memcontrol.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 14f1375..3561d0b 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -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)
> --
> 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/
--
Michal Hocko
SUSE Labs
On Mon 08-04-13 16:22:11, Li Zefan wrote:
> 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.
Acked-by: Michal Hocko <[email protected]>
> ---
> mm/memcontrol.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 3561d0b..c4e0173 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -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;
> --
> 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
--
Michal Hocko
SUSE Labs
On Mon 08-04-13 16:22:34, Li Zefan wrote:
> 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.
Acked-by: Michal Hocko <[email protected]>
Minor nit bellow.
> ---
> mm/memcontrol.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index c4e0173..947dff1 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -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 ?
> +
> static inline unsigned short mem_cgroup_id(struct mem_cgroup *memcg)
> {
> return memcg->css.cgroup->id;
--
Michal Hocko
SUSE Labs
On Mon 08-04-13 16:23:16, Li Zefan wrote:
> Now memcg uses cgroup->id instead of css_id. Update some comments and
> set mem_cgroup_subsys->use_id to 0.
>
> Signed-off-by: Li Zefan <[email protected]>
Acked-by: Michal Hocko <[email protected]>
> ---
> mm/memcontrol.c | 21 +++++++--------------
> 1 file changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 947dff1..26ee672 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -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
> --
> 1.8.0.2
>
--
Michal Hocko
SUSE Labs
Hello, Li.
On Mon, Apr 08, 2013 at 04:20:11PM +0800, Li Zefan wrote:
> +/**
> + * 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.)
> + * Returns true if "root" is an ancestor of "child" in its hierarchy.
> + */
> +bool cgroup_is_ancestor(struct cgroup *child, struct cgroup *root)
s/root/ancestor/
> +{
> + 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.
--
tejun
On Mon, Apr 08, 2013 at 04:20:59PM +0800, Li Zefan wrote:
> +/**
> + * 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(), ...);
> + return idr_find(&ss->root->cgroup_idr, id);
Thanks.
--
tejun
Oops, one more thing.
On Mon, Apr 08, 2013 at 04:20:59PM +0800, Li Zefan wrote:
> - 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.
--
tejun
On Mon, Apr 08, 2013 at 04:23:42PM +0800, Li Zefan wrote:
> The only user of css_id was memcg, and it has been converted to
> use cgroup->id, so kill css_id.
>
> Signed-off-by: Li Zefan <[email protected]>
Violently-acked-by: Tejun Heo <[email protected]>
Thanks a lot for killing this abomination.
--
tejun
Hello,
On Mon, Apr 08, 2013 at 04:47:50PM +0200, Michal Hocko wrote:
> On Mon 08-04-13 16:20:11, Li Zefan wrote:
> [...]
> > @@ -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;
>
> 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:
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.
--
tejun
On Mon 08-04-13 08:57:26, Tejun Heo wrote:
> Hello,
>
> On Mon, Apr 08, 2013 at 04:47:50PM +0200, Michal Hocko wrote:
> > On Mon 08-04-13 16:20:11, Li Zefan wrote:
> > [...]
> > > @@ -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;
> >
> > 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:
>
> It's a basic hierarchy operation.
Which nobody seem to need ATM so it could be added later when an user
emerges.
> 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.
> It doesn't seem to end well.
>
> Thanks.
>
> --
> 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
--
Michal Hocko
SUSE Labs
On Mon 08-04-13 16:47:50, Michal Hocko wrote:
> On Mon 08-04-13 16:20:11, Li Zefan wrote:
> [...]
> > @@ -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;
>
> 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:
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).
Signed-off-by: Michal Hocko <[email protected]>
---
mm/memcontrol.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index f608546..177bec2 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -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,
--
1.7.10.4
--
Michal Hocko
SUSE Labs
On Mon, Apr 08, 2013 at 08:03:44PM +0200, Michal Hocko wrote:
> __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).
Huh? Isn't that avoided by the !root_memcg->use_hierarchy test?
> @@ -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,
> --
> 1.7.10.4
>
> --
> Michal Hocko
> SUSE Labs
--
tejun
On 2013/4/8 22:53, Michal Hocko wrote:
> On Mon 08-04-13 16:21:29, Li Zefan wrote:
>> 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.
>
yeah it's not a big deal, just want to separate changes to cgroup and memcg.
> Acked-by: Michal Hocko <[email protected]>
>
>> ---
>> mm/memcontrol.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 14f1375..3561d0b 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -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)
>> --
>> 1.8.0.2
On 2013/4/8 22:57, Michal Hocko wrote:
> On Mon 08-04-13 16:22:11, Li Zefan wrote:
>> 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.
> The patch on its own looks good.
>
> Acked-by: Michal Hocko <[email protected]>
On 2013/4/8 23:43, Tejun Heo wrote:
> On Mon, Apr 08, 2013 at 04:20:59PM +0800, Li Zefan wrote:
>> +/**
>> + * 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
>
>> + return idr_find(&ss->root->cgroup_idr, id);
(2013/04/08 17:20), Li Zefan wrote:
> This will be used as a replacement for css_is_ancestor().
>
> Signed-off-by: Li Zefan <[email protected]>
Acked-by: KAMEZAWA Hiroyuki <[email protected]>
Hmm....but do we need "depth" ?
On 2013/4/9 11:21, Kamezawa Hiroyuki wrote:
> (2013/04/08 17:20), Li Zefan wrote:
>> This will be used as a replacement for css_is_ancestor().
>>
>> Signed-off-by: Li Zefan <[email protected]>
>
> Acked-by: KAMEZAWA Hiroyuki <[email protected]>
>
> Hmm....but do we need "depth" ?
>
which was removed in Tejun's
"[PATCHSET] perf, cgroup: implement hierarchy support for perf_event controller"
(2013/04/08 17:20), Li Zefan wrote:
> 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(-)
>
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 96072e4..6ae8ae1 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -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 */
>
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index e87872c..5ae1e87 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -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);
> +}
> +
How about checking cgroup_populated_dir() have been called once ?
If not, returning NULL.
Thanks,
-Kame
(2013/04/08 17:21), Li Zefan wrote:
> 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(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 5aa6e91..14f1375 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -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,
>
Acked-by: KAMEZAWA Hiroyuki <[email protected]>
(2013/04/08 17:21), Li Zefan wrote:
> 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(-)
Acked-by: KAMEZAWA Hiroyoku <[email protected]>
(2013/04/08 17:22), Li Zefan wrote:
> This is a preparation to kill css_id.
>
> Signed-off-by: Li Zefan <[email protected]>
Acked-by: KAMEZAWA Hiroyuki <[email protected]>
(2013/04/08 17:20), Li Zefan wrote:
> 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(-)
>
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 96072e4..6ae8ae1 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -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 */
>
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index e87872c..5ae1e87 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -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;
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
(2013/04/08 17:23), Li Zefan wrote:
> Now memcg uses cgroup->id instead of css_id. Update some comments and
> set mem_cgroup_subsys->use_id to 0.
>
> Signed-off-by: Li Zefan <[email protected]>
Acked-by: KAMEZAWA Hiroyuki <[email protected]>
On Mon 08-04-13 14:36:46, Tejun Heo wrote:
> On Mon, Apr 08, 2013 at 08:03:44PM +0200, Michal Hocko wrote:
> > __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).
>
> 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.
> > @@ -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;
> > }
> >
--
Michal Hocko
SUSE Labs
On Tue 09-04-13 11:02:20, Li Zefan wrote:
> On 2013/4/8 22:57, Michal Hocko wrote:
> > On Mon 08-04-13 16:22:11, Li Zefan wrote:
> >> 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.
Right you are.
Thanks
--
Michal Hocko
SUSE Labs
On Mon 08-04-13 16:21:14, Li Zefan wrote:
> This is a preparation to kill css_id.
>
> Signed-off-by: Li Zefan <[email protected]>
Acked-by: Michal Hocko <[email protected]>
> ---
> mm/memcontrol.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 5aa6e91..14f1375 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -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,
> --
> 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
--
Michal Hocko
SUSE Labs