[patch] export percpu cpuacct cgroup stats

[patch] export percpu cpuacct cgroup stats

[Ken Chen]

This patch export per-cpu CPU cycle usage for a given cpuacct cgroup.
There is a need for a user space monitor daemon to track group CPU
usage on per-cpu base.  It is also useful for monitor CFS load
balancer behavior by tracking per CPU group usage.


Signed-off-by: Ken Chen <kenchen@google.com>

diff --git a/kernel/sched.c b/kernel/sched.c
index b7480fb..cd78948 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -9345,12 +9345,34 @@ out:
 	return err;
 }

+static int cpuacct_percpu_seq_read(struct cgroup *cgroup, struct cftype *cft,
+				   struct seq_file *m)
+{
+	struct cpuacct *ca = cgroup_ca(cgroup);
+	u64 percpu;
+	int i;
+
+	for_each_possible_cpu(i) {
+		spin_lock_irq(&cpu_rq(i)->lock);
+		percpu = *percpu_ptr(ca->cpuusage, i);
+		spin_unlock_irq(&cpu_rq(i)->lock);
+		seq_printf(m, "%lld ", percpu);
+	}
+	seq_printf(m, "\n");
+	return 0;
+}
+
 static struct cftype files[] = {
 	{
 		.name = "usage",
 		.read_u64 = cpuusage_read,
 		.write_u64 = cpuusage_write,
 	},
+	{
+		.name = "percpu",
+		.read_seq_string = cpuacct_percpu_seq_read,
+	},
+
 };

 static int cpuacct_populate(struct cgroup_subsys *ss, struct cgroup *cgrp)

Re: [patch] export percpu cpuacct cgroup stats

[Li Zefan]

(I guess it's more appropriate to ask Ingo to pick up this patch)

Ken Chen wrote:
> This patch export per-cpu CPU cycle usage for a given cpuacct cgroup.
> There is a need for a user space monitor daemon to track group CPU
> usage on per-cpu base.  It is also useful for monitor CFS load
> balancer behavior by tracking per CPU group usage.
> 
> 
> Signed-off-by: Ken Chen <kenchen@google.com>
> 
> diff --git a/kernel/sched.c b/kernel/sched.c
> index b7480fb..cd78948 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -9345,12 +9345,34 @@ out:
>  	return err;
>  }
> 
> +static int cpuacct_percpu_seq_read(struct cgroup *cgroup, struct cftype *cft,
> +				   struct seq_file *m)
> +{
> +	struct cpuacct *ca = cgroup_ca(cgroup);
> +	u64 percpu;
> +	int i;
> +
> +	for_each_possible_cpu(i) {
> +		spin_lock_irq(&cpu_rq(i)->lock);
> +		percpu = *percpu_ptr(ca->cpuusage, i);
> +		spin_unlock_irq(&cpu_rq(i)->lock);
> +		seq_printf(m, "%lld ", percpu);

Should be %llu. And if we don't cast percpu to unsigned long long, will it trigger
compile warning in some archs?

> +	}
> +	seq_printf(m, "\n");
> +	return 0;
> +}
> +
>  static struct cftype files[] = {
>  	{
>  		.name = "usage",
>  		.read_u64 = cpuusage_read,
>  		.write_u64 = cpuusage_write,
>  	},
> +	{
> +		.name = "percpu",

IMO percpu_usage is a better name.

> +		.read_seq_string = cpuacct_percpu_seq_read,
> +	},
> +
>  };
> 

Re: [patch] export percpu cpuacct cgroup stats

[Ingo Molnar]

* Li Zefan <lizf@cn.fujitsu.com> wrote:

> (I guess it's more appropriate to ask Ingo to pick up this patch)
> 
> Ken Chen wrote:
> > This patch export per-cpu CPU cycle usage for a given cpuacct cgroup.
> > There is a need for a user space monitor daemon to track group CPU
> > usage on per-cpu base.  It is also useful for monitor CFS load
> > balancer behavior by tracking per CPU group usage.
> > 
> > 
> > Signed-off-by: Ken Chen <kenchen@google.com>
> > 
> > diff --git a/kernel/sched.c b/kernel/sched.c
> > index b7480fb..cd78948 100644
> > --- a/kernel/sched.c
> > +++ b/kernel/sched.c
> > @@ -9345,12 +9345,34 @@ out:
> >  	return err;
> >  }
> > 
> > +static int cpuacct_percpu_seq_read(struct cgroup *cgroup, struct cftype *cft,
> > +				   struct seq_file *m)
> > +{
> > +	struct cpuacct *ca = cgroup_ca(cgroup);
> > +	u64 percpu;
> > +	int i;
> > +
> > +	for_each_possible_cpu(i) {
> > +		spin_lock_irq(&cpu_rq(i)->lock);
> > +		percpu = *percpu_ptr(ca->cpuusage, i);
> > +		spin_unlock_irq(&cpu_rq(i)->lock);
> > +		seq_printf(m, "%lld ", percpu);
> 
> Should be %llu. And if we don't cast percpu to unsigned long long, will 
> it trigger compile warning in some archs?

yes.

> > +	}
> > +	seq_printf(m, "\n");
> > +	return 0;
> > +}
> > +
> >  static struct cftype files[] = {
> >  	{
> >  		.name = "usage",
> >  		.read_u64 = cpuusage_read,
> >  		.write_u64 = cpuusage_write,
> >  	},
> > +	{
> > +		.name = "percpu",
> 
> IMO percpu_usage is a better name.

agreed. Looks good otherwise - will wait for v2 of the patch.

	Ingo

Re: [patch] export percpu cpuacct cgroup stats

[Paul Menage]

On Wed, Dec 3, 2008 at 9:34 PM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>> +
>>  static struct cftype files[] = {
>>       {
>>               .name = "usage",
>>               .read_u64 = cpuusage_read,
>>               .write_u64 = cpuusage_write,
>>       },
>> +     {
>> +             .name = "percpu",
>
> IMO percpu_usage is a better name.
>

Maybe usage_percpu ?

That fits in with the style used in e.g. memory.limit_in_bytes where
the first word is the important quantity, and the subsequent words
indicate some aspect of the quantity.

Paul

Re: [patch] export percpu cpuacct cgroup stats

[Ken Chen]

On Wed, Dec 3, 2008 at 9:34 PM, Li Zefan wrote:
>> +             seq_printf(m, "%lld ", percpu);
>
> Should be %llu. And if we don't cast percpu to unsigned long long, will
> it trigger compile warning in some archs?


On Thu, Dec 4, 2008 at 3:02 PM, Paul Menage wrote:
>> IMO percpu_usage is a better name.
>
> Maybe usage_percpu ?


Thank you all for your comments.  Patch updated.


=====

This patch export per-cpu CPU cycle usage for a given cpuacct cgroup.
There is a need for a user space monitor daemon to track group CPU
usage on per-cpu base.  It is also useful for monitor CFS load
balancer behavior by tracking per CPU group usage.


Signed-off-by: Ken Chen <kenchen@google.com>

diff --git a/kernel/sched.c b/kernel/sched.c
index b7480fb..e32c094 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -9345,12 +9345,34 @@ out:
 	return err;
 }

+static int cpuacct_percpu_seq_read(struct cgroup *cgroup, struct cftype *cft,
+				   struct seq_file *m)
+{
+	struct cpuacct *ca = cgroup_ca(cgroup);
+	u64 percpu;
+	int i;
+
+	for_each_possible_cpu(i) {
+		spin_lock_irq(&cpu_rq(i)->lock);
+		percpu = *percpu_ptr(ca->cpuusage, i);
+		spin_unlock_irq(&cpu_rq(i)->lock);
+		seq_printf(m, "%llu ", percpu);
+	}
+	seq_printf(m, "\n");
+	return 0;
+}
+
 static struct cftype files[] = {
 	{
 		.name = "usage",
 		.read_u64 = cpuusage_read,
 		.write_u64 = cpuusage_write,
 	},
+	{
+		.name = "usage_percpu",
+		.read_seq_string = cpuacct_percpu_seq_read,
+	},
+
 };

 static int cpuacct_populate(struct cgroup_subsys *ss, struct cgroup *cgrp)

Re: [patch] export percpu cpuacct cgroup stats

[Li Zefan]

> +static int cpuacct_percpu_seq_read(struct cgroup *cgroup, struct cftype *cft,
> +				   struct seq_file *m)
> +{
> +	struct cpuacct *ca = cgroup_ca(cgroup);
> +	u64 percpu;
> +	int i;
> +
> +	for_each_possible_cpu(i) {
> +		spin_lock_irq(&cpu_rq(i)->lock);
> +		percpu = *percpu_ptr(ca->cpuusage, i);
> +		spin_unlock_irq(&cpu_rq(i)->lock);
> +		seq_printf(m, "%llu ", percpu);

Should be:
+		seq_printf(m, "%llu ", (unsigned long long)percpu);

Otherwsise:
kernel/sched.c: In function ‘cpuacct_percpu_seq_read’:
kernel/sched.c:9359: warning: format ‘%llu’ expects type ‘long long unsigned int’, but argument 3 has type ‘u64’

> +	}
> +	seq_printf(m, "\n");
> +	return 0;
> +}

Re: [patch] export percpu cpuacct cgroup stats

[Ken Chen]

On Thu, Dec 4, 2008 at 11:54 PM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>> +             seq_printf(m, "%llu ", percpu);
>
> Should be:
> +               seq_printf(m, "%llu ", (unsigned long long)percpu);
>
> Otherwsise:
> kernel/sched.c: In function 'cpuacct_percpu_seq_read':
> kernel/sched.c:9359: warning: format '%llu' expects type 'long long unsigned int',
> but argument 3 has type 'u64'

huh, I was just wondering about the type case in cgroup_read_u64()
when I was looking at it earlier.  That explains it.  Diff patch
attached.

diff --git a/kernel/sched.c b/kernel/sched.c
index e32c094..055c54f 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -9356,7 +9356,7 @@ static int cpuacct_percpu_seq_read
 		spin_lock_irq(&cpu_rq(i)->lock);
 		percpu = *percpu_ptr(ca->cpuusage, i);
 		spin_unlock_irq(&cpu_rq(i)->lock);
-		seq_printf(m, "%llu ", percpu);
+		seq_printf(m, "%llu ", (unsigned long long) percpu);
 	}
 	seq_printf(m, "\n");
 	return 0;

Re: [patch] export percpu cpuacct cgroup stats

[Li Zefan]

Ken Chen wrote:
> On Thu, Dec 4, 2008 at 11:54 PM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>>> +             seq_printf(m, "%llu ", percpu);
>> Should be:
>> +               seq_printf(m, "%llu ", (unsigned long long)percpu);
>>
>> Otherwsise:
>> kernel/sched.c: In function 'cpuacct_percpu_seq_read':
>> kernel/sched.c:9359: warning: format '%llu' expects type 'long long unsigned int',
>> but argument 3 has type 'u64'
> 
> huh, I was just wondering about the type case in cgroup_read_u64()
> when I was looking at it earlier.  That explains it.  Diff patch
> attached.
> 

Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>

> diff --git a/kernel/sched.c b/kernel/sched.c
> index e32c094..055c54f 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -9356,7 +9356,7 @@ static int cpuacct_percpu_seq_read
>  		spin_lock_irq(&cpu_rq(i)->lock);
>  		percpu = *percpu_ptr(ca->cpuusage, i);
>  		spin_unlock_irq(&cpu_rq(i)->lock);
> -		seq_printf(m, "%llu ", percpu);
> +		seq_printf(m, "%llu ", (unsigned long long) percpu);
>  	}
>  	seq_printf(m, "\n");
>  	return 0;
> 
> 

Re: [patch] export percpu cpuacct cgroup stats

[Ingo Molnar]

* Li Zefan <lizf@cn.fujitsu.com> wrote:

> Ken Chen wrote:
> > On Thu, Dec 4, 2008 at 11:54 PM, Li Zefan <lizf@cn.fujitsu.com> wrote:
> >>> +             seq_printf(m, "%llu ", percpu);
> >> Should be:
> >> +               seq_printf(m, "%llu ", (unsigned long long)percpu);
> >>
> >> Otherwsise:
> >> kernel/sched.c: In function 'cpuacct_percpu_seq_read':
> >> kernel/sched.c:9359: warning: format '%llu' expects type 'long long unsigned int',
> >> but argument 3 has type 'u64'
> > 
> > huh, I was just wondering about the type case in cgroup_read_u64()
> > when I was looking at it earlier.  That explains it.  Diff patch
> > attached.
> > 
> 
> Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>

Could someone please send the final patch with a final changelog, with 
all fixlets and tags in place please?

Thanks,

	Ingo

Re: [patch] export percpu cpuacct cgroup stats

[Ken Chen]

On Thu, Dec 4, 2008 at 11:54 PM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>> +             seq_printf(m, "%llu ", percpu);
>
> Should be:
> +               seq_printf(m, "%llu ", (unsigned long long)percpu);
>
> Otherwsise:
> kernel/sched.c: In function 'cpuacct_percpu_seq_read':
> kernel/sched.c:9359: warning: format '%llu' expects type 'long long unsigned int',
> but argument 3 has type 'u64'

I'm curious what compiler and CPU arch target are you using that cause
compiler to spit out this warning message?  On x86_64 u64 is unsigned
long long and gcc v4.2.4 doesn't give such warning.

- Ken

Re: [patch] export percpu cpuacct cgroup stats

[Ken Chen]

On Fri, Dec 5, 2008 at 5:52 AM, Ingo Molnar <mingo@elte.hu> wrote:
> Could someone please send the final patch with a final changelog, with
> all fixlets and tags in place please?

Here it is:

This patch export per-cpu CPU cycle usage for a given cpuacct cgroup.
There is a need for a user space monitor daemon to track group CPU
usage on per-cpu base.  It is also useful for monitoring CFS load
balancer behavior by tracking per CPU group usage.


Signed-off-by: Ken Chen <kenchen@google.com>
Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>

diff --git a/kernel/sched.c b/kernel/sched.c
index b7480fb..055c54f 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -9345,12 +9345,34 @@ out:
 	return err;
 }

