[PATCH] change cpuacct usage percpu format

[PATCH] change cpuacct usage percpu format

[KAMEZAWA Hiroyuki]

I'd like to change cpuacct.usage_percpu format before it's widely used.
Current format is not easy to use in hardware with DR feature.

==
cpuacct.usage_percpu shows per cpu usage information via cgroupfs.
It shows all present cpus information and "whose information ?"
is explained just by index of data implicitly.

IIRC, there is physical cpu hotplug and it can remove
cpus and change present map. Considering a software which records
this information pediodically(like sar.), its better to show id of cpu
in explict way.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 kernel/sched.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: mmotm-2.6.29-Feb24/kernel/sched.c
===================================================================
--- mmotm-2.6.29-Feb24.orig/kernel/sched.c
+++ mmotm-2.6.29-Feb24/kernel/sched.c
@@ -9719,7 +9719,7 @@ static int cpuacct_percpu_seq_read(struc
 
 	for_each_present_cpu(i) {
 		percpu = cpuacct_cpuusage_read(ca, i);
-		seq_printf(m, "%llu ", (unsigned long long) percpu);
+		seq_printf(m, "%d %llu\n", i, (unsigned long long) percpu);
 	}
 	seq_printf(m, "\n");
 	return 0;

Re: [PATCH] change cpuacct usage percpu format

[Balbir Singh]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2009-02-27 14:05:37]:

