2007-05-13 19:00:19

by Arjan van de Ven

[permalink] [raw]
Subject: Re: + timer_stats-slimmed-down-using-statistics-infrastucture.patch added to -mm tree

On Fri, 2007-04-20 at 17:18 -0700, [email protected] wrote:
> The patch titled
> timer_stats slimmed down: using statistics infrastucture
> has been added to the -mm tree. Its filename is
> timer_stats-slimmed-down-using-statistics-infrastucture.patch
>
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
>
> See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
> out what to do about this
>
> ------------------------------------------------------
> Subject: timer_stats slimmed down: using statistics infrastucture
> From: Martin Peschke <[email protected]>
>
> This patch implements timer_stats based on the statistics infrastructure.
>
> Here is some sample output, which even somewhat
> resembles /proc/timer_stats.
> (>80 char lines because of symbol names, sorry for line breaks).
>
> It reads:
> <statistic name> <basket> <hits> <label>
>
> with label being:
> <pid> <task name> <start_fn>(<stop_fn>)
>
> [root@t2930041 timer_stats]# cat data

this patch changes the userspace API though, and breaks PowerTOP :(


2007-05-13 19:20:29

by Cédric Augonnet

[permalink] [raw]
Subject: Re: + timer_stats-slimmed-down-using-statistics-infrastucture.patch added to -mm tree

2007/5/13, Arjan van de Ven <[email protected]>:
> On Fri, 2007-04-20 at 17:18 -0700, [email protected] wrote:
> > The patch titled
> > timer_stats slimmed down: using statistics infrastucture
> > has been added to the -mm tree. Its filename is
> > timer_stats-slimmed-down-using-statistics-infrastucture.patch
> >
> > *** Remember to use Documentation/SubmitChecklist when testing your code ***
> >
> > See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
> > out what to do about this
> >
> > ------------------------------------------------------
> > Subject: timer_stats slimmed down: using statistics infrastucture
> > From: Martin Peschke <[email protected]>
> >
> > This patch implements timer_stats based on the statistics infrastructure.
> >
> > Here is some sample output, which even somewhat
> > resembles /proc/timer_stats.
> > (>80 char lines because of symbol names, sorry for line breaks).
> >
> > It reads:
> > <statistic name> <basket> <hits> <label>
> >
> > with label being:
> > <pid> <task name> <start_fn>(<stop_fn>)
> >
> > [root@t2930041 timer_stats]# cat data
>
> this patch changes the userspace API though, and breaks PowerTOP :(
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

Moreover, the documentation still refers to /proc/timer_stats which do
not appear in my 2.6.21-mm2 kernel.

Regards,
C?dric

2007-05-14 08:26:38

by Martin Peschke

[permalink] [raw]
Subject: Re: + timer_stats-slimmed-down-using-statistics-infrastucture.patch added to -mm tree

Arjan van de Ven wrote:
> On Fri, 2007-04-20 at 17:18 -0700, [email protected] wrote:
>> The patch titled
>> timer_stats slimmed down: using statistics infrastucture
>> has been added to the -mm tree. Its filename is
>> timer_stats-slimmed-down-using-statistics-infrastucture.patch
>>
>> *** Remember to use Documentation/SubmitChecklist when testing your code ***
>>
>> See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
>> out what to do about this
>>
>> ------------------------------------------------------
>> Subject: timer_stats slimmed down: using statistics infrastucture
>> From: Martin Peschke <[email protected]>
>>
>> This patch implements timer_stats based on the statistics infrastructure.
>>
>> Here is some sample output, which even somewhat
>> resembles /proc/timer_stats.
>> (>80 char lines because of symbol names, sorry for line breaks).
>>
>> It reads:
>> <statistic name> <basket> <hits> <label>
>>
>> with label being:
>> <pid> <task name> <start_fn>(<stop_fn>)
>>
>> [root@t2930041 timer_stats]# cat data
>
> this patch changes the userspace API though, and breaks PowerTOP :(

Though I think it was unfortunate to add the "old" proc API in 2.6.21.
timer_stats is clearly labeled as debug stuff in config help, which is why
debugfs would have been a proper place.


2007-05-15 00:08:17

by Arjan van de Ven

[permalink] [raw]
Subject: Re: + timer_stats-slimmed-down-using-statistics-infrastucture.patch added to -mm tree

On Mon, 2007-05-14 at 10:26 +0200, Martin Peschke wrote:
> >
> >> [root@t2930041 timer_stats]# cat data
> >
> > this patch changes the userspace API though, and breaks PowerTOP :(
>
> Though I think it was unfortunate to add the "old" proc API in 2.6.21.
> timer_stats is clearly labeled as debug stuff in config help, which is why
> debugfs would have been a proper place.

but it's not there, it's in it's current place. And userspace apps
depend on it... so what's the reason to change this after it became
ABI ? If there's a good one I can fix powertop.. but....
oh well I hope you kept the rest of the file format the same ;)

--
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

2007-05-15 15:25:29

by Martin Peschke

[permalink] [raw]
Subject: Re: + timer_stats-slimmed-down-using-statistics-infrastucture.patch added to -mm tree

Arjan van de Ven wrote:
> On Mon, 2007-05-14 at 10:26 +0200, Martin Peschke wrote:
>>>> [root@t2930041 timer_stats]# cat data
>>> this patch changes the userspace API though, and breaks PowerTOP :(
>> Though I think it was unfortunate to add the "old" proc API in 2.6.21.
>> timer_stats is clearly labeled as debug stuff in config help, which is why
>> debugfs would have been a proper place.
>
> but it's not there, it's in it's current place. And userspace apps
> depend on it...

mea culpa

> so what's the reason to change this after it became ABI ?

A sort of race condition ;-)

I posted this as an RFC when one of the last 2.6.21-rc's came out, that is
timerstats wasn't ABI yet, anticipating that I was late anyway.

> If there's a good one I can fix powertop..

kernel/time/timer_stats.c | 385 +++++-------------------------------
kernel/timer.c | 2
2 files changed, 59 insertions(+), 328 deletions(-)

It would be similar for other kernel code I have looked at.
And I think there would be an advantage if one was able to understand device
driver A's statistics code immediately because it was written against some
common library functions.

But it isn't easy to get this interface into the kernel by finding users.
I have got to "intercept" candidates before they add some own ABI to proc or
whereever, while I can't change established statistics.

In short, if you would like to help in this case, I would appreciate it.

> but.... oh well I hope you kept the rest of the file format the same ;)

Almost. Though, there might be ways to keep the old format totally unchanged.
It would be a matter of stretching the concepts of a unified user interface for
statistics.

Currently, my format comes with a substring representing <statistics name>
<basket> preceding each line, which currently reads "expired - " plus the line
powertop is familiar with.

Would it be feasible to to teach powertop to skip the first two elements?


Martin