Merge lp:~ctf/checkbox/bug811177 into lp:checkbox

Proposed by TienFu Chen on 2011-09-16
Status: Merged
Merged at revision: 1070
Proposed branch: lp:~ctf/checkbox/bug811177
Merge into: lp:checkbox
Diff against target: 108 lines (+50/-9)
1 file modified
scripts/cpu_scaling_test (+50/-9)
To merge this branch: bzr merge lp:~ctf/checkbox/bug811177
Reviewer Review Type Date Requested Status
Daniel Manrique 2011-09-16 Approve on 2011-09-26
TienFu Chen (community) Resubmit on 2011-09-23
Review via email: mp+75683@code.launchpad.net

Description of the change

In Oneiric, the conservative sys directory in /sys/devices/system/cpu/cpux/cpufreq has been removed, it will be produced in /sys/devices/system/cpu/cpufreq while the governor set to conservative.
This change added findParameter() inside of setParameter() and automatch flag to setParameter() for searching the target sys file and return the full path.

To post a comment you must log in.
Daniel Manrique (roadmr) wrote :

Hi Tim,

I ran the script on a Natty system and it works fine, however on Oneiric it still fails due to absence of cpufreq-selector. When run, the test produces output as seen here:

http://paste.ubuntu.com/693991/

The exit code is 1, indicating failure (and as the output shows, "Error: performance setting vs minimum of 58.1% is not within 10.0% margin").

Do you know of a way to replace cpufreq-selector? I think once we have that we can consider this fixed, but we're still missing that one part (even though the test now runs, it will fail most of the time).

Thanks!

review: Needs Information
TienFu Chen (ctf) wrote :

Hi Daniel,
cpufreq-selector is in the package: gnome-applets which is not the
default package in Oneiric.
Actually in the cpu_scaling_test script, while
setParameterWithSelector(using cpufreq-selector) failed, it will run
setParameter(change governor using echo to sys file), so even we saw
the cpufreq-selector error, it is still working.
My patch is a fix to the problem that the cpufreq sys file location is
changed in Oneiric, I use findParameter() to search the sys file, so
it will backward compatible.

I have reviewed the source code of cpufreq-selector, I think it's ok
to make a checkbox-version cpufreq-selector, but since the
cpu_scaling_test is working without cpufreq-selector(we may remove the
error message).
Do we still need that? Please let me know your idea, thanks.

On Wed, Sep 21, 2011 at 3:57 AM, Daniel Manrique
<email address hidden> wrote:
> Review: Needs Information
>
> Hi Tim,
>
> I ran the script on a Natty system and it works fine, however on Oneiric it still fails due to absence of cpufreq-selector. When run, the test produces output as seen here:
>
> http://paste.ubuntu.com/693991/
>
> The exit code is 1, indicating failure (and as the output shows, "Error: performance setting vs minimum of 58.1% is not within 10.0% margin").
>
> Do you know of a way to replace cpufreq-selector? I think once we have that we can consider this fixed, but we're still missing that one part (even though the test now runs, it will fail most of the time).
>
> Thanks!
>
> --
> https://code.launchpad.net/~ctf/checkbox/bug811177/+merge/75683
> You are the owner of lp:~ctf/checkbox/bug811177.
>

--
Tim

Daniel Manrique (roadmr) wrote :

> Hi Daniel,
> cpufreq-selector is in the package: gnome-applets which is not the
> default package in Oneiric.
> Actually in the cpu_scaling_test script, while
> setParameterWithSelector(using cpufreq-selector) failed, it will run
> setParameter(change governor using echo to sys file), so even we saw
> the cpufreq-selector error, it is still working.
> My patch is a fix to the problem that the cpufreq sys file location is
> changed in Oneiric, I use findParameter() to search the sys file, so
> it will backward compatible.

Hey, thanks for the backward compatibility bit! I saw that, very clever!

I offer my apologies, I tested this on a system that actually fails the test and I assumed the script to be at fault. Based on your reply I tested on another system that passes the test and it works fine, exiting with the correct "0" code when all tests pass. Governors are set correctly.

> I have reviewed the source code of cpufreq-selector, I think it's ok
> to make a checkbox-version cpufreq-selector, but since the
> cpu_scaling_test is working without cpufreq-selector(we may remove the
> error message).
> Do we still need that? Please let me know your idea, thanks.

No, as I mention it was my mistake, if cpufreq-selector is no longer needed then it's even better for us.

My only suggestion would be to fix it so, if cpufreq-selector is not available, this error message is not printed:

/bin/sh: cpufreq-selector: not found
Note: command failed: cpufreq-selector -g conservative

This is in the setParameterWithSelector method.

However, the test is working now, so the code could be merged as it is. Let me know if you agree with my suggestion or you would prefer I merge in the current state.

Thanks again! great job with this script.

>
> On Wed, Sep 21, 2011 at 3:57 AM, Daniel Manrique
> <email address hidden> wrote:
> > Review: Needs Information
> >
> > Hi Tim,
> >
> > I ran the script on a Natty system and it works fine, however on Oneiric it
> still fails due to absence of cpufreq-selector. When run, the test produces
> output as seen here:
> >
> > http://paste.ubuntu.com/693991/
> >
> > The exit code is 1, indicating failure (and as the output shows, "Error:
> performance setting vs minimum of 58.1% is not within 10.0% margin").
> >
> > Do you know of a way to replace cpufreq-selector? I think once we have that
> we can consider this fixed, but we're still missing that one part (even though
> the test now runs, it will fail most of the time).
> >
> > Thanks!
> >
> > --
> > https://code.launchpad.net/~ctf/checkbox/bug811177/+merge/75683
> > You are the owner of lp:~ctf/checkbox/bug811177.
> >
>
>
>
> --
> Tim