> I'd like to change cpuacct.usage_percpu format before it's widely used.
> Current format is not easy to use in hardware with DR feature.
> 
> ==
> cpuacct.usage_percpu shows per cpu usage information via cgroupfs.
> It shows all present cpus information and "whose information ?"
> is explained just by index of data implicitly.
> 
> IIRC, there is physical cpu hotplug and it can remove
> cpus and change present map. Considering a software which records
> this information pediodically(like sar.), its better to show id of cpu
> in explict way.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  kernel/sched.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: mmotm-2.6.29-Feb24/kernel/sched.c
> ===================================================================
> --- mmotm-2.6.29-Feb24.orig/kernel/sched.c
> +++ mmotm-2.6.29-Feb24/kernel/sched.c
> @@ -9719,7 +9719,7 @@ static int cpuacct_percpu_seq_read(struc
> 
>  	for_each_present_cpu(i) {
>  		percpu = cpuacct_cpuusage_read(ca, i);
> -		seq_printf(m, "%llu ", (unsigned long long) percpu);
> +		seq_printf(m, "%d %llu\n", i, (unsigned long long) percpu);

Seems like a reasonable change, but I wonder if " " should the
separator between the CPU and data, I wounder if we should use
cpu:usage_per_cpu.
-- 
	Balbir

Re: [PATCH] change cpuacct usage percpu format

[KAMEZAWA Hiroyuki]

On Fri, 27 Feb 2009 12:09:55 +0530
Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

> * KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2009-02-27 14:05:37]:
> 
> > I'd like to change cpuacct.usage_percpu format before it's widely used.
> > Current format is not easy to use in hardware with DR feature.
> > 
> > ==
> > cpuacct.usage_percpu shows per cpu usage information via cgroupfs.
> > It shows all present cpus information and "whose information ?"
> > is explained just by index of data implicitly.
> > 
> > IIRC, there is physical cpu hotplug and it can remove
> > cpus and change present map. Considering a software which records
> > this information pediodically(like sar.), its better to show id of cpu
> > in explict way.
> > 
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > ---
> >  kernel/sched.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > Index: mmotm-2.6.29-Feb24/kernel/sched.c
> > ===================================================================
> > --- mmotm-2.6.29-Feb24.orig/kernel/sched.c
> > +++ mmotm-2.6.29-Feb24/kernel/sched.c
> > @@ -9719,7 +9719,7 @@ static int cpuacct_percpu_seq_read(struc
> > 
> >  	for_each_present_cpu(i) {
> >  		percpu = cpuacct_cpuusage_read(ca, i);
> > -		seq_printf(m, "%llu ", (unsigned long long) percpu);
> > +		seq_printf(m, "%d %llu\n", i, (unsigned long long) percpu);
> 
> Seems like a reasonable change, but I wonder if " " should the
> separator between the CPU and data, I wounder if we should use
> cpu:usage_per_cpu.

Ah, no strong reason for " ". 

Hmm, like this ?
==
seq_printf(m, "%d: %llu\n", i, (unsigned long long) percpu);
or
seq_printf(m, "%d\t: %llu\n", i, (unsigned long long) percpu);
==

I'll update this patch.
Thanks,
-kame

Re: [PATCH] change cpuacct usage percpu format

[Balbir Singh]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2009-02-27 15:55:17]:

> On Fri, 27 Feb 2009 12:09:55 +0530
> Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> 
> > * KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2009-02-27 14:05:37]:
> > 
> > > I'd like to change cpuacct.usage_percpu format before it's widely used.
> > > Current format is not easy to use in hardware with DR feature.
> > > 
> > > ==
> > > cpuacct.usage_percpu shows per cpu usage information via cgroupfs.
> > > It shows all present cpus information and "whose information ?"
> > > is explained just by index of data implicitly.
> > > 
> > > IIRC, there is physical cpu hotplug and it can remove
> > > cpus and change present map. Considering a software which records
> > > this information pediodically(like sar.), its better to show id of cpu
> > > in explict way.
> > > 
> > > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > > ---
> > >  kernel/sched.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > Index: mmotm-2.6.29-Feb24/kernel/sched.c
> > > ===================================================================
> > > --- mmotm-2.6.29-Feb24.orig/kernel/sched.c
> > > +++ mmotm-2.6.29-Feb24/kernel/sched.c
> > > @@ -9719,7 +9719,7 @@ static int cpuacct_percpu_seq_read(struc
> > > 
> > >  	for_each_present_cpu(i) {
> > >  		percpu = cpuacct_cpuusage_read(ca, i);
> > > -		seq_printf(m, "%llu ", (unsigned long long) percpu);
> > > +		seq_printf(m, "%d %llu\n", i, (unsigned long long) percpu);
> > 
> > Seems like a reasonable change, but I wonder if " " should the
> > separator between the CPU and data, I wounder if we should use
> > cpu:usage_per_cpu.
> 
> Ah, no strong reason for " ". 
> 
> Hmm, like this ?
> ==
> seq_printf(m, "%d: %llu\n", i, (unsigned long long) percpu);
> or
> seq_printf(m, "%d\t: %llu\n", i, (unsigned long long) percpu);
> ==
> 
>

I'd like to see : and not space after it so something like

seq_printf(m, "%d:%llu\n", i, (unsigned long long) percpu);
-- 
	Balbir

Re: [PATCH] change cpuacct usage percpu format

[Li Zefan]

>> Hmm, like this ?
>> ==
>> seq_printf(m, "%d: %llu\n", i, (unsigned long long) percpu);
>> or
>> seq_printf(m, "%d\t: %llu\n", i, (unsigned long long) percpu);
>> ==
>>
>>
> 
> I'd like to see : and not space after it so something like
> 
> seq_printf(m, "%d:%llu\n", i, (unsigned long long) percpu);

I vote for this. :)

Re: [PATCH] change cpuacct usage percpu format

[KAMEZAWA Hiroyuki]

On Fri, 27 Feb 2009 15:03:53 +0800
Li Zefan <lizf@cn.fujitsu.com> wrote:

> >> Hmm, like this ?
> >> ==
> >> seq_printf(m, "%d: %llu\n", i, (unsigned long long) percpu);
> >> or
> >> seq_printf(m, "%d\t: %llu\n", i, (unsigned long long) percpu);
> >> ==
> >>
> >>
> > 
> > I'd like to see : and not space after it so something like
> > 
> > seq_printf(m, "%d:%llu\n", i, (unsigned long long) percpu);
> 
> I vote for this. :)
> 
ok, I'll take that and prepare new patch.

Regards,
-Kame

[PATCH] change cpuacct usage percpu format v2

[KAMEZAWA Hiroyuki]

From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

cpuacct.usage_percpu shows per cpu usage information via cgroupfs.
It shows all "present" cpus information and "whose information ?"
is explained just by index of data implicitly.

IIRC, there is physical cpu hotplug system and it can remove
cpus and change map. Considering a software which records
this information pediodically(like sar), its better to show id of cpu
in explict way.

Changelog: v1 -> v2
 - added ":" instead of " " as split charactor.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
Index: mmotm-2.6.29-Feb26/kernel/sched.c
===================================================================
--- mmotm-2.6.29-Feb26.orig/kernel/sched.c
+++ mmotm-2.6.29-Feb26/kernel/sched.c
@@ -9719,7 +9719,7 @@ static int cpuacct_percpu_seq_read(struc
 
 	for_each_present_cpu(i) {
 		percpu = cpuacct_cpuusage_read(ca, i);
-		seq_printf(m, "%llu ", (unsigned long long) percpu);
+		seq_printf(m, "%d:%llu\n", i, (unsigned long long) percpu);
 	}
 	seq_printf(m, "\n");
 	return 0;

Re: [PATCH] change cpuacct usage percpu format v2

[Paul Menage]

On Thu, Feb 26, 2009 at 11:29 PM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
>
> IIRC, there is physical cpu hotplug system and it can remove
> cpus and change map. Considering a software which records
> this information pediodically(like sar), its better to show id of cpu
> in explict way.

Can we just use the read_map() handler for this instead?

Paul

Re: [PATCH] change cpuacct usage percpu format v2

[KAMEZAWA Hiroyuki]

On Thu, 26 Feb 2009 23:55:06 -0800
Paul Menage <menage@google.com> wrote:

> On Thu, Feb 26, 2009 at 11:29 PM, KAMEZAWA Hiroyuki
> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> >
> > IIRC, there is physical cpu hotplug system and it can remove
> > cpus and change map. Considering a software which records
> > this information pediodically(like sar), its better to show id of cpu
> > in explict way.
> 
> Can we just use the read_map() handler for this instead?
> 
Ah, I forgeted that we have read_map handler.

Hmm...but read_map()'s fill handler format is "%s %llu\n"
For using read_map, I have to convert integer to string in this routine.

Will we add "%d:%llu\n" style fill() function ?

Thanks,
-Kame

Re: [PATCH] change cpuacct usage percpu format v2

[Ken Chen]

On Thu, Feb 26, 2009 at 11:29 PM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>
> cpuacct.usage_percpu shows per cpu usage information via cgroupfs.
> It shows all "present" cpus information and "whose information ?"
> is explained just by index of data implicitly.
>
> IIRC, there is physical cpu hotplug system and it can remove
> cpus and change map. Considering a software which records
> this information pediodically(like sar), its better to show id of cpu
> in explict way.

The new format carries a lot of overhead.  I would think user space
software can record /sys/devices/system/cpu/present and use that to
map cpu index.

- Ken

Re: [PATCH] change cpuacct usage percpu format v2

[KAMEZAWA Hiroyuki]

Ken Chen さんは書きました：
> On Thu, Feb 26, 2009 at 11:29 PM, KAMEZAWA Hiroyuki
> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>>
>> cpuacct.usage_percpu shows per cpu usage information via cgroupfs.
>> It shows all "present" cpus information and "whose information ?"
>> is explained just by index of data implicitly.
>>
>> IIRC, there is physical cpu hotplug system and it can remove
>> cpus and change map. Considering a software which records
>> this information pediodically(like sar), its better to show id of cpu
>> in explict way.
>
> The new format carries a lot of overhead.  I would think user space
> software can record /sys/devices/system/cpu/present and use that to
> map cpu index.
>
"a lot of" ? I talking about cpu hotplug and reading another file as
/sys/devices/system/cpu/present every time before reading this file
gives much much much more overhead ;)

A program which handles this kind of information tend to ignore hotplug
event and continue to use informaton which it gathers at start-up and
will not see updated information....as libnuma etc.

Thanks,
-Kame

Re: [PATCH] change cpuacct usage percpu format v2

[Ken Chen]

On Fri, Feb 27, 2009 at 3:34 PM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> "a lot of" ? I talking about cpu hotplug and reading another file as
> /sys/devices/system/cpu/present every time before reading this file
> gives much much much more overhead ;)

