[PATCHSET] cgroup: implement task_cgroup_path_from_hierarchy()

[PATCHSET] cgroup: implement task_cgroup_path_from_hierarchy()

[Tejun Heo]

kdbus folks want a sane way to determine the cgroup path that a given
task belongs to on a given hierarchy, which is a reasonble thing to
expect from cgroup core.

This patchset make hierarchy_id allocation use idr instead of ida and
implement task_cgroup_path_from_hierarchy().  In the process, the
yucky ida cyclic allocation is replaced with idr_alloc_cyclic().

 0001-cgroup-refactor-hierarchy_id-handling.patch
 0002-cgroup-drop-hierarchy_id_lock.patch
 0003-cgroup-make-hierarchy_id-use-cyclic-idr.patch
 0004-cgroup-implement-task_cgroup_path_from_hierarchy.patch

0001-0002 prepare for conversion to idr, which 0003 does.

0004 implements the new function.

This patchset is on top of next-20130412 as idr_alloc_cyclic() patch
is currently in -mm.  Given that this isn't an urgent thing and the
merge window is just around the corner, it'd be probably best to route
these through cgroup/for-3.11 once v3.10-rc1 drops.

These patches are also available in the following git branch.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git review-task_cgroup_path_from_hierarchy

And it actually reduces LOC.  Woot Woot.

 include/linux/cgroup.h |    2
 kernel/cgroup.c        |  128 +++++++++++++++++++++++++++++++++----------------
 2 files changed, 89 insertions(+), 41 deletions(-)

--
tejun

[PATCH 1/4] cgroup: refactor hierarchy_id handling

[Tejun Heo]

We're planning to converting hierarchy_ida to an idr and use it to
look up hierarchy from its id.  As we want the mapping to happen
atomically with cgroupfs_root registration, this patch refactors
hierarchy_id init / exit so that ida operations happen inside
cgroup_[root_]mutex.

* s/init_root_id()/cgroup_init_root_id()/ and make it return 0 or
  -errno like a normal function.

* Move hierarchy_id initialization from cgroup_root_from_opts() into
  cgroup_mount() block where the root is confirmed to be used and
  being registered while holding both mutexes.

* Split cgroup_drop_id() into cgroup_exit_root_id() and
  cgroup_free_root(), so that ID release can happen before dropping
  the mutexes in cgroup_kill_sb().  The latter expects hierarchy_id to
  be exited before being invoked.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 kernel/cgroup.c | 56 +++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 21 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index a790409..823cb56 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1452,13 +1452,13 @@ static void init_cgroup_root(struct cgroupfs_root *root)
 	list_add_tail(&cgrp->allcg_node, &root->allcg_list);
 }
 
-static bool init_root_id(struct cgroupfs_root *root)
+static int cgroup_init_root_id(struct cgroupfs_root *root)
 {
-	int ret = 0;
+	int ret;
 
 	do {
 		if (!ida_pre_get(&hierarchy_ida, GFP_KERNEL))
-			return false;
+			return -ENOMEM;
 		spin_lock(&hierarchy_id_lock);
 		/* Try to allocate the next unused ID */
 		ret = ida_get_new_above(&hierarchy_ida, next_hierarchy_id,
@@ -1474,7 +1474,18 @@ static bool init_root_id(struct cgroupfs_root *root)
 		}
 		spin_unlock(&hierarchy_id_lock);
 	} while (ret);
-	return true;
+	return 0;
+}
+
+static void cgroup_exit_root_id(struct cgroupfs_root *root)
+{
+	if (root->hierarchy_id) {
+		spin_lock(&hierarchy_id_lock);
+		ida_remove(&hierarchy_ida, root->hierarchy_id);
+		spin_unlock(&hierarchy_id_lock);
+
+		root->hierarchy_id = 0;
+	}
 }
 
 static int cgroup_test_super(struct super_block *sb, void *data)
