Re: [Powertop] segfault on Sheevaplug (ARM Kirkwood)
Rui DaCosta <ruidc at yahoo.com> Tue, 22 May 2012 12:19:43 -0700
| Newsgroups | dev.linux.lists.powertop |
|---|---|
| Message-ID | <[email protected]> |
I'd love to help, but development in low-level languages is far from my "forte", but I can try to submituser-level bugs and perform some testing, ( I'm merely a user of powertop and an enthusiast for low-power computing).
I understand ARM support in Linux is a mess, but nevertheless it is Linux's most successful platform (taking mobile devices into account), and there's lots of effort being put in to cleaning it up, though i can also understand a reluctance on Intel's part to commit much in that direction ;)
I don't know enough about the issues here to comment on *where* the bug is, but am happy to chase things upstream to get it fixed, if, after review, you think it is a bug there.
>From my understanding, this Kirkwood platform is also a reasonably popular ARM device, particularly with the power-conscious users, and has been incorporated in many mass-market devices, so i'd think patching until things are fixed upstream would be in many people's interest - and particularly as this appears as a regression from powertop 1.13.
I could also perhaps, on arrangement, organize root access to this device for any devs wishing to test support - so long as I don't have to go too wild with kernel changes (I still rely upon this device as a home NAS).
But i look forward to your conclusion and respect your decision regardless.
... Great tool by the way - doing it's own massive bit to keep the world a greener place.
Kind Regards,
Rui
________________________________
From: Chris Ferron <chris.e.ferron(a)linux.intel.com>
To: Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com>
Cc: Rui DaCosta <ruidc(a)yahoo.com>; Arjan van de Ven <arjan(a)linux.intel.com>; "powertop(a)lists.01.org" <powertop(a)lists.01.org>
Sent: Tuesday, 22 May 2012, 1:09
Subject: Re: [Powertop] segfault on Sheevaplug (ARM Kirkwood)
On 05/20/2012 02:57 PM, Sergey Senozhatsky wrote:
> On (05/20/12 04:01), Rui DaCosta wrote:
>> Thanks,
>> this has gotten past the issue. I now get:
>> PowerTOP v2.0 needs the kernel to support the 'perf' subsystem
>> as well as support for trace points in the kernel:
>>
>> CONFIG_PERF_EVENTS=y
>> CONFIG_PERF_COUNTERS=y
>> CONFIG_TRACEPOINTS=y
>> CONFIG_TRACING=y
>>
>> all these except CONFIG_PERF_COUNTERS are already set, so i'll need to see
>> if i can get a kernel built with that option on.
>>
>> Will this patch make it into trunk?
>>
> Well, it depends. The patch itself is quite innocent -- assuming default
> number of processors being 1 instead of -1 will not do any harm. Of course,
> such default value could be considered as debugging friendly, yet segfault
> is still no good.
>
> We'll see what project owners think about that.
*In GENERAL*
Well that is an interesting question. I personally will not be spending
any time on arm.
That said, i am also not opposed to somewhat blindly accepting patches
for ARM, especially from trusted and active members.
*As long as ARM work isn't effecting PowerTOP.*
Now if you find a bug, then that is a different story, as long as the
bug is not purely ARM specific it will get attention.
*this instance*
I will pull this patch and take a good look at it, since it looks to me
to be a bug as well. Thanks Sergey
-Chris
>
> -ss
>
>> Many thanks.
>>
>> ──────────────────────────────────────────────────────────────────────────
>>
>> From: Sergey Senozhatsky<sergey.senozhatsky(a)gmail.com>
>> To: Rui DaCosta<ruidc(a)yahoo.com>
>> Cc: Arjan van de Ven<arjan(a)linux.intel.com>; Chris Ferron
>> <chris.e.ferron(a)linux.intel.com>; powertop(a)lists.01.org
>> Sent: Sunday, 20 May 2012, 11:34
>> Subject: Re: [Powertop] segfault on Sheevaplug (ARM Kirkwood)
>> On (05/20/12 02:19), Rui DaCosta wrote:
>> > Sure and thanks,
>> > (v1.13 worked fine btw)
>> > Processor : Feroceon 88FR131 rev 1 (v5l)
>> > BogoMIPS : 1191.11
>> > Features : swp half thumb fastmult edsp
>> > CPU implementer : 0x56
>> > CPU architecture: 5TE
>> > CPU variant : 0x2
>> > CPU part : 0x131
>> > CPU revision : 1
>> >
>> > Hardware : Marvell SheevaPlug Reference Board
>> > Revision : 0000
>> > Serial : 0000000000000000
>> >
>> >
>>
>> Thanks,
>>
>> Well, that's the problem. Current cpu info parser doesn't understand your
>> cpuinfo format. It awaits for sane values on special places. For example,
>> word
>> "processor" should be followed by a number, not model name.
>>
>> processor : 2
>> vendor_id : GenuineIntel
>> cpu family : 6
>> model : 37
>> bogomips : 4522.66
>>
>> while cpuinfo on your system is totally different.
>>
>> the following is untested patch (I'm a bit skeptical) plus I don't have
>> ARM device for testing.
>>
>> ---
>>
>> src/cpu/cpu.cpp | 26 ++++++++++++++------------
>> 1 file changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/cpu/cpu.cpp b/src/cpu/cpu.cpp
>> index 09d4a2d..143e18c 100644
>> --- a/src/cpu/cpu.cpp
>> +++ b/src/cpu/cpu.cpp
>> @@ -225,7 +225,7 @@ void enumerate_cpus(void)
>> ifstream file;
>> char line[1024];
>>
>> - int number = -1;
>> + int number = 1;
>> char vendor[128];
>> int family = 0;
>> int model = 0;
>> @@ -236,7 +236,6 @@ void enumerate_cpus(void)
>> return;
>>
>> while (file) {
>> -
>> file.getline(line, sizeof(line));
>> if (strncmp(line, "vendor_id\t",10) == 0) {
>> char *c;
>> @@ -247,42 +246,45 @@ void enumerate_cpus(void)
>> c++;
>> strncpy(vendor,c, 127);
>> }
>> - }
>> - if (strncmp(line, "processor\t",10) == 0) {
>> + } else if (strncmp(line, "processor\t",10) == 0) {
>> char *c;
>> c = strchr(line, ':');
>> if (c) {
>> c++;
>> number = strtoull(c, NULL, 10);
>> }
>> - }
>> - if (strncmp(line, "cpu family\t",11) == 0) {
>> + } else if (strncmp(line, "Processor\t",10) == 0) {
>> + char *c;
>> + c = strchr(line, ':');
>> + if (c) {
>> + c++;
>> + if (*c == ' ')
>> + c++;
>> + strncpy(vendor, c, 127);
>> + }
>> + } else if (strncmp(line, "cpu family\t",11) == 0) {
>> char *c;
>> c = strchr(line, ':');
>> if (c) {
>> c++;
>> family = strtoull(c, NULL, 10);
>> }
>> - }
>> - if (strncmp(line, "model\t",6) == 0) {
>> + } else if (strncmp(line, "model\t",6) == 0) {
>> char *c;
>> c = strchr(line, ':');
>> if (c) {
>> c++;
>> model = strtoull(c, NULL, 10);
>> }
>> - }
>> - if (strncasecmp(line, "bogomips\t", 9) == 0) {
>> + } else if (strncasecmp(line, "bogomips\t", 9) == 0) {
>> handle_one_cpu(number, vendor, family, model);
>> set_max_cpu(number);
>> }
>> }
>>
>> -
>> file.close();
>>
>> perf_events = new perf_power_bundle();
>> -
>> if (!perf_events->add_event("power:cpu_idle")){
>> perf_events->add_event("power:power_start");
>> perf_events->add_event("power:power_end");
attachment.html
(text/html, 13.8 KB)
<html><body><div style="color:#000; background-color:#fff; font-family:times new roman, new york, times, serif;font-size:12pt"><div><span>I'd love to help, but development in low-level languages is far from my "forte", </span>but I can try to submit<span> user-level bugs and perform some testing,</span> ( <span>I'm merely a user of powertop and an enthusiast for low-power computing).<br></span></div><div><span><br></span></div><div><span>I understand ARM support in Linux is a mess, but nevertheless it is Linux's most successful platform (taking mobile devices into account), and there's lots of effort being put in to cleaning it up, though i can also understand a reluctance on Intel's part to commit much in that direction ;)<br></span></div><div><span><br></span></div><div> <span>I don't know enough about the issues here to comment on *where* the bug is, but am happy to chase things upstream to get it fixed, if, after review, you think it is a bug there.</span> <span><br></span></div><div><span><br></span></div><div><span>From my understanding, this Kirkwood platform is also a reasonably popular ARM device, particularly with the power-conscious users, and has been incorporated in many mass-market devices, so i'd think patching until things are fixed upstream would be in many people's interest - and particularly as this appears as a regression from powertop 1.13.<br></span></div><div><span><br></span></div><div><span>I could also perhaps, on arrangement, organize root access to this device for any devs wishing to test support - so long as I don't have to go too wild with kernel changes (I still rely upon this device as a home NAS).</span><span><br></span></div><div><br></div><div>But i look forward to your conclusion and respect your decision regardless.<br></div><div>... Great tool by the way - doing it's own massive bit to keep the world a greener place.</div><div><br></div><div>Kind Regards,</div><div>Rui</div> <div style="font-size: 12pt; font-family: "times new roman", "new york", "times", serif"> <div style="font-size: 12pt; font-family: "times new roman", "new york", "times", serif"> <div dir="ltr"> <font size="2" face="Arial"> <hr size="1"> <b><span style="font-weight:bold;">From:</span></b> Chris Ferron <[email protected]><br> <b><span style="font-weight: bold;">To:</span></b> Sergey Senozhatsky <[email protected]> <br><b><span style="font-weight: bold;">Cc:</span></b> Rui DaCosta <[email protected]>; Arjan van de Ven <[email protected]>; "[email protected]" <[email protected]> <br> <b><span style="font-weight: bold;">Sent:</span></b> Tuesday, 22 May 2012, 1:09<br> <b><span style="font-weight: bold;">Subject:</span></b> Re: [Powertop] segfault on Sheevaplug (ARM Kirkwood)<br> </font> </div> <br>On 05/20/2012 02:57 PM, Sergey Senozhatsky wrote:<br>> On (05/20/12 04:01), Rui DaCosta wrote:<br>>> Thanks,<br>>> this has gotten past the issue. I now get:<br>>> PowerTOP v2.0 needs the kernel to support the 'perf' subsystem<br>>> as well as support for trace points in the kernel:<br>>><br>>> CONFIG_PERF_EVENTS=y<br>>> CONFIG_PERF_COUNTERS=y<br>>> CONFIG_TRACEPOINTS=y<br>>> CONFIG_TRACING=y<br>>><br>>> all these except CONFIG_PERF_COUNTERS are already set, so i'll need to see<br>>> if i can get a kernel built with that option on.<br>>><br>>> Will this patch make it into trunk?<br>>><br>> Well, it depends. The patch itself is quite innocent -- assuming default<br>> number of processors being 1 instead of -1 will not do any harm. Of course,<br>> such default value could be considered as debugging friendly, yet segfault<br>> is still no good.<br>><br>> We'll see what project owners think about that.<br>*In GENERAL*<br>Well that is an interesting question. I personally will not be spending <br>any time on arm.<br>That said, i am also not opposed to somewhat blindly accepting patches <br>for ARM, especially from trusted and active members.<br>*As long as ARM work isn't effecting PowerTOP.*<br><br>Now if you find a bug, then that is a different story, as long as the <br>bug is not purely ARM specific it will get attention.<br><br>*this instance*<br>I will pull this patch and take a good look at it, since it looks to me <br>to be a bug as well. Thanks Sergey<br><br><br>-Chris<br><br><br><br>><br>> -ss<br>><br>>> Many thanks.<br>>><br>>> ──────────────────────────────────────────────────────────────────────────<br>>><br>>> From: Sergey Senozhatsky<<a ymailto="mailto:[email protected]" href="mailto:[email protected]">[email protected]</a>><br>>> To: Rui DaCosta<<a ymailto="mailto:[email protected]" href="mailto:[email protected]">[email protected]</a>><br>>> Cc: Arjan van de Ven<<a ymailto="mailto:[email protected]" href="mailto:[email protected]">[email protected]</a>>; Chris Ferron<br>>> <<a ymailto="mailto:[email protected]" href="mailto:[email protected]">[email protected]</a>>; <a ymailto="mailto:[email protected]" href="mailto:[email protected]">[email protected]</a><br>>> Sent: Sunday, 20 May 2012, 11:34<br>>> Subject: Re: [Powertop] segfault on Sheevaplug (ARM Kirkwood)<br>>> On (05/20/12 02:19), Rui DaCosta wrote:<br>>> > Sure and thanks,<br>>> > (v1.13 worked fine btw)<br>>> > Processor : Feroceon 88FR131 rev 1 (v5l)<br>>> > BogoMIPS : 1191.11<br>>> > Features : swp half thumb fastmult edsp<br>>> > CPU implementer : 0x56<br>>> > CPU architecture: 5TE<br>>> > CPU variant : 0x2<br>>> > CPU part : 0x131<br>>> > CPU revision : 1<br>>> ><br>>> > Hardware : Marvell SheevaPlug Reference Board<br>>> > Revision : 0000<br>>> > Serial : 0000000000000000<br>>> ><br>>> ><br>>><br>>> Thanks,<br>>><br>>> Well, that's the problem. Current cpu info parser doesn't understand your<br>>> cpuinfo format. It awaits for sane values on special places. For example,<br>>> word<br>>> "processor" should be followed by a number, not model name.<br>>><br>>> processor : 2<br>>> vendor_id : GenuineIntel<br>>> cpu family : 6<br>>> model : 37<br>>> bogomips : 4522.66<br>>><br>>> while cpuinfo on your system is totally different.<br>>><br>>> the following is untested patch (I'm a bit skeptical) plus I don't have<br>>> ARM device for testing.<br>>><br>>> ---<br>>><br>>> src/cpu/cpu.cpp | 26 ++++++++++++++------------<br>>> 1 file changed, 14 insertions(+), 12 deletions(-)<br>>><br>>> diff --git a/src/cpu/cpu.cpp b/src/cpu/cpu.cpp<br>>> index 09d4a2d..143e18c 100644<br>>> --- a/src/cpu/cpu.cpp<br>>> +++ b/src/cpu/cpu.cpp<br>>> @@ -225,7 +225,7 @@ void enumerate_cpus(void)<br>>> ifstream file;<br>>> char line[1024];<br>>><br>>> - int number = -1;<br>>> + int number = 1;<br>>> char vendor[128];<br>>> int family = 0;<br>>> int model = 0;<br>>> @@ -236,7 +236,6 @@ void enumerate_cpus(void)<br>>> return;<br>>><br>>> while (file) {<br>>> -<br>>> file.getline(line, sizeof(line));<br>>> if (strncmp(line, "vendor_id\t",10) == 0) {<br>>> char *c;<br>>> @@ -247,42 +246,45 @@ void enumerate_cpus(void)<br>>> c++;<br>>> strncpy(vendor,c, 127);<br>>> }<br>>> - }<br>>> - if (strncmp(line, "processor\t",10) == 0) {<br>>> + } else if (strncmp(line, "processor\t",10) == 0) {<br>>> char *c;<br>>> c = strchr(line, ':');<br>>> if (c) {<br>>> c++;<br>>> number = strtoull(c, NULL, 10);<br>>> }<br>>> - }<br>>> - if (strncmp(line, "cpu family\t",11) == 0) {<br>>> + } else if (strncmp(line, "Processor\t",10) == 0) {<br>>> + char *c;<br>>> + c = strchr(line, ':');<br>>> + if (c) {<br>>> + c++;<br>>> + if (*c == ' ')<br>>> + c++;<br>>> + strncpy(vendor, c, 127);<br>>> + }<br>>> + } else if (strncmp(line, "cpu family\t",11) == 0) {<br>>> char *c;<br>>> c = strchr(line, ':');<br>>> if (c) {<br>>> c++;<br>>> family = strtoull(c, NULL, 10);<br>>> }<br>>> - }<br>>> - if (strncmp(line, "model\t",6) == 0) {<br>>> + } else if (strncmp(line, "model\t",6) == 0) {<br>>> char *c;<br>>> c = strchr(line, ':');<br>>> if (c) {<br>>> c++;<br>>> model = strtoull(c, NULL, 10);<br>>> }<br>>> - }<br>>> - if (strncasecmp(line, "bogomips\t", 9) == 0) {<br>>> + } else if (strncasecmp(line, "bogomips\t", 9) == 0) {<br>>> handle_one_cpu(number, vendor, family, model);<br>>> set_max_cpu(number);<br>>> }<br>>> }<br>>><br>>> -<br>>> file.close();<br>>><br>>> perf_events = new perf_power_bundle();<br>>> -<br>>> if (!perf_events->add_event("power:cpu_idle")){<br>>> perf_events->add_event("power:power_start");<br>>> perf_events->add_event("power:power_end");<br><br><br><br> </div> </div> </div></body></html>