yes, really a lot.  CPU hotplug is an uncommon event.  It happens
perhaps once a day? maybe once an hour?

User monitoring process usually reads usage_percpu at fairly high
rate, say once a sec.  At each pass it will need to parse N number of
CPU index.  The overhead is N_CPU * T, where T is time in second
between cpu hotplug event.  Assume T = one day, on a moderate sized
64-CPU size machine, the overhead is:

64 * 86400 : 1, that's like 5.5 million to 1 ratio.  To me that is
*high* overhead.

- Ken

Re: [PATCH] change cpuacct usage percpu format v2

[KAMEZAWA Hiroyuki]

Ken Chen さんは書きました：
> On Fri, Feb 27, 2009 at 3:34 PM, KAMEZAWA Hiroyuki
> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>> "a lot of" ? I talking about cpu hotplug and reading another file as
>> /sys/devices/system/cpu/present every time before reading this file
>> gives much much much more overhead ;)
>
> yes, really a lot.  CPU hotplug is an uncommon event.  It happens
> perhaps once a day? maybe once an hour?
>
Are you saying the software should have hotplug script and send SIGHUP or
some to reload the present map ?

> User monitoring process usually reads usage_percpu at fairly high
> rate, say once a sec.  At each pass it will need to parse N number of
> CPU index.  The overhead is N_CPU * T, where T is time in second
> between cpu hotplug event.  Assume T = one day, on a moderate sized
> 64-CPU size machine, the overhead is:
>
> 64 * 86400 : 1, that's like 5.5 million to 1 ratio.  To me that is
> *high* overhead.
>
Sounds strange. I can't catch hat you want to say.

