[v2] cpu_layout: refactor to meet python standards

Message ID 20201104065304.26886-1-stephen@networkplumber.org (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v2] cpu_layout: refactor to meet python standards |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/travis-robot success Travis build: passed
ci/iol-testing success Testing PASS
ci/iol-intel-Functional fail Functional Testing issues
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Stephen Hemminger Nov. 4, 2020, 6:53 a.m. UTC
  Rearrange code to make it pass python lint totally clean!
This includes add a main function, docstring, and some
variable name changes.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v2 - rebase to current main

 usertools/cpu_layout.py | 143 ++++++++++++++++++++++++----------------
 1 file changed, 85 insertions(+), 58 deletions(-)
  

Comments

Bruce Richardson Nov. 4, 2020, 9:21 a.m. UTC | #1
On Tue, Nov 03, 2020 at 10:53:04PM -0800, Stephen Hemminger wrote:
> Rearrange code to make it pass python lint totally clean!  This includes
> add a main function, docstring, and some variable name changes.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- v2 -
> rebase to current main
> 
>  usertools/cpu_layout.py | 143 ++++++++++++++++++++++++---------------- 1
>  file changed, 85 insertions(+), 58 deletions(-)
>

Hi Stephen,

Thanks for looking at this, but I honestly query the value of this scale of
rework, since we take a 58 line linear script and increase it to an 85 line
script with multiple functions being called. Rather than trying for full
lint cleanliness, I think we'd be better to keep it simple and just aim for
pep8/pycodestyle cleanliness.  This shows only two small things to fix and
saves massive rework.

  $ pycodestyle cpu_layout.py
  cpu_layout.py:18:5: E722 do not use bare 'except'
  cpu_layout.py:62:14: E231 missing whitespace after ','

Regards,
/Bruce
  
Stephen Hemminger Nov. 4, 2020, 4:22 p.m. UTC | #2
On Wed, 4 Nov 2020 09:21:05 +0000
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Tue, Nov 03, 2020 at 10:53:04PM -0800, Stephen Hemminger wrote:
> > Rearrange code to make it pass python lint totally clean!  This includes
> > add a main function, docstring, and some variable name changes.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- v2 -
> > rebase to current main
> > 
> >  usertools/cpu_layout.py | 143 ++++++++++++++++++++++++---------------- 1
> >  file changed, 85 insertions(+), 58 deletions(-)
> >  
> 
> Hi Stephen,
> 
> Thanks for looking at this, but I honestly query the value of this scale of
> rework, since we take a 58 line linear script and increase it to an 85 line
> script with multiple functions being called. Rather than trying for full
> lint cleanliness, I think we'd be better to keep it simple and just aim for
> pep8/pycodestyle cleanliness.  This shows only two small things to fix and
> saves massive rework.
> 
>   $ pycodestyle cpu_layout.py
>   cpu_layout.py:18:5: E722 do not use bare 'except'
>   cpu_layout.py:62:14: E231 missing whitespace after ','
> 

I was mostly hoping that the python code could be cleaned up so
that when the next set of python code comes for some new feature it
would be more readable.

Could  you send an alternative patch that addresses those changes.
  

Patch

diff --git a/usertools/cpu_layout.py b/usertools/cpu_layout.py
index cc39638213d0..1e4577143ac5 100755
--- a/usertools/cpu_layout.py
+++ b/usertools/cpu_layout.py
@@ -2,65 +2,92 @@ 
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright(c) 2010-2014 Intel Corporation
 # Copyright(c) 2017 Cavium, Inc. All rights reserved.
+"""
+Show CPU layout
+"""
+SYS_DEVICES_CPU = "/sys/devices/system/cpu"
 
