Skip to contentSkip to conversation

cpuacct: Show all detail stats in one file

PATCH Superseded Zhao Lei 3 participants 7 messages May 10, 2016 – May 12, 2016 Scheduler

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Zhao Lei

author
#0 lore raw

cover cpuacct: Show all detail stats in one file

In current code, we can get cpuacct data from severial files, but each file have its lilmit.

For example:
we can get cpu usage in user and kernel mode by cpuacct.stat, but we can't get detail data of each cpu in above file. we can get each cpu's kernel mode usage in cpuacct.usage_percpu_sys, but we can't get user mode data in the same time.

This patch introduce cpuacct.usage_all, to show all detailed cpu accounting data altogether:

 # cat cpuacct.usage_all
 [cpu] user system
 [0] 3809760299 5807968992
 [1] 3250329855 454612211
 ..

Any aspects of statistics data can be get from this file with a simple data-processing code.

This patchset should be applied on top of:
sched/cpuacct: Check for NULL when using task_pt_regs() in tip tree.

Commit-ID:  bd92883051a0228cc34996b8e766111ba10c9aac
Gitweb:     http://git.kernel.org/tip/bd92883051a0228cc34996b8e766111ba10c9aac
Author:     Anton Blanchard <[email protected]>
AuthorDate: Wed, 6 Apr 2016 21:59:50 +1000
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 13 Apr 2016 13:22:37 +0200

Zhao Lei (3):
cpuacct: Merge cpuacct_usage_index and cpuacct_usage_index
cpuacct: Simplify cpuacct_stats_show
cpuacct: Show all detail stats in one file

 kernel/sched/cpuacct.c | 114 +++++++++++++++++++++++++++++++------------------
 1 file changed, 73 insertions(+), 41 deletions(-)
Signature · 1 line
1.8.5.1
author
#1 lore raw

PATCH 1/3 cpuacct: Merge cpuacct_usage_index and cpuacct_usage_index

These two types have similar function.
No need to separate them.

Signed-off-by Zhao Lei <[email protected]>
---
 kernel/sched/cpuacct.c | 47 ++++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 27 deletions(-)
kernel/sched/cpuacct.c +20 -27
index 41f85c4..74241eb 100644
@@ -25,15 +25,13 @@ enum cpuacct_stat_index {
 	CPUACCT_STAT_NSTATS,
 };
 
-enum cpuacct_usage_index {
-	CPUACCT_USAGE_USER,	/* ... user mode */
-	CPUACCT_USAGE_SYSTEM,	/* ... kernel mode */
-
-	CPUACCT_USAGE_NRUSAGE,
+static const char * const cpuacct_stat_desc[] = {
+	[CPUACCT_STAT_USER] = "user",
+	[CPUACCT_STAT_SYSTEM] = "system",
 };
 
 struct cpuacct_usage {
-	u64	usages[CPUACCT_USAGE_NRUSAGE];
+	u64	usages[CPUACCT_STAT_NSTATS];
 };
 
 /* track cpu usage of a group of tasks and its child groups */

@@ -108,16 +106,16 @@ static void cpuacct_css_free(struct cgroup_subsys_state *css)
 }
 
 static u64 cpuacct_cpuusage_read(struct cpuacct *ca, int cpu,
-				 enum cpuacct_usage_index index)
+				 enum cpuacct_stat_index index)
 {
 	struct cpuacct_usage *cpuusage = per_cpu_ptr(ca->cpuusage, cpu);
 	u64 data;
 
 	/*
-	 * We allow index == CPUACCT_USAGE_NRUSAGE here to read
+	 * We allow index == CPUACCT_STAT_NSTATS here to read
 	 * the sum of suages.
 	 */
-	BUG_ON(index > CPUACCT_USAGE_NRUSAGE);
+	BUG_ON(index > CPUACCT_STAT_NSTATS);
 
 #ifndef CONFIG_64BIT
 	/*

@@ -126,11 +124,11 @@ static u64 cpuacct_cpuusage_read(struct cpuacct *ca, int cpu,
 	raw_spin_lock_irq(&cpu_rq(cpu)->lock);
 #endif
 
-	if (index == CPUACCT_USAGE_NRUSAGE) {
+	if (index == CPUACCT_STAT_NSTATS) {
 		int i = 0;
 
 		data = 0;
-		for (i = 0; i < CPUACCT_USAGE_NRUSAGE; i++)
+		for (i = 0; i < CPUACCT_STAT_NSTATS; i++)
 			data += cpuusage->usages[i];
 	} else {
 		data = cpuusage->usages[index];

@@ -155,7 +153,7 @@ static void cpuacct_cpuusage_write(struct cpuacct *ca, int cpu, u64 val)
 	raw_spin_lock_irq(&cpu_rq(cpu)->lock);
 #endif
 
-	for (i = 0; i < CPUACCT_USAGE_NRUSAGE; i++)
+	for (i = 0; i < CPUACCT_STAT_NSTATS; i++)
 		cpuusage->usages[i] = val;
 
 #ifndef CONFIG_64BIT

@@ -165,7 +163,7 @@ static void cpuacct_cpuusage_write(struct cpuacct *ca, int cpu, u64 val)
 
 /* return total cpu usage (in nanoseconds) of a group */
 static u64 __cpuusage_read(struct cgroup_subsys_state *css,
-			   enum cpuacct_usage_index index)
+			   enum cpuacct_stat_index index)
 {
 	struct cpuacct *ca = css_ca(css);
 	u64 totalcpuusage = 0;

@@ -180,18 +178,18 @@ static u64 __cpuusage_read(struct cgroup_subsys_state *css,
 static u64 cpuusage_user_read(struct cgroup_subsys_state *css,
 			      struct cftype *cft)
 {
-	return __cpuusage_read(css, CPUACCT_USAGE_USER);
+	return __cpuusage_read(css, CPUACCT_STAT_USER);
 }
 
 static u64 cpuusage_sys_read(struct cgroup_subsys_state *css,
 			     struct cftype *cft)
 {
-	return __cpuusage_read(css, CPUACCT_USAGE_SYSTEM);
+	return __cpuusage_read(css, CPUACCT_STAT_SYSTEM);
 }
 
 static u64 cpuusage_read(struct cgroup_subsys_state *css, struct cftype *cft)
 {
-	return __cpuusage_read(css, CPUACCT_USAGE_NRUSAGE);
+	return __cpuusage_read(css, CPUACCT_STAT_NSTATS);
 }
 
 static int cpuusage_write(struct cgroup_subsys_state *css, struct cftype *cft,

@@ -213,7 +211,7 @@ static int cpuusage_write(struct cgroup_subsys_state *css, struct cftype *cft,
 }
 
 static int __cpuacct_percpu_seq_show(struct seq_file *m,
-				     enum cpuacct_usage_index index)
+				     enum cpuacct_stat_index index)
 {
 	struct cpuacct *ca = css_ca(seq_css(m));
 	u64 percpu;

@@ -229,24 +227,19 @@ static int __cpuacct_percpu_seq_show(struct seq_file *m,
 
 static int cpuacct_percpu_user_seq_show(struct seq_file *m, void *V)
 {
-	return __cpuacct_percpu_seq_show(m, CPUACCT_USAGE_USER);
+	return __cpuacct_percpu_seq_show(m, CPUACCT_STAT_USER);
 }
 
 static int cpuacct_percpu_sys_seq_show(struct seq_file *m, void *V)
 {
-	return __cpuacct_percpu_seq_show(m, CPUACCT_USAGE_SYSTEM);
+	return __cpuacct_percpu_seq_show(m, CPUACCT_STAT_SYSTEM);
 }
 
 static int cpuacct_percpu_seq_show(struct seq_file *m, void *V)
 {
-	return __cpuacct_percpu_seq_show(m, CPUACCT_USAGE_NRUSAGE);
+	return __cpuacct_percpu_seq_show(m, CPUACCT_STAT_NSTATS);
 }
 
-static const char * const cpuacct_stat_desc[] = {
-	[CPUACCT_STAT_USER] = "user",
-	[CPUACCT_STAT_SYSTEM] = "system",
-};
-
 static int cpuacct_stats_show(struct seq_file *sf, void *v)
 {
 	struct cpuacct *ca = css_ca(seq_css(sf));

@@ -316,11 +309,11 @@ static struct cftype files[] = {
 void cpuacct_charge(struct task_struct *tsk, u64 cputime)
 {
 	struct cpuacct *ca;
-	int index = CPUACCT_USAGE_SYSTEM;
+	int index = CPUACCT_STAT_SYSTEM;
 	struct pt_regs *regs = task_pt_regs(tsk);
 
 	if (regs && user_mode(regs))
-		index = CPUACCT_USAGE_USER;
+		index = CPUACCT_STAT_USER;
 
 	rcu_read_lock();
Signature · 1 line
1.8.5.1
#2 lore raw

Re: [PATCH 1/3] cpuacct: Merge cpuacct_usage_index and cpuacct_usage_index

Quoted 2 lines · Zhao Lei

These two types have similar function.
No need to separate them.

My question, have you read the title you've written for this patch _even once_??

Thanks,

Ingo

author
#3 lore raw

PATCH 2/3 cpuacct: Simplify cpuacct_stats_show

In reply to Zhao Lei

Merge code for each cpustat(system/user) into a loop, to avoid clone of code blocks.
Only a little cleanup.

Signed-off-by Zhao Lei <[email protected]>
---
 kernel/sched/cpuacct.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)
kernel/sched/cpuacct.c +14 -15
index 74241eb..e643483 100644
@@ -243,28 +243,27 @@ static int cpuacct_percpu_seq_show(struct seq_file *m, void *V)
 static int cpuacct_stats_show(struct seq_file *sf, void *v)
 {
 	struct cpuacct *ca = css_ca(seq_css(sf));
+	s64 val[CPUACCT_STAT_NSTATS];
 	int cpu;
-	s64 val = 0;
+	int stat;
 
+	memset(val, 0, sizeof(val));
 	for_each_possible_cpu(cpu) {
-		struct kernel_cpustat *kcpustat = per_cpu_ptr(ca->cpustat, cpu);
-		val += kcpustat->cpustat[CPUTIME_USER];
-		val += kcpustat->cpustat[CPUTIME_NICE];
+		struct kernel_cpustat *kcpustat = per_cpu_ptr(ca->cpustat,
+							      cpu);
+		val[CPUACCT_STAT_USER]   += kcpustat->cpustat[CPUTIME_USER];
+		val[CPUACCT_STAT_USER]   += kcpustat->cpustat[CPUTIME_NICE];
+		val[CPUACCT_STAT_SYSTEM] += kcpustat->cpustat[CPUTIME_SYSTEM];
+		val[CPUACCT_STAT_SYSTEM] += kcpustat->cpustat[CPUTIME_IRQ];
+		val[CPUACCT_STAT_SYSTEM] += kcpustat->cpustat[CPUTIME_SOFTIRQ];
 	}
-	val = cputime64_to_clock_t(val);
-	seq_printf(sf, "%s %lld\n", cpuacct_stat_desc[CPUACCT_STAT_USER], val);
 
-	val = 0;
-	for_each_possible_cpu(cpu) {
-		struct kernel_cpustat *kcpustat = per_cpu_ptr(ca->cpustat, cpu);
-		val += kcpustat->cpustat[CPUTIME_SYSTEM];
-		val += kcpustat->cpustat[CPUTIME_IRQ];
-		val += kcpustat->cpustat[CPUTIME_SOFTIRQ];
+	for (stat = 0; stat < CPUACCT_STAT_NSTATS; stat++) {
+		seq_printf(sf, "%s %lld\n",
+			   cpuacct_stat_desc[stat],
+			   cputime64_to_clock_t(val[stat]));
 	}
 
-	val = cputime64_to_clock_t(val);
-	seq_printf(sf, "%s %lld\n", cpuacct_stat_desc[CPUACCT_STAT_SYSTEM], val);
-
 	return 0;
 }
Signature · 1 line
1.8.5.1
#4 lore raw

Re: [PATCH 2/3] cpuacct: Simplify cpuacct_stats_show

Quoted 8 lines · Zhao Lei

Merge code for each cpustat(system/user) into a loop, to avoid clone of code blocks.
Only a little cleanup.

Signed-off-by Zhao Lei <[email protected]>
---
 kernel/sched/cpuacct.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

I see a couple of problems with this patch:

- please Cc: all scheduler maintainers to scheduler patches.

- please fix the title of the patch: have a look at 'git log kernel/sched/cpuacct.c' how recent titles to that code look like.

- when referring to functions in changelogs, please add '()' to separate them from variable and other names. I.e. it's "cpuacct_stats_show()".

Quoted 5 lines

static int cpuacct_stats_show(struct seq_file *sf, void *v) {

kernel/sched/cpuacct.c +1 -0
 	struct cpuacct *ca = css_ca(seq_css(sf));
+	s64 val[CPUACCT_STAT_NSTATS];
 	int cpu;

kernel/sched/cpuacct.c static int cpuacct_percpu_seq_show(struct seq_file *m, void *V) · line 253

Quoted 10 lines
kernel/sched/cpuacct.c +4 -4
-	s64 val = 0;
+	int stat;
 
+	memset(val, 0, sizeof(val));
 	for_each_possible_cpu(cpu) {
-		struct kernel_cpustat *kcpustat = per_cpu_ptr(ca->cpustat, cpu);
-		val += kcpustat->cpustat[CPUTIME_USER];
-		val += kcpustat->cpustat[CPUTIME_NICE];
+		struct kernel_cpustat *kcpustat = per_cpu_ptr(ca->cpustat,
+							      cpu);

Horrible taste: in what universe is that linebreak an improvement to the code?

kernel/sched/cpuacct.c static int cpuacct_percpu_seq_show(struct seq_file *m, void *V) · line 254

Quoted 1 line
kernel/sched/cpuacct.c +1 -0
+		val[CPUACCT_STAT_USER]   += kcpustat->cpustat[CPUTIME_USER];

Also, please put a newline between variable definitions and the first non-definition C statement...

kernel/sched/cpuacct.c static int cpuacct_percpu_seq_show(struct seq_file *m, void *V) · line 258

Quoted 4 lines
kernel/sched/cpuacct.c +4 -0
+		val[CPUACCT_STAT_USER]   += kcpustat->cpustat[CPUTIME_NICE];
+		val[CPUACCT_STAT_SYSTEM] += kcpustat->cpustat[CPUTIME_SYSTEM];
+		val[CPUACCT_STAT_SYSTEM] += kcpustat->cpustat[CPUTIME_IRQ];
+		val[CPUACCT_STAT_SYSTEM] += kcpustat->cpustat[CPUTIME_SOFTIRQ];

Also, if you introduce a helper variable to shorten the code, you might as well introduce one for the cpustat array itself, and skip the whole 'kcpustat->' repetition ...

Thanks,

Ingo

author
#5 lore raw

PATCH 3/3 cpuacct: Show all detail stats in one file

In reply to Zhao Lei

In current code, we can get cpuacct data from severial files, but each file have its lilmit.

For example:
we can get cpu usage in user and kernel mode by cpuacct.stat, but we can't get detail data of each cpu in above file. we can get each cpu's kernel mode usage in cpuacct.usage_percpu_sys, but we can't get user mode data in the same time.

This patch introduce cpuacct.usage_all, to show all detailed cpu accounting data altogether:

 # cat cpuacct.usage_all
 [cpu] user system
 [0] 3809760299 5807968992
 [1] 3250329855 454612211
 ..

Any aspects of statistics data can be get from this file with a simple data-processing code.

Signed-off-by Zhao Lei <[email protected]>
---
 kernel/sched/cpuacct.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
kernel/sched/cpuacct.c +40 -0
index e643483..b84ae40 100644
@@ -240,6 +240,42 @@ static int cpuacct_percpu_seq_show(struct seq_file *m, void *V)
 	return __cpuacct_percpu_seq_show(m, CPUACCT_STAT_NSTATS);
 }
 
+static int cpuacct_all_seq_show(struct seq_file *m, void *V)
+{
+	struct cpuacct *ca = css_ca(seq_css(m));
+	int index;
+	int cpu;
+
+	seq_puts(m, "[cpu]");
+	for (index = 0; index < CPUACCT_STAT_NSTATS; index++)
+		seq_printf(m, " %s", cpuacct_stat_desc[index]);
+	seq_puts(m, "\n");
+
+	for_each_possible_cpu(cpu) {
+		struct cpuacct_usage *cpuusage = per_cpu_ptr(ca->cpuusage, cpu);
+
+		seq_printf(m, "[%d]", cpu);
+
+		for (index = 0; index < CPUACCT_STAT_NSTATS; index++) {
+#ifndef CONFIG_64BIT
+			/*
+			 * Take rq->lock to make 64-bit read safe on 32-bit
+			 * platforms.
+			 */
+			raw_spin_lock_irq(&cpu_rq(cpu)->lock);
+#endif
+
+			seq_printf(m, " %llu", cpuusage->usages[index]);
+
+#ifndef CONFIG_64BIT
+			raw_spin_unlock_irq(&cpu_rq(cpu)->lock);
+#endif
+		}
+		seq_puts(m, "\n");
+	}
+	return 0;
+}
+
 static int cpuacct_stats_show(struct seq_file *sf, void *v)
 {
 	struct cpuacct *ca = css_ca(seq_css(sf));

@@ -294,6 +330,10 @@ static struct cftype files[] = {
 		.seq_show = cpuacct_percpu_sys_seq_show,
 	},
 	{
+		.name = "usage_all",
+		.seq_show = cpuacct_all_seq_show,
+	},
+	{
 		.name = "stat",
 		.seq_show = cpuacct_stats_show,
 	},
Signature · 1 line
1.8.5.1
maintainer
#6 lore raw
Quoted 8 lines · Zhao Lei
 # cat cpuacct.usage_all
 [cpu] user system
 [0] 3809760299 5807968992
 [1] 3250329855 454612211
 ..

Any aspects of statistics data can be get from this file with a simple data-processing code.

Is there a precedent for using [%d] for the first column? Would it not be easier to parse without that?