parsing overhead is
==
 1. read cpu.usage_perpu.
 2. parse the line to number (separated by " ")
 3. tie the number to cpu id which is loadead at startup
v.s.
 1. read cpu.usage_percpu.
 2. parse the line to number (sparated by ":" and "\n")
 3. record it
==
Hmm ? no big difference.

But okay, if you don't want this, I'll just add warning text to
Documentation/cgroup/cpuacct.txt as the user may have to prepare
cpu hotplug script if you write flight-recorder like "sar".

Thanks,
-Kame

Re: [PATCH] change cpuacct usage percpu format v2

[KAMEZAWA Hiroyuki]

KAMEZAWA Hiroyuki wrote：
> Ken Chen wrote：
>> On Fri, Feb 27, 2009 at 3:34 PM, KAMEZAWA Hiroyuki
>> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>>> "a lot of" ? I talking about cpu hotplug and reading another file as
>>> /sys/devices/system/cpu/present every time before reading this file
>>> gives much much much more overhead ;)
>>
>> yes, really a lot.  CPU hotplug is an uncommon event.  It happens
>> perhaps once a day? maybe once an hour?
>>
> Are you saying the software should have hotplug script and send SIGHUP or
> some to reload the present map ?
>
>> User monitoring process usually reads usage_percpu at fairly high
>> rate, say once a sec.  At each pass it will need to parse N number of
>> CPU index.  The overhead is N_CPU * T, where T is time in second
>> between cpu hotplug event.  Assume T = one day, on a moderate sized
>> 64-CPU size machine, the overhead is:
>>
>> 64 * 86400 : 1, that's like 5.5 million to 1 ratio.  To me that is
>> *high* overhead.
>>
> Sounds strange. I can't catch hat you want to say.
>
Ignore above, I caught, at last. I'll add text to documenation.

BTW, current interface to reset cpuacct (write ops) just reset
specified level of cpuacct and will not clear other hierarchical levels.
Doesn't this behavior confuse software ?

Thanks,
-Kame

Re: [PATCH] change cpuacct usage percpu format v2

[Dhaval Giani]

On Sat, Feb 28, 2009 at 09:42:17AM +0900, KAMEZAWA Hiroyuki wrote:
> KAMEZAWA Hiroyuki wrote：
> > Ken Chen wrote：
> >> On Fri, Feb 27, 2009 at 3:34 PM, KAMEZAWA Hiroyuki
> >> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> >>> "a lot of" ? I talking about cpu hotplug and reading another file as
> >>> /sys/devices/system/cpu/present every time before reading this file
> >>> gives much much much more overhead ;)
> >>
> >> yes, really a lot.  CPU hotplug is an uncommon event.  It happens
> >> perhaps once a day? maybe once an hour?
> >>
> > Are you saying the software should have hotplug script and send SIGHUP or
> > some to reload the present map ?
> >
> >> User monitoring process usually reads usage_percpu at fairly high
> >> rate, say once a sec.  At each pass it will need to parse N number of
> >> CPU index.  The overhead is N_CPU * T, where T is time in second
> >> between cpu hotplug event.  Assume T = one day, on a moderate sized
> >> 64-CPU size machine, the overhead is:
> >>
> >> 64 * 86400 : 1, that's like 5.5 million to 1 ratio.  To me that is
> >> *high* overhead.
> >>
> > Sounds strange. I can't catch hat you want to say.
> >
> Ignore above, I caught, at last. I'll add text to documenation.
> 
> BTW, current interface to reset cpuacct (write ops) just reset
> specified level of cpuacct and will not clear other hierarchical levels.
> Doesn't this behavior confuse software ?
> 

