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

Groups > linux.kernel > #1350101 > unrolled thread

[PATCH v2 0/2] cpuacct: rename parameter in cpuusage_write for readability

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/2] cpuacct: rename parameter in cpuusage_write for readability Zhao Lei <zhaolei@cn.fujitsu.com> - 2016-03-04 11:00 +0100
    [PATCH v2 2/2] cpuacct: split usage into user_usage and sys_usage. Zhao Lei <zhaolei@cn.fujitsu.com> - 2016-03-04 11:00 +0100
      Re: [PATCH v2 2/2] cpuacct: split usage into user_usage and  sys_usage. Peter Zijlstra <peterz@infradead.org> - 2016-03-10 14:30 +0100
        RE: [PATCH v2 2/2] cpuacct: split usage into user_usage and sys_usage. Zhao Lei <zhaolei@cn.fujitsu.com> - 2016-03-16 11:50 +0100

#1350101 — [PATCH v2 0/2] cpuacct: rename parameter in cpuusage_write for readability

The name of 'reset' makes a little confusion in reading, we would
say, if we want to reset usage, return -EINVAL. That's not true.

Actually, we want to say, we only allow user to do a reset. This
patch rename reset to val and add a comment here, making the code
more readable.

Changelog v1->v2:
1: Rebase on top of 4.5-rc6
2: Fix little spelling typo in description.
3: Fix line over 80 characters
4: Add Acked-by: Tejun Heo <tj@kernel.org>

Yang Dongsheng (2):
  cpuacct: rename parameter in cpuusage_write for readability
  cpuacct: split usage into user_usage and sys_usage.

 kernel/sched/cpuacct.c | 151 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 129 insertions(+), 22 deletions(-)

-- 
1.8.5.1

[toc] | [next] | [standalone]

#1350111 — [PATCH v2 2/2] cpuacct: split usage into user_usage and sys_usage.

From: Yang Dongsheng <yangds.fnst@cn.fujitsu.com>

Sometimes, cpuacct.usage is not detialed enough to user
to see how much usage a group used. We want to know how
much time it used in user mode and how much in kernel mode.

This patch introduce some more files to tell user these information.
 # ls /sys/fs/cgroup/cpuacct/cpuacct.usage*
 /sys/fs/cgroup/cpuacct/cpuacct.usage
 /sys/fs/cgroup/cpuacct/cpuacct.usage_percpu
 /sys/fs/cgroup/cpuacct/cpuacct.usage_user
 /sys/fs/cgroup/cpuacct/cpuacct.usage_percpu_user
 /sys/fs/cgroup/cpuacct/cpuacct.usage_sys
 /sys/fs/cgroup/cpuacct/cpuacct.usage_percpu_sys

From: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Acked-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 kernel/sched/cpuacct.c | 144 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 124 insertions(+), 20 deletions(-)

diff --git a/kernel/sched/cpuacct.c b/kernel/sched/cpuacct.c
index 9c2bbf7..f3446b8 100644
--- a/kernel/sched/cpuacct.c
+++ b/kernel/sched/cpuacct.c
@@ -25,11 +25,22 @@ enum cpuacct_stat_index {
 	CPUACCT_STAT_NSTATS,
 };
 
+enum cpuacct_usage_index {
+	CPUACCT_USAGE_USER,	/* ... user mode */
+	CPUACCT_USAGE_SYSTEM,	/* ... kernel mode */
+
+	CPUACCT_USAGE_NRUSAGE,
+};
+
+struct cpuacct_usage {
+	u64	usages[CPUACCT_USAGE_NRUSAGE];
+};
+
 /* track cpu usage of a group of tasks and its child groups */
 struct cpuacct {
 	struct cgroup_subsys_state css;
 	/* cpuusage holds pointer to a u64-type object on every cpu */
-	u64 __percpu *cpuusage;
+	struct cpuacct_usage __percpu *cpuusage;
 	struct kernel_cpustat __percpu *cpustat;
 };
 
@@ -49,7 +60,7 @@ static inline struct cpuacct *parent_ca(struct cpuacct *ca)
 	return css_ca(ca->css.parent);
 }
 
-static DEFINE_PER_CPU(u64, root_cpuacct_cpuusage);
+static DEFINE_PER_CPU(struct cpuacct_usage, root_cpuacct_cpuusage);
 static struct cpuacct root_cpuacct = {
 	.cpustat	= &kernel_cpustat,
 	.cpuusage	= &root_cpuacct_cpuusage,
@@ -68,7 +79,7 @@ cpuacct_css_alloc(struct cgroup_subsys_state *parent_css)
 	if (!ca)
 		goto out;
 
-	ca->cpuusage = alloc_percpu(u64);
+	ca->cpuusage = alloc_percpu(struct cpuacct_usage);
 	if (!ca->cpuusage)
 		goto out_free_ca;
 
@@ -96,54 +107,107 @@ static void cpuacct_css_free(struct cgroup_subsys_state *css)
 	kfree(ca);
 }
 