-sockets = []
-cores = []
-core_map = {}
-base_path = "/sys/devices/system/cpu"
-fd = open("{}/kernel_max".format(base_path))
-max_cpus = int(fd.read())
-fd.close()
-for cpu in range(max_cpus + 1):
-    try:
-        fd = open("{}/cpu{}/topology/core_id".format(base_path, cpu))
-    except IOError:
-        continue
-    except:
-        break
-    core = int(fd.read())
-    fd.close()
-    fd = open("{}/cpu{}/topology/physical_package_id".format(base_path, cpu))
-    socket = int(fd.read())
-    fd.close()
-    if core not in cores:
-        cores.append(core)
-    if socket not in sockets:
-        sockets.append(socket)
-    key = (socket, core)
-    if key not in core_map:
-        core_map[key] = []
-    core_map[key].append(cpu)
-
-print(format("=" * (47 + len(base_path))))
-print("Core and Socket Information (as reported by '{}')".format(base_path))
-print("{}\n".format("=" * (47 + len(base_path))))
-print("cores = ", cores)
-print("sockets = ", sockets)
-print("")
-
-max_processor_len = len(str(len(cores) * len(sockets) * 2 - 1))
-max_thread_count = len(list(core_map.values())[0])
-max_core_map_len = (max_processor_len * max_thread_count)  \
+
+def print_coremap(sockets, cores, core_map):
+    '''print core, thread, socket mapping'''
+    max_processor_len = len(str(len(cores) * len(sockets) * 2 - 1))
+    max_thread_count = len(list(core_map.values())[0])
+    max_core_map_len = (max_processor_len * max_thread_count)  \
                       + len(", ") * (max_thread_count - 1) \
                       + len('[]') + len('Socket ')
-max_core_id_len = len(str(max(cores)))
-
-output = " ".ljust(max_core_id_len + len('Core '))
-for s in sockets:
-    output += " Socket %s" % str(s).ljust(max_core_map_len - len('Socket '))
-print(output)
-
-output = " ".ljust(max_core_id_len + len('Core '))
-for s in sockets:
-    output += " --------".ljust(max_core_map_len)
-    output += " "
-print(output)
-
-for c in cores:
-    output = "Core %s" % str(c).ljust(max_core_id_len)
-    for s in sockets:
-        if (s,c) in core_map:
-            output += " " + str(core_map[(s, c)]).ljust(max_core_map_len)
-        else:
-            output += " " * (max_core_map_len + 1)
+
+    max_core_id_len = len(str(max(cores)))
+
+    output = " ".ljust(max_core_id_len + len('Core '))
+    for socket in sockets:
+        output += " Socket %s" % str(socket).ljust(max_core_map_len -
+                                                   len('Socket '))
+    print(output)
+
+    output = " ".ljust(max_core_id_len + len('Core '))
+    for socket in sockets:
+        output += " --------".ljust(max_core_map_len)
+        output += " "
     print(output)
+
+    for core in cores:
+        output = "Core %s" % str(core).ljust(max_core_id_len)
+        for socket in sockets:
+            if (socket, core) in core_map:
+                output += " " + str(core_map[(socket,
+                                              core)]).ljust(max_core_map_len)
+            else:
+                output += " " * (max_core_map_len + 1)
+        print(output)
+
+
+def print_header(sockets, cores):
+    '''print the core socket information header'''
+    header_len = 47 + len(SYS_DEVICES_CPU)
+    print(format("=" * header_len))
+    print("Core and Socket Information (as reported by '{}')".format(
+        SYS_DEVICES_CPU))
+    print("{}\n".format("=" * header_len))
+    print("cores = ", cores)
+    print("sockets = ", sockets)
+    print("")
+
+
+def main():
+    '''program main function'''
+
+    with open("{}/kernel_max".format(SYS_DEVICES_CPU)) as kernel_max:
+        max_cpus = int(kernel_max.read())
+
+    core_map = {}
+    sockets = []
+    cores = []
+
+    for cpu in range(max_cpus + 1):
+        topo_path = "{}/cpu{}/topology/".format(SYS_DEVICES_CPU, cpu)
+        try:
+            with open(topo_path + "core_id") as core_id:
+                core = int(core_id.read())
+        except FileNotFoundError:
+            break
+        except IOError:
+            continue
+
+        with open(topo_path + "physical_package_id") as package_id:
+            socket = int(package_id.read())
+
+        if core not in cores:
+            cores.append(core)
+        if socket not in sockets:
+            sockets.append(socket)
+        key = (socket, core)
+        if key not in core_map:
+            core_map[key] = []
+        core_map[key].append(cpu)
+
+    print_header(sockets, cores)
+    print_coremap(sockets, cores, core_map)
+
+
+if __name__ == "__main__":
+    main()