Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhance systemtools.py to also support POWER systems #1044

Merged
merged 23 commits into from
Feb 26, 2015
Merged
Changes from 2 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f3db58e
fix get_cpu_model in case /proc/cpuinfo doesn't list a model name
boegel Sep 12, 2014
b2ab456
enhance get_cpu_speed to also support determining CPU frequency on PO…
boegel Sep 12, 2014
b97f84f
Merge branch 'develop' into systemtools_fixes_power
boegel Feb 3, 2015
00bfdb5
clean up implementation of get_cpu_speed
boegel Feb 25, 2015
6f900a4
thoroughly test possible code paths in get_cpu_speed by mocking used …
boegel Feb 25, 2015
82f3b4a
clean up get_avail_core_count, enhance testing for it
boegel Feb 25, 2015
526c4fe
clean up get_cpu_vendor and enhance dedicated test for it
boegel Feb 25, 2015
793af55
clean up get_cpu_model and enhance dedicated test for it
boegel Feb 25, 2015
d6b33a0
enhance error message in get_glibc_version
boegel Feb 25, 2015
708de3e
moar systemtools testing via mocking used functions
boegel Feb 25, 2015
1a9f9a3
add get_cpu_family() function in systemtools.py + dedicated test for it
boegel Feb 25, 2015
b6f57c3
use get_cpu_family() rather than get_cpu_vendor() for picking optarch…
boegel Feb 25, 2015
c977b3e
use -mcpu=native for GCC/Clang on POWER
boegel Feb 25, 2015
9bde497
split up systemtools tests into smaller ones (native vs mocked)
boegel Feb 25, 2015
900b660
Merge branch 'develop' into systemtools_fixes_power
boegel Feb 25, 2015
0f8b583
re-add code that shouldn't have been removed in compiler.py
boegel Feb 25, 2015
297dea1
fix broken test
boegel Feb 25, 2015
11d199b
fix small remark
boegel Feb 25, 2015
eab3153
add support for letting get_cpu_vendor return IBM + code cleanup
boegel Feb 25, 2015
cdc875d
fix test_cpu_vendors test
boegel Feb 25, 2015
403acba
fix mocked version of os.path.exists in systemtools tests, only retur…
boegel Feb 25, 2015
06ec32a
fix remarks in systemtools.py
boegel Feb 26, 2015
7ff5511
use a single regex for obtaining model name
boegel Feb 26, 2015
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 24 additions & 11 deletions easybuild/tools/systemtools.py
Original file line number Diff line number Diff line change
Expand Up @@ -190,23 +190,26 @@ def get_cpu_model():
returns cpu model
f.ex Intel(R) Core(TM) i5-2540M CPU @ 2.60GHz
"""
model = UNKNOWN
os_type = get_os_type()
if os_type == LINUX:
regexp = re.compile(r"^model name\s+:\s*(?P<modelname>.+)\s*$", re.M)
try:
txt = read_file('/proc/cpuinfo', log_error=False)
if txt is not None:
return regexp.search(txt).groupdict()['modelname'].strip()
res = regexp.search(txt)
if res is not None:
model = res.group('modelname').strip()
except IOError, err:
raise SystemToolsException("An error occured when determining CPU model: %s" % err)

elif os_type == DARWIN:
out, exitcode = run_cmd("sysctl -n machdep.cpu.brand_string")
out = out.strip()
if not exitcode:
return out
model = out

return UNKNOWN
return model


def get_cpu_speed():
Expand All @@ -219,30 +222,40 @@ def get_cpu_speed():
try:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seeing this block made me think of http://www.isitfriday.org/ on a thursday....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this whole block wrapped in a try/except?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please recheck, cleaned up significantly now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The affinity mask is a POSIX thing right? So why not try the BSD thing and then fall back to the affinity mask?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the BSD one takes into account cpuset and all that crap, the affinity one does, so that should be tried first (and it should always work on Linux)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah agreed. But now, it's only tried on Linux. Why not try it always?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it won't work on non-Linux systems :)

# Linux with cpu scaling
max_freq_fp = '/sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq'
_log.debug("Trying to determine CPU frequency via %s" % max_freq_fp)
try:
f = open(max_freq_fp, 'r')
cpu_freq = float(f.read())/1000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, if the max_freq_fp exists, this should actually be an error. this should only be run if the file exists...

f.close()
return cpu_freq
except IOError, err:
_log.warning("Failed to read %s to determine max. CPU clock frequency with CPU scaling: %s" % (max_freq_fp, err))
_log.debug("Failed to read %s to determine max. CPU clock frequency with CPU scaling: %s" % (max_freq_fp, err))

# Linux without cpu scaling
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, this is linux with procfs mounted, nothing more then that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's Linux without a cpu governor. procfs and sysfs are always mounted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, if we are assuming this is the case, os.path.exists(max_freq_fp) should be a conditional (cpufreq or not), and all exceptions be treated as real errors. no sneaky return in try/except blocks; the logic should be clear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree

cpuinfo_fp = '/proc/cpuinfo'
_log.debug("Trying to determine CPU frequency via %s" % cpuinfo_fp)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test if cpuinfo_fp exists and run this in separate block

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? it it's not there, then we're not in Linux.

try:
cpu_freq = None
f = open(cpuinfo_fp, 'r')
for line in f:
cpu_freq = re.match("^cpu MHz\s*:\s*([0-9.]+)", line)
if cpu_freq is not None:
cpuinfo_txt = open(cpuinfo_fp, 'r').read()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apperently we have a read_file function for this

cpu_freq_patterns = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this a list?

cpu_freq_reg = re.compile(r'^(?:cpu MHz|clock)\s*:\s*(?P<cpu_freq>[0-9.]+)', re.M)

is all you need. if you want to add the output on x86 and power, then add a line of such output as a comment

r"^cpu MHz\s*:\s*(?P<cpu_freq>[0-9.]+)", # Linux x86 & more
r"^clock\s*:\s*(?P<cpu_freq>[0-9.]+)", # Linux on POWER
]
for cpu_freq_pattern in cpu_freq_patterns:
cpu_freq_re = re.compile(cpu_freq_pattern, re.M)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the point in compiling this?

res = cpu_freq_re.search(cpuinfo_txt)
if res:
cpu_freq = res.group('cpu_freq')
_log.debug("Found CPU frequency using regex '%s': %s" % (cpu_freq_pattern, cpu_freq))
break
f.close()
else:
_log.debug("Failed to determine CPU frequency using regex '%s'" % cpu_freq_re.pattern)
if cpu_freq is None:
raise SystemToolsException("Failed to determine CPU frequency from %s" % cpuinfo_fp)
else:
return float(cpu_freq.group(1))
return float(cpu_freq)
except IOError, err:
_log.warning("Failed to read %s to determine CPU clock frequency: %s" % (cpuinfo_fp, err))
_log.debug("Failed to read %s to determine CPU clock frequency: %s" % (cpuinfo_fp, err))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not error? something is serioulsy wrong, but i would limit the try/excpet to the part wher you read /proc/info, nothing more then that


except (IOError, OSError), err:
raise SystemToolsException("Determining CPU speed failed, exception occured: %s" % err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add on linux.

Expand Down