-static u64 cpuacct_cpuusage_read(struct cpuacct *ca, int cpu)
+static u64 cpuacct_cpuusage_read(struct cpuacct *ca, int cpu,
+				 enum cpuacct_usage_index index)
 {
-	u64 *cpuusage = per_cpu_ptr(ca->cpuusage, cpu);
-	u64 data;
+	struct cpuacct_usage *cpuusage = per_cpu_ptr(ca->cpuusage, cpu);
+	u64 data = 0;
+	int i = 0;
+
+	/*
+	 * We allow index == CPUACCT_USAGE_NRUSAGE here to read
+	 * the sum of suages.
+	 */
+	BUG_ON(index > CPUACCT_USAGE_NRUSAGE);
+
+	if (index == CPUACCT_USAGE_NRUSAGE) {
+		raw_spin_lock_irq(&cpu_rq(cpu)->lock);
+		for (i = 0; i < CPUACCT_USAGE_NRUSAGE; i++)
+			data += cpuusage->usages[i];
+		raw_spin_unlock_irq(&cpu_rq(cpu)->lock);
+
+		goto out;
+	}
 
 #ifndef CONFIG_64BIT
 	/*
 	 * Take rq->lock to make 64-bit read safe on 32-bit platforms.
 	 */
 	raw_spin_lock_irq(&cpu_rq(cpu)->lock);
-	data = *cpuusage;
+	data = cpuusage->usages[index];
 	raw_spin_unlock_irq(&cpu_rq(cpu)->lock);
 #else
-	data = *cpuusage;
+	data = cpuusage->usages[index];
 #endif
 
+out:
 	return data;
 }
 
-static void cpuacct_cpuusage_write(struct cpuacct *ca, int cpu, u64 val)
+static void cpuacct_cpuusage_write(struct cpuacct *ca, int cpu,
+				   enum cpuacct_usage_index index, u64 val)
 {
-	u64 *cpuusage = per_cpu_ptr(ca->cpuusage, cpu);
+	struct cpuacct_usage *cpuusage = per_cpu_ptr(ca->cpuusage, cpu);
+	int i = 0;
+
+	/*
+	 * We allow index == CPUACCT_USAGE_NRUSAGE here to write
+	 * val to each index of usages.
+	 */
+	BUG_ON(index > CPUACCT_USAGE_NRUSAGE);
+
+	if (index == CPUACCT_USAGE_NRUSAGE) {
+		raw_spin_lock_irq(&cpu_rq(cpu)->lock);
+		for (i = 0; i < CPUACCT_USAGE_NRUSAGE; i++)
+			cpuusage->usages[i] = val;
+		raw_spin_unlock_irq(&cpu_rq(cpu)->lock);
+
+		return;
+	}
 
 #ifndef CONFIG_64BIT
 	/*
 	 * Take rq->lock to make 64-bit write safe on 32-bit platforms.
 	 */
 	raw_spin_lock_irq(&cpu_rq(cpu)->lock);
-	*cpuusage = val;
+	cpuusage->usages[index] = val;
 	raw_spin_unlock_irq(&cpu_rq(cpu)->lock);
 #else
-	*cpuusage = val;
+	cpuusage->usages[index] = val;
 #endif
 }
 
 /* return total cpu usage (in nanoseconds) of a group */
-static u64 cpuusage_read(struct cgroup_subsys_state *css, struct cftype *cft)
+static u64 __cpuusage_read(struct cgroup_subsys_state *css,
+			   enum cpuacct_usage_index index)
 {
 	struct cpuacct *ca = css_ca(css);
 	u64 totalcpuusage = 0;
 	int i;
 
 	for_each_present_cpu(i)
-		totalcpuusage += cpuacct_cpuusage_read(ca, i);
+		totalcpuusage += cpuacct_cpuusage_read(ca, i, index);
 
 	return totalcpuusage;
 }
 
