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 <[email protected]>
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)
(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 <[email protected]>
>
> 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,
> + },
> +
> };
>
* Li Zefan <[email protected]> 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 <[email protected]>
> >
> > 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
On Wed, Dec 3, 2008 at 9:34 PM, Li Zefan <[email protected]> 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
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 <[email protected]>
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)
> +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;
> +}
On Thu, Dec 4, 2008 at 11:54 PM, Li Zefan <[email protected]> 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;
Ken Chen wrote:
> On Thu, Dec 4, 2008 at 11:54 PM, Li Zefan <[email protected]> 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 <[email protected]>
> 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;
>
>
* Li Zefan <[email protected]> wrote:
> Ken Chen wrote:
> > On Thu, Dec 4, 2008 at 11:54 PM, Li Zefan <[email protected]> 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 <[email protected]>
Could someone please send the final patch with a final changelog, with
all fixlets and tags in place please?
Thanks,
Ingo
On Thu, Dec 4, 2008 at 11:54 PM, Li Zefan <[email protected]> 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
On Fri, Dec 5, 2008 at 5:52 AM, Ingo Molnar <[email protected]> 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 <[email protected]>
Reviewed-by: Li Zefan <[email protected]>
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)
On Fri, 5 Dec 2008 10:16:30 -0800 Ken Chen <[email protected]> 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).
Ken Chen wrote:
> On Thu, Dec 4, 2008 at 11:54 PM, Li Zefan <[email protected]> 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.
* Andrew Morton <[email protected]> wrote:
> On Fri, 5 Dec 2008 10:16:30 -0800 Ken Chen <[email protected]> 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