+static int cpuacct_percpu_seq_read(struct cgroup *cgroup, struct cftype *cft,
+				   struct seq_file *m)
+{
+	struct cpuacct *ca = cgroup_ca(cgroup);
+	u64 percpu;
+	int i;
+
+	for_each_possible_cpu(i) {
+		spin_lock_irq(&cpu_rq(i)->lock);
+		percpu = *percpu_ptr(ca->cpuusage, i);
+		spin_unlock_irq(&cpu_rq(i)->lock);
+		seq_printf(m, "%llu ", (unsigned long long) percpu);
+	}
+	seq_printf(m, "\n");
+	return 0;
+}
+
 static struct cftype files[] = {
 	{
 		.name = "usage",
 		.read_u64 = cpuusage_read,
 		.write_u64 = cpuusage_write,
 	},
+	{
+		.name = "usage_percpu",
+		.read_seq_string = cpuacct_percpu_seq_read,
+	},
+
 };

 static int cpuacct_populate(struct cgroup_subsys *ss, struct cgroup *cgrp)

Re: [patch] export percpu cpuacct cgroup stats

[Andrew Morton]

On Fri, 5 Dec 2008 10:16:30 -0800 Ken Chen <kenchen@google.com> wrote:

> +static int cpuacct_percpu_seq_read(struct cgroup *cgroup, struct cftype *cft,
> +				   struct seq_file *m)
> +{
> +	struct cpuacct *ca = cgroup_ca(cgroup);
> +	u64 percpu;
> +	int i;
> +
> +	for_each_possible_cpu(i) {
> +		spin_lock_irq(&cpu_rq(i)->lock);
> +		percpu = *percpu_ptr(ca->cpuusage, i);
> +		spin_unlock_irq(&cpu_rq(i)->lock);
> +		seq_printf(m, "%llu ", (unsigned long long) percpu);
> +	}
> +	seq_printf(m, "\n");
> +	return 0;
> +}