hmmm. This got missed when we introduced hierarchy. But I wonder if it
is needed?

-- 
regards,
Dhaval

Re: [PATCH] change cpuacct usage percpu format v2

[KAMEZAWA Hiroyuki]

Dhaval Giani wrote：
> On Sat, Feb 28, 2009 at 09:42:17AM +0900, KAMEZAWA Hiroyuki wrote:
>> KAMEZAWA Hiroyuki wrote：
>> BTW, current interface to reset cpuacct (write ops) just reset
>> specified level of cpuacct and will not clear other hierarchical levels.
>> Doesn't this behavior confuse software ?
>>
>
> hmmm. This got missed when we introduced hierarchy. But I wonder if it
> is needed?
>
IIUC, cpuacct.usage just shows sum of itself and all children's usage and
its own usage can be calclated by reading all usage of hierarchy.
So, reset ops seems to be a bit broken.
And by this, parent's usage can be smaller than children.

How about adding limitation as "you can clear usage only when there are no
children" ? Maybe not very strange limitaton under hierarchical system.


Thanks,
-Kame

Re: [PATCH] change cpuacct usage percpu format v2

[Dhaval Giani]

On Sat, Feb 28, 2009 at 02:25:22PM +0900, KAMEZAWA Hiroyuki wrote:
> Dhaval Giani wrote：
> > On Sat, Feb 28, 2009 at 09:42:17AM +0900, KAMEZAWA Hiroyuki wrote:
> >> KAMEZAWA Hiroyuki wrote：
> >> BTW, current interface to reset cpuacct (write ops) just reset
> >> specified level of cpuacct and will not clear other hierarchical levels.
> >> Doesn't this behavior confuse software ?
> >>
> >
> > hmmm. This got missed when we introduced hierarchy. But I wonder if it
> > is needed?
> >
> IIUC, cpuacct.usage just shows sum of itself and all children's usage and
> its own usage can be calclated by reading all usage of hierarchy.
> So, reset ops seems to be a bit broken.
> And by this, parent's usage can be smaller than children.
> 

Actually now that I think about it, it does make sense, but when I
started implementing it, a question popped up, if we reset a child's
counter, then the parent is going to have a much greater usage than the
sum of the children. I wonder if this behavior is fine?

> How about adding limitation as "you can clear usage only when there are no
> children" ? Maybe not very strange limitaton under hierarchical system.
> 

Just the question I asked. Beyond that I don't think we should have such a
limitation on reset.

thanks,
-- 
regards,
Dhaval

Re: [PATCH] change cpuacct usage percpu format v2

[KAMEZAWA Hiroyuki]

Dhaval Giani wrote:
> On Sat, Feb 28, 2009 at 02:25:22PM +0900, KAMEZAWA Hiroyuki wrote:
>> Dhaval Giani wrote:
>> > On Sat, Feb 28, 2009 at 09:42:17AM +0900, KAMEZAWA Hiroyuki wrote:
>> >> KAMEZAWA Hiroyuki wrote:
>> >> BTW, current interface to reset cpuacct (write ops) just reset
>> >> specified level of cpuacct and will not clear other hierarchical
>> levels.
>> >> Doesn't this behavior confuse software ?
>> >>
>> >
>> > hmmm. This got missed when we introduced hierarchy. But I wonder if it
>> > is needed?
>> >
>> IIUC, cpuacct.usage just shows sum of itself and all children's usage
>> and
>> its own usage can be calclated by reading all usage of hierarchy.
>> So, reset ops seems to be a bit broken.
>> And by this, parent's usage can be smaller than children.
>>
>
> Actually now that I think about it, it does make sense, but when I
> started implementing it, a question popped up, if we reset a child's
> counter, then the parent is going to have a much greater usage than the
> sum of the children. I wonder if this behavior is fine?
>
As far as the user remembers he did reset...

>> How about adding limitation as "you can clear usage only when there are
>> no
>> children" ? Maybe not very strange limitaton under hierarchical system.
>>
>
> Just the question I asked. Beyond that I don't think we should have such a
> limitation on reset.
>
Ok, I don't have strong requirements. I'll recomend users that you should
reset all children(hierarchy) at the same time.

Maybe no problem if we have private-usage counter....but it means one more
percpu array...

Thanks,
-Kame