cgroup, sched: Move basic cpu stats from cgroup.stat to cpu.stat

The basic cpu stat is currently shown with "cpu." prefix in cgroup.stat, and the same information is duplicated in cpu.stat when cpu controller is enabled. This is ugly and not very scalable as we want to expand the coverage of stat information which is always available.

This patch makes cgroup core always create "cpu.stat" file and show the basic cpu stat there and calls the cpu controller to show the extra stats when enabled. This ensures that the same information isn't presented in multiple places and makes future expansion of basic stats easier.

---
Hello,

The more I think about showing cpu stat in cgroup.stat, the uglier it seems. This patch flips it so that "cpu.stat" is always available with basic cpu stat instead. It only changes the presentation and changes to the scheduler code is minimal. Will route with the other cpu controller changes through cgroup/for-4.15 unless there are objections.

Thanks!

 Documentation/cgroup-v2.txt     |   15 +++-------
 include/linux/cgroup-defs.h     |    2 +
 include/linux/cgroup.h          |    2 -
 kernel/cgroup/cgroup-internal.h |    1 
 kernel/cgroup/cgroup.c          |   60 ++++++++++++++++++++++++++++++++++++++--
 kernel/cgroup/stat.c            |   10 +++---
 kernel/sched/core.c             |   13 ++------
 7 files changed, 75 insertions(+), 28 deletions(-)

@@ -886,15 +886,6 @@ All cgroup core files are prefixed with
 		A dying cgroup can consume system resources not exceeding
 		limits, which were active at the moment of cgroup deletion.
 
-	  cpu.usage_usec
-		CPU time consumed in the subtree.
-
-	  cpu.user_usec
-		User CPU time consumed in the subtree.
-
-	  cpu.system_usec
-		System CPU time consumed in the subtree.
-
 
 Controllers
 ===========

@@ -915,12 +906,16 @@ All time durations are in microseconds.
 
   cpu.stat
 	A read-only flat-keyed file which exists on non-root cgroups.
+	This file exists whether the controller is enabled or not.
 
-	It reports the following six stats:
+	It always reports the following three stats:
 
 	- usage_usec
 	- user_usec
 	- system_usec
+
+	and the following three when the controller is enabled:
+
 	- nr_periods
 	- nr_throttled
 	- throttled_usec

@@ -569,6 +569,8 @@ struct cgroup_subsys {
 	void (*css_released)(struct cgroup_subsys_state *css);
 	void (*css_free)(struct cgroup_subsys_state *css);
 	void (*css_reset)(struct cgroup_subsys_state *css);
+	int (*css_extra_stat_show)(struct seq_file *seq,
+				   struct cgroup_subsys_state *css);
 
 	int (*can_attach)(struct cgroup_taskset *tset);
 	void (*cancel_attach)(struct cgroup_taskset *tset);

@@ -703,8 +703,6 @@ static inline void cpuacct_account_field
 					 u64 val) {}
 #endif
 
-void cgroup_stat_show_cputime(struct seq_file *seq, const char *prefix);
-
 void __cgroup_account_cputime(struct cgroup *cgrp, u64 delta_exec);
 void __cgroup_account_cputime_field(struct cgroup *cgrp,
 				    enum cpu_usage_stat index, u64 delta_exec);

@@ -205,6 +205,7 @@ int cgroup_task_count(const struct cgrou
 void cgroup_stat_flush(struct cgroup *cgrp);
 int cgroup_stat_init(struct cgroup *cgrp);
 void cgroup_stat_exit(struct cgroup *cgrp);
+void cgroup_stat_show_cputime(struct seq_file *seq);
 void cgroup_stat_boot(void);
 
 /*

@@ -256,7 +256,7 @@ void __cgroup_account_cputime_field(stru
 	cgroup_cpu_stat_account_end(cgrp, cstat);
 }
 
-void cgroup_stat_show_cputime(struct seq_file *seq, const char *prefix)
+void cgroup_stat_show_cputime(struct seq_file *seq)
 {
 	struct cgroup *cgrp = seq_css(seq)->cgroup;
 	u64 usage, utime, stime;

@@ -278,10 +278,10 @@ void cgroup_stat_show_cputime(struct seq
 	do_div(utime, NSEC_PER_USEC);
 	do_div(stime, NSEC_PER_USEC);
 
-	seq_printf(seq, "%susage_usec %llu\n"
-		   "%suser_usec %llu\n"
-		   "%ssystem_usec %llu\n",
-		   prefix, usage, prefix, utime, prefix, stime);
+	seq_printf(seq, "usage_usec %llu\n"
+		   "user_usec %llu\n"
+		   "system_usec %llu\n",
+		   usage, utime, stime);
 }
 
 int cgroup_stat_init(struct cgroup *cgrp)

@@ -6678,13 +6678,12 @@ static struct cftype cpu_legacy_files[]
 	{ }	/* Terminate */
 };
 
-static int cpu_stat_show(struct seq_file *sf, void *v)
+static int cpu_extra_stat_show(struct seq_file *sf,
+			       struct cgroup_subsys_state *css)
 {
-	cgroup_stat_show_cputime(sf, "");
-
 #ifdef CONFIG_CFS_BANDWIDTH
 	{
-		struct task_group *tg = css_tg(seq_css(sf));
+		struct task_group *tg = css_tg(css);
 		struct cfs_bandwidth *cfs_b = &tg->cfs_bandwidth;
 		u64 throttled_usec;
 

@@ -6817,11 +6816,6 @@ static ssize_t cpu_max_write(struct kern
 #endif
 
 static struct cftype cpu_files[] = {
-	{
-		.name = "stat",
-		.flags = CFTYPE_NOT_ON_ROOT,
-		.seq_show = cpu_stat_show,
-	},
 #ifdef CONFIG_FAIR_GROUP_SCHED
 	{
 		.name = "weight",

@@ -6852,6 +6846,7 @@ struct cgroup_subsys cpu_cgrp_subsys = {
 	.css_online	= cpu_cgroup_css_online,
 	.css_released	= cpu_cgroup_css_released,
 	.css_free	= cpu_cgroup_css_free,
+	.css_extra_stat_show = cpu_extra_stat_show,
 	.fork		= cpu_cgroup_fork,
 	.can_attach	= cpu_cgroup_can_attach,
 	.attach		= cpu_cgroup_attach,

I've not been paying much attention to this, could you elaborate on the problems there?

And this is -v2 only? I'm a little lost on how all that connects.

Hello, Peter.

Sure, so, on cgroup2, the basic stat collects cpu usage info whether cpu controller is enabled or not. As the hot path overhead is always per-cpu and constant, there's no reason to not to and always having the information is useful, especially as enabling cpu isn't free of side-effects. This is similar to what cpuacct did in cgroup1 but cgroup2's single hierarchy makes the dedicated controller unnecessary.

The issue at hand with this patch is how the stat is presented when the controller is not enabled. There were two alternatives.

1. Make it available in the core file cgroup.stat with subsystem prefix ("cpu." for cpu).

This is easy to implement but a bit ugly because when the controller is enabled, the same information is available in two places - cgroup.stat and cpu.stat. Access from userspace becomes ugly too especially in cases where we make more fields available in basic stat.

2. Make $SUBSYS.stat available whether the controller is enabled or not. Show the basic stat when the controller is disabled, show the full thing when enabled.

This is somewhat more complicated to implement and having a subsystem specific file around when the controller is not enabled might be a bit confusing. However, it ensures that a given piece of information is available in only one place and makes it less awkward to make more information available through basic stat.

The original implementation went for #1 and this patch switches it to #2.

Yeap, basic stat is v2 only. We can't do it against multiple hierarchies with constant overhead.

Thanks.

tejun

Applied to cgroup/for-4.15.

Thanks.

tejun