[patch 0/2] sched: cpuacct: minor cleanups and reset for cpuusage

[patch 0/2] sched: cpuacct: minor cleanups and reset for cpuusage

[Dhaval Giani]

This patchset cleans up the cpuacct subsystem variable names and gives
the capability to reset the cpuusage variable for the group.

Thanks,
--
regards,
Dhaval

[patch 1/2] sched: cleanup cpuacct variable names

[Dhaval Giani]

[-- Attachment #1: cleanup-cpuacct.patch --]
[-- Type: text/plain, Size: 2306 bytes --]

Change the variable names to the common convention for the cpuacct
subsystem.

Signed-off-by: Dhaval Giani <dhaval@linux.vnet.ibm.com>

---
 kernel/sched.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

Index: linux-2.6/kernel/sched.c
===================================================================
--- linux-2.6.orig/kernel/sched.c	2008-02-27 16:21:15.000000000 +0530
+++ linux-2.6/kernel/sched.c	2008-02-28 20:05:27.000000000 +0530
@@ -8180,9 +8180,9 @@ struct cpuacct {
 struct cgroup_subsys cpuacct_subsys;
 
 /* return cpu accounting group corresponding to this container */
-static inline struct cpuacct *cgroup_ca(struct cgroup *cont)
+static inline struct cpuacct *cgroup_ca(struct cgroup *cgrp)
 {
-	return container_of(cgroup_subsys_state(cont, cpuacct_subsys_id),
+	return container_of(cgroup_subsys_state(cgrp, cpuacct_subsys_id),
 			    struct cpuacct, css);
 }
 
@@ -8195,7 +8195,7 @@ static inline struct cpuacct *task_ca(st
 
 /* create a new cpu accounting group */
 static struct cgroup_subsys_state *cpuacct_create(
-	struct cgroup_subsys *ss, struct cgroup *cont)
+	struct cgroup_subsys *ss, struct cgroup *cgrp)
 {
 	struct cpuacct *ca = kzalloc(sizeof(*ca), GFP_KERNEL);
 
@@ -8213,18 +8213,18 @@ static struct cgroup_subsys_state *cpuac
 
 /* destroy an existing cpu accounting group */
 static void
-cpuacct_destroy(struct cgroup_subsys *ss, struct cgroup *cont)
+cpuacct_destroy(struct cgroup_subsys *ss, struct cgroup *cgrp)
 {
-	struct cpuacct *ca = cgroup_ca(cont);
+	struct cpuacct *ca = cgroup_ca(cgrp);
 
 	free_percpu(ca->cpuusage);
 	kfree(ca);
 }
 
 /* return total cpu usage (in nanoseconds) of a group */
-static u64 cpuusage_read(struct cgroup *cont, struct cftype *cft)
+static u64 cpuusage_read(struct cgroup *cgrp, struct cftype *cft)
 {
-	struct cpuacct *ca = cgroup_ca(cont);
+	struct cpuacct *ca = cgroup_ca(cgrp);
 	u64 totalcpuusage = 0;
 	int i;
 
@@ -8250,9 +8250,9 @@ static struct cftype files[] = {
 	},
 };
 
-static int cpuacct_populate(struct cgroup_subsys *ss, struct cgroup *cont)
+static int cpuacct_populate(struct cgroup_subsys *ss, struct cgroup *cgrp)
 {
-	return cgroup_add_files(cont, ss, files, ARRAY_SIZE(files));
+	return cgroup_add_files(cgrp, ss, files, ARRAY_SIZE(files));
 }
 
 /*

--
regards,
Dhaval

Re: [patch 1/2] sched: cleanup cpuacct variable names

[Balbir Singh]

* Dhaval Giani <dhaval@linux.vnet.ibm.com> [2008-02-29 10:02:43]:

> Change the variable names to the common convention for the cpuacct
> subsystem.
> 
> Signed-off-by: Dhaval Giani <dhaval@linux.vnet.ibm.com>
> 
> ---
>  kernel/sched.c |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)
> 
> Index: linux-2.6/kernel/sched.c
> ===================================================================
> --- linux-2.6.orig/kernel/sched.c	2008-02-27 16:21:15.000000000 +0530
> +++ linux-2.6/kernel/sched.c	2008-02-28 20:05:27.000000000 +0530
> @@ -8180,9 +8180,9 @@ struct cpuacct {
>  struct cgroup_subsys cpuacct_subsys;
> 
>  /* return cpu accounting group corresponding to this container */
> -static inline struct cpuacct *cgroup_ca(struct cgroup *cont)
> +static inline struct cpuacct *cgroup_ca(struct cgroup *cgrp)
>  {
> -	return container_of(cgroup_subsys_state(cont, cpuacct_subsys_id),
> +	return container_of(cgroup_subsys_state(cgrp, cpuacct_subsys_id),
>  			    struct cpuacct, css);
>  }
> 
> @@ -8195,7 +8195,7 @@ static inline struct cpuacct *task_ca(st
> 
>  /* create a new cpu accounting group */
>  static struct cgroup_subsys_state *cpuacct_create(
> -	struct cgroup_subsys *ss, struct cgroup *cont)
> +	struct cgroup_subsys *ss, struct cgroup *cgrp)
>  {
>  	struct cpuacct *ca = kzalloc(sizeof(*ca), GFP_KERNEL);
> 
> @@ -8213,18 +8213,18 @@ static struct cgroup_subsys_state *cpuac
> 
>  /* destroy an existing cpu accounting group */
>  static void
> -cpuacct_destroy(struct cgroup_subsys *ss, struct cgroup *cont)
> +cpuacct_destroy(struct cgroup_subsys *ss, struct cgroup *cgrp)
>  {
> -	struct cpuacct *ca = cgroup_ca(cont);
> +	struct cpuacct *ca = cgroup_ca(cgrp);
> 
>  	free_percpu(ca->cpuusage);
>  	kfree(ca);
>  }
> 
>  /* return total cpu usage (in nanoseconds) of a group */
> -static u64 cpuusage_read(struct cgroup *cont, struct cftype *cft)
> +static u64 cpuusage_read(struct cgroup *cgrp, struct cftype *cft)
>  {
> -	struct cpuacct *ca = cgroup_ca(cont);
> +	struct cpuacct *ca = cgroup_ca(cgrp);
>  	u64 totalcpuusage = 0;
>  	int i;
> 
> @@ -8250,9 +8250,9 @@ static struct cftype files[] = {
>  	},
>  };
> 
> -static int cpuacct_populate(struct cgroup_subsys *ss, struct cgroup *cont)
> +static int cpuacct_populate(struct cgroup_subsys *ss, struct cgroup *cgrp)
>  {
> -	return cgroup_add_files(cont, ss, files, ARRAY_SIZE(files));
> +	return cgroup_add_files(cgrp, ss, files, ARRAY_SIZE(files));
>  }
> 
>  /*
> 
>

Looks good

Acked-by: Balbir Singh <balbir@linux.vnet.ibm.com> 

-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

Re: [patch 1/2] sched: cleanup cpuacct variable names

[Ingo Molnar]

* Dhaval Giani <dhaval@linux.vnet.ibm.com> wrote:

> Change the variable names to the common convention for the cpuacct 
> subsystem.
> 
> Signed-off-by: Dhaval Giani <dhaval@linux.vnet.ibm.com>

thanks, applied.

	Ingo

[patch 2/2] sched: allow cpuacct stats to be reset

[Dhaval Giani]

[-- Attachment #1: allow-reset-of-sched-stats.patch --]
[-- Type: text/plain, Size: 1250 bytes --]

Currently the schedstats implementation does not allow the statistics
to be reset. This patch aims to allow that.

echo 0 > cpuacct.usage

resets the usage. Any other value is not allowed and returns -EINVAL.

Signed-off-by: Dhaval Giani <dhaval@linux.vnet.ibm.com>

---
 kernel/sched.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+)

Index: linux-2.6/kernel/sched.c
===================================================================
--- linux-2.6.orig/kernel/sched.c	2008-02-28 20:05:27.000000000 +0530
+++ linux-2.6/kernel/sched.c	2008-02-28 20:05:30.000000000 +0530
@@ -8243,10 +8243,34 @@ static u64 cpuusage_read(struct cgroup *
 	return totalcpuusage;
 }
 
+static int cpuusage_write(struct cgroup *cgrp, struct cftype *cftype,
+								u64 reset)
+{
+	struct cpuacct *ca = cgroup_ca(cgrp);
+	int err = 0;
+	int i;
+
+	if (reset) {
+		err = -EINVAL;
+		goto out;
+	}
+
+	for_each_possible_cpu(i) {
+		u64 *cpuusage = percpu_ptr(ca->cpuusage, i);
+
+		spin_lock_irq(&cpu_rq(i)->lock);
+		*cpuusage = 0;
+		spin_unlock_irq(&cpu_rq(i)->lock);
+	}
+out:
+	return err;
+}
+
 static struct cftype files[] = {
 	{
 		.name = "usage",
 		.read_uint = cpuusage_read,
+		.write_uint = cpuusage_write,
 	},
 };
 

--
regards,
Dhaval

Re: [patch 2/2] sched: allow cpuacct stats to be reset

[Paul Menage]

On Thu, Feb 28, 2008 at 8:32 PM, Dhaval Giani <dhaval@linux.vnet.ibm.com> wrote:
> Currently the schedstats implementation does not allow the statistics
>  to be reset. This patch aims to allow that.
>
>  echo 0 > cpuacct.usage
>
>  resets the usage. Any other value is not allowed and returns -EINVAL.
>
>  Signed-off-by: Dhaval Giani <dhaval@linux.vnet.ibm.com>
>
>  ---
>   kernel/sched.c |   24 ++++++++++++++++++++++++
>   1 files changed, 24 insertions(+)
>
>  Index: linux-2.6/kernel/sched.c
>  ===================================================================
>  --- linux-2.6.orig/kernel/sched.c       2008-02-28 20:05:27.000000000 +0530
>  +++ linux-2.6/kernel/sched.c    2008-02-28 20:05:30.000000000 +0530
>  @@ -8243,10 +8243,34 @@ static u64 cpuusage_read(struct cgroup *
>         return totalcpuusage;
>   }
>
>  +static int cpuusage_write(struct cgroup *cgrp, struct cftype *cftype,
>  +                                                               u64 reset)
>  +{
>  +       struct cpuacct *ca = cgroup_ca(cgrp);
>  +       int err = 0;
>  +       int i;
>  +
>  +       if (reset) {
>  +               err = -EINVAL;
>  +               goto out;
>  +       }
>  +
>  +       for_each_possible_cpu(i) {
>  +               u64 *cpuusage = percpu_ptr(ca->cpuusage, i);
>  +
>  +               spin_lock_irq(&cpu_rq(i)->lock);
>  +               *cpuusage = 0;
>  +               spin_unlock_irq(&cpu_rq(i)->lock);
>  +       }
>  +out:
>  +       return err;
>  +}
>  +
>   static struct cftype files[] = {
>         {
>                 .name = "usage",
>                 .read_uint = cpuusage_read,
>  +               .write_uint = cpuusage_write,
>         },
>   };
>

Can I suggest, in order to be more generic, that this patch instead
set each CPU's counter to the written value divided by the number of
CPUs? (Either forgetting the remainder, or spreading it among the
first few CPUs).

Paul

Re: [patch 2/2] sched: allow cpuacct stats to be reset

[Srivatsa Vaddagiri]

On Thu, Feb 28, 2008 at 09:48:51PM -0800, Paul Menage wrote:
> Can I suggest, in order to be more generic, that this patch instead
> set each CPU's counter to the written value divided by the number of
> CPUs? (Either forgetting the remainder, or spreading it among the
> first few CPUs).

Paul,
	When would you want to reset the counters to non-zero value?
What would be its usecase?

-- 
Regards,
vatsa

Re: [patch 2/2] sched: allow cpuacct stats to be reset

[Dhaval Giani]

On Thu, Feb 28, 2008 at 09:48:51PM -0800, Paul Menage wrote:
> On Thu, Feb 28, 2008 at 8:32 PM, Dhaval Giani <dhaval@linux.vnet.ibm.com> wrote:
> 
> Can I suggest, in order to be more generic, that this patch instead
> set each CPU's counter to the written value divided by the number of
> CPUs? (Either forgetting the remainder, or spreading it among the
> first few CPUs).
> 

This patch is only allowing a reset of the stats. In what sort of a
situation would one be looking for changing the usage value?

thanks,
-- 
regards,
Dhaval

Re: [patch 2/2] sched: allow cpuacct stats to be reset

[Paul Menage]

On Thu, Feb 28, 2008 at 10:04 PM, Dhaval Giani
<dhaval@linux.vnet.ibm.com> wrote:
> On Thu, Feb 28, 2008 at 09:48:51PM -0800, Paul Menage wrote:
>
> > On Thu, Feb 28, 2008 at 8:32 PM, Dhaval Giani <dhaval@linux.vnet.ibm.com> wrote:
>  >
>
> > Can I suggest, in order to be more generic, that this patch instead
>  > set each CPU's counter to the written value divided by the number of
>  > CPUs? (Either forgetting the remainder, or spreading it among the
>  > first few CPUs).
>  >
>
>  This patch is only allowing a reset of the stats. In what sort of a
>  situation would one be looking for changing the usage value?

How about Checkpoint/Restart?

It seems that if you're going to the effort to notice a non-zero value
and return an error, it wouldn't be much more effort to do something
useful with that non-zero value instead.

Paul

Re: [patch 2/2] sched: allow cpuacct stats to be reset

[Peter Zijlstra]

On Fri, 2008-02-29 at 02:08 -0800, Paul Menage wrote:
> On Thu, Feb 28, 2008 at 10:04 PM, Dhaval Giani
> <dhaval@linux.vnet.ibm.com> wrote:
> > On Thu, Feb 28, 2008 at 09:48:51PM -0800, Paul Menage wrote:
> >
> > > On Thu, Feb 28, 2008 at 8:32 PM, Dhaval Giani <dhaval@linux.vnet.ibm.com> wrote:
> >  >
> >
> > > Can I suggest, in order to be more generic, that this patch instead
> >  > set each CPU's counter to the written value divided by the number of
> >  > CPUs? (Either forgetting the remainder, or spreading it among the
> >  > first few CPUs).
> >  >
> >
> >  This patch is only allowing a reset of the stats. In what sort of a
> >  situation would one be looking for changing the usage value?
> 
> How about Checkpoint/Restart?

Shouldn't that be a kernel side thing? And from the kernel you can put
back any value you fancy.

Re: [patch 2/2] sched: allow cpuacct stats to be reset

[Balbir Singh]

* Dhaval Giani <dhaval@linux.vnet.ibm.com> [2008-02-29 10:02:44]:

> Currently the schedstats implementation does not allow the statistics
> to be reset. This patch aims to allow that.
> 
> echo 0 > cpuacct.usage
> 
> resets the usage. Any other value is not allowed and returns -EINVAL.
> 
> Signed-off-by: Dhaval Giani <dhaval@linux.vnet.ibm.com>
> 
> ---
>  kernel/sched.c |   24 ++++++++++++++++++++++++
>  1 files changed, 24 insertions(+)
> 
> Index: linux-2.6/kernel/sched.c
> ===================================================================
> --- linux-2.6.orig/kernel/sched.c	2008-02-28 20:05:27.000000000 +0530
> +++ linux-2.6/kernel/sched.c	2008-02-28 20:05:30.000000000 +0530
> @@ -8243,10 +8243,34 @@ static u64 cpuusage_read(struct cgroup *
>  	return totalcpuusage;
>  }
> 
> +static int cpuusage_write(struct cgroup *cgrp, struct cftype *cftype,
> +								u64 reset)
> +{
> +	struct cpuacct *ca = cgroup_ca(cgrp);
> +	int err = 0;
> +	int i;
> +
> +	if (reset) {
> +		err = -EINVAL;
> +		goto out;
> +	}
> +

Why not use

        if (reset)
                return -EINVAL;


> +	for_each_possible_cpu(i) {
> +		u64 *cpuusage = percpu_ptr(ca->cpuusage, i);
> +
> +		spin_lock_irq(&cpu_rq(i)->lock);
> +		*cpuusage = 0;
> +		spin_unlock_irq(&cpu_rq(i)->lock);
> +	}
> +out:
> +	return err;
> +}
> +
>  static struct cftype files[] = {
>  	{
>  		.name = "usage",
>  		.read_uint = cpuusage_read,
> +		.write_uint = cpuusage_write,
>  	},
>  };
> 
> 

Looks good

Acked-by: Balbir Singh <balbir@linux.vnet.ibm.com>

-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

Re: [patch 2/2] sched: allow cpuacct stats to be reset

[Ingo Molnar]

* Dhaval Giani <dhaval@linux.vnet.ibm.com> wrote:

> Currently the schedstats implementation does not allow the statistics 
> to be reset. This patch aims to allow that.
> 
> echo 0 > cpuacct.usage
> 
> resets the usage. Any other value is not allowed and returns -EINVAL.

i've applied this for now, please send any updates that might come out 
of the interface discussions.

	Ingo