The locking is only needed for 32-bit, I assume?

The iteration across all possible CPUs seems a bit lame - that code
looks pretty easy to convert to hotplug goodness.

(All of which pertains to existing code, not to this patch).

Re: [patch] export percpu cpuacct cgroup stats

[Li Zefan]

Ken Chen wrote:
> On Thu, Dec 4, 2008 at 11:54 PM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>>> +             seq_printf(m, "%llu ", percpu);
>> Should be:
>> +               seq_printf(m, "%llu ", (unsigned long long)percpu);
>>
>> Otherwsise:
>> kernel/sched.c: In function 'cpuacct_percpu_seq_read':
>> kernel/sched.c:9359: warning: format '%llu' expects type 'long long unsigned int',
>> but argument 3 has type 'u64'
> 
> I'm curious what compiler and CPU arch target are you using that cause
> compiler to spit out this warning message?  On x86_64 u64 is unsigned
> long long and gcc v4.2.4 doesn't give such warning.
> 

I tried it on IA64, and gcc version is 4.1.2.

On IA64, u64 is unsigned long.

Re: [patch] export percpu cpuacct cgroup stats

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Fri, 5 Dec 2008 10:16:30 -0800 Ken Chen <kenchen@google.com> wrote:
> 
> > +static int cpuacct_percpu_seq_read(struct cgroup *cgroup, struct cftype *cft,
> > +				   struct seq_file *m)
> > +{
> > +	struct cpuacct *ca = cgroup_ca(cgroup);
> > +	u64 percpu;
> > +	int i;
> > +
> > +	for_each_possible_cpu(i) {
> > +		spin_lock_irq(&cpu_rq(i)->lock);
> > +		percpu = *percpu_ptr(ca->cpuusage, i);
> > +		spin_unlock_irq(&cpu_rq(i)->lock);
> > +		seq_printf(m, "%llu ", (unsigned long long) percpu);
> > +	}
> > +	seq_printf(m, "\n");
> > +	return 0;
> > +}
> 
> The locking is only needed for 32-bit, I assume?
> 
> The iteration across all possible CPUs seems a bit lame - that code
> looks pretty easy to convert to hotplug goodness.
> 
> (All of which pertains to existing code, not to this patch).

fair enough. Ken, would you mind updating it in this fashion?

	Ingo