Improved ARM platform/CPU detection by geimer · Pull Request #1953 · easybuilders/easybuild-framework

geimer · 2016-10-09T11:25:10Z

This PR revises several functions in framework.tools.systemtools, especially to improve the platform/CPU detection for ARM-based systems.

for x86_64, POWER, AArch32, and AArch64

boegel · 2016-10-12T17:24:49Z

+POWER = 'POWER'
+X86_64 = 'x86_64'
+
 AMD = 'AMD'


make it clear that these are vendors, while the above are architectures (with a comment)

boegel · 2016-10-12T17:25:22Z

-    'ARM': ARM,
+CPU_ARCHITECTURES = [AARCH32, AARCH64, POWER, X86_64]
+CPU_FAMILIES = [AMD, ARM, INTEL, POWER]
+CPU_VENDORS = [AMD, APM, ARM, BROADCOM, CAVIUM, DEC, IBM, INTEL, MARVELL, MOTOROLA, NVIDIA, QUALCOMM, MARVELL]


@geimer MARVELL is listed twice, your sorting algorithm is broken ;)

Hmm... seems like it's using [begin,end] rather than [begin,end(
Have to check... ;)

boegel · 2016-10-12T17:26:36Z

+CPU_ARCHITECTURES = [AARCH32, AARCH64, POWER, X86_64]
+CPU_FAMILIES = [AMD, ARM, INTEL, POWER]
+CPU_VENDORS = [AMD, APM, ARM, BROADCOM, CAVIUM, DEC, IBM, INTEL, MARVELL, MOTOROLA, NVIDIA, QUALCOMM, MARVELL]
+VENDOR_IDS = {


any reference for this list?

boegel · 2016-10-12T17:26:50Z

    'GenuineIntel': INTEL,
    'IBM': IBM,
 }
+ARM_CORTEX_IDS = {


boegel · 2016-10-12T17:29:12Z

+        arch = AARCH64
+    elif power_regex.match(machine):
+        arch = POWER
+    elif machine == 'x86_64':


rather than sorting alphabetically, I would try and rank these by how common they are, to short-circuit early in the most common case (i.e. x86_64)?

also, compare with X86_64 constant here?

boegel · 2016-10-12T17:34:29Z

+        elif arch == POWER:
+            vendor_regex = re.compile(r"model\s+:\s*(\w+)")
+        elif arch == X86_64:
+            vendor_regex = re.compile(r"vendor_id\s+:\s*(\S+)")


same here, sort by most common?

boegel · 2016-10-12T17:37:38Z


    else:
+        arch = get_cpu_architecture()
+        if arch in [AARCH32, AARCH64]:


if get_cpu_architecture() in [AARCH32, AARCH64]:

since arch is only used in one place?

I personally find the function call within the if statement less readable. But this is a matter of taste, of course.

well, consider this a suggestion, ok to leave it as is if you like this better

boegel · 2016-10-12T17:39:30Z

-        # we need 'model name' on Linux/x86, but 'model' is there first with different info
-        # 'model name' is not there for Linux/POWER, but 'model' has the right info
-        model_regex = re.compile(r"^model(?:\s+name)?\s+:\s*(?P<model>.*[A-Za-z].+)\s*$", re.M)
        txt = read_file(PROC_CPUINFO_FP)


move this down too, to where it's actually used (i.e. in the else), and maybe rename txt to proc_cpuinfo?

The txt variable is actually used in both branches of the if statement...
But I'm fine with renaming. Makes sense.

ah, ok, my bad...

if you rename txt, do it consistently throughout this module where we read PROC_CPUINFO_FP please :)

boegel · 2016-10-12T17:41:40Z

+                if res:
+                    model_ids = list(set(res))
+                    id_list = []
+                    for model_id in model_ids:


model_ids = model_regex.findall(txt) if model_ids: id_list = [] for model_id in set(model_ids):

Will the iteration order be the same for every run? OK, I may have missed a sort() call in my first attempt, but that's a different story...

No, there's no order in a set, and creating a list from it isn't going to change that (the order you get in the list may in theory be different every time).

If you want to guarantee order over unique elements, use sorted(set(model_ids))

boegel · 2016-10-12T17:42:19Z

+                        if model_id in ARM_CORTEX_IDS:
+                            id_list.append(ARM_CORTEX_IDS[model_id])
+                        else:
+                            id_list.append(UNKNOWN)


collapse to:

id_list.append(ARM_CORTEX_IDS.get(model_id, UNKNOWN))

geimer · 2016-10-27T07:49:18Z

@boegel Would you please take another look at this? I almost believe that this PR is a prerequisite for further work on #1952. Thanks!

boegel · 2016-10-27T07:58:26Z

@geimer: thanks for the bump, I lost track of this, sorry

Looks perfect now, so going in, thanks again for all your effort on this one!

geimer added 7 commits October 8, 2016 19:15

Implement get_cpu_architecture() call based on platform.uname()

d613175

for x86_64, POWER, AArch32, and AArch64

Rework get_cpu_vendor() and improve support for ARM-based systems

794862b

Add tests for AMD platform

91758fe

Add implementors of custom ARM-based designs

bcf4435

Add ARM CPU model detection

0eeee2b

Fix get_cpu_family() for custom ARM-based designs from other vendors

d22c39f

Add part number for ARM Cortex-A5

3633a77

geimer mentioned this pull request Oct 9, 2016

Revise ARM platform support #1952

Open

4 tasks

boegel added this to the v3.0 milestone Oct 10, 2016

boegel requested changes Oct 12, 2016

View reviewed changes

Addressed remarks

46749f0

boegel approved these changes Oct 27, 2016

View reviewed changes

boegel merged commit 2aa1e1c into easybuilders:develop Oct 27, 2016

geimer deleted the cpu_detection branch October 27, 2016 09:20

migueldiascosta mentioned this pull request Dec 17, 2020

provide access to architecture type via easyconfig constants easybuilders/easybuild#144

Open

Conversation

geimer commented Oct 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boegel Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geimer commented Oct 27, 2016

Uh oh!

boegel commented Oct 27, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

boegel Oct 12, 2016 •

edited

Loading