@@ -1508,10 +1519,6 @@ static struct cgroupfs_root *cgroup_root_from_opts(struct cgroup_sb_opts *opts)
 	if (!root)
 		return ERR_PTR(-ENOMEM);
 
-	if (!init_root_id(root)) {
-		kfree(root);
-		return ERR_PTR(-ENOMEM);
-	}
 	init_cgroup_root(root);
 
 	root->subsys_mask = opts->subsys_mask;
@@ -1526,17 +1533,15 @@ static struct cgroupfs_root *cgroup_root_from_opts(struct cgroup_sb_opts *opts)
 	return root;
 }
 
-static void cgroup_drop_root(struct cgroupfs_root *root)
+static void cgroup_free_root(struct cgroupfs_root *root)
 {
-	if (!root)
-		return;
+	if (root) {
+		/* hierarhcy ID shoulid already have been released */
+		WARN_ON_ONCE(root->hierarchy_id);
 
-	BUG_ON(!root->hierarchy_id);
-	spin_lock(&hierarchy_id_lock);
-	ida_remove(&hierarchy_ida, root->hierarchy_id);
-	spin_unlock(&hierarchy_id_lock);
-	ida_destroy(&root->cgroup_ida);
-	kfree(root);
+		ida_destroy(&root->cgroup_ida);
+		kfree(root);
+	}
 }
 
 static int cgroup_set_super(struct super_block *sb, void *data)
@@ -1623,7 +1628,7 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,
 	sb = sget(fs_type, cgroup_test_super, cgroup_set_super, 0, &opts);
 	if (IS_ERR(sb)) {
 		ret = PTR_ERR(sb);
-		cgroup_drop_root(opts.new_root);
+		cgroup_free_root(opts.new_root);
 		goto drop_modules;
 	}
 
@@ -1667,6 +1672,10 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,
 		if (ret)
 			goto unlock_drop;
 
+		ret = cgroup_init_root_id(root);
+		if (ret)
+			goto unlock_drop;
+
 		ret = rebind_subsystems(root, root->subsys_mask);
 		if (ret == -EBUSY) {
 			free_cg_links(&tmp_cg_links);
@@ -1710,7 +1719,7 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,
 		 * We re-used an existing hierarchy - the new root (if
 		 * any) is not needed
 		 */
-		cgroup_drop_root(opts.new_root);
+		cgroup_free_root(opts.new_root);
 		/* no subsys rebinding, so refcounts don't change */
 		drop_parsed_module_refcounts(opts.subsys_mask);
 	}
@@ -1720,6 +1729,7 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,
 	return dget(sb->s_root);
 
  unlock_drop:
+	cgroup_exit_root_id(root);
 	mutex_unlock(&cgroup_root_mutex);
 	mutex_unlock(&cgroup_mutex);
 	mutex_unlock(&inode->i_mutex);
@@ -1772,13 +1782,15 @@ static void cgroup_kill_sb(struct super_block *sb) {
 		root_count--;
 	}
 
+	cgroup_exit_root_id(root);
+
 	mutex_unlock(&cgroup_root_mutex);
 	mutex_unlock(&cgroup_mutex);
 
 	simple_xattrs_free(&cgrp->xattrs);
 
 	kill_litter_super(sb);
-	cgroup_drop_root(root);
+	cgroup_free_root(root);
 }
 
 static struct file_system_type cgroup_fs_type = {
@@ -4642,7 +4654,9 @@ int __init cgroup_init(void)
 	/* Add init_css_set to the hash table */
 	key = css_set_hash(init_css_set.subsys);
 	hash_add(css_set_table, &init_css_set.hlist, key);
-	BUG_ON(!init_root_id(&rootnode));
+
+	/* allocate id for the dummy hierarchy */
+	BUG_ON(cgroup_init_root_id(&rootnode));
 
 	cgroup_kobj = kobject_create_and_add("cgroup", fs_kobj);
 	if (!cgroup_kobj) {
-- 
1.8.1.4

[PATCH 2/4] cgroup: drop hierarchy_id_lock

[Tejun Heo]

Now that hierarchy_id alloc / free are protected by the cgroup
mutexes, there's no need for this separate lock.  Drop it.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 kernel/cgroup.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 823cb56..e15bdb7 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -237,9 +237,13 @@ struct cgroup_event {
 static LIST_HEAD(roots);
 static int root_count;
 
+/*
+ * Hierarchy ID allocation and mapping.  It follows the same exclusion
+ * rules as other root ops - both cgroup_mutex and cgroup_root_mutex for
+ * writes, either for reads.
+ */
 static DEFINE_IDA(hierarchy_ida);
 static int next_hierarchy_id;
-static DEFINE_SPINLOCK(hierarchy_id_lock);
 
 /* dummytop is a shorthand for the dummy hierarchy's top cgroup */
 #define dummytop (&rootnode.top_cgroup)
@@ -1456,10 +1460,12 @@ static int cgroup_init_root_id(struct cgroupfs_root *root)
 {
 	int ret;
 
+	lockdep_assert_held(&cgroup_mutex);
+	lockdep_assert_held(&cgroup_root_mutex);
+
 	do {
 		if (!ida_pre_get(&hierarchy_ida, GFP_KERNEL))
 			return -ENOMEM;
-		spin_lock(&hierarchy_id_lock);
 		/* Try to allocate the next unused ID */
 		ret = ida_get_new_above(&hierarchy_ida, next_hierarchy_id,
 					&root->hierarchy_id);
@@ -1472,18 +1478,17 @@ static int cgroup_init_root_id(struct cgroupfs_root *root)
 			/* Can only get here if the 31-bit IDR is full ... */
 			BUG_ON(ret);
 		}
-		spin_unlock(&hierarchy_id_lock);
 	} while (ret);
 	return 0;
 }
 
 static void cgroup_exit_root_id(struct cgroupfs_root *root)
 {
+	lockdep_assert_held(&cgroup_mutex);
+	lockdep_assert_held(&cgroup_root_mutex);
+
 	if (root->hierarchy_id) {
-		spin_lock(&hierarchy_id_lock);
 		ida_remove(&hierarchy_ida, root->hierarchy_id);
-		spin_unlock(&hierarchy_id_lock);
-
 		root->hierarchy_id = 0;
 	}
 }
@@ -4656,8 +4661,14 @@ int __init cgroup_init(void)
 	hash_add(css_set_table, &init_css_set.hlist, key);
 
 	/* allocate id for the dummy hierarchy */
+	mutex_lock(&cgroup_mutex);
+	mutex_lock(&cgroup_root_mutex);
+
 	BUG_ON(cgroup_init_root_id(&rootnode));
 
+	mutex_unlock(&cgroup_root_mutex);
+	mutex_unlock(&cgroup_mutex);
+
 	cgroup_kobj = kobject_create_and_add("cgroup", fs_kobj);
 	if (!cgroup_kobj) {
 		err = -ENOMEM;
-- 
1.8.1.4

[PATCH 3/4] cgroup: make hierarchy_id use cyclic idr

[Tejun Heo]

We want to be able to lookup a hierarchy from its id and cyclic
allocation is a whole lot simpler with idr.  Convert to idr and use
idr_alloc_cyclc().

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 kernel/cgroup.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index e15bdb7..75d85e8 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -242,8 +242,7 @@ static int root_count;
  * rules as other root ops - both cgroup_mutex and cgroup_root_mutex for
  * writes, either for reads.
  */
-static DEFINE_IDA(hierarchy_ida);
-static int next_hierarchy_id;
+static DEFINE_IDR(cgroup_hierarchy_idr);
 
 /* dummytop is a shorthand for the dummy hierarchy's top cgroup */
 #define dummytop (&rootnode.top_cgroup)
@@ -1458,27 +1457,16 @@ static void init_cgroup_root(struct cgroupfs_root *root)
 
 static int cgroup_init_root_id(struct cgroupfs_root *root)
 {
-	int ret;
+	int id;
 
 	lockdep_assert_held(&cgroup_mutex);
 	lockdep_assert_held(&cgroup_root_mutex);
 
-	do {
-		if (!ida_pre_get(&hierarchy_ida, GFP_KERNEL))
-			return -ENOMEM;
-		/* Try to allocate the next unused ID */
-		ret = ida_get_new_above(&hierarchy_ida, next_hierarchy_id,
-					&root->hierarchy_id);
-		if (ret == -ENOSPC)
-			/* Try again starting from 0 */
-			ret = ida_get_new(&hierarchy_ida, &root->hierarchy_id);
-		if (!ret) {
-			next_hierarchy_id = root->hierarchy_id + 1;
-		} else if (ret != -EAGAIN) {
-			/* Can only get here if the 31-bit IDR is full ... */
-			BUG_ON(ret);
-		}
-	} while (ret);
+	id = idr_alloc_cyclic(&cgroup_hierarchy_idr, root, 2, 0, GFP_KERNEL);
+	if (id < 0)
+		return id;
+
+	root->hierarchy_id = id;
 	return 0;
 }
 
@@ -1488,7 +1476,7 @@ static void cgroup_exit_root_id(struct cgroupfs_root *root)
 	lockdep_assert_held(&cgroup_root_mutex);
 
 	if (root->hierarchy_id) {
-		ida_remove(&hierarchy_ida, root->hierarchy_id);
+		idr_remove(&cgroup_hierarchy_idr, root->hierarchy_id);
 		root->hierarchy_id = 0;
 	}
 }
-- 
1.8.1.4

[PATCH 4/4] cgroup: implement task_cgroup_path_from_hierarchy()

[Tejun Heo]

kdbus folks want a sane way to determine the cgroup path that a given
task belongs to on a given hierarchy, which is a reasonble thing to
expect from cgroup core.

Implement task_cgroup_path_from_hierarchy().

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Kay Sievers <kay@vrfy.org>
Cc: Greg Kroah-Hartman <greg@kroah.com>
Cc: Lennart Poettering <lennart@poettering.net>
Cc: Daniel Mack <daniel@zonque.org>
---
 include/linux/cgroup.h |  2 ++
 kernel/cgroup.c        | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 17ed818..ee83af2 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -443,6 +443,8 @@ int cgroup_is_removed(const struct cgroup *cgrp);
 bool cgroup_is_descendant(struct cgroup *cgrp, struct cgroup *ancestor);
 
 int cgroup_path(const struct cgroup *cgrp, char *buf, int buflen);
+int task_cgroup_path_from_hierarchy(struct task_struct *task, int hierarchy_id,
+				    char *buf, size_t buflen);
 
 int cgroup_task_count(const struct cgroup *cgrp);
 
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 75d85e8..5184fcd 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1842,6 +1842,39 @@ out:
 }
 EXPORT_SYMBOL_GPL(cgroup_path);
 
+/**
+ * task_cgroup_path_from_hierarchy - cgroup path of a task on a hierarchy
+ * @task: target task
+ * @hierarchy_id: the hierarchy to look up @task's cgroup from
+ * @buf: the buffer to write the path into
+ * @buflen: the length of the buffer
+ *
+ * Determine @task's cgroup on the hierarchy specified by @hierarchy_id and
+ * copy its path into @buf.  This function grabs cgroup_mutex and shouldn't
+ * be used inside locks used by cgroup controller callbacks.
+ */
+int task_cgroup_path_from_hierarchy(struct task_struct *task, int hierarchy_id,
+				    char *buf, size_t buflen)
+{
+	struct cgroupfs_root *root;
+	struct cgroup *cgrp = NULL;
+	int ret = -ENOENT;
+
+	mutex_lock(&cgroup_mutex);
+
+	root = idr_find(&cgroup_hierarchy_idr, hierarchy_id);
+	if (root) {
+		cgrp = task_cgroup_from_root(task, root);
+		if (cgrp)
+			ret = cgroup_path(cgrp, buf, buflen);
+	}
+
+	mutex_unlock(&cgroup_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(task_cgroup_path_from_hierarchy);
+
 /*
  * Control Group taskset
  */
-- 
1.8.1.4

Re: [PATCH 4/4] cgroup: implement task_cgroup_path_from_hierarchy()

[Greg KH]

On Sun, Apr 14, 2013 at 11:36:59AM -0700, Tejun Heo wrote:
> kdbus folks want a sane way to determine the cgroup path that a given
> task belongs to on a given hierarchy, which is a reasonble thing to
> expect from cgroup core.
> 
> Implement task_cgroup_path_from_hierarchy().
> 
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Kay Sievers <kay-tD+1rO4QERM@public.gmane.org>
> Cc: Greg Kroah-Hartman <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
> Cc: Lennart Poettering <lennart-mdGvqq1h2p+GdvJs77BJ7Q@public.gmane.org>
> Cc: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>

Thanks so much for doing this.

Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>

Re: [PATCHSET] cgroup: implement task_cgroup_path_from_hierarchy()

[Tejun Heo]

On Sun, Apr 14, 2013 at 11:36:55AM -0700, Tejun Heo wrote:
> kdbus folks want a sane way to determine the cgroup path that a given
> task belongs to on a given hierarchy, which is a reasonble thing to
> expect from cgroup core.
> 
> This patchset make hierarchy_id allocation use idr instead of ida and
> implement task_cgroup_path_from_hierarchy().  In the process, the
> yucky ida cyclic allocation is replaced with idr_alloc_cyclic().
> 
>  0001-cgroup-refactor-hierarchy_id-handling.patch
>  0002-cgroup-drop-hierarchy_id_lock.patch
>  0003-cgroup-make-hierarchy_id-use-cyclic-idr.patch
>  0004-cgroup-implement-task_cgroup_path_from_hierarchy.patch
> 
> 0001-0002 prepare for conversion to idr, which 0003 does.
> 
> 0004 implements the new function.
> 
> This patchset is on top of next-20130412 as idr_alloc_cyclic() patch
> is currently in -mm.  Given that this isn't an urgent thing and the
> merge window is just around the corner, it'd be probably best to route
> these through cgroup/for-3.11 once v3.10-rc1 drops.
> 
> These patches are also available in the following git branch.
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git review-task_cgroup_path_from_hierarchy
> 
> And it actually reduces LOC.  Woot Woot.
> 
>  include/linux/cgroup.h |    2
>  kernel/cgroup.c        |  128 +++++++++++++++++++++++++++++++++----------------
>  2 files changed, 89 insertions(+), 41 deletions(-)

Heh, I must have been tripping or something.  89 > 41.  It's still a
lot cleaner tho. :)

-- 
tejun

Re: [PATCH 4/4] cgroup: implement task_cgroup_path_from_hierarchy()

[Li Zefan]

> +int task_cgroup_path_from_hierarchy(struct task_struct *task, int hierarchy_id,
> +				    char *buf, size_t buflen)
> +{
> +	struct cgroupfs_root *root;
> +	struct cgroup *cgrp = NULL;
> +	int ret = -ENOENT;
> +
> +	mutex_lock(&cgroup_mutex);
> +
> +	root = idr_find(&cgroup_hierarchy_idr, hierarchy_id);
> +	if (root) {
> +		cgrp = task_cgroup_from_root(task, root);

task_cgroup_from_root() will never return NULL, and there's a BUG_ON(!res) in it.

> +		if (cgrp)
> +			ret = cgroup_path(cgrp, buf, buflen);
> +	}
> +
> +	mutex_unlock(&cgroup_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(task_cgroup_path_from_hierarchy);

Re: [PATCHSET] cgroup: implement task_cgroup_path_from_hierarchy()

[Li Zefan]

On 2013/4/15 2:36, Tejun Heo wrote:
> kdbus folks want a sane way to determine the cgroup path that a given
> task belongs to on a given hierarchy, which is a reasonble thing to
> expect from cgroup core.
> 
> This patchset make hierarchy_id allocation use idr instead of ida and
> implement task_cgroup_path_from_hierarchy().  In the process, the
> yucky ida cyclic allocation is replaced with idr_alloc_cyclic().
> 
>  0001-cgroup-refactor-hierarchy_id-handling.patch
>  0002-cgroup-drop-hierarchy_id_lock.patch
>  0003-cgroup-make-hierarchy_id-use-cyclic-idr.patch
>  0004-cgroup-implement-task_cgroup_path_from_hierarchy.patch
> 
> 0001-0002 prepare for conversion to idr, which 0003 does.
> 
> 0004 implements the new function.
> 

Acked-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Re: [PATCH 4/4] cgroup: implement task_cgroup_path_from_hierarchy()

[Tejun Heo]

On Mon, Apr 15, 2013 at 11:43:15AM +0800, Li Zefan wrote:
> > +int task_cgroup_path_from_hierarchy(struct task_struct *task, int hierarchy_id,
> > +				    char *buf, size_t buflen)
> > +{
> > +	struct cgroupfs_root *root;
> > +	struct cgroup *cgrp = NULL;
> > +	int ret = -ENOENT;
> > +
> > +	mutex_lock(&cgroup_mutex);
> > +
> > +	root = idr_find(&cgroup_hierarchy_idr, hierarchy_id);
> > +	if (root) {
> > +		cgrp = task_cgroup_from_root(task, root);
> 
> task_cgroup_from_root() will never return NULL, and there's a BUG_ON(!res) in it.

@hierarchy_id may come from userland, so we probably should update
task_cgroup_from_root() to return NULL if the id is invalid.  Will add
a patch.

Thanks.

-- 
tejun

Re: [PATCH 4/4] cgroup: implement task_cgroup_path_from_hierarchy()

[Tejun Heo]

On Sun, Apr 14, 2013 at 08:46:13PM -0700, Tejun Heo wrote:
> On Mon, Apr 15, 2013 at 11:43:15AM +0800, Li Zefan wrote:
> > > +int task_cgroup_path_from_hierarchy(struct task_struct *task, int hierarchy_id,
> > > +				    char *buf, size_t buflen)
> > > +{
> > > +	struct cgroupfs_root *root;
> > > +	struct cgroup *cgrp = NULL;
> > > +	int ret = -ENOENT;
> > > +
> > > +	mutex_lock(&cgroup_mutex);
> > > +
> > > +	root = idr_find(&cgroup_hierarchy_idr, hierarchy_id);
> > > +	if (root) {
> > > +		cgrp = task_cgroup_from_root(task, root);
> > 
> > task_cgroup_from_root() will never return NULL, and there's a BUG_ON(!res) in it.
> 
> @hierarchy_id may come from userland, so we probably should update
> task_cgroup_from_root() to return NULL if the id is invalid.  Will add
> a patch.

For some reason, I thought you were talking about hierarchy lookup.
Yeap, task_cgroup_from_root() can't be NULL for a valid hierarchy.
Will drop the if.

-- 
tejun

Re: [PATCH 4/4] cgroup: implement task_cgroup_path_from_hierarchy()

[Li Zefan]

On 2013/4/15 11:46, Tejun Heo wrote:
> On Mon, Apr 15, 2013 at 11:43:15AM +0800, Li Zefan wrote:
>>> +int task_cgroup_path_from_hierarchy(struct task_struct *task, int hierarchy_id,
>>> +				    char *buf, size_t buflen)
>>> +{
>>> +	struct cgroupfs_root *root;
>>> +	struct cgroup *cgrp = NULL;
>>> +	int ret = -ENOENT;
>>> +
>>> +	mutex_lock(&cgroup_mutex);
>>> +
>>> +	root = idr_find(&cgroup_hierarchy_idr, hierarchy_id);
>>> +	if (root) {
>>> +		cgrp = task_cgroup_from_root(task, root);
>>
>> task_cgroup_from_root() will never return NULL, and there's a BUG_ON(!res) in it.
> 
> @hierarchy_id may come from userland, so we probably should update
> task_cgroup_from_root() to return NULL if the id is invalid.  Will add
> a patch.
> 

But if id is invalid, idr_find() will return NULL. As long as root is not NULL,
task_cgroup_from_root() will always return a valid cgroup.

[PATCH UPDATED 4/4] cgroup: implement task_cgroup_path_from_hierarchy()

[Tejun Heo]

kdbus folks want a sane way to determine the cgroup path that a given
task belongs to on a given hierarchy, which is a reasonble thing to
expect from cgroup core.

Implement task_cgroup_path_from_hierarchy().

v2: Dropped unnecessary NULL check on the return value of
    task_cgroup_from_root() as suggested by Li Zefan.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Acked-by: Greg Kroah-Hartman <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
Acked-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: Kay Sievers <kay-tD+1rO4QERM@public.gmane.org>
Cc: Lennart Poettering <lennart-mdGvqq1h2p+GdvJs77BJ7Q@public.gmane.org>
Cc: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
---
 include/linux/cgroup.h |    2 ++
 kernel/cgroup.c        |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -443,6 +443,8 @@ int cgroup_is_removed(const struct cgrou
 bool cgroup_is_descendant(struct cgroup *cgrp, struct cgroup *ancestor);
 
 int cgroup_path(const struct cgroup *cgrp, char *buf, int buflen);
+int task_cgroup_path_from_hierarchy(struct task_struct *task, int hierarchy_id,
+				    char *buf, size_t buflen);
 
 int cgroup_task_count(const struct cgroup *cgrp);
 
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1842,6 +1842,38 @@ out:
 }
 EXPORT_SYMBOL_GPL(cgroup_path);
 
+/**
+ * task_cgroup_path_from_hierarchy - cgroup path of a task on a hierarchy
+ * @task: target task
+ * @hierarchy_id: the hierarchy to look up @task's cgroup from
+ * @buf: the buffer to write the path into
+ * @buflen: the length of the buffer
+ *
+ * Determine @task's cgroup on the hierarchy specified by @hierarchy_id and
+ * copy its path into @buf.  This function grabs cgroup_mutex and shouldn't
+ * be used inside locks used by cgroup controller callbacks.
+ */
+int task_cgroup_path_from_hierarchy(struct task_struct *task, int hierarchy_id,
+				    char *buf, size_t buflen)
+{
+	struct cgroupfs_root *root;
+	struct cgroup *cgrp = NULL;
+	int ret = -ENOENT;
+
+	mutex_lock(&cgroup_mutex);
+
+	root = idr_find(&cgroup_hierarchy_idr, hierarchy_id);
+	if (root) {
+		cgrp = task_cgroup_from_root(task, root);
+		ret = cgroup_path(cgrp, buf, buflen);
+	}
+
+	mutex_unlock(&cgroup_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(task_cgroup_path_from_hierarchy);
+
 /*
  * Control Group taskset
  */

Re: [PATCH 4/4] cgroup: implement task_cgroup_path_from_hierarchy()

[Tejun Heo]

Hey, Li.

On Mon, Apr 15, 2013 at 11:49:57AM +0800, Li Zefan wrote:
> But if id is invalid, idr_find() will return NULL. As long as root is not NULL,
> task_cgroup_from_root() will always return a valid cgroup.

I'm hopping across multiple devel branches quickly today and somewhat
tipsy.  Sorry about the blabber.  :)

-- 
tejun

Re: [PATCHSET] cgroup: implement task_cgroup_path_from_hierarchy()

[Tejun Heo]

On Sun, Apr 14, 2013 at 11:36:55AM -0700, Tejun Heo wrote:
> kdbus folks want a sane way to determine the cgroup path that a given
> task belongs to on a given hierarchy, which is a reasonble thing to
> expect from cgroup core.

Applied to cgroup/for-3.11.

Thanks.

-- 
tejun