lp:~ctf/checkbox/bug811177 updated on 2011-09-23
1049. By TienFu Chen on 2011-09-23

merged from rev 1048-1064

1050. By TienFu Chen on 2011-09-23

check if cpufreq-selector available, remove error while it's not available

TienFu Chen (ctf) wrote :

Hi Daniel,
Please see rev 1050, I have removed the error message while cpufreq-selector is not available.
Also did a check if cpufreq-selector is in the $PATH, if the system has cpufreq-selector, cpufreq-selector still can be used.

review: Resubmit
Daniel Manrique (roadmr) wrote :

> Hi Daniel,
> Please see rev 1050, I have removed the error message while cpufreq-selector
> is not available.
> Also did a check if cpufreq-selector is in the $PATH, if the system has
> cpufreq-selector, cpufreq-selector still can be used.

Excellent, tested with these changes and it doesn't show any strange errors if cpufreq-selector is not present. I also updated the changelog to reflect the related bug.

Thanks and sorry for the delay reviewing this!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'scripts/cpu_scaling_test'
2--- scripts/cpu_scaling_test 2010-06-04 17:05:15 +0000
3+++ scripts/cpu_scaling_test 2011-09-23 10:24:35 +0000
4@@ -20,6 +20,8 @@
5 self.cpufreqDirectory = os.path.join(self.sysCPUDirectory, "cpu0", "cpufreq")
6 self.idaFlag = "ida"
7 self.idaSpeedupFactor = 8.0 # percent
8+ self.selectorExe = "cpufreq-selector"
9+ self.ifSelectorExe = None
10
11 def getCPUFreqDirectories(self):
12 if not os.path.exists(self.sysCPUDirectory):
13@@ -71,9 +73,22 @@
14 return line.strip().split()
15 return None
16
17- def setParameter(self, setFile, readFile, value, skip=False):
18+ def setParameter(self, setFile, readFile, value, skip=False, automatch=False):
19+ def findParameter(targetFile):
20+ for root, _, files in os.walk(self.sysCPUDirectory):
21+ for f in files:
22+ rf = os.path.join(root,f)
23+ if targetFile in rf:
24+ return rf
25+ return None
26+
27+ path = None
28 if not skip:
29- path = os.path.join(self.cpufreqDirectory, setFile)
30+ if automatch:
31+ path = findParameter(setFile)
32+ else:
33+ path = os.path.join(self.cpufreqDirectory, setFile)
34+
35 try:
36 check_call("echo \"%s\" > %s" % (value, path), shell=True)
37 except CalledProcessError, exception:
38@@ -82,7 +97,11 @@
39 return False
40
41 # verify it has changed
42- path = os.path.join(self.cpufreqDirectory, readFile)
43+ if automatch:
44+ path = findParameter(readFile)
45+ else:
46+ path = os.path.join(self.cpufreqDirectory, readFile)
47+
48 parameterFile = open(path)
49 line = parameterFile.readline()
50 if not line or line.strip() != str(value):
51@@ -95,13 +114,35 @@
52
53 return True
54
55+ def checkSelectorExecutable(self):
56+ def is_exe(fpath):
57+ return os.path.exists(fpath) and os.access(fpath, os.X_OK)
58+
59+ if self.ifSelectorExe is None:
60+ # cpufreq-selector default path
61+ exe = os.path.join("/usr/bin/",self.selectorExe)
62+ if is_exe(exe):
63+ self.ifSelectorExe = True
64+ return True
65+ for path in os.environ["PATH"].split(os.pathsep):
66+ exe = os.path.join(path, self.selectorExe)
67+ if is_exe(exe):
68+ self.ifSelectorExe = True
69+ return True
70+
71+ self.ifSelectorExe = False
72+ return False
73+
74 def setParameterWithSelector(self, switch, setFile, readFile, value):
75 # Try the command for all CPUs
76 skip = True
77- try:
78- check_call("cpufreq-selector -%s %s" % (switch, value), shell=True)
79- except CalledProcessError, exception:
80- print "Note: command failed: %s" % exception.cmd
81+ if self.checkSelectorExecutable():
82+ try:
83+ check_call("cpufreq-selector -%s %s" % (switch, value), shell=True)
84+ except CalledProcessError, exception:
85+ print "Note: command failed: %s" % exception.cmd
86+ skip = False
87+ else:
88 skip = False
89
90 return self.setParameter(setFile, readFile, value, skip)
91@@ -450,7 +491,7 @@
92
93 # Set the frequency step to 20, so that it jumps to minimum frequency
94 path = os.path.join("conservative", "freq_step")
95- if not self.setParameter(path, path, 20):
96+ if not self.setParameter(path, path, 20, automatch=True):
97 success = False
98
99 # Wait a fixed period of time, then verify current speed is the slowest in as before
100@@ -459,7 +500,7 @@
101 success = False
102
103 # Set the frequency step to 0, so that it doesn't gradually increase
104- if not self.setParameter(path, path, 0):
105+ if not self.setParameter(path, path, 0, automatch=True):
106 success = False
107
108 # Repeat work load test

Subscribers

People subscribed via source and target branches