+static u64 cpuusage_user_read(struct cgroup_subsys_state *css,
+			      struct cftype *cft)
+{
+	return __cpuusage_read(css, CPUACCT_USAGE_USER);
+}
+
+static u64 cpuusage_sys_read(struct cgroup_subsys_state *css,
+			     struct cftype *cft)
+{
+	return __cpuusage_read(css, CPUACCT_USAGE_SYSTEM);
+}
+
+static u64 cpuusage_read(struct cgroup_subsys_state *css, struct cftype *cft)
+{
+	return __cpuusage_read(css, CPUACCT_USAGE_NRUSAGE);
+}
+
 static int cpuusage_write(struct cgroup_subsys_state *css, struct cftype *cft,
 			  u64 val)
 {
@@ -159,27 +223,44 @@ static int cpuusage_write(struct cgroup_subsys_state *css, struct cftype *cft,
 		goto out;
 	}
 
-	for_each_present_cpu(i)
-		cpuacct_cpuusage_write(ca, i, 0);
+	for_each_present_cpu(i) {
+		cpuacct_cpuusage_write(ca, i, CPUACCT_USAGE_NRUSAGE, 0);
+	}
 
 out:
 	return err;
 }
 
-static int cpuacct_percpu_seq_show(struct seq_file *m, void *V)
+static int __cpuacct_percpu_seq_show(struct seq_file *m,
+				     enum cpuacct_usage_index index)
 {
 	struct cpuacct *ca = css_ca(seq_css(m));
 	u64 percpu;
 	int i;
 
 	for_each_present_cpu(i) {
-		percpu = cpuacct_cpuusage_read(ca, i);
+		percpu = cpuacct_cpuusage_read(ca, i, index);
 		seq_printf(m, "%llu ", (unsigned long long) percpu);
 	}
 	seq_printf(m, "\n");
 	return 0;
 }
 
+static int cpuacct_percpu_user_seq_show(struct seq_file *m, void *V)
+{
+	return __cpuacct_percpu_seq_show(m, CPUACCT_USAGE_USER);
+}
+
+static int cpuacct_percpu_sys_seq_show(struct seq_file *m, void *V)
+{
+	return __cpuacct_percpu_seq_show(m, CPUACCT_USAGE_SYSTEM);
+}
+
+static int cpuacct_percpu_seq_show(struct seq_file *m, void *V)
+{
+	return __cpuacct_percpu_seq_show(m, CPUACCT_USAGE_NRUSAGE);
+}
+
 static const char * const cpuacct_stat_desc[] = {
 	[CPUACCT_STAT_USER] = "user",
 	[CPUACCT_STAT_SYSTEM] = "system",
@@ -220,10 +301,26 @@ static struct cftype files[] = {
 		.write_u64 = cpuusage_write,
 	},
 	{
+		.name = "usage_user",
+		.read_u64 = cpuusage_user_read,
+	},
+	{
+		.name = "usage_sys",
+		.read_u64 = cpuusage_sys_read,
+	},
+	{
 		.name = "usage_percpu",
 		.seq_show = cpuacct_percpu_seq_show,
 	},
 	{
+		.name = "usage_percpu_user",
+		.seq_show = cpuacct_percpu_user_seq_show,
+	},
+	{
+		.name = "usage_percpu_sys",
+		.seq_show = cpuacct_percpu_sys_seq_show,
+	},
+	{
 		.name = "stat",
 		.seq_show = cpuacct_stats_show,
 	},
@@ -239,6 +336,7 @@ void cpuacct_charge(struct task_struct *tsk, u64 cputime)
 {
 	struct cpuacct *ca;
 	int cpu;
+	int user_time;
 
 	cpu = task_cpu(tsk);
 
@@ -246,9 +344,15 @@ void cpuacct_charge(struct task_struct *tsk, u64 cputime)
 
 	ca = task_ca(tsk);
 
+	user_time = user_mode(task_pt_regs(tsk));
+
 	while (true) {
-		u64 *cpuusage = per_cpu_ptr(ca->cpuusage, cpu);
-		*cpuusage += cputime;
+		struct cpuacct_usage *cpuusage = per_cpu_ptr(ca->cpuusage, cpu);
+
+		if (user_time)
+			cpuusage->usages[CPUACCT_USAGE_USER] += cputime;
+		else
+			cpuusage->usages[CPUACCT_USAGE_SYSTEM] += cputime;
 
 		ca = parent_ca(ca);
 		if (!ca)
-- 
1.8.5.1

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

#1355119 — Re: [PATCH v2 2/2] cpuacct: split usage into user_usage and sys_usage.

On Fri, Mar 04, 2016 at 05:47:06PM +0800, Zhao Lei wrote:
> +static u64 cpuacct_cpuusage_read(struct cpuacct *ca, int cpu,
> +				 enum cpuacct_usage_index index)
>  {
> +	struct cpuacct_usage *cpuusage = per_cpu_ptr(ca->cpuusage, cpu);
> +	u64 data = 0;
> +	int i = 0;
> +
> +	/*
> +	 * We allow index == CPUACCT_USAGE_NRUSAGE here to read
> +	 * the sum of suages.
> +	 */
> +	BUG_ON(index > CPUACCT_USAGE_NRUSAGE);
> +
> +	if (index == CPUACCT_USAGE_NRUSAGE) {
> +		raw_spin_lock_irq(&cpu_rq(cpu)->lock);
> +		for (i = 0; i < CPUACCT_USAGE_NRUSAGE; i++)
> +			data += cpuusage->usages[i];
> +		raw_spin_unlock_irq(&cpu_rq(cpu)->lock);

Why do you unconditionally take the lock here? You really don't need it
on 64 bit.

> +
> +		goto out;
> +	}
>  
>  #ifndef CONFIG_64BIT
>  	/*
>  	 * Take rq->lock to make 64-bit read safe on 32-bit platforms.
>  	 */
>  	raw_spin_lock_irq(&cpu_rq(cpu)->lock);
> +	data = cpuusage->usages[index];
>  	raw_spin_unlock_irq(&cpu_rq(cpu)->lock);
>  #else
> +	data = cpuusage->usages[index];
>  #endif
>  
> +out:
>  	return data;
>  }
>  
> +static void cpuacct_cpuusage_write(struct cpuacct *ca, int cpu,
> +				   enum cpuacct_usage_index index, u64 val)
>  {
> +	struct cpuacct_usage *cpuusage = per_cpu_ptr(ca->cpuusage, cpu);
> +	int i = 0;
> +
> +	/*
> +	 * We allow index == CPUACCT_USAGE_NRUSAGE here to write
> +	 * val to each index of usages.
> +	 */
> +	BUG_ON(index > CPUACCT_USAGE_NRUSAGE);
> +
> +	if (index == CPUACCT_USAGE_NRUSAGE) {
> +		raw_spin_lock_irq(&cpu_rq(cpu)->lock);
> +		for (i = 0; i < CPUACCT_USAGE_NRUSAGE; i++)
> +			cpuusage->usages[i] = val;
> +		raw_spin_unlock_irq(&cpu_rq(cpu)->lock);
> +
> +		return;
> +	}

Same for the above, and the below is dead code, you only ever call this
with NRUSAGE.

>  #ifndef CONFIG_64BIT
>  	/*
>  	 * Take rq->lock to make 64-bit write safe on 32-bit platforms.
>  	 */
>  	raw_spin_lock_irq(&cpu_rq(cpu)->lock);
> +	cpuusage->usages[index] = val;
>  	raw_spin_unlock_irq(&cpu_rq(cpu)->lock);
>  #else
> +	cpuusage->usages[index] = val;
>  #endif
>  }
>  

> @@ -246,9 +344,15 @@ void cpuacct_charge(struct task_struct *tsk, u64 cputime)
>  
>  	ca = task_ca(tsk);
>  
> +	user_time = user_mode(task_pt_regs(tsk));
> +
>  	while (true) {
> -		u64 *cpuusage = per_cpu_ptr(ca->cpuusage, cpu);
> -		*cpuusage += cputime;
> +		struct cpuacct_usage *cpuusage = per_cpu_ptr(ca->cpuusage, cpu);
> +
> +		if (user_time)
> +			cpuusage->usages[CPUACCT_USAGE_USER] += cputime;
> +		else
> +			cpuusage->usages[CPUACCT_USAGE_SYSTEM] += cputime;
>  
>  		ca = parent_ca(ca);
>  		if (!ca)

Have you tried to measure the performance impact of this?

Also, that code seems particularly silly for not using this_cpu_ptr().
After all, we only ever call this on current.

Also that ca iteration looks daft, should we fix that to read:

	for (ca = task_ca(tsk); ca; ca = parent_ca(ca))

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

#1358817 — RE: [PATCH v2 2/2] cpuacct: split usage into user_usage and sys_usage.

Hi, Peter Zijlstra

Thanks for so detailed review.

> -----Original Message-----
> From: Peter Zijlstra [mailto:peterz@infradead.org]
> Sent: Thursday, March 10, 2016 9:27 PM
> To: Zhao Lei <zhaolei@cn.fujitsu.com>
> Cc: cgroups@vger.kernel.org; linux-kernel@vger.kernel.org;
> mingo@redhat.com; tj@kernel.org; Yang Dongsheng
> <yangds.fnst@cn.fujitsu.com>
> Subject: Re: [PATCH v2 2/2] cpuacct: split usage into user_usage and
> sys_usage.
> 
> On Fri, Mar 04, 2016 at 05:47:06PM +0800, Zhao Lei wrote:
> > +static u64 cpuacct_cpuusage_read(struct cpuacct *ca, int cpu,
> > +				 enum cpuacct_usage_index index)
> >  {
> > +	struct cpuacct_usage *cpuusage = per_cpu_ptr(ca->cpuusage, cpu);
> > +	u64 data = 0;
> > +	int i = 0;
> > +
> > +	/*
> > +	 * We allow index == CPUACCT_USAGE_NRUSAGE here to read
> > +	 * the sum of suages.
> > +	 */
> > +	BUG_ON(index > CPUACCT_USAGE_NRUSAGE);
> > +
> > +	if (index == CPUACCT_USAGE_NRUSAGE) {
> > +		raw_spin_lock_irq(&cpu_rq(cpu)->lock);
> > +		for (i = 0; i < CPUACCT_USAGE_NRUSAGE; i++)
> > +			data += cpuusage->usages[i];
> > +		raw_spin_unlock_irq(&cpu_rq(cpu)->lock);
> 
> Why do you unconditionally take the lock here? You really don't need it
> on 64 bit.
> 
Yes, will fix.

> > +
> > +		goto out;
> > +	}
> >
> >  #ifndef CONFIG_64BIT
> >  	/*
> >  	 * Take rq->lock to make 64-bit read safe on 32-bit platforms.
> >  	 */
> >  	raw_spin_lock_irq(&cpu_rq(cpu)->lock);
> > +	data = cpuusage->usages[index];
> >  	raw_spin_unlock_irq(&cpu_rq(cpu)->lock);
> >  #else
> > +	data = cpuusage->usages[index];
> >  #endif
> >
> > +out:
> >  	return data;
> >  }
> >
> > +static void cpuacct_cpuusage_write(struct cpuacct *ca, int cpu,
> > +				   enum cpuacct_usage_index index, u64 val)
> >  {
> > +	struct cpuacct_usage *cpuusage = per_cpu_ptr(ca->cpuusage, cpu);
> > +	int i = 0;
> > +
> > +	/*
> > +	 * We allow index == CPUACCT_USAGE_NRUSAGE here to write
> > +	 * val to each index of usages.
> > +	 */
> > +	BUG_ON(index > CPUACCT_USAGE_NRUSAGE);
> > +
> > +	if (index == CPUACCT_USAGE_NRUSAGE) {
> > +		raw_spin_lock_irq(&cpu_rq(cpu)->lock);
> > +		for (i = 0; i < CPUACCT_USAGE_NRUSAGE; i++)
> > +			cpuusage->usages[i] = val;
> > +		raw_spin_unlock_irq(&cpu_rq(cpu)->lock);
> > +
> > +		return;
> > +	}
> 
> Same for the above, and the below is dead code, you only ever call this
> with NRUSAGE.
> 
Good point.

> >  #ifndef CONFIG_64BIT
> >  	/*
> >  	 * Take rq->lock to make 64-bit write safe on 32-bit platforms.
> >  	 */
> >  	raw_spin_lock_irq(&cpu_rq(cpu)->lock);
> > +	cpuusage->usages[index] = val;
> >  	raw_spin_unlock_irq(&cpu_rq(cpu)->lock);
> >  #else
> > +	cpuusage->usages[index] = val;
> >  #endif
> >  }
> >
> 
> > @@ -246,9 +344,15 @@ void cpuacct_charge(struct task_struct *tsk, u64
> cputime)
> >
> >  	ca = task_ca(tsk);
> >
> > +	user_time = user_mode(task_pt_regs(tsk));
> > +
> >  	while (true) {
> > -		u64 *cpuusage = per_cpu_ptr(ca->cpuusage, cpu);
> > -		*cpuusage += cputime;
> > +		struct cpuacct_usage *cpuusage = per_cpu_ptr(ca->cpuusage, cpu);
> > +
> > +		if (user_time)
> > +			cpuusage->usages[CPUACCT_USAGE_USER] += cputime;
> > +		else
> > +			cpuusage->usages[CPUACCT_USAGE_SYSTEM] += cputime;
> >
> >  		ca = parent_ca(ca);
> >  		if (!ca)
> 
> Have you tried to measure the performance impact of this?
> 
> Also, that code seems particularly silly for not using this_cpu_ptr().
> After all, we only ever call this on current.
> 
> Also that ca iteration looks daft, should we fix that to read:
> 
> 	for (ca = task_ca(tsk); ca; ca = parent_ca(ca))
I'll rewrite this code block.

Thanks
Zhaolei

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web