Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1641673 > unrolled thread

[RFC PATCH v2 08/17] cgroup: Move debug cgroup to its own file

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC PATCH v2 08/17] cgroup: Move debug cgroup to its own file Waiman Long <longman@redhat.com> - 2017-05-15 15:40 +0200
    Re: [RFC PATCH v2 08/17] cgroup: Move debug cgroup to its own file Tejun Heo <tj@kernel.org> - 2017-05-17 23:40 +0200
      Re: [RFC PATCH v2 08/17] cgroup: Move debug cgroup to its own file Waiman Long <longman@redhat.com> - 2017-05-18 17:40 +0200
      Re: [RFC PATCH v2 08/17] cgroup: Move debug cgroup to its own file Waiman Long <longman@redhat.com> - 2017-05-18 18:00 +0200
        Re: [RFC PATCH v2 08/17] cgroup: Move debug cgroup to its own file Tejun Heo <tj@kernel.org> - 2017-05-19 21:30 +0200
          Re: [RFC PATCH v2 08/17] cgroup: Move debug cgroup to its own file Waiman Long <longman@redhat.com> - 2017-05-19 21:40 +0200
            Re: [RFC PATCH v2 08/17] cgroup: Move debug cgroup to its own file Tejun Heo <tj@kernel.org> - 2017-05-19 22:30 +0200

#1641673 — [RFC PATCH v2 08/17] cgroup: Move debug cgroup to its own file

The debug cgroup currently resides within cgroup-v1.c and is enabled
only for v1 cgroup. To enable the debug cgroup also for v2, it
makes sense to put the code into its own file as it will no longer
be v1 specific. The only change in this patch is the expansion of
cgroup_task_count() within the debug_taskcount_read() function.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 kernel/cgroup/Makefile    |   1 +
 kernel/cgroup/cgroup-v1.c | 147 -----------------------------------------
 kernel/cgroup/debug.c     | 165 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 166 insertions(+), 147 deletions(-)
 create mode 100644 kernel/cgroup/debug.c

diff --git a/kernel/cgroup/Makefile b/kernel/cgroup/Makefile
index 387348a..ce693cc 100644
--- a/kernel/cgroup/Makefile
+++ b/kernel/cgroup/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_CGROUP_FREEZER) += freezer.o
 obj-$(CONFIG_CGROUP_PIDS) += pids.o
 obj-$(CONFIG_CGROUP_RDMA) += rdma.o
 obj-$(CONFIG_CPUSETS) += cpuset.o
+obj-$(CONFIG_CGROUP_DEBUG) += debug.o
diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
index 1e101b9..7ad6b17 100644
--- a/kernel/cgroup/cgroup-v1.c
+++ b/kernel/cgroup/cgroup-v1.c
@@ -1311,150 +1311,3 @@ static int __init cgroup_no_v1(char *str)
 	return 1;
 }
 __setup("cgroup_no_v1=", cgroup_no_v1);
-
-
-#ifdef CONFIG_CGROUP_DEBUG
-static struct cgroup_subsys_state *
-debug_css_alloc(struct cgroup_subsys_state *parent_css)
-{
-	struct cgroup_subsys_state *css = kzalloc(sizeof(*css), GFP_KERNEL);
-
-	if (!css)
-		return ERR_PTR(-ENOMEM);
-
-	return css;
-}
-
-static void debug_css_free(struct cgroup_subsys_state *css)
-{
-	kfree(css);
-}
-
-static u64 debug_taskcount_read(struct cgroup_subsys_state *css,
-				struct cftype *cft)
-{
-	return cgroup_task_count(css->cgroup);
-}
-
-static u64 current_css_set_read(struct cgroup_subsys_state *css,
-				struct cftype *cft)
-{
-	return (u64)(unsigned long)current->cgroups;
-}
-
-static u64 current_css_set_refcount_read(struct cgroup_subsys_state *css,
-					 struct cftype *cft)
-{
-	u64 count;
-
-	rcu_read_lock();
-	count = refcount_read(&task_css_set(current)->refcount);
-	rcu_read_unlock();
-	return count;
-}
-
-static int current_css_set_cg_links_read(struct seq_file *seq, void *v)
-{
-	struct cgrp_cset_link *link;
-	struct css_set *cset;
-	char *name_buf;
-
-	name_buf = kmalloc(NAME_MAX + 1, GFP_KERNEL);
-	if (!name_buf)
-		return -ENOMEM;
-
-	spin_lock_irq(&css_set_lock);
-	rcu_read_lock();
-	cset = rcu_dereference(current->cgroups);
-	list_for_each_entry(link, &cset->cgrp_links, cgrp_link) {
-		struct cgroup *c = link->cgrp;
-
-		cgroup_name(c, name_buf, NAME_MAX + 1);
-		seq_printf(seq, "Root %d group %s\n",
-			   c->root->hierarchy_id, name_buf);
-	}
-	rcu_read_unlock();
-	spin_unlock_irq(&css_set_lock);
-	kfree(name_buf);
-	return 0;
-}
-
-#define MAX_TASKS_SHOWN_PER_CSS 25
-static int cgroup_css_links_read(struct seq_file *seq, void *v)
-{
-	struct cgroup_subsys_state *css = seq_css(seq);
-	struct cgrp_cset_link *link;
-
-	spin_lock_irq(&css_set_lock);
-	list_for_each_entry(link, &css->cgroup->cset_links, cset_link) {
-		struct css_set *cset = link->cset;
-		struct task_struct *task;
-		int count = 0;
-
-		seq_printf(seq, "css_set %pK\n", cset);
-
-		list_for_each_entry(task, &cset->tasks, cg_list) {
-			if (count++ > MAX_TASKS_SHOWN_PER_CSS)
-				goto overflow;
-			seq_printf(seq, "  task %d\n", task_pid_vnr(task));
-		}
-
-		list_for_each_entry(task, &cset->mg_tasks, cg_list) {
-			if (count++ > MAX_TASKS_SHOWN_PER_CSS)
-				goto overflow;
-			seq_printf(seq, "  task %d\n", task_pid_vnr(task));
-		}
-		continue;
-	overflow:
-		seq_puts(seq, "  ...\n");
-	}
-	spin_unlock_irq(&css_set_lock);
-	return 0;
-}
-
-static u64 releasable_read(struct cgroup_subsys_state *css, struct cftype *cft)
-{
-	return (!cgroup_is_populated(css->cgroup) &&
-		!css_has_online_children(&css->cgroup->self));
-}
-
-static struct cftype debug_files[] =  {
-	{
-		.name = "taskcount",
-		.read_u64 = debug_taskcount_read,
-	},
-
-	{
-		.name = "current_css_set",
-		.read_u64 = current_css_set_read,
-	},
-
-	{
-		.name = "current_css_set_refcount",
-		.read_u64 = current_css_set_refcount_read,
-	},
-
-	{
-		.name = "current_css_set_cg_links",
-		.seq_show = current_css_set_cg_links_read,
-	},
-
-	{
-		.name = "cgroup_css_links",
-		.seq_show = cgroup_css_links_read,
-	},
-
-	{
-		.name = "releasable",
-		.read_u64 = releasable_read,
-	},
-
-	{ }	/* terminate */
-};
-
-struct cgroup_subsys debug_cgrp_subsys = {
-	.css_alloc = debug_css_alloc,
-	.css_free = debug_css_free,
-	.legacy_cftypes = debug_files,
-};
-#endif /* CONFIG_CGROUP_DEBUG */
diff --git a/kernel/cgroup/debug.c b/kernel/cgroup/debug.c
new file mode 100644
index 0000000..56e60a2
--- /dev/null
+++ b/kernel/cgroup/debug.c
@@ -0,0 +1,165 @@
+#include <linux/ctype.h>
+#include <linux/mm.h>
+#include <linux/slab.h>
+
+#include "cgroup-internal.h"
+
+static struct cgroup_subsys_state *
+debug_css_alloc(struct cgroup_subsys_state *parent_css)
+{
+	struct cgroup_subsys_state *css = kzalloc(sizeof(*css), GFP_KERNEL);
+
+	if (!css)
+		return ERR_PTR(-ENOMEM);
+
+	return css;
+}
+
+static void debug_css_free(struct cgroup_subsys_state *css)
+{
+	kfree(css);
+}
+
+/*
+ * debug_taskcount_read - return the number of tasks in a cgroup.
+ * @cgrp: the cgroup in question
+ *
+ * Return the number of tasks in the cgroup.  The returned number can be
+ * higher than the actual number of tasks due to css_set references from
+ * namespace roots and temporary usages.
+ */
+static u64 debug_taskcount_read(struct cgroup_subsys_state *css,
+				struct cftype *cft)
+{
+	struct cgroup *cgrp = css->cgroup;
+	u64 count = 0;
+	struct cgrp_cset_link *link;
+
+	spin_lock_irq(&css_set_lock);
+	list_for_each_entry(link, &cgrp->cset_links, cset_link)
+		count += refcount_read(&link->cset->refcount);
+	spin_unlock_irq(&css_set_lock);
+	return count;
+}
+
+static u64 current_css_set_read(struct cgroup_subsys_state *css,
+				struct cftype *cft)
+{
+	return (u64)(unsigned long)current->cgroups;
+}
+
+static u64 current_css_set_refcount_read(struct cgroup_subsys_state *css,
+					 struct cftype *cft)
+{
+	u64 count;
+
+	rcu_read_lock();
+	count = refcount_read(&task_css_set(current)->refcount);
+	rcu_read_unlock();
+	return count;
+}
+
+static int current_css_set_cg_links_read(struct seq_file *seq, void *v)
+{
+	struct cgrp_cset_link *link;
+	struct css_set *cset;
+	char *name_buf;
+
+	name_buf = kmalloc(NAME_MAX + 1, GFP_KERNEL);
+	if (!name_buf)
+		return -ENOMEM;
+
+	spin_lock_irq(&css_set_lock);
+	rcu_read_lock();
+	cset = rcu_dereference(current->cgroups);
+	list_for_each_entry(link, &cset->cgrp_links, cgrp_link) {
+		struct cgroup *c = link->cgrp;
+
+		cgroup_name(c, name_buf, NAME_MAX + 1);
+		seq_printf(seq, "Root %d group %s\n",
+			   c->root->hierarchy_id, name_buf);
+	}
+	rcu_read_unlock();
+	spin_unlock_irq(&css_set_lock);
+	kfree(name_buf);
+	return 0;
+}
+
+#define MAX_TASKS_SHOWN_PER_CSS 25
+static int cgroup_css_links_read(struct seq_file *seq, void *v)
+{
+	struct cgroup_subsys_state *css = seq_css(seq);
+	struct cgrp_cset_link *link;
+
+	spin_lock_irq(&css_set_lock);
+	list_for_each_entry(link, &css->cgroup->cset_links, cset_link) {
+		struct css_set *cset = link->cset;
+		struct task_struct *task;
+		int count = 0;
+
+		seq_printf(seq, "css_set %pK\n", cset);
+
+		list_for_each_entry(task, &cset->tasks, cg_list) {
+			if (count++ > MAX_TASKS_SHOWN_PER_CSS)
+				goto overflow;
+			seq_printf(seq, "  task %d\n", task_pid_vnr(task));
+		}
+
+		list_for_each_entry(task, &cset->mg_tasks, cg_list) {
+			if (count++ > MAX_TASKS_SHOWN_PER_CSS)
+				goto overflow;
+			seq_printf(seq, "  task %d\n", task_pid_vnr(task));
+		}
+		continue;
+	overflow:
+		seq_puts(seq, "  ...\n");
+	}
+	spin_unlock_irq(&css_set_lock);
+	return 0;
+}
+
+static u64 releasable_read(struct cgroup_subsys_state *css, struct cftype *cft)
+{
+	return (!cgroup_is_populated(css->cgroup) &&
+		!css_has_online_children(&css->cgroup->self));
+}
+
+static struct cftype debug_files[] =  {
+	{
+		.name = "taskcount",
+		.read_u64 = debug_taskcount_read,
+	},
+
+	{
+		.name = "current_css_set",
+		.read_u64 = current_css_set_read,
+	},
+
+	{
+		.name = "current_css_set_refcount",
+		.read_u64 = current_css_set_refcount_read,
+	},
+
+	{
+		.name = "current_css_set_cg_links",
+		.seq_show = current_css_set_cg_links_read,
+	},
+
+	{
+		.name = "cgroup_css_links",
+		.seq_show = cgroup_css_links_read,
+	},
+
+	{
+		.name = "releasable",
+		.read_u64 = releasable_read,
+	},
+
+	{ }	/* terminate */
+};
+
+struct cgroup_subsys debug_cgrp_subsys = {
+	.css_alloc = debug_css_alloc,
+	.css_free = debug_css_free,
+	.legacy_cftypes = debug_files,
+};
-- 
1.8.3.1

[toc] | [next] | [standalone]

#1643646

Hello, Waiman.

On Mon, May 15, 2017 at 09:34:07AM -0400, Waiman Long wrote:
> The debug cgroup currently resides within cgroup-v1.c and is enabled
> only for v1 cgroup. To enable the debug cgroup also for v2, it
> makes sense to put the code into its own file as it will no longer
> be v1 specific. The only change in this patch is the expansion of
> cgroup_task_count() within the debug_taskcount_read() function.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>

I don't mind enabling the debug controller for v2 but let's please
hide it behind an unwieldy boot param / controller name so that it's
clear that its interface isn't expected to be stable.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1644692

On 05/17/2017 05:36 PM, Tejun Heo wrote:
> Hello, Waiman.
>
> On Mon, May 15, 2017 at 09:34:07AM -0400, Waiman Long wrote:
>> The debug cgroup currently resides within cgroup-v1.c and is enabled
>> only for v1 cgroup. To enable the debug cgroup also for v2, it
>> makes sense to put the code into its own file as it will no longer
>> be v1 specific. The only change in this patch is the expansion of
>> cgroup_task_count() within the debug_taskcount_read() function.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
> I don't mind enabling the debug controller for v2 but let's please
> hide it behind an unwieldy boot param / controller name so that it's
> clear that its interface isn't expected to be stable.
>
> Thanks.
>
The controller name is "debug". So it is pretty obvious what it is for.
However, the config prompt of "Example controller" is indeed a bit
vague. So I think we can make the prompt more descriptive here. As for
boot param, are you saying something like "cgroup_debug" has to be
specified in the command line also to have this controller activated
even if the CGROUP_DEBUG config parameter is specified? I am fine with
that if you think it is necessary.

Regards,
Longman

[toc] | [prev] | [next] | [standalone]

#1644709

On 05/17/2017 05:36 PM, Tejun Heo wrote:
> Hello, Waiman.
>
> On Mon, May 15, 2017 at 09:34:07AM -0400, Waiman Long wrote:
>> The debug cgroup currently resides within cgroup-v1.c and is enabled
>> only for v1 cgroup. To enable the debug cgroup also for v2, it
>> makes sense to put the code into its own file as it will no longer
>> be v1 specific. The only change in this patch is the expansion of
>> cgroup_task_count() within the debug_taskcount_read() function.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
> I don't mind enabling the debug controller for v2 but let's please
> hide it behind an unwieldy boot param / controller name so that it's
> clear that its interface isn't expected to be stable.
>
> Thanks.
>
The controller name is "debug" and so it is obvious what this controller
is for. However, the config prompt "Example controller" is indeed vague
in meaning. So we can make the prompt more descriptive here. As for the
boot param, are you saying something like "cgroup_debug" has to be
specified in the command line even if CGROUP_DEBUG config is there for
the debug controller to be enabled? I am fine with that if you think it
is necessary.

Regards,
Longman

[toc] | [prev] | [next] | [standalone]

#1645861

Hello, Waiman.

On Thu, May 18, 2017 at 11:52:18AM -0400, Waiman Long wrote:
> The controller name is "debug" and so it is obvious what this controller
> is for. However, the config prompt "Example controller" is indeed vague

Yeah but it also shows up as an integral part of stable interface
rather than e.g. /sys/kernel/debug.  This isn't of any interest to
people who aren't developing cgroup core code.  There is no reason to
risk growing dependencies on it.

> in meaning. So we can make the prompt more descriptive here. As for the
> boot param, are you saying something like "cgroup_debug" has to be
> specified in the command line even if CGROUP_DEBUG config is there for
> the debug controller to be enabled? I am fine with that if you think it
> is necessary.

Yeah, I think that'd be a good idea.  cgroup_debug should do.  While
at it, can you also please make CGROUP_DEBUG depend on DEBUG_KERNEL?

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1645869

On 05/19/2017 03:21 PM, Tejun Heo wrote:
> Hello, Waiman.
>
> On Thu, May 18, 2017 at 11:52:18AM -0400, Waiman Long wrote:
>> The controller name is "debug" and so it is obvious what this controller
>> is for. However, the config prompt "Example controller" is indeed vague
> Yeah but it also shows up as an integral part of stable interface
> rather than e.g. /sys/kernel/debug.  This isn't of any interest to
> people who aren't developing cgroup core code.  There is no reason to
> risk growing dependencies on it.

The debug controller is used to show information relevant to the cgroup
its css'es are attached to. So it will be very hard to use if we
relocate to /sys/kernel/debug, for example. Currently, nothing in the
debug controller other than debug_cgrp_subsys are exported. I don't see
any risk of having dependency on that controller from other parts of the
kernel.

>> in meaning. So we can make the prompt more descriptive here. As for the
>> boot param, are you saying something like "cgroup_debug" has to be
>> specified in the command line even if CGROUP_DEBUG config is there for
>> the debug controller to be enabled? I am fine with that if you think it
>> is necessary.
> Yeah, I think that'd be a good idea.  cgroup_debug should do.  While
> at it, can you also please make CGROUP_DEBUG depend on DEBUG_KERNEL?
>
> Thanks.
>
Sure. I will do that.

Cheers,
Longman

[toc] | [prev] | [next] | [standalone]

#1645896

Hello,

On Fri, May 19, 2017 at 03:33:14PM -0400, Waiman Long wrote:
> On 05/19/2017 03:21 PM, Tejun Heo wrote:
> > Yeah but it also shows up as an integral part of stable interface
> > rather than e.g. /sys/kernel/debug.  This isn't of any interest to
> > people who aren't developing cgroup core code.  There is no reason to
> > risk growing dependencies on it.
> 
> The debug controller is used to show information relevant to the cgroup
> its css'es are attached to. So it will be very hard to use if we
> relocate to /sys/kernel/debug, for example. Currently, nothing in the
> debug controller other than debug_cgrp_subsys are exported. I don't see
> any risk of having dependency on that controller from other parts of the
> kernel.

Oh, sure, I wasn't suggesting moving it under /sys/kernel/debug but
that we'd want to take extra precautions as we can't.

Thanks.

-- 
tejun

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web