[PATCH 0/4] rteval: Add relative cpusets

[PATCH 0/4] rteval: Add relative cpusets

[tglozar]

From: Tomas Glozar <tglozar@redhat.com>

This is a follow-up to "rteval: Refactor CPU list logic" patchset. First three
patches contain some cleanups, most importantly the removal of CpuList class
and moving the code into a module rteval.cpulist_utils.

Fourth patch adds support for relative cpulists for measurements, e.g.
--measurement-cpulist=+1,2,-5,6, for details, see the commit message.

Tomas Glozar (4):
  rteval: Refactor collapse_cpulist in systopology
  rteval: Minor improvements to CpuList class
  rteval: Convert CpuList class to a module
  rteval: Add relative cpulists for measurements

 rteval-cmd                               |  31 ++--
 rteval/cpulist_utils.py                  | 161 ++++++++++++++++
 rteval/modules/loads/__init__.py         |   8 +-
 rteval/modules/loads/hackbench.py        |   9 +-
 rteval/modules/loads/kcompile.py         |  14 +-
 rteval/modules/loads/stressng.py         |   9 +-
 rteval/modules/measurement/__init__.py   |  11 +-
 rteval/modules/measurement/cyclictest.py |  52 +-----
 rteval/systopology.py                    | 223 +++++------------------
 9 files changed, 252 insertions(+), 266 deletions(-)
 create mode 100644 rteval/cpulist_utils.py

-- 
2.39.3

[PATCH 1/4] rteval: Refactor collapse_cpulist in systopology

[tglozar]

From: Tomas Glozar <tglozar@redhat.com>

Instead of having duplicate code in two functions, one top-level and
one member function of CpuList, have only one static function in
CpuList.

Additionally re-write the implementation to use a more straight forward
one-pass algorithm.

Signed-off-by: Tomas Glozar <tglozar@redhat.com>
---
 rteval-cmd                               |  4 +-
 rteval/modules/loads/__init__.py         |  4 +-
 rteval/modules/measurement/__init__.py   |  4 +-
 rteval/modules/measurement/cyclictest.py |  6 +--
 rteval/systopology.py                    | 68 ++++++++----------------
 5 files changed, 30 insertions(+), 56 deletions(-)

diff --git a/rteval-cmd b/rteval-cmd
index 6f613a3..7c41429 100755
--- a/rteval-cmd
+++ b/rteval-cmd
@@ -30,7 +30,7 @@ from rteval import RtEval, rtevalConfig
 from rteval.modules.loads import LoadModules
 from rteval.modules.measurement import MeasurementModules
 from rteval.version import RTEVAL_VERSION
-from rteval.systopology import CpuList, SysTopology, collapse_cpulist
+from rteval.systopology import CpuList, SysTopology
 from rteval.modules.loads.kcompile import ModuleParameters
 
 compress_cpulist = CpuList.compress_cpulist
@@ -211,7 +211,7 @@ def remove_offline(cpulist):
     """ return cpulist in collapsed compressed form with only online cpus """
     tmplist = expand_cpulist(cpulist)
     tmplist = SysTopology().online_cpulist(tmplist)
-    return collapse_cpulist(tmplist)
+    return CpuList.collapse_cpulist(tmplist)
 
 
 if __name__ == '__main__':
diff --git a/rteval/modules/loads/__init__.py b/rteval/modules/loads/__init__.py
index aca0c9f..13fba1e 100644
--- a/rteval/modules/loads/__init__.py
+++ b/rteval/modules/loads/__init__.py
@@ -11,7 +11,7 @@ import libxml2
 from rteval.Log import Log
 from rteval.rtevalConfig import rtevalCfgSection
 from rteval.modules import RtEvalModules, rtevalModulePrototype
-from rteval.systopology import collapse_cpulist, CpuList, SysTopology as SysTop
+from rteval.systopology import CpuList, SysTopology as SysTop
 
 class LoadThread(rtevalModulePrototype):
     def __init__(self, name, config, logger=None):
@@ -120,7 +120,7 @@ class LoadModules(RtEvalModules):
             cpulist = CpuList(cpulist).cpulist
         else:
             cpulist = SysTop().default_cpus()
-        rep_n.newProp("loadcpus", collapse_cpulist(cpulist))
+        rep_n.newProp("loadcpus", CpuList.collapse_cpulist(cpulist))
 
         return rep_n
 
diff --git a/rteval/modules/measurement/__init__.py b/rteval/modules/measurement/__init__.py
index 2a0556b..41b8022 100644
--- a/rteval/modules/measurement/__init__.py
+++ b/rteval/modules/measurement/__init__.py
@@ -5,7 +5,7 @@
 
 import libxml2
 from rteval.modules import RtEvalModules, ModuleContainer
-from rteval.systopology import collapse_cpulist, CpuList, SysTopology as SysTop
+from rteval.systopology import CpuList, SysTopology as SysTop
 
 class MeasurementProfile(RtEvalModules):
     """Keeps and controls all the measurement modules with the same measurement profile"""
@@ -187,7 +187,7 @@ measurement profiles, based on their characteristics"""
             cpulist = CpuList(cpulist).cpulist
         else:
             cpulist = SysTop().online_cpus() if run_on_isolcpus else SysTop().default_cpus()
-        rep_n.newProp("measurecpus", collapse_cpulist(cpulist))
+        rep_n.newProp("measurecpus", CpuList.collapse_cpulist(cpulist))
 
         for mp in self.__measureprofiles:
             mprep_n = mp.MakeReport()
diff --git a/rteval/modules/measurement/cyclictest.py b/rteval/modules/measurement/cyclictest.py
index 0af1d31..1b14e7e 100644
--- a/rteval/modules/measurement/cyclictest.py
+++ b/rteval/modules/measurement/cyclictest.py
@@ -17,7 +17,7 @@ import libxml2
 from rteval.Log import Log
 from rteval.modules import rtevalModulePrototype
 from rteval.systopology import cpuinfo
-from rteval.systopology import CpuList, SysTopology, collapse_cpulist
+from rteval.systopology import CpuList, SysTopology
 
 expand_cpulist = CpuList.expand_cpulist
 
@@ -203,7 +203,7 @@ class Cyclictest(rtevalModulePrototype):
             # Only include online cpus
             self.__cpus = CpuList(self.__cpus).cpulist
             # Reset cpulist from the newly calculated self.__cpus
-            self.__cpulist = collapse_cpulist(self.__cpus)
+            self.__cpulist = CpuList.collapse_cpulist(self.__cpus)
             self.__cpus = [str(c) for c in self.__cpus]
             self.__sparse = True
             if self.__run_on_isolcpus:
@@ -220,7 +220,7 @@ class Cyclictest(rtevalModulePrototype):
             self.__cpus = [c for c in self.__cpus if c in cpuset or self.__run_on_isolcpus and c in isolcpus]
             if self.__run_on_isolcpus:
                 self.__sparse = True
-                self.__cpulist = collapse_cpulist(self.__cpus)
+                self.__cpulist = CpuList.collapse_cpulist([int(c) for c in self.__cpus])
 
         # Sort the list of cpus to align with the order reported by cyclictest
         self.__cpus.sort(key=int)
diff --git a/rteval/systopology.py b/rteval/systopology.py
index 62ad355..ea8e242 100644
--- a/rteval/systopology.py
+++ b/rteval/systopology.py
@@ -10,25 +10,6 @@ import os
 import os.path
 import glob
 
-# Utility version of collapse_cpulist that doesn't require a CpuList object
-def collapse_cpulist(cpulist):
-    """ Collapse a list of cpu numbers into a string range
-        of cpus (e.g. 0-5, 7, 9) """
-    if len(cpulist) == 0:
-        return ""
-    idx = CpuList.longest_sequence(cpulist)
-    if idx == 0:
-        seq = str(cpulist[0])
-    else:
-        if idx == 1:
-            seq = f"{cpulist[0]},{cpulist[idx]}"
-        else:
-            seq = f"{cpulist[0]}-{cpulist[idx]}"
-
-    rest = collapse_cpulist(cpulist[idx+1:])
-    if rest == "":
-        return seq
-    return ",".join((seq, rest))
 
 def sysread(path, obj):
     """ Helper function for reading system files """
@@ -93,7 +74,7 @@ class CpuList:
         self.cpulist.sort()
 
     def __str__(self):
-        return self.__collapse_cpulist(self.cpulist)
+        return self.collapse_cpulist(self.cpulist)
 
     def __contains__(self, cpu):
         return cpu in self.cpulist
@@ -114,35 +95,28 @@ class CpuList:
         return os.path.exists(os.path.join(CpuList.cpupath, "isolated"))
 
     @staticmethod
-    def longest_sequence(cpulist):
-        """ return index of last element of a sequence that steps by one """
-        lim = len(cpulist)
-        for idx, _ in enumerate(cpulist):
-            if idx+1 == lim:
-                break
-            if int(cpulist[idx+1]) != (int(cpulist[idx])+1):
-                return idx
-        return lim - 1
-
-    def __collapse_cpulist(self, cpulist):
-        """ Collapse a list of cpu numbers into a string range
+    def collapse_cpulist(cpulist):
+        """
+        Collapse a list of cpu numbers into a string range
         of cpus (e.g. 0-5, 7, 9)
         """
-        if len(cpulist) == 0:
-            return ""
-        idx = self.longest_sequence(cpulist)
-        if idx == 0:
-            seq = str(cpulist[0])
-        else:
-            if idx == 1:
-                seq = f"{cpulist[0]},{cpulist[idx]}"
+        cur_range = [None, None]
+        result = []
+        for cpu in cpulist + [None]:
+            if cur_range[0] is None:
+                cur_range[0] = cur_range[1] = cpu
+                continue
+            if cpu is not None and cpu == cur_range[1] + 1:
+                # Extend currently processed range
+                cur_range[1] += 1
             else:
-                seq = f"{cpulist[0]}-{cpulist[idx]}"
-
-        rest = self.__collapse_cpulist(cpulist[idx+1:])
-        if rest == "":
-            return seq
-        return ",".join((seq, rest))
+                # Range processing finished, add range to string
+                result.append(f"{cur_range[0]}-{cur_range[1]}"
+                              if cur_range[0] != cur_range[1]
+                              else str(cur_range[0]))
+                # Reset
+                cur_range[0] = cur_range[1] = cpu
+        return ",".join(result)
 
     @staticmethod
     def compress_cpulist(cpulist):
@@ -428,7 +402,7 @@ if __name__ == "__main__":
 
         onlcpus = s.online_cpus()
         print(f'onlcpus = {onlcpus}')
-        onlcpus = collapse_cpulist(onlcpus)
+        onlcpus = CpuList.collapse_cpulist(onlcpus)
         print(f'onlcpus = {onlcpus}')
 
         onlcpus_str = s.online_cpus_str()
-- 
2.39.3

Re: [PATCH 1/4] rteval: Refactor collapse_cpulist in systopology

[John Kacur]

On Wed, 29 Nov 2023, tglozar@redhat.com wrote:

> From: Tomas Glozar <tglozar@redhat.com>
> 
> Instead of having duplicate code in two functions, one top-level and
> one member function of CpuList, have only one static function in
> CpuList.
> 
> Additionally re-write the implementation to use a more straight forward
> one-pass algorithm.
> 
> Signed-off-by: Tomas Glozar <tglozar@redhat.com>
> ---
>  rteval-cmd                               |  4 +-
>  rteval/modules/loads/__init__.py         |  4 +-
>  rteval/modules/measurement/__init__.py   |  4 +-
>  rteval/modules/measurement/cyclictest.py |  6 +--
>  rteval/systopology.py                    | 68 ++++++++----------------
>  5 files changed, 30 insertions(+), 56 deletions(-)
> 
> diff --git a/rteval-cmd b/rteval-cmd
> index 6f613a3..7c41429 100755
> --- a/rteval-cmd
> +++ b/rteval-cmd
> @@ -30,7 +30,7 @@ from rteval import RtEval, rtevalConfig
>  from rteval.modules.loads import LoadModules
>  from rteval.modules.measurement import MeasurementModules
>  from rteval.version import RTEVAL_VERSION
> -from rteval.systopology import CpuList, SysTopology, collapse_cpulist
> +from rteval.systopology import CpuList, SysTopology
>  from rteval.modules.loads.kcompile import ModuleParameters
>  
>  compress_cpulist = CpuList.compress_cpulist
> @@ -211,7 +211,7 @@ def remove_offline(cpulist):
>      """ return cpulist in collapsed compressed form with only online cpus """
>      tmplist = expand_cpulist(cpulist)
>      tmplist = SysTopology().online_cpulist(tmplist)
> -    return collapse_cpulist(tmplist)
> +    return CpuList.collapse_cpulist(tmplist)
>  
>  
>  if __name__ == '__main__':
> diff --git a/rteval/modules/loads/__init__.py b/rteval/modules/loads/__init__.py
> index aca0c9f..13fba1e 100644
> --- a/rteval/modules/loads/__init__.py
> +++ b/rteval/modules/loads/__init__.py
> @@ -11,7 +11,7 @@ import libxml2
>  from rteval.Log import Log
>  from rteval.rtevalConfig import rtevalCfgSection
>  from rteval.modules import RtEvalModules, rtevalModulePrototype
> -from rteval.systopology import collapse_cpulist, CpuList, SysTopology as SysTop
> +from rteval.systopology import CpuList, SysTopology as SysTop
>  
>  class LoadThread(rtevalModulePrototype):
>      def __init__(self, name, config, logger=None):
> @@ -120,7 +120,7 @@ class LoadModules(RtEvalModules):
>              cpulist = CpuList(cpulist).cpulist
>          else:
>              cpulist = SysTop().default_cpus()
> -        rep_n.newProp("loadcpus", collapse_cpulist(cpulist))
> +        rep_n.newProp("loadcpus", CpuList.collapse_cpulist(cpulist))
>  
>          return rep_n
>  
> diff --git a/rteval/modules/measurement/__init__.py b/rteval/modules/measurement/__init__.py
> index 2a0556b..41b8022 100644
> --- a/rteval/modules/measurement/__init__.py
> +++ b/rteval/modules/measurement/__init__.py
> @@ -5,7 +5,7 @@
>  
>  import libxml2
>  from rteval.modules import RtEvalModules, ModuleContainer
> -from rteval.systopology import collapse_cpulist, CpuList, SysTopology as SysTop
> +from rteval.systopology import CpuList, SysTopology as SysTop
>  
>  class MeasurementProfile(RtEvalModules):
>      """Keeps and controls all the measurement modules with the same measurement profile"""
> @@ -187,7 +187,7 @@ measurement profiles, based on their characteristics"""
>              cpulist = CpuList(cpulist).cpulist
>          else:
>              cpulist = SysTop().online_cpus() if run_on_isolcpus else SysTop().default_cpus()
> -        rep_n.newProp("measurecpus", collapse_cpulist(cpulist))
> +        rep_n.newProp("measurecpus", CpuList.collapse_cpulist(cpulist))
>  
>          for mp in self.__measureprofiles:
>              mprep_n = mp.MakeReport()
> diff --git a/rteval/modules/measurement/cyclictest.py b/rteval/modules/measurement/cyclictest.py
> index 0af1d31..1b14e7e 100644
> --- a/rteval/modules/measurement/cyclictest.py
> +++ b/rteval/modules/measurement/cyclictest.py
> @@ -17,7 +17,7 @@ import libxml2
>  from rteval.Log import Log
>  from rteval.modules import rtevalModulePrototype
>  from rteval.systopology import cpuinfo
> -from rteval.systopology import CpuList, SysTopology, collapse_cpulist
> +from rteval.systopology import CpuList, SysTopology
>  
>  expand_cpulist = CpuList.expand_cpulist
>  
> @@ -203,7 +203,7 @@ class Cyclictest(rtevalModulePrototype):
>              # Only include online cpus
>              self.__cpus = CpuList(self.__cpus).cpulist
>              # Reset cpulist from the newly calculated self.__cpus
> -            self.__cpulist = collapse_cpulist(self.__cpus)
> +            self.__cpulist = CpuList.collapse_cpulist(self.__cpus)
>              self.__cpus = [str(c) for c in self.__cpus]
>              self.__sparse = True
>              if self.__run_on_isolcpus:
> @@ -220,7 +220,7 @@ class Cyclictest(rtevalModulePrototype):
>              self.__cpus = [c for c in self.__cpus if c in cpuset or self.__run_on_isolcpus and c in isolcpus]
>              if self.__run_on_isolcpus:
>                  self.__sparse = True
> -                self.__cpulist = collapse_cpulist(self.__cpus)
> +                self.__cpulist = CpuList.collapse_cpulist([int(c) for c in self.__cpus])
>  
>          # Sort the list of cpus to align with the order reported by cyclictest
>          self.__cpus.sort(key=int)
> diff --git a/rteval/systopology.py b/rteval/systopology.py
> index 62ad355..ea8e242 100644
> --- a/rteval/systopology.py
> +++ b/rteval/systopology.py
> @@ -10,25 +10,6 @@ import os
>  import os.path
>  import glob
>  
> -# Utility version of collapse_cpulist that doesn't require a CpuList object
> -def collapse_cpulist(cpulist):
> -    """ Collapse a list of cpu numbers into a string range
> -        of cpus (e.g. 0-5, 7, 9) """
> -    if len(cpulist) == 0:
> -        return ""
> -    idx = CpuList.longest_sequence(cpulist)
> -    if idx == 0:
> -        seq = str(cpulist[0])
> -    else:
> -        if idx == 1:
> -            seq = f"{cpulist[0]},{cpulist[idx]}"
> -        else:
> -            seq = f"{cpulist[0]}-{cpulist[idx]}"
> -
> -    rest = collapse_cpulist(cpulist[idx+1:])
> -    if rest == "":
> -        return seq
> -    return ",".join((seq, rest))
>  
>  def sysread(path, obj):
>      """ Helper function for reading system files """
> @@ -93,7 +74,7 @@ class CpuList:
>          self.cpulist.sort()
>  
>      def __str__(self):
> -        return self.__collapse_cpulist(self.cpulist)
> +        return self.collapse_cpulist(self.cpulist)
>  
>      def __contains__(self, cpu):
>          return cpu in self.cpulist
> @@ -114,35 +95,28 @@ class CpuList:
>          return os.path.exists(os.path.join(CpuList.cpupath, "isolated"))
>  
>      @staticmethod
> -    def longest_sequence(cpulist):
> -        """ return index of last element of a sequence that steps by one """
> -        lim = len(cpulist)
> -        for idx, _ in enumerate(cpulist):
> -            if idx+1 == lim:
> -                break
> -            if int(cpulist[idx+1]) != (int(cpulist[idx])+1):
> -                return idx
> -        return lim - 1
> -
> -    def __collapse_cpulist(self, cpulist):
> -        """ Collapse a list of cpu numbers into a string range
> +    def collapse_cpulist(cpulist):
> +        """
> +        Collapse a list of cpu numbers into a string range
>          of cpus (e.g. 0-5, 7, 9)
>          """
> -        if len(cpulist) == 0:
> -            return ""
> -        idx = self.longest_sequence(cpulist)
> -        if idx == 0:
> -            seq = str(cpulist[0])
> -        else:
> -            if idx == 1:
> -                seq = f"{cpulist[0]},{cpulist[idx]}"
> +        cur_range = [None, None]
> +        result = []
> +        for cpu in cpulist + [None]:
> +            if cur_range[0] is None:
> +                cur_range[0] = cur_range[1] = cpu
> +                continue
> +            if cpu is not None and cpu == cur_range[1] + 1:
> +                # Extend currently processed range
> +                cur_range[1] += 1
>              else:
> -                seq = f"{cpulist[0]}-{cpulist[idx]}"
> -
> -        rest = self.__collapse_cpulist(cpulist[idx+1:])
> -        if rest == "":
> -            return seq
> -        return ",".join((seq, rest))
> +                # Range processing finished, add range to string
> +                result.append(f"{cur_range[0]}-{cur_range[1]}"
> +                              if cur_range[0] != cur_range[1]
> +                              else str(cur_range[0]))
> +                # Reset
> +                cur_range[0] = cur_range[1] = cpu
> +        return ",".join(result)
>  
>      @staticmethod
>      def compress_cpulist(cpulist):
> @@ -428,7 +402,7 @@ if __name__ == "__main__":
>  
>          onlcpus = s.online_cpus()
>          print(f'onlcpus = {onlcpus}')
> -        onlcpus = collapse_cpulist(onlcpus)
> +        onlcpus = CpuList.collapse_cpulist(onlcpus)
>          print(f'onlcpus = {onlcpus}')
>  
>          onlcpus_str = s.online_cpus_str()
> -- 
> 2.39.3
> 

I don't necessarily find the iterative version of the function more 
readable than the recursive one, although I'm sure it is more 
efficient. This is all fine, although not all the refractoring here
is necessary to implement the new feature. Sometimes I would
prefer those efforts to come later.

Signed-off-by: John Kacur <jkacur@redhat.com>

[PATCH 2/4] rteval: Minor improvements to CpuList class

[tglozar]

From: Tomas Glozar <tglozar@redhat.com>

- Remove unnecessary if-else from online_file_exists
- Use cpupath in online_file_exists
- Check for cpu0 instead of cpu1 in online_file_exists
- In is_online, remove check for n in cpuset and make it static
- Mark also the remaining methods static since they do not rely on
any fields of the class

Signed-off-by: Tomas Glozar <tglozar@redhat.com>
---
 rteval/systopology.py | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/rteval/systopology.py b/rteval/systopology.py
index ea8e242..60ac8e8 100644
--- a/rteval/systopology.py
+++ b/rteval/systopology.py
@@ -82,12 +82,17 @@ class CpuList:
     def __len__(self):
         return len(self.cpulist)
 
+    def getcpulist(self):
+        """ return the list of cpus tracked """
+        return self.cpulist
+
     @staticmethod
     def online_file_exists():
         """ Check whether machine / kernel is configured with online file """
-        if os.path.exists('/sys/devices/system/cpu/cpu1/online'):
-            return True
-        return False
+        # Note: some machines do not have cpu0/online so we check cpu1/online.
+        # In the case of machines with a single CPU, there is no cpu1, but
+        # that is not a problem, since a single CPU cannot be offline
+        return os.path.exists(os.path.join(CpuList.cpupath, "cpu1/online"))
 
     @staticmethod
     def isolated_file_exists():
@@ -147,14 +152,9 @@ class CpuList:
                 result.append(a)
         return [int(i) for i in list(set(result))]
 
-    def getcpulist(self):
-        """ return the list of cpus tracked """
-        return self.cpulist
-
-    def is_online(self, n):
+    @staticmethod
+    def is_online(n):
         """ check whether cpu n is online """
-        if n not in self.cpulist:
-            raise RuntimeError(f"invalid cpu number {n}")
         path = os.path.join(CpuList.cpupath, f'cpu{n}')
 
         # Some hardware doesn't allow cpu0 to be turned off
@@ -163,16 +163,17 @@ class CpuList:
 
         return sysread(path, "online") == "1"
 
-    def online_cpulist(self, cpulist):
+    @staticmethod
+    def online_cpulist(cpulist):
         """ Given a cpulist, return a cpulist of online cpus """
         # This only works if the sys online files exist
-        if not self.online_file_exists():
+        if not CpuList.online_file_exists():
             return cpulist
         newlist = []
         for cpu in cpulist:
-            if not self.online_file_exists() and cpu == '0':
+            if not CpuList.online_file_exists() and cpu == '0':
                 newlist.append(cpu)
-            elif self.is_online(int(cpu)):
+            elif CpuList.is_online(int(cpu)):
                 newlist.append(cpu)
         return newlist
 
-- 
2.39.3

Re: [PATCH 2/4] rteval: Minor improvements to CpuList class

[John Kacur]

On Wed, 29 Nov 2023, tglozar@redhat.com wrote:

> From: Tomas Glozar <tglozar@redhat.com>
> 
> - Remove unnecessary if-else from online_file_exists
> - Use cpupath in online_file_exists
> - Check for cpu0 instead of cpu1 in online_file_exists

Oops, your code is correct but you left in the old wrong explanation
you need to fix this up before I can apply this.

> - In is_online, remove check for n in cpuset and make it static
> - Mark also the remaining methods static since they do not rely on
> any fields of the class
> 
> Signed-off-by: Tomas Glozar <tglozar@redhat.com>
> ---
>  rteval/systopology.py | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/rteval/systopology.py b/rteval/systopology.py
> index ea8e242..60ac8e8 100644
> --- a/rteval/systopology.py
> +++ b/rteval/systopology.py
> @@ -82,12 +82,17 @@ class CpuList:
>      def __len__(self):
>          return len(self.cpulist)
>  
> +    def getcpulist(self):
> +        """ return the list of cpus tracked """
> +        return self.cpulist
> +
>      @staticmethod
>      def online_file_exists():
>          """ Check whether machine / kernel is configured with online file """
> -        if os.path.exists('/sys/devices/system/cpu/cpu1/online'):
> -            return True
> -        return False
> +        # Note: some machines do not have cpu0/online so we check cpu1/online.
> +        # In the case of machines with a single CPU, there is no cpu1, but
> +        # that is not a problem, since a single CPU cannot be offline
> +        return os.path.exists(os.path.join(CpuList.cpupath, "cpu1/online"))
>  
>      @staticmethod
>      def isolated_file_exists():
> @@ -147,14 +152,9 @@ class CpuList:
>                  result.append(a)
>          return [int(i) for i in list(set(result))]
>  
> -    def getcpulist(self):
> -        """ return the list of cpus tracked """
> -        return self.cpulist
> -
> -    def is_online(self, n):
> +    @staticmethod
> +    def is_online(n):
>          """ check whether cpu n is online """
> -        if n not in self.cpulist:
> -            raise RuntimeError(f"invalid cpu number {n}")

Are you sure you want to remove this? We have three states,
1. cpu is online
2. cpu is offline
3. cpus is not legitimate, ie, we have 12 cpus and the code asks
whether cpu number 1000 is online
I think you're trying to write more elegant code than the original 
implementation, but is it safer? Should we maybe check if the path
exists before calling sysread()?

>          path = os.path.join(CpuList.cpupath, f'cpu{n}')
>  
>          # Some hardware doesn't allow cpu0 to be turned off
> @@ -163,16 +163,17 @@ class CpuList:
>  
>          return sysread(path, "online") == "1"
>  
> -    def online_cpulist(self, cpulist):
> +    @staticmethod
> +    def online_cpulist(cpulist):
>          """ Given a cpulist, return a cpulist of online cpus """
>          # This only works if the sys online files exist
> -        if not self.online_file_exists():
> +        if not CpuList.online_file_exists():
>              return cpulist
>          newlist = []
>          for cpu in cpulist:
> -            if not self.online_file_exists() and cpu == '0':
> +            if not CpuList.online_file_exists() and cpu == '0':
>                  newlist.append(cpu)
> -            elif self.is_online(int(cpu)):
> +            elif CpuList.is_online(int(cpu)):
>                  newlist.append(cpu)
>          return newlist
>  
> -- 
> 2.39.3
> 
> 
> 

Re: [PATCH 2/4] rteval: Minor improvements to CpuList class

[Tomas Glozar]

út 12. 12. 2023 v 22:23 odesílatel John Kacur <jkacur@redhat.com> napsal:
> Oops, your code is correct but you left in the old wrong explanation
> you need to fix this up before I can apply this.
>

Yeah I completely missed that!

> Are you sure you want to remove this? We have three states,
> 1. cpu is online
> 2. cpu is offline
> 3. cpus is not legitimate, ie, we have 12 cpus and the code asks
> whether cpu number 1000 is online
> I think you're trying to write more elegant code than the original
> implementation, but is it safer? Should we maybe check if the path
> exists before calling sysread()?
>

What I didn't like about the original implementation was that checking
if a CPU is online has little to do with the CpuList object. Thus, I
made it static and removed the CpuList argument (self). I agree that
the proper solution is to check if the CPU exists in the system
(instead of in the cpulist), not removing the check altogether.

Tomas

Re: [PATCH 2/4] rteval: Minor improvements to CpuList class

[John Kacur]

[-- Attachment #1: Type: text/plain, Size: 1385 bytes --]



On Thu, 14 Dec 2023, Tomas Glozar wrote:

> út 12. 12. 2023 v 22:23 odesílatel John Kacur <jkacur@redhat.com> napsal:
> > Oops, your code is correct but you left in the old wrong explanation
> > you need to fix this up before I can apply this.
> >
> 
> Yeah I completely missed that!
> 
> > Are you sure you want to remove this? We have three states,
> > 1. cpu is online
> > 2. cpu is offline
> > 3. cpus is not legitimate, ie, we have 12 cpus and the code asks
> > whether cpu number 1000 is online
> > I think you're trying to write more elegant code than the original
> > implementation, but is it safer? Should we maybe check if the path
> > exists before calling sysread()?
> >
> 
> What I didn't like about the original implementation was that checking
> if a CPU is online has little to do with the CpuList object. Thus, I
> made it static and removed the CpuList argument (self). I agree that
> the proper solution is to check if the CPU exists in the system
> (instead of in the cpulist), not removing the check altogether.
> 
> Tomas
> 
> 
> 

Alright, I amended the commit message to fix it.
I might make you check for the file's existence in some later patches
but I'm applying this for now to move forward, but I'm waiting for
any changes in patches 3 and 4 from you.

    - Removed incorrect line from commit message
    Signed-off-by: John Kacur <jkacur@redhat.com>

[PATCH 3/4] rteval: Convert CpuList class to a module

[tglozar]

From: Tomas Glozar <tglozar@redhat.com>

Move out code from CpuList class in rteval.systopology into a separate
module named rteval.cpulist_utils and avoid wrapping CPU lists in
a CpuList object.

Almost all uses of CpuList in the code either use the static methods of
the class or its constructor to filter online CPUs by running
CpuList(...).cpulist. The only exception to this are NumaNode and
SimNumaNode classes in systopology; these store a CpuList object,
however their .getcpulist() method extracts the list out. Thus,
the class is completely unnecessary.

Note: A better name for the module would be cpulist, consistent with the
original name of the class, but this name is already used for variables
throughout the code, hence cpulist_utils is used instead.

Signed-off-by: Tomas Glozar <tglozar@redhat.com>
---
 rteval-cmd                               |  17 ++-
 rteval/cpulist_utils.py                  | 128 ++++++++++++++++++
 rteval/modules/loads/__init__.py         |   8 +-
 rteval/modules/loads/hackbench.py        |   9 +-
 rteval/modules/loads/kcompile.py         |  14 +-
 rteval/modules/loads/stressng.py         |   9 +-
 rteval/modules/measurement/__init__.py   |   8 +-
 rteval/modules/measurement/cyclictest.py |  12 +-
 rteval/systopology.py                    | 165 ++---------------------
 9 files changed, 178 insertions(+), 192 deletions(-)
 create mode 100644 rteval/cpulist_utils.py

diff --git a/rteval-cmd b/rteval-cmd
index 7c41429..56b2c95 100755
--- a/rteval-cmd
+++ b/rteval-cmd
@@ -30,11 +30,10 @@ from rteval import RtEval, rtevalConfig
 from rteval.modules.loads import LoadModules
 from rteval.modules.measurement import MeasurementModules
 from rteval.version import RTEVAL_VERSION
-from rteval.systopology import CpuList, SysTopology
+from rteval.systopology import SysTopology
 from rteval.modules.loads.kcompile import ModuleParameters
+import rteval.cpulist_utils as cpulist_utils
 
-compress_cpulist = CpuList.compress_cpulist
-expand_cpulist = CpuList.expand_cpulist
 
 def summarize(repfile, xslt):
     """ Summarize an already existing XML report """
@@ -209,9 +208,9 @@ def parse_options(cfg, parser, cmdargs):
 
 def remove_offline(cpulist):
     """ return cpulist in collapsed compressed form with only online cpus """
-    tmplist = expand_cpulist(cpulist)
+    tmplist = cpulist_utils.expand_cpulist(cpulist)
     tmplist = SysTopology().online_cpulist(tmplist)
-    return CpuList.collapse_cpulist(tmplist)
+    return cpulist_utils.collapse_cpulist(tmplist)
 
 
 if __name__ == '__main__':
@@ -343,14 +342,14 @@ if __name__ == '__main__':
         # if we only specified one set of cpus (loads or measurement)
         # default the other to the inverse of the specified list
         if not ldcfg.cpulist and msrcfg.cpulist:
-            tmplist = expand_cpulist(msrcfg.cpulist)
+            tmplist = cpulist_utils.expand_cpulist(msrcfg.cpulist)
             tmplist = SysTopology().invert_cpulist(tmplist)
-            ldcfg.cpulist = compress_cpulist(tmplist)
+            ldcfg.cpulist = cpulist_utils.compress_cpulist(tmplist)
             msrcfg.cpulist = remove_offline(msrcfg.cpulist)
         if not msrcfg.cpulist and ldcfg.cpulist:
-            tmplist = expand_cpulist(ldcfg.cpulist)
+            tmplist = cpulist_utils.expand_cpulist(ldcfg.cpulist)
             tmplist = SysTopology().invert_cpulist(tmplist)
-            msrcfg.cpulist = compress_cpulist(tmplist)
+            msrcfg.cpulist = cpulist_utils.compress_cpulist(tmplist)
             ldcfg.cpulist = remove_offline(ldcfg.cpulist)
 
         if ldcfg.cpulist:
diff --git a/rteval/cpulist_utils.py b/rteval/cpulist_utils.py
new file mode 100644
index 0000000..402d579
--- /dev/null
+++ b/rteval/cpulist_utils.py
@@ -0,0 +1,128 @@
+# -*- coding: utf-8 -*-
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+#   Copyright 2016 - Clark Williams <williams@redhat.com>
+#   Copyright 2021 - John Kacur <jkacur@redhat.com>
+#   Copyright 2023 - Tomas Glozar <tglozar@redhat.com>
+#
+"""Module providing utility functions for working with CPU lists"""
+
+import os
+
+
+cpupath = "/sys/devices/system/cpu"
+
+
+def sysread(path, obj):
+    """ Helper function for reading system files """
+    with open(os.path.join(path, obj), "r") as fp:
+        return fp.readline().strip()
+
+
+def _online_file_exists():
+    """ Check whether machine / kernel is configured with online file """
+    # Note: some machines do not have cpu0/online so we check cpu1/online.
+    # In the case of machines with a single CPU, there is no cpu1, but
+    # that is not a problem, since a single CPU cannot be offline
+    return os.path.exists(os.path.join(cpupath, "cpu1/online"))
+
+
+def _isolated_file_exists():
+    """ Check whether machine / kernel is configured with isolated file """
+    return os.path.exists(os.path.join(cpupath, "isolated"))
+
+
+def collapse_cpulist(cpulist):
+    """
+    Collapse a list of cpu numbers into a string range
+    of cpus (e.g. 0-5, 7, 9)
+    """
+    cur_range = [None, None]
+    result = []
+    for cpu in cpulist + [None]:
+        if cur_range[0] is None:
+            cur_range[0] = cur_range[1] = cpu
+            continue
+        if cpu is not None and cpu == cur_range[1] + 1:
+            # Extend currently processed range
+            cur_range[1] += 1
+        else:
+            # Range processing finished, add range to string
+            result.append(f"{cur_range[0]}-{cur_range[1]}"
+                          if cur_range[0] != cur_range[1]
+                          else str(cur_range[0]))
+            # Reset
+            cur_range[0] = cur_range[1] = cpu
+    return ",".join(result)
+
+
+def compress_cpulist(cpulist):
+    """ return a string representation of cpulist """
+    if not cpulist:
+        return ""
+    if isinstance(cpulist[0], int):
+        return ",".join(str(e) for e in cpulist)
+    return ",".join(cpulist)
+
+
+def expand_cpulist(cpulist):
+    """ expand a range string into an array of cpu numbers
+    don't error check against online cpus
+    """
+    result = []
+
+    if not cpulist:
+        return result
+
+    for part in cpulist.split(','):
+        if '-' in part:
+            a, b = part.split('-')
+            a, b = int(a), int(b)
+            result.extend(list(range(a, b + 1)))
+        else:
+            a = int(part)
+            result.append(a)
+    return [int(i) for i in list(set(result))]
+
+
+def is_online(n):
+    """ check whether cpu n is online """
+    path = os.path.join(cpupath, f'cpu{n}')
+
+    # Some hardware doesn't allow cpu0 to be turned off
+    if not os.path.exists(path + '/online') and n == 0:
+        return True
+
+    return sysread(path, "online") == "1"
+
+
+def online_cpulist(cpulist):
+    """ Given a cpulist, return a cpulist of online cpus """
+    # This only works if the sys online files exist
+    if not _online_file_exists():
+        return cpulist
+    newlist = []
+    for cpu in cpulist:
+        if not _online_file_exists() and cpu == '0':
+            newlist.append(cpu)
+        elif is_online(int(cpu)):
+            newlist.append(cpu)
+    return newlist
+
+
+def isolated_cpulist(cpulist):
+    """Given a cpulist, return a cpulist of isolated CPUs"""
+    if not _isolated_file_exists():
+        return cpulist
+    isolated_cpulist = sysread(cpupath, "isolated")
+    isolated_cpulist = expand_cpulist(isolated_cpulist)
+    return list(set(isolated_cpulist) & set(cpulist))
+
+
+def nonisolated_cpulist(cpulist):
+    """Given a cpulist, return a cpulist of non-isolated CPUs"""
+    if not _isolated_file_exists():
+        return cpulist
+    isolated_cpulist = sysread(cpupath, "isolated")
+    isolated_cpulist = expand_cpulist(isolated_cpulist)
+    return list(set(cpulist).difference(set(isolated_cpulist)))
diff --git a/rteval/modules/loads/__init__.py b/rteval/modules/loads/__init__.py
index 13fba1e..0845742 100644
--- a/rteval/modules/loads/__init__.py
+++ b/rteval/modules/loads/__init__.py
@@ -11,7 +11,8 @@ import libxml2
 from rteval.Log import Log
 from rteval.rtevalConfig import rtevalCfgSection
 from rteval.modules import RtEvalModules, rtevalModulePrototype
-from rteval.systopology import CpuList, SysTopology as SysTop
+from rteval.systopology import SysTopology as SysTop
+import rteval.cpulist_utils as cpulist_utils
 
 class LoadThread(rtevalModulePrototype):
     def __init__(self, name, config, logger=None):
@@ -117,10 +118,11 @@ class LoadModules(RtEvalModules):
         cpulist = self._cfg.GetSection(self._module_config).cpulist
         if cpulist:
             # Convert str to list and remove offline cpus
-            cpulist = CpuList(cpulist).cpulist
+            cpulist = cpulist_utils.expand_cpulist(cpulist)
+            cpulist = cpulist_utils.online_cpulist(cpulist)
         else:
             cpulist = SysTop().default_cpus()
-        rep_n.newProp("loadcpus", CpuList.collapse_cpulist(cpulist))
+        rep_n.newProp("loadcpus", cpulist_utils.collapse_cpulist(cpulist))
 
         return rep_n
 
diff --git a/rteval/modules/loads/hackbench.py b/rteval/modules/loads/hackbench.py
index cfc7063..8df2d0a 100644
--- a/rteval/modules/loads/hackbench.py
+++ b/rteval/modules/loads/hackbench.py
@@ -16,10 +16,9 @@ import errno
 from signal import SIGKILL
 from rteval.modules.loads import CommandLineLoad
 from rteval.Log import Log
-from rteval.systopology import CpuList, SysTopology
+from rteval.systopology import SysTopology
+import rteval.cpulist_utils as cpulist_utils
 
-expand_cpulist = CpuList.expand_cpulist
-isolated_cpulist = CpuList.isolated_cpulist
 
 class Hackbench(CommandLineLoad):
     def __init__(self, config, logger):
@@ -58,10 +57,10 @@ class Hackbench(CommandLineLoad):
             self.cpus[n] = sysTop.getcpus(int(n))
             # if a cpulist was specified, only allow cpus in that list on the node
             if self.cpulist:
-                self.cpus[n] = [c for c in self.cpus[n] if c in expand_cpulist(self.cpulist)]
+                self.cpus[n] = [c for c in self.cpus[n] if c in cpulist_utils.expand_cpulist(self.cpulist)]
             # if a cpulist was not specified, exclude isolated cpus
             else:
-                self.cpus[n] = CpuList.nonisolated_cpulist(self.cpus[n])
+                self.cpus[n] = cpulist_utils.nonisolated_cpulist(self.cpus[n])
 
             # track largest number of cpus used on a node
             node_biggest = len(self.cpus[n])
diff --git a/rteval/modules/loads/kcompile.py b/rteval/modules/loads/kcompile.py
index 0d02577..93605e1 100644
--- a/rteval/modules/loads/kcompile.py
+++ b/rteval/modules/loads/kcompile.py
@@ -14,11 +14,9 @@ import subprocess
 from rteval.modules import rtevalRuntimeError
 from rteval.modules.loads import CommandLineLoad
 from rteval.Log import Log
-from rteval.systopology import CpuList, SysTopology
+from rteval.systopology import SysTopology
+import rteval.cpulist_utils as cpulist_utils
 
-expand_cpulist = CpuList.expand_cpulist
-compress_cpulist = CpuList.compress_cpulist
-nonisolated_cpulist = CpuList.nonisolated_cpulist
 
 DEFAULT_KERNEL_PREFIX = "linux-6.6"
 
@@ -38,7 +36,7 @@ class KBuildJob:
             os.mkdir(self.objdir)
 
         # Exclude isolated CPUs if cpulist not set
-        cpus_available = len(nonisolated_cpulist(self.node.cpus.cpulist))
+        cpus_available = len(cpulist_utils.nonisolated_cpulist(self.node.cpus))
 
         if os.path.exists('/usr/bin/numactl') and not cpulist:
             # Use numactl
@@ -47,7 +45,7 @@ class KBuildJob:
         elif cpulist:
             # Use taskset
             self.jobs = self.calc_jobs_per_cpu() * len(cpulist)
-            self.binder = f'taskset -c {compress_cpulist(cpulist)}'
+            self.binder = f'taskset -c {cpulist_utils.compress_cpulist(cpulist)}'
         else:
             # Without numactl calculate number of jobs from the node
             self.jobs = self.calc_jobs_per_cpu() * cpus_available
@@ -220,7 +218,7 @@ class Kcompile(CommandLineLoad):
 
             # if a cpulist was specified, only allow cpus in that list on the node
             if self.cpulist:
-                self.cpus[n] = [c for c in self.cpus[n] if c in expand_cpulist(self.cpulist)]
+                self.cpus[n] = [c for c in self.cpus[n] if c in cpulist_utils.expand_cpulist(self.cpulist)]
 
         # remove nodes with no cpus available for running
         for node, cpus in self.cpus.items():
@@ -282,7 +280,7 @@ class Kcompile(CommandLineLoad):
 
         if 'cpulist' in self._cfg and self._cfg.cpulist:
             cpulist = self._cfg.cpulist
-            self.num_cpus = len(expand_cpulist(cpulist))
+            self.num_cpus = len(cpulist_utils.expand_cpulist(cpulist))
         else:
             cpulist = ""
 
diff --git a/rteval/modules/loads/stressng.py b/rteval/modules/loads/stressng.py
index 7c9e63f..524604e 100644
--- a/rteval/modules/loads/stressng.py
+++ b/rteval/modules/loads/stressng.py
@@ -7,10 +7,9 @@ import subprocess
 import signal
 from rteval.modules.loads import CommandLineLoad
 from rteval.Log import Log
-from rteval.systopology import CpuList, SysTopology
+from rteval.systopology import SysTopology
+import rteval.cpulist_utils as cpulist_utils
 
-expand_cpulist = CpuList.expand_cpulist
-nonisolated_cpulist = CpuList.nonisolated_cpulist
 
 class Stressng(CommandLineLoad):
     " This class creates a load module that runs stress-ng "
@@ -70,10 +69,10 @@ class Stressng(CommandLineLoad):
             cpus[n] = systop.getcpus(int(n))
             # if a cpulist was specified, only allow cpus in that list on the node
             if self.cpulist:
-                cpus[n] = [c for c in cpus[n] if c in expand_cpulist(self.cpulist)]
+                cpus[n] = [c for c in cpus[n] if c in cpulist_utils.expand_cpulist(self.cpulist)]
             # if a cpulist was not specified, exclude isolated cpus
             else:
-                cpus[n] = CpuList.nonisolated_cpulist(cpus[n])
+                cpus[n] = cpulist_utils.nonisolated_cpulist(cpus[n])
 
 
         # remove nodes with no cpus available for running
diff --git a/rteval/modules/measurement/__init__.py b/rteval/modules/measurement/__init__.py
index 41b8022..66dc9c5 100644
--- a/rteval/modules/measurement/__init__.py
+++ b/rteval/modules/measurement/__init__.py
@@ -5,7 +5,8 @@
 
 import libxml2
 from rteval.modules import RtEvalModules, ModuleContainer
-from rteval.systopology import CpuList, SysTopology as SysTop
+from rteval.systopology import SysTopology as SysTop
+import rteval.cpulist_utils as cpulist_utils
 
 class MeasurementProfile(RtEvalModules):
     """Keeps and controls all the measurement modules with the same measurement profile"""
@@ -184,10 +185,11 @@ measurement profiles, based on their characteristics"""
         run_on_isolcpus = self.__cfg.GetSection("measurement").run_on_isolcpus
         if cpulist:
             # Convert str to list and remove offline cpus
-            cpulist = CpuList(cpulist).cpulist
+            cpulist = cpulist_utils.expand_cpulist(cpulist)
+            cpulist = cpulist_utils.online_cpulist(cpulist)
         else:
             cpulist = SysTop().online_cpus() if run_on_isolcpus else SysTop().default_cpus()
-        rep_n.newProp("measurecpus", CpuList.collapse_cpulist(cpulist))
+        rep_n.newProp("measurecpus", cpulist_utils.collapse_cpulist(cpulist))
 
         for mp in self.__measureprofiles:
             mprep_n = mp.MakeReport()
diff --git a/rteval/modules/measurement/cyclictest.py b/rteval/modules/measurement/cyclictest.py
index 1b14e7e..dcfca0b 100644
--- a/rteval/modules/measurement/cyclictest.py
+++ b/rteval/modules/measurement/cyclictest.py
@@ -17,9 +17,9 @@ import libxml2
 from rteval.Log import Log
 from rteval.modules import rtevalModulePrototype
 from rteval.systopology import cpuinfo
-from rteval.systopology import CpuList, SysTopology
+from rteval.systopology import SysTopology
+import rteval.cpulist_utils as cpulist_utils
 
-expand_cpulist = CpuList.expand_cpulist
 
 class RunData:
     '''class to keep instance data from a cyclictest run'''
@@ -199,11 +199,11 @@ class Cyclictest(rtevalModulePrototype):
 
         if self.__cfg.cpulist:
             self.__cpulist = self.__cfg.cpulist
-            self.__cpus = expand_cpulist(self.__cpulist)
+            self.__cpus = cpulist_utils.expand_cpulist(self.__cpulist)
             # Only include online cpus
-            self.__cpus = CpuList(self.__cpus).cpulist
+            self.__cpus = cpulist_utils.online_cpulist(self.__cpus)
             # Reset cpulist from the newly calculated self.__cpus
-            self.__cpulist = CpuList.collapse_cpulist(self.__cpus)
+            self.__cpulist = cpulist_utils.collapse_cpulist(self.__cpus)
             self.__cpus = [str(c) for c in self.__cpus]
             self.__sparse = True
             if self.__run_on_isolcpus:
@@ -220,7 +220,7 @@ class Cyclictest(rtevalModulePrototype):
             self.__cpus = [c for c in self.__cpus if c in cpuset or self.__run_on_isolcpus and c in isolcpus]
             if self.__run_on_isolcpus:
                 self.__sparse = True
-                self.__cpulist = CpuList.collapse_cpulist([int(c) for c in self.__cpus])
+                self.__cpulist = cpulist_utils.collapse_cpulist([int(c) for c in self.__cpus])
 
         # Sort the list of cpus to align with the order reported by cyclictest
         self.__cpus.sort(key=int)
diff --git a/rteval/systopology.py b/rteval/systopology.py
index 60ac8e8..9db1a80 100644
--- a/rteval/systopology.py
+++ b/rteval/systopology.py
@@ -9,12 +9,8 @@
 import os
 import os.path
 import glob
-
-
-def sysread(path, obj):
-    """ Helper function for reading system files """
-    with open(os.path.join(path, obj), "r") as fp:
-        return fp.readline().strip()
+import rteval.cpulist_utils as cpulist_utils
+from rteval.cpulist_utils import sysread
 
 def cpuinfo():
     ''' return a dictionary of cpu keys with various cpu information '''
@@ -56,145 +52,6 @@ def cpuinfo():
     return info
 
 
-#
-# class to provide access to a list of cpus
-#
-
-class CpuList:
-    "Object that represents a group of system cpus"
-
-    cpupath = '/sys/devices/system/cpu'
-
-    def __init__(self, cpulist):
-        if isinstance(cpulist, list):
-            self.cpulist = cpulist
-        elif isinstance(cpulist, str):
-            self.cpulist = self.expand_cpulist(cpulist)
-        self.cpulist = self.online_cpulist(self.cpulist)
-        self.cpulist.sort()
-
-    def __str__(self):
-        return self.collapse_cpulist(self.cpulist)
-
-    def __contains__(self, cpu):
-        return cpu in self.cpulist
-
-    def __len__(self):
-        return len(self.cpulist)
-
-    def getcpulist(self):
-        """ return the list of cpus tracked """
-        return self.cpulist
-
-    @staticmethod
-    def online_file_exists():
-        """ Check whether machine / kernel is configured with online file """
-        # Note: some machines do not have cpu0/online so we check cpu1/online.
-        # In the case of machines with a single CPU, there is no cpu1, but
-        # that is not a problem, since a single CPU cannot be offline
-        return os.path.exists(os.path.join(CpuList.cpupath, "cpu1/online"))
-
-    @staticmethod
-    def isolated_file_exists():
-        """ Check whether machine / kernel is configured with isolated file """
-        return os.path.exists(os.path.join(CpuList.cpupath, "isolated"))
-
-    @staticmethod
-    def collapse_cpulist(cpulist):
-        """
-        Collapse a list of cpu numbers into a string range
-        of cpus (e.g. 0-5, 7, 9)
-        """
-        cur_range = [None, None]
-        result = []
-        for cpu in cpulist + [None]:
-            if cur_range[0] is None:
-                cur_range[0] = cur_range[1] = cpu
-                continue
-            if cpu is not None and cpu == cur_range[1] + 1:
-                # Extend currently processed range
-                cur_range[1] += 1
-            else:
-                # Range processing finished, add range to string
-                result.append(f"{cur_range[0]}-{cur_range[1]}"
-                              if cur_range[0] != cur_range[1]
-                              else str(cur_range[0]))
-                # Reset
-                cur_range[0] = cur_range[1] = cpu
-        return ",".join(result)
-
-    @staticmethod
-    def compress_cpulist(cpulist):
-        """ return a string representation of cpulist """
-        if not cpulist:
-            return ""
-        if isinstance(cpulist[0], int):
-            return ",".join(str(e) for e in cpulist)
-        return ",".join(cpulist)
-
-    @staticmethod
-    def expand_cpulist(cpulist):
-        """ expand a range string into an array of cpu numbers
-        don't error check against online cpus
-        """
-        result = []
-
-        if not cpulist:
-            return result
-
-        for part in cpulist.split(','):
-            if '-' in part:
-                a, b = part.split('-')
-                a, b = int(a), int(b)
-                result.extend(list(range(a, b + 1)))
-            else:
-                a = int(part)
-                result.append(a)
-        return [int(i) for i in list(set(result))]
-
-    @staticmethod
-    def is_online(n):
-        """ check whether cpu n is online """
-        path = os.path.join(CpuList.cpupath, f'cpu{n}')
-
-        # Some hardware doesn't allow cpu0 to be turned off
-        if not os.path.exists(path + '/online') and n == 0:
-            return True
-
-        return sysread(path, "online") == "1"
-
-    @staticmethod
-    def online_cpulist(cpulist):
-        """ Given a cpulist, return a cpulist of online cpus """
-        # This only works if the sys online files exist
-        if not CpuList.online_file_exists():
-            return cpulist
-        newlist = []
-        for cpu in cpulist:
-            if not CpuList.online_file_exists() and cpu == '0':
-                newlist.append(cpu)
-            elif CpuList.is_online(int(cpu)):
-                newlist.append(cpu)
-        return newlist
-
-    @staticmethod
-    def isolated_cpulist(cpulist):
-        """Given a cpulist, return a cpulist of isolated CPUs"""
-        if not CpuList.isolated_file_exists():
-            return cpulist
-        isolated_cpulist = sysread(CpuList.cpupath, "isolated")
-        isolated_cpulist = CpuList.expand_cpulist(isolated_cpulist)
-        return list(set(isolated_cpulist) & set(cpulist))
-
-    @staticmethod
-    def nonisolated_cpulist(cpulist):
-        """Given a cpulist, return a cpulist of non-isolated CPUs"""
-        if not CpuList.isolated_file_exists():
-            return cpulist
-        isolated_cpulist = sysread(CpuList.cpupath, "isolated")
-        isolated_cpulist = CpuList.expand_cpulist(isolated_cpulist)
-        return list(set(cpulist).difference(set(isolated_cpulist)))
-
 #
 # class to abstract access to NUMA nodes in /sys filesystem
 #
@@ -208,7 +65,8 @@ class NumaNode:
         """
         self.path = path
         self.nodeid = int(os.path.basename(path)[4:].strip())
-        self.cpus = CpuList(sysread(self.path, "cpulist"))
+        self.cpus = cpulist_utils.expand_cpulist(sysread(self.path, "cpulist"))
+        self.cpus = cpulist_utils.online_cpulist(self.cpus)
         self.getmeminfo()
 
     def __contains__(self, cpu):
@@ -240,11 +98,11 @@ class NumaNode:
 
     def getcpustr(self):
         """ return list of cpus for this node as a string """
-        return str(self.cpus)
+        return cpulist_utils.collapse_cpulist(self.cpus)
 
     def getcpulist(self):
         """ return list of cpus for this node """
-        return self.cpus.getcpulist()
+        return self.cpus
 
 class SimNumaNode(NumaNode):
     """class representing a simulated NUMA node.
@@ -257,7 +115,8 @@ class SimNumaNode(NumaNode):
 
     def __init__(self):
         self.nodeid = 0
-        self.cpus = CpuList(sysread(SimNumaNode.cpupath, "possible"))
+        self.cpus = cpulist_utils.expand_cpulist(sysread(SimNumaNode.cpupath, "possible"))
+        self.cpus = cpulist_utils.online_cpulist(self.cpus)
         self.getmeminfo()
 
     def getmeminfo(self):
@@ -339,7 +198,7 @@ class SysTopology:
         """ return a list of integers of all online cpus """
         cpulist = []
         for n in self.nodes:
-            cpulist += self.getcpus(n)
+            cpulist += cpulist_utils.online_cpulist(self.getcpus(n))
         cpulist.sort()
         return cpulist
 
@@ -347,7 +206,7 @@ class SysTopology:
         """ return a list of integers of all isolated cpus """
         cpulist = []
         for n in self.nodes:
-            cpulist += CpuList.isolated_cpulist(self.getcpus(n))
+            cpulist += cpulist_utils.isolated_cpulist(self.getcpus(n))
         cpulist.sort()
         return cpulist
 
@@ -355,7 +214,7 @@ class SysTopology:
         """ return a list of integers of all default schedulable cpus, i.e. online non-isolated cpus """
         cpulist = []
         for n in self.nodes:
-            cpulist += CpuList.nonisolated_cpulist(self.getcpus(n))
+            cpulist += cpulist_utils.nonisolated_cpulist(self.getcpus(n))
         cpulist.sort()
         return cpulist
 
@@ -403,7 +262,7 @@ if __name__ == "__main__":
 
         onlcpus = s.online_cpus()
         print(f'onlcpus = {onlcpus}')
-        onlcpus = CpuList.collapse_cpulist(onlcpus)
+        onlcpus = cpulist_utils.collapse_cpulist(onlcpus)
         print(f'onlcpus = {onlcpus}')
 
         onlcpus_str = s.online_cpus_str()
-- 
2.39.3

Re: [PATCH 3/4] rteval: Convert CpuList class to a module

[John Kacur]

On Wed, 29 Nov 2023, tglozar@redhat.com wrote:

> From: Tomas Glozar <tglozar@redhat.com>
> 
> Move out code from CpuList class in rteval.systopology into a separate
> module named rteval.cpulist_utils and avoid wrapping CPU lists in
> a CpuList object.
> 
> Almost all uses of CpuList in the code either use the static methods of
> the class or its constructor to filter online CPUs by running
> CpuList(...).cpulist. The only exception to this are NumaNode and
> SimNumaNode classes in systopology; these store a CpuList object,
> however their .getcpulist() method extracts the list out. Thus,
> the class is completely unnecessary.
> 
> Note: A better name for the module would be cpulist, consistent with the
> original name of the class, but this name is already used for variables
> throughout the code, hence cpulist_utils is used instead.

If you're removing the CpuList class then CpuList, or Cpulist is available
instead of cpulist_utils

I'm almost ready to sign-off on this, just a few comments
> 
> Signed-off-by: Tomas Glozar <tglozar@redhat.com>
> ---
>  rteval-cmd                               |  17 ++-
>  rteval/cpulist_utils.py                  | 128 ++++++++++++++++++
>  rteval/modules/loads/__init__.py         |   8 +-
>  rteval/modules/loads/hackbench.py        |   9 +-
>  rteval/modules/loads/kcompile.py         |  14 +-
>  rteval/modules/loads/stressng.py         |   9 +-
>  rteval/modules/measurement/__init__.py   |   8 +-
>  rteval/modules/measurement/cyclictest.py |  12 +-
>  rteval/systopology.py                    | 165 ++---------------------
>  9 files changed, 178 insertions(+), 192 deletions(-)
>  create mode 100644 rteval/cpulist_utils.py
> 
> diff --git a/rteval-cmd b/rteval-cmd
> index 7c41429..56b2c95 100755
> --- a/rteval-cmd
> +++ b/rteval-cmd
> @@ -30,11 +30,10 @@ from rteval import RtEval, rtevalConfig
>  from rteval.modules.loads import LoadModules
>  from rteval.modules.measurement import MeasurementModules
>  from rteval.version import RTEVAL_VERSION
> -from rteval.systopology import CpuList, SysTopology
> +from rteval.systopology import SysTopology
>  from rteval.modules.loads.kcompile import ModuleParameters
> +import rteval.cpulist_utils as cpulist_utils
>  
> -compress_cpulist = CpuList.compress_cpulist
> -expand_cpulist = CpuList.expand_cpulist
>  

Instead of removing these, you could change them to

compress_cpulist = cpulist_utils.compress_cpulist
expand_cpulist = cpulist_utils.expand_cpulist

and then you don't need to make all of the rest of 
the related changes in the file


>  def summarize(repfile, xslt):
>      """ Summarize an already existing XML report """
> @@ -209,9 +208,9 @@ def parse_options(cfg, parser, cmdargs):
>  
>  def remove_offline(cpulist):
>      """ return cpulist in collapsed compressed form with only online cpus """
> -    tmplist = expand_cpulist(cpulist)
> +    tmplist = cpulist_utils.expand_cpulist(cpulist)
>      tmplist = SysTopology().online_cpulist(tmplist)
> -    return CpuList.collapse_cpulist(tmplist)
> +    return cpulist_utils.collapse_cpulist(tmplist)
>  
>  
>  if __name__ == '__main__':
> @@ -343,14 +342,14 @@ if __name__ == '__main__':
>          # if we only specified one set of cpus (loads or measurement)
>          # default the other to the inverse of the specified list
>          if not ldcfg.cpulist and msrcfg.cpulist:
> -            tmplist = expand_cpulist(msrcfg.cpulist)
> +            tmplist = cpulist_utils.expand_cpulist(msrcfg.cpulist)
>              tmplist = SysTopology().invert_cpulist(tmplist)
> -            ldcfg.cpulist = compress_cpulist(tmplist)
> +            ldcfg.cpulist = cpulist_utils.compress_cpulist(tmplist)
>              msrcfg.cpulist = remove_offline(msrcfg.cpulist)
>          if not msrcfg.cpulist and ldcfg.cpulist:
> -            tmplist = expand_cpulist(ldcfg.cpulist)
> +            tmplist = cpulist_utils.expand_cpulist(ldcfg.cpulist)
>              tmplist = SysTopology().invert_cpulist(tmplist)
> -            msrcfg.cpulist = compress_cpulist(tmplist)
> +            msrcfg.cpulist = cpulist_utils.compress_cpulist(tmplist)
>              ldcfg.cpulist = remove_offline(ldcfg.cpulist)
>  
>          if ldcfg.cpulist:
> diff --git a/rteval/cpulist_utils.py b/rteval/cpulist_utils.py
> new file mode 100644
> index 0000000..402d579
> --- /dev/null
> +++ b/rteval/cpulist_utils.py
> @@ -0,0 +1,128 @@
> +# -*- coding: utf-8 -*-
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +#   Copyright 2016 - Clark Williams <williams@redhat.com>
> +#   Copyright 2021 - John Kacur <jkacur@redhat.com>
> +#   Copyright 2023 - Tomas Glozar <tglozar@redhat.com>
> +#
> +"""Module providing utility functions for working with CPU lists"""
> +
> +import os
> +
> +
> +cpupath = "/sys/devices/system/cpu"
> +
> +
> +def sysread(path, obj):
> +    """ Helper function for reading system files """
> +    with open(os.path.join(path, obj), "r") as fp:
> +        return fp.readline().strip()
> +
> +
> +def _online_file_exists():
> +    """ Check whether machine / kernel is configured with online file """
> +    # Note: some machines do not have cpu0/online so we check cpu1/online.
> +    # In the case of machines with a single CPU, there is no cpu1, but
> +    # that is not a problem, since a single CPU cannot be offline
> +    return os.path.exists(os.path.join(cpupath, "cpu1/online"))
> +
> +
> +def _isolated_file_exists():
> +    """ Check whether machine / kernel is configured with isolated file """
> +    return os.path.exists(os.path.join(cpupath, "isolated"))
> +
> +
> +def collapse_cpulist(cpulist):
> +    """
> +    Collapse a list of cpu numbers into a string range
> +    of cpus (e.g. 0-5, 7, 9)
> +    """
> +    cur_range = [None, None]
> +    result = []
> +    for cpu in cpulist + [None]:
> +        if cur_range[0] is None:
> +            cur_range[0] = cur_range[1] = cpu
> +            continue
> +        if cpu is not None and cpu == cur_range[1] + 1:
> +            # Extend currently processed range
> +            cur_range[1] += 1
> +        else:
> +            # Range processing finished, add range to string
> +            result.append(f"{cur_range[0]}-{cur_range[1]}"
> +                          if cur_range[0] != cur_range[1]
> +                          else str(cur_range[0]))
> +            # Reset
> +            cur_range[0] = cur_range[1] = cpu
> +    return ",".join(result)
> +
> +
> +def compress_cpulist(cpulist):
> +    """ return a string representation of cpulist """
> +    if not cpulist:
> +        return ""
> +    if isinstance(cpulist[0], int):
> +        return ",".join(str(e) for e in cpulist)
> +    return ",".join(cpulist)
> +
> +
> +def expand_cpulist(cpulist):
> +    """ expand a range string into an array of cpu numbers
> +    don't error check against online cpus
> +    """
> +    result = []
> +
> +    if not cpulist:
> +        return result
> +
> +    for part in cpulist.split(','):
> +        if '-' in part:
> +            a, b = part.split('-')
> +            a, b = int(a), int(b)
> +            result.extend(list(range(a, b + 1)))
> +        else:
> +            a = int(part)
> +            result.append(a)
> +    return [int(i) for i in list(set(result))]
> +
> +
> +def is_online(n):
> +    """ check whether cpu n is online """
> +    path = os.path.join(cpupath, f'cpu{n}')
> +
> +    # Some hardware doesn't allow cpu0 to be turned off
> +    if not os.path.exists(path + '/online') and n == 0:
> +        return True
> +
> +    return sysread(path, "online") == "1"
> +
> +
> +def online_cpulist(cpulist):
> +    """ Given a cpulist, return a cpulist of online cpus """
> +    # This only works if the sys online files exist
> +    if not _online_file_exists():
> +        return cpulist
> +    newlist = []
> +    for cpu in cpulist:
> +        if not _online_file_exists() and cpu == '0':
> +            newlist.append(cpu)
> +        elif is_online(int(cpu)):
> +            newlist.append(cpu)
> +    return newlist
> +
> +
> +def isolated_cpulist(cpulist):
> +    """Given a cpulist, return a cpulist of isolated CPUs"""
> +    if not _isolated_file_exists():
> +        return cpulist
> +    isolated_cpulist = sysread(cpupath, "isolated")
> +    isolated_cpulist = expand_cpulist(isolated_cpulist)
> +    return list(set(isolated_cpulist) & set(cpulist))
> +
> +
> +def nonisolated_cpulist(cpulist):
> +    """Given a cpulist, return a cpulist of non-isolated CPUs"""
> +    if not _isolated_file_exists():
> +        return cpulist
> +    isolated_cpulist = sysread(cpupath, "isolated")
> +    isolated_cpulist = expand_cpulist(isolated_cpulist)
> +    return list(set(cpulist).difference(set(isolated_cpulist)))


You don't have to do it in this patch, but some builtin testing would be 
nice under this
if __name__ == "__main__":

> diff --git a/rteval/modules/loads/__init__.py b/rteval/modules/loads/__init__.py
> index 13fba1e..0845742 100644
> --- a/rteval/modules/loads/__init__.py
> +++ b/rteval/modules/loads/__init__.py
> @@ -11,7 +11,8 @@ import libxml2
>  from rteval.Log import Log
>  from rteval.rtevalConfig import rtevalCfgSection
>  from rteval.modules import RtEvalModules, rtevalModulePrototype
> -from rteval.systopology import CpuList, SysTopology as SysTop
> +from rteval.systopology import SysTopology as SysTop
> +import rteval.cpulist_utils as cpulist_utils
>  
>  class LoadThread(rtevalModulePrototype):
>      def __init__(self, name, config, logger=None):
> @@ -117,10 +118,11 @@ class LoadModules(RtEvalModules):
>          cpulist = self._cfg.GetSection(self._module_config).cpulist
>          if cpulist:
>              # Convert str to list and remove offline cpus
> -            cpulist = CpuList(cpulist).cpulist
> +            cpulist = cpulist_utils.expand_cpulist(cpulist)
> +            cpulist = cpulist_utils.online_cpulist(cpulist)
>          else:
>              cpulist = SysTop().default_cpus()
> -        rep_n.newProp("loadcpus", CpuList.collapse_cpulist(cpulist))
> +        rep_n.newProp("loadcpus", cpulist_utils.collapse_cpulist(cpulist))
>  
>          return rep_n
>  
> diff --git a/rteval/modules/loads/hackbench.py b/rteval/modules/loads/hackbench.py
> index cfc7063..8df2d0a 100644
> --- a/rteval/modules/loads/hackbench.py
> +++ b/rteval/modules/loads/hackbench.py
> @@ -16,10 +16,9 @@ import errno
>  from signal import SIGKILL
>  from rteval.modules.loads import CommandLineLoad
>  from rteval.Log import Log
> -from rteval.systopology import CpuList, SysTopology
> +from rteval.systopology import SysTopology
> +import rteval.cpulist_utils as cpulist_utils
>  
> -expand_cpulist = CpuList.expand_cpulist
> -isolated_cpulist = CpuList.isolated_cpulist
>  
>  class Hackbench(CommandLineLoad):
>      def __init__(self, config, logger):
> @@ -58,10 +57,10 @@ class Hackbench(CommandLineLoad):
>              self.cpus[n] = sysTop.getcpus(int(n))
>              # if a cpulist was specified, only allow cpus in that list on the node
>              if self.cpulist:
> -                self.cpus[n] = [c for c in self.cpus[n] if c in expand_cpulist(self.cpulist)]
> +                self.cpus[n] = [c for c in self.cpus[n] if c in cpulist_utils.expand_cpulist(self.cpulist)]
>              # if a cpulist was not specified, exclude isolated cpus
>              else:
> -                self.cpus[n] = CpuList.nonisolated_cpulist(self.cpus[n])
> +                self.cpus[n] = cpulist_utils.nonisolated_cpulist(self.cpus[n])
>  
>              # track largest number of cpus used on a node
>              node_biggest = len(self.cpus[n])
> diff --git a/rteval/modules/loads/kcompile.py b/rteval/modules/loads/kcompile.py
> index 0d02577..93605e1 100644
> --- a/rteval/modules/loads/kcompile.py
> +++ b/rteval/modules/loads/kcompile.py
> @@ -14,11 +14,9 @@ import subprocess
>  from rteval.modules import rtevalRuntimeError
>  from rteval.modules.loads import CommandLineLoad
>  from rteval.Log import Log
> -from rteval.systopology import CpuList, SysTopology
> +from rteval.systopology import SysTopology
> +import rteval.cpulist_utils as cpulist_utils
>  
> -expand_cpulist = CpuList.expand_cpulist
> -compress_cpulist = CpuList.compress_cpulist
> -nonisolated_cpulist = CpuList.nonisolated_cpulist
>  
>  DEFAULT_KERNEL_PREFIX = "linux-6.6"
>  
> @@ -38,7 +36,7 @@ class KBuildJob:
>              os.mkdir(self.objdir)
>  
>          # Exclude isolated CPUs if cpulist not set
> -        cpus_available = len(nonisolated_cpulist(self.node.cpus.cpulist))
> +        cpus_available = len(cpulist_utils.nonisolated_cpulist(self.node.cpus))
>  
>          if os.path.exists('/usr/bin/numactl') and not cpulist:
>              # Use numactl
> @@ -47,7 +45,7 @@ class KBuildJob:
>          elif cpulist:
>              # Use taskset
>              self.jobs = self.calc_jobs_per_cpu() * len(cpulist)
> -            self.binder = f'taskset -c {compress_cpulist(cpulist)}'
> +            self.binder = f'taskset -c {cpulist_utils.compress_cpulist(cpulist)}'
>          else:
>              # Without numactl calculate number of jobs from the node
>              self.jobs = self.calc_jobs_per_cpu() * cpus_available
> @@ -220,7 +218,7 @@ class Kcompile(CommandLineLoad):
>  
>              # if a cpulist was specified, only allow cpus in that list on the node
>              if self.cpulist:
> -                self.cpus[n] = [c for c in self.cpus[n] if c in expand_cpulist(self.cpulist)]
> +                self.cpus[n] = [c for c in self.cpus[n] if c in cpulist_utils.expand_cpulist(self.cpulist)]
>  
>          # remove nodes with no cpus available for running
>          for node, cpus in self.cpus.items():
> @@ -282,7 +280,7 @@ class Kcompile(CommandLineLoad):
>  
>          if 'cpulist' in self._cfg and self._cfg.cpulist:
>              cpulist = self._cfg.cpulist
> -            self.num_cpus = len(expand_cpulist(cpulist))
> +            self.num_cpus = len(cpulist_utils.expand_cpulist(cpulist))
>          else:
>              cpulist = ""
>  
> diff --git a/rteval/modules/loads/stressng.py b/rteval/modules/loads/stressng.py
> index 7c9e63f..524604e 100644
> --- a/rteval/modules/loads/stressng.py
> +++ b/rteval/modules/loads/stressng.py
> @@ -7,10 +7,9 @@ import subprocess
>  import signal
>  from rteval.modules.loads import CommandLineLoad
>  from rteval.Log import Log
> -from rteval.systopology import CpuList, SysTopology
> +from rteval.systopology import SysTopology
> +import rteval.cpulist_utils as cpulist_utils
>  
> -expand_cpulist = CpuList.expand_cpulist
> -nonisolated_cpulist = CpuList.nonisolated_cpulist
>  
>  class Stressng(CommandLineLoad):
>      " This class creates a load module that runs stress-ng "
> @@ -70,10 +69,10 @@ class Stressng(CommandLineLoad):
>              cpus[n] = systop.getcpus(int(n))
>              # if a cpulist was specified, only allow cpus in that list on the node
>              if self.cpulist:
> -                cpus[n] = [c for c in cpus[n] if c in expand_cpulist(self.cpulist)]
> +                cpus[n] = [c for c in cpus[n] if c in cpulist_utils.expand_cpulist(self.cpulist)]
>              # if a cpulist was not specified, exclude isolated cpus
>              else:
> -                cpus[n] = CpuList.nonisolated_cpulist(cpus[n])
> +                cpus[n] = cpulist_utils.nonisolated_cpulist(cpus[n])
>  
>  
>          # remove nodes with no cpus available for running
> diff --git a/rteval/modules/measurement/__init__.py b/rteval/modules/measurement/__init__.py
> index 41b8022..66dc9c5 100644
> --- a/rteval/modules/measurement/__init__.py
> +++ b/rteval/modules/measurement/__init__.py
> @@ -5,7 +5,8 @@
>  
>  import libxml2
>  from rteval.modules import RtEvalModules, ModuleContainer
> -from rteval.systopology import CpuList, SysTopology as SysTop
> +from rteval.systopology import SysTopology as SysTop
> +import rteval.cpulist_utils as cpulist_utils
>  
>  class MeasurementProfile(RtEvalModules):
>      """Keeps and controls all the measurement modules with the same measurement profile"""
> @@ -184,10 +185,11 @@ measurement profiles, based on their characteristics"""
>          run_on_isolcpus = self.__cfg.GetSection("measurement").run_on_isolcpus
>          if cpulist:
>              # Convert str to list and remove offline cpus
> -            cpulist = CpuList(cpulist).cpulist
> +            cpulist = cpulist_utils.expand_cpulist(cpulist)
> +            cpulist = cpulist_utils.online_cpulist(cpulist)
>          else:
>              cpulist = SysTop().online_cpus() if run_on_isolcpus else SysTop().default_cpus()
> -        rep_n.newProp("measurecpus", CpuList.collapse_cpulist(cpulist))
> +        rep_n.newProp("measurecpus", cpulist_utils.collapse_cpulist(cpulist))
>  
>          for mp in self.__measureprofiles:
>              mprep_n = mp.MakeReport()
> diff --git a/rteval/modules/measurement/cyclictest.py b/rteval/modules/measurement/cyclictest.py
> index 1b14e7e..dcfca0b 100644
> --- a/rteval/modules/measurement/cyclictest.py
> +++ b/rteval/modules/measurement/cyclictest.py
> @@ -17,9 +17,9 @@ import libxml2
>  from rteval.Log import Log
>  from rteval.modules import rtevalModulePrototype
>  from rteval.systopology import cpuinfo
> -from rteval.systopology import CpuList, SysTopology
> +from rteval.systopology import SysTopology
> +import rteval.cpulist_utils as cpulist_utils
>  
> -expand_cpulist = CpuList.expand_cpulist
>  
>  class RunData:
>      '''class to keep instance data from a cyclictest run'''
> @@ -199,11 +199,11 @@ class Cyclictest(rtevalModulePrototype):
>  
>          if self.__cfg.cpulist:
>              self.__cpulist = self.__cfg.cpulist
> -            self.__cpus = expand_cpulist(self.__cpulist)
> +            self.__cpus = cpulist_utils.expand_cpulist(self.__cpulist)
>              # Only include online cpus
> -            self.__cpus = CpuList(self.__cpus).cpulist
> +            self.__cpus = cpulist_utils.online_cpulist(self.__cpus)
>              # Reset cpulist from the newly calculated self.__cpus
> -            self.__cpulist = CpuList.collapse_cpulist(self.__cpus)
> +            self.__cpulist = cpulist_utils.collapse_cpulist(self.__cpus)
>              self.__cpus = [str(c) for c in self.__cpus]
>              self.__sparse = True
>              if self.__run_on_isolcpus:
> @@ -220,7 +220,7 @@ class Cyclictest(rtevalModulePrototype):
>              self.__cpus = [c for c in self.__cpus if c in cpuset or self.__run_on_isolcpus and c in isolcpus]
>              if self.__run_on_isolcpus:
>                  self.__sparse = True
> -                self.__cpulist = CpuList.collapse_cpulist([int(c) for c in self.__cpus])
> +                self.__cpulist = cpulist_utils.collapse_cpulist([int(c) for c in self.__cpus])
>  
>          # Sort the list of cpus to align with the order reported by cyclictest
>          self.__cpus.sort(key=int)
> diff --git a/rteval/systopology.py b/rteval/systopology.py
> index 60ac8e8..9db1a80 100644
> --- a/rteval/systopology.py
> +++ b/rteval/systopology.py
> @@ -9,12 +9,8 @@
>  import os
>  import os.path
>  import glob
> -
> -
> -def sysread(path, obj):
> -    """ Helper function for reading system files """
> -    with open(os.path.join(path, obj), "r") as fp:
> -        return fp.readline().strip()
> +import rteval.cpulist_utils as cpulist_utils
> +from rteval.cpulist_utils import sysread
>  
>  def cpuinfo():
>      ''' return a dictionary of cpu keys with various cpu information '''
> @@ -56,145 +52,6 @@ def cpuinfo():
>      return info
>  
>  
> -#
> -# class to provide access to a list of cpus
> -#
> -
> -class CpuList:
> -    "Object that represents a group of system cpus"
> -
> -    cpupath = '/sys/devices/system/cpu'
> -
> -    def __init__(self, cpulist):
> -        if isinstance(cpulist, list):
> -            self.cpulist = cpulist
> -        elif isinstance(cpulist, str):
> -            self.cpulist = self.expand_cpulist(cpulist)
> -        self.cpulist = self.online_cpulist(self.cpulist)
> -        self.cpulist.sort()
> -
> -    def __str__(self):
> -        return self.collapse_cpulist(self.cpulist)
> -
> -    def __contains__(self, cpu):
> -        return cpu in self.cpulist
> -
> -    def __len__(self):
> -        return len(self.cpulist)
> -
> -    def getcpulist(self):
> -        """ return the list of cpus tracked """
> -        return self.cpulist
> -
> -    @staticmethod
> -    def online_file_exists():
> -        """ Check whether machine / kernel is configured with online file """
> -        # Note: some machines do not have cpu0/online so we check cpu1/online.
> -        # In the case of machines with a single CPU, there is no cpu1, but
> -        # that is not a problem, since a single CPU cannot be offline
> -        return os.path.exists(os.path.join(CpuList.cpupath, "cpu1/online"))
> -
> -    @staticmethod
> -    def isolated_file_exists():
> -        """ Check whether machine / kernel is configured with isolated file """
> -        return os.path.exists(os.path.join(CpuList.cpupath, "isolated"))
> -
> -    @staticmethod
> -    def collapse_cpulist(cpulist):
> -        """
> -        Collapse a list of cpu numbers into a string range
> -        of cpus (e.g. 0-5, 7, 9)
> -        """
> -        cur_range = [None, None]
> -        result = []
> -        for cpu in cpulist + [None]:
> -            if cur_range[0] is None:
> -                cur_range[0] = cur_range[1] = cpu
> -                continue
> -            if cpu is not None and cpu == cur_range[1] + 1:
> -                # Extend currently processed range
> -                cur_range[1] += 1
> -            else:
> -                # Range processing finished, add range to string
> -                result.append(f"{cur_range[0]}-{cur_range[1]}"
> -                              if cur_range[0] != cur_range[1]
> -                              else str(cur_range[0]))
> -                # Reset
> -                cur_range[0] = cur_range[1] = cpu
> -        return ",".join(result)
> -
> -    @staticmethod
> -    def compress_cpulist(cpulist):
> -        """ return a string representation of cpulist """
> -        if not cpulist:
> -            return ""
> -        if isinstance(cpulist[0], int):
> -            return ",".join(str(e) for e in cpulist)
> -        return ",".join(cpulist)
> -
> -    @staticmethod
> -    def expand_cpulist(cpulist):
> -        """ expand a range string into an array of cpu numbers
> -        don't error check against online cpus
> -        """
> -        result = []
> -
> -        if not cpulist:
> -            return result
> -
> -        for part in cpulist.split(','):
> -            if '-' in part:
> -                a, b = part.split('-')
> -                a, b = int(a), int(b)
> -                result.extend(list(range(a, b + 1)))
> -            else:
> -                a = int(part)
> -                result.append(a)
> -        return [int(i) for i in list(set(result))]
> -
> -    @staticmethod
> -    def is_online(n):
> -        """ check whether cpu n is online """
> -        path = os.path.join(CpuList.cpupath, f'cpu{n}')
> -
> -        # Some hardware doesn't allow cpu0 to be turned off
> -        if not os.path.exists(path + '/online') and n == 0:
> -            return True
> -
> -        return sysread(path, "online") == "1"
> -
> -    @staticmethod
> -    def online_cpulist(cpulist):
> -        """ Given a cpulist, return a cpulist of online cpus """
> -        # This only works if the sys online files exist
> -        if not CpuList.online_file_exists():
> -            return cpulist
> -        newlist = []
> -        for cpu in cpulist:
> -            if not CpuList.online_file_exists() and cpu == '0':
> -                newlist.append(cpu)
> -            elif CpuList.is_online(int(cpu)):
> -                newlist.append(cpu)
> -        return newlist
> -
> -    @staticmethod
> -    def isolated_cpulist(cpulist):
> -        """Given a cpulist, return a cpulist of isolated CPUs"""
> -        if not CpuList.isolated_file_exists():
> -            return cpulist
> -        isolated_cpulist = sysread(CpuList.cpupath, "isolated")
> -        isolated_cpulist = CpuList.expand_cpulist(isolated_cpulist)
> -        return list(set(isolated_cpulist) & set(cpulist))
> -
> -    @staticmethod
> -    def nonisolated_cpulist(cpulist):
> -        """Given a cpulist, return a cpulist of non-isolated CPUs"""
> -        if not CpuList.isolated_file_exists():
> -            return cpulist
> -        isolated_cpulist = sysread(CpuList.cpupath, "isolated")
> -        isolated_cpulist = CpuList.expand_cpulist(isolated_cpulist)
> -        return list(set(cpulist).difference(set(isolated_cpulist)))
> -
>  #
>  # class to abstract access to NUMA nodes in /sys filesystem
>  #
> @@ -208,7 +65,8 @@ class NumaNode:
>          """
>          self.path = path
>          self.nodeid = int(os.path.basename(path)[4:].strip())
> -        self.cpus = CpuList(sysread(self.path, "cpulist"))
> +        self.cpus = cpulist_utils.expand_cpulist(sysread(self.path, "cpulist"))
> +        self.cpus = cpulist_utils.online_cpulist(self.cpus)
>          self.getmeminfo()
>  
>      def __contains__(self, cpu):
> @@ -240,11 +98,11 @@ class NumaNode:
>  
>      def getcpustr(self):
>          """ return list of cpus for this node as a string """
> -        return str(self.cpus)
> +        return cpulist_utils.collapse_cpulist(self.cpus)
>  
>      def getcpulist(self):
>          """ return list of cpus for this node """
> -        return self.cpus.getcpulist()
> +        return self.cpus
>  
>  class SimNumaNode(NumaNode):
>      """class representing a simulated NUMA node.
> @@ -257,7 +115,8 @@ class SimNumaNode(NumaNode):
>  
>      def __init__(self):
>          self.nodeid = 0
> -        self.cpus = CpuList(sysread(SimNumaNode.cpupath, "possible"))
> +        self.cpus = cpulist_utils.expand_cpulist(sysread(SimNumaNode.cpupath, "possible"))
> +        self.cpus = cpulist_utils.online_cpulist(self.cpus)
>          self.getmeminfo()
>  
>      def getmeminfo(self):
> @@ -339,7 +198,7 @@ class SysTopology:
>          """ return a list of integers of all online cpus """
>          cpulist = []
>          for n in self.nodes:
> -            cpulist += self.getcpus(n)
> +            cpulist += cpulist_utils.online_cpulist(self.getcpus(n))
>          cpulist.sort()
>          return cpulist
>  
> @@ -347,7 +206,7 @@ class SysTopology:
>          """ return a list of integers of all isolated cpus """
>          cpulist = []
>          for n in self.nodes:
> -            cpulist += CpuList.isolated_cpulist(self.getcpus(n))
> +            cpulist += cpulist_utils.isolated_cpulist(self.getcpus(n))
>          cpulist.sort()
>          return cpulist
>  
> @@ -355,7 +214,7 @@ class SysTopology:
>          """ return a list of integers of all default schedulable cpus, i.e. online non-isolated cpus """
>          cpulist = []
>          for n in self.nodes:
> -            cpulist += CpuList.nonisolated_cpulist(self.getcpus(n))
> +            cpulist += cpulist_utils.nonisolated_cpulist(self.getcpus(n))
>          cpulist.sort()
>          return cpulist
>  
> @@ -403,7 +262,7 @@ if __name__ == "__main__":
>  
>          onlcpus = s.online_cpus()
>          print(f'onlcpus = {onlcpus}')
> -        onlcpus = CpuList.collapse_cpulist(onlcpus)
> +        onlcpus = cpulist_utils.collapse_cpulist(onlcpus)
>          print(f'onlcpus = {onlcpus}')
>  
>          onlcpus_str = s.online_cpus_str()
> -- 
> 2.39.3
> 
> 
> 

Re: [PATCH 3/4] rteval: Convert CpuList class to a module

[Tomas Glozar]

út 12. 12. 2023 v 22:46 odesílatel John Kacur <jkacur@redhat.com> napsal:
>
> If you're removing the CpuList class then CpuList, or Cpulist is available
> instead of cpulist_utils
>

cpulist_utils is a module, unlike CpuList which is a class; these have
different spelling conventions for case, see
https://peps.python.org/pep-0008/#package-and-module-names.

>
> Instead of removing these, you could change them to
>
> compress_cpulist = cpulist_utils.compress_cpulist
> expand_cpulist = cpulist_utils.expand_cpulist
>
> and then you don't need to make all of the rest of
> the related changes in the file
>

Or I could directly import them like this:
from rteval.cpulist_utils import compress_cpulist, expand_cpulist

The reason why I did the change like this was that I wanted the usage
of cpulist_utils functions to be consistent throughout the entire
rteval source code. In systopology.py and other files, CpuList.* is
used directly (which became cpulist_utils.* in this patch).

Tomas

Re: [PATCH 3/4] rteval: Convert CpuList class to a module

[Tomas Glozar]

út 12. 12. 2023 v 22:46 odesílatel John Kacur <jkacur@redhat.com> napsal:
>
> You don't have to do it in this patch, but some builtin testing would be
> nice under this
> if __name__ == "__main__":
>

Yeah that's definitely a good idea, moving the cpulist logic into a
separate module will allow for more elegant unit tests directly in the
file.

In the future, we might also consider moving tests into separate files
using some kind of unit test library like pytest [1] or the builtin
unit test [2] module. If I understand it correctly, currently you have
to run tests manually for each module that has them, and you also have
to check the results manually; changing that to a more systematic and
automated approach should make development easier and prevent bugs.

[1] - https://docs.pytest.org/en/7.4.x/
[2] - https://docs.python.org/3/library/unittest.html

Tomas

Re: [PATCH 3/4] rteval: Convert CpuList class to a module

[John Kacur]

[-- Attachment #1: Type: text/plain, Size: 1078 bytes --]



On Thu, 14 Dec 2023, Tomas Glozar wrote:

> út 12. 12. 2023 v 22:46 odesílatel John Kacur <jkacur@redhat.com> napsal:
> >
> > You don't have to do it in this patch, but some builtin testing would be
> > nice under this
> > if __name__ == "__main__":
> >
> 
> Yeah that's definitely a good idea, moving the cpulist logic into a
> separate module will allow for more elegant unit tests directly in the
> file.
> 
> In the future, we might also consider moving tests into separate files
> using some kind of unit test library like pytest [1] or the builtin
> unit test [2] module. If I understand it correctly, currently you have
> to run tests manually for each module that has them, and you also have
> to check the results manually; changing that to a more systematic and
> automated approach should make development easier and prevent bugs.
> 
> [1] - https://docs.pytest.org/en/7.4.x/
> [2] - https://docs.python.org/3/library/unittest.html
> 
> Tomas

Indeed, that would be welcome, but I still wouldn't remove the 
quick and dirty tests that are already available.

John

[PATCH 4/4] rteval: Add relative cpulists for measurements

[tglozar]

From: Tomas Glozar <tglozar@redhat.com>

Instead of specifying an absolute list of CPUs to run measurements on
in --measurement-cpulist, implement an option to specify a relative list
with respect to the current cpuset of rteval.

The relative cpulist can include CPUs both for addition and for removal,
e.g. +0,1,-7,8.

Also move the logic for processing cpulists specified by the user as
a string into cpulists usable by rteval to a single function.

Signed-off-by: Tomas Glozar <tglozar@redhat.com>
---
 rteval-cmd                               | 26 +++++++-----
 rteval/cpulist_utils.py                  | 33 ++++++++++++++++
 rteval/modules/measurement/__init__.py   |  9 +----
 rteval/modules/measurement/cyclictest.py | 50 +++---------------------
 rteval/systopology.py                    | 33 ++++++++++++++++
 5 files changed, 90 insertions(+), 61 deletions(-)

diff --git a/rteval-cmd b/rteval-cmd
index 56b2c95..dd7d645 100755
--- a/rteval-cmd
+++ b/rteval-cmd
@@ -30,7 +30,7 @@ from rteval import RtEval, rtevalConfig
 from rteval.modules.loads import LoadModules
 from rteval.modules.measurement import MeasurementModules
 from rteval.version import RTEVAL_VERSION
-from rteval.systopology import SysTopology
+from rteval.systopology import SysTopology, parse_cpulist_from_config
 from rteval.modules.loads.kcompile import ModuleParameters
 import rteval.cpulist_utils as cpulist_utils
 
@@ -336,26 +336,32 @@ if __name__ == '__main__':
 
         ldcfg = config.GetSection('loads')
         msrcfg = config.GetSection('measurement')
-        if ldcfg.cpulist and msrcfg.cpulist:
+        msrcfg_cpulist_present = msrcfg.cpulist != ""
+        # Parse measurement cpulist using parse_cpulist_from_config to account for run-on-isolcpus
+        # and relative cpusets
+        cpulist = parse_cpulist_from_config(msrcfg.cpulist, msrcfg.run_on_isolcpus)
+        if msrcfg_cpulist_present and not cpulist_utils.is_relative(msrcfg.cpulist) and msrcfg.run_on_isolcpus:
+            logger.log(Log.WARN, "ignoring --measurement-run-on-isolcpus, since cpulist is specified")
+        msrcfg.cpulist = cpulist_utils.collapse_cpulist(cpulist)
+        if ldcfg.cpulist:
             ldcfg.cpulist = remove_offline(ldcfg.cpulist)
-            msrcfg.cpulist = remove_offline(msrcfg.cpulist)
         # if we only specified one set of cpus (loads or measurement)
         # default the other to the inverse of the specified list
-        if not ldcfg.cpulist and msrcfg.cpulist:
+        if not ldcfg.cpulist and msrcfg_cpulist_present:
             tmplist = cpulist_utils.expand_cpulist(msrcfg.cpulist)
             tmplist = SysTopology().invert_cpulist(tmplist)
-            ldcfg.cpulist = cpulist_utils.compress_cpulist(tmplist)
-            msrcfg.cpulist = remove_offline(msrcfg.cpulist)
-        if not msrcfg.cpulist and ldcfg.cpulist:
+            tmplist = cpulist_utils.online_cpulist(tmplist)
+            ldcfg.cpulist = cpulist_utils.collapse_cpulist(tmplist)
+        if not msrcfg_cpulist_present and ldcfg.cpulist:
             tmplist = cpulist_utils.expand_cpulist(ldcfg.cpulist)
             tmplist = SysTopology().invert_cpulist(tmplist)
-            msrcfg.cpulist = cpulist_utils.compress_cpulist(tmplist)
-            ldcfg.cpulist = remove_offline(ldcfg.cpulist)
+            tmplist = cpulist_utils.online_cpulist(tmplist)
+            msrcfg.cpulist = cpulist_utils.collapse_cpulist(tmplist)
 
         if ldcfg.cpulist:
             logger.log(Log.DEBUG, f"loads cpulist: {ldcfg.cpulist}")
         # if --onlyload is specified msrcfg.cpulist is unused
-        if msrcfg.cpulist and not rtevcfg.onlyload:
+        if msrcfg_cpulist_present and not rtevcfg.onlyload:
             logger.log(Log.DEBUG, f"measurement cpulist: {msrcfg.cpulist}")
         logger.log(Log.DEBUG, f"workdir: {rtevcfg.workdir}")
 
diff --git a/rteval/cpulist_utils.py b/rteval/cpulist_utils.py
index 402d579..7abc45a 100644
--- a/rteval/cpulist_utils.py
+++ b/rteval/cpulist_utils.py
@@ -126,3 +126,36 @@ def nonisolated_cpulist(cpulist):
     isolated_cpulist = sysread(cpupath, "isolated")
     isolated_cpulist = expand_cpulist(isolated_cpulist)
     return list(set(cpulist).difference(set(isolated_cpulist)))
+
+
+def is_relative(cpulist):
+    return cpulist.startswith("+") or cpulist.startswith("-")
+
+
+def expand_relative_cpulist(cpulist):
+    """
+    Expand a relative cpulist into a tuple of lists.
+    :param cpulist: Relative cpulist of form +1,2,3,-4,5,6
+    :return: Tuple of two lists, one for added CPUs, one for removed CPUs
+    """
+    added_cpus = []
+    removed_cpus = []
+
+    if not cpulist:
+        return added_cpus, removed_cpus
+
+    cpus = None
+
+    for part in cpulist.split(','):
+        if part.startswith('+') or part.startswith('-'):
+            cpus = added_cpus if part[0] == '+' else removed_cpus
+            part = part[1:]
+        if '-' in part:
+            a, b = part.split('-')
+            a, b = int(a), int(b)
+            cpus.extend(list(range(a, b + 1)))
+        else:
+            a = int(part)
+            cpus.append(a)
+
+    return list(set(added_cpus)), list(set(removed_cpus))
diff --git a/rteval/modules/measurement/__init__.py b/rteval/modules/measurement/__init__.py
index 66dc9c5..11bd7b0 100644
--- a/rteval/modules/measurement/__init__.py
+++ b/rteval/modules/measurement/__init__.py
@@ -5,7 +5,7 @@
 
 import libxml2
 from rteval.modules import RtEvalModules, ModuleContainer
-from rteval.systopology import SysTopology as SysTop
+from rteval.systopology import parse_cpulist_from_config
 import rteval.cpulist_utils as cpulist_utils
 
 class MeasurementProfile(RtEvalModules):
@@ -183,12 +183,7 @@ measurement profiles, based on their characteristics"""
         rep_n = libxml2.newNode("Measurements")
         cpulist = self.__cfg.GetSection("measurement").cpulist
         run_on_isolcpus = self.__cfg.GetSection("measurement").run_on_isolcpus
-        if cpulist:
-            # Convert str to list and remove offline cpus
-            cpulist = cpulist_utils.expand_cpulist(cpulist)
-            cpulist = cpulist_utils.online_cpulist(cpulist)
-        else:
-            cpulist = SysTop().online_cpus() if run_on_isolcpus else SysTop().default_cpus()
+        cpulist = parse_cpulist_from_config(cpulist, run_on_isolcpus)
         rep_n.newProp("measurecpus", cpulist_utils.collapse_cpulist(cpulist))
 
         for mp in self.__measureprofiles:
diff --git a/rteval/modules/measurement/cyclictest.py b/rteval/modules/measurement/cyclictest.py
index dcfca0b..87fa9d8 100644
--- a/rteval/modules/measurement/cyclictest.py
+++ b/rteval/modules/measurement/cyclictest.py
@@ -16,8 +16,7 @@ import math
 import libxml2
 from rteval.Log import Log
 from rteval.modules import rtevalModulePrototype
-from rteval.systopology import cpuinfo
-from rteval.systopology import SysTopology
+from rteval.systopology import cpuinfo, parse_cpulist_from_config
 import rteval.cpulist_utils as cpulist_utils
 
 
@@ -192,39 +191,9 @@ class Cyclictest(rtevalModulePrototype):
         self.__priority = int(self.__cfg.setdefault('priority', 95))
         self.__buckets = int(self.__cfg.setdefault('buckets', 2000))
         self.__numcores = 0
-        self.__cpus = []
         self.__cyclicdata = {}
-        self.__sparse = False
-        self.__run_on_isolcpus = bool(self.__cfg.setdefault('run-on-isolcpus', False))
-
-        if self.__cfg.cpulist:
-            self.__cpulist = self.__cfg.cpulist
-            self.__cpus = cpulist_utils.expand_cpulist(self.__cpulist)
-            # Only include online cpus
-            self.__cpus = cpulist_utils.online_cpulist(self.__cpus)
-            # Reset cpulist from the newly calculated self.__cpus
-            self.__cpulist = cpulist_utils.collapse_cpulist(self.__cpus)
-            self.__cpus = [str(c) for c in self.__cpus]
-            self.__sparse = True
-            if self.__run_on_isolcpus:
-                self._log(Log.WARN, "ignoring --measurement-run-on-isolcpus, since cpulist is specified")
-        else:
-            self.__cpus = SysTopology().online_cpus_str()
-            # Get the cpuset from the environment
-            cpuset = os.sched_getaffinity(0)
-            # Convert the elements to strings
-            cpuset = [str(c) for c in cpuset]
-            # Get isolated CPU list
-            isolcpus = [str(c) for c in SysTopology().isolated_cpus()]
-            # Only include cpus that are in the cpuset and isolated CPUs if run_on_isolcpus is enabled
-            self.__cpus = [c for c in self.__cpus if c in cpuset or self.__run_on_isolcpus and c in isolcpus]
-            if self.__run_on_isolcpus:
-                self.__sparse = True
-                self.__cpulist = cpulist_utils.collapse_cpulist([int(c) for c in self.__cpus])
-
-        # Sort the list of cpus to align with the order reported by cyclictest
-        self.__cpus.sort(key=int)
-
+        self.__cpulist = self.__cfg.cpulist
+        self.__cpus = [str(c) for c in cpulist_utils.expand_cpulist(self.__cpulist)]
         self.__numcores = len(self.__cpus)
 
         info = cpuinfo()
@@ -241,10 +210,7 @@ class Cyclictest(rtevalModulePrototype):
                                               logfnc=self._log)
         self.__cyclicdata['system'].description = (f"({self.__numcores} cores) ") + info['0']['model name']
 
-        if self.__sparse:
-            self._log(Log.DEBUG, f"system using {self.__numcores} cpu cores")
-        else:
-            self._log(Log.DEBUG, f"system has {self.__numcores} cpu cores")
+        self._log(Log.DEBUG, f"system using {self.__numcores} cpu cores")
         self.__started = False
         self.__cyclicoutput = None
         self.__breaktraceval = None
@@ -279,12 +245,8 @@ class Cyclictest(rtevalModulePrototype):
                       f'-h {self.__buckets}',
                       f"-p{int(self.__priority)}",
                       ]
-        if self.__sparse:
-            self.__cmd.append(f'-t{self.__numcores}')
-            self.__cmd.append(f'-a{self.__cpulist}')
-        else:
-            self.__cmd.append('-t')
-            self.__cmd.append('-a')
+        self.__cmd.append(f'-t{self.__numcores}')
+        self.__cmd.append(f'-a{self.__cpulist}')
 
         if 'threads' in self.__cfg and self.__cfg.threads:
             self.__cmd.append(f"-t{int(self.__cfg.threads)}")
diff --git a/rteval/systopology.py b/rteval/systopology.py
index 9db1a80..40ac5ab 100644
--- a/rteval/systopology.py
+++ b/rteval/systopology.py
@@ -241,6 +241,39 @@ class SysTopology:
         """ return a list of online cpus in cpulist """
         return [c for c in self.online_cpus() if c in cpulist]
 
+
+def parse_cpulist_from_config(cpulist, run_on_isolcpus=False):
+    """
+    Generates a cpulist based on --*-cpulist argument given by user
+    :param cpulist: Value of --*-cpulist argument
+    :param run_on_isolcpus: Value of --*-run-on-isolcpus argument
+    :return: Sorted list of CPUs as integers
+    """
+    if cpulist and not cpulist_utils.is_relative(cpulist):
+        result = cpulist_utils.expand_cpulist(cpulist)
+        # Only include online cpus
+        result = cpulist_utils.online_cpulist(result)
+    else:
+        result = SysTopology().online_cpus()
+        # Get the cpuset from the environment
+        cpuset = os.sched_getaffinity(0)
+        # Get isolated CPU list
+        isolcpus = SysTopology().isolated_cpus()
+        if cpulist and cpulist_utils.is_relative(cpulist):
+            # Include cpus that are not removed in relative cpuset and are either in cpuset from affinity,
+            # isolcpus (with run_on_isolcpus enabled, or added by relative cpuset
+            added_cpus, removed_cpus = cpulist_utils.expand_relative_cpulist(cpulist)
+            result = [c for c in result
+                      if (c in cpuset or
+                          c in added_cpus or
+                          run_on_isolcpus and c in isolcpus) and
+                      c not in removed_cpus]
+        else:
+            # Only include cpus that are in the cpuset and isolated CPUs if run_on_isolcpus is enabled
+            result = [c for c in result if c in cpuset or run_on_isolcpus and c in isolcpus]
+    return result
+
+
 if __name__ == "__main__":
 
     def unit_test():
-- 
2.39.3

Re: [PATCH 4/4] rteval: Add relative cpulists for measurements

[John Kacur]

On Wed, 29 Nov 2023, tglozar@redhat.com wrote:

> From: Tomas Glozar <tglozar@redhat.com>
> 
> Instead of specifying an absolute list of CPUs to run measurements on
> in --measurement-cpulist, implement an option to specify a relative list
> with respect to the current cpuset of rteval.
> 
> The relative cpulist can include CPUs both for addition and for removal,
> e.g. +0,1,-7,8.
> 
> Also move the logic for processing cpulists specified by the user as
> a string into cpulists usable by rteval to a single function.
> 
> Signed-off-by: Tomas Glozar <tglozar@redhat.com>
> ---
>  rteval-cmd                               | 26 +++++++-----
>  rteval/cpulist_utils.py                  | 33 ++++++++++++++++
>  rteval/modules/measurement/__init__.py   |  9 +----
>  rteval/modules/measurement/cyclictest.py | 50 +++---------------------
>  rteval/systopology.py                    | 33 ++++++++++++++++
>  5 files changed, 90 insertions(+), 61 deletions(-)
> 
> diff --git a/rteval-cmd b/rteval-cmd
> index 56b2c95..dd7d645 100755
> --- a/rteval-cmd
> +++ b/rteval-cmd
> @@ -30,7 +30,7 @@ from rteval import RtEval, rtevalConfig
>  from rteval.modules.loads import LoadModules
>  from rteval.modules.measurement import MeasurementModules
>  from rteval.version import RTEVAL_VERSION
> -from rteval.systopology import SysTopology
> +from rteval.systopology import SysTopology, parse_cpulist_from_config
>  from rteval.modules.loads.kcompile import ModuleParameters
>  import rteval.cpulist_utils as cpulist_utils
>  
> @@ -336,26 +336,32 @@ if __name__ == '__main__':
>  
>          ldcfg = config.GetSection('loads')
>          msrcfg = config.GetSection('measurement')
> -        if ldcfg.cpulist and msrcfg.cpulist:
> +        msrcfg_cpulist_present = msrcfg.cpulist != ""
> +        # Parse measurement cpulist using parse_cpulist_from_config to account for run-on-isolcpus
> +        # and relative cpusets
> +        cpulist = parse_cpulist_from_config(msrcfg.cpulist, msrcfg.run_on_isolcpus)
> +        if msrcfg_cpulist_present and not cpulist_utils.is_relative(msrcfg.cpulist) and msrcfg.run_on_isolcpus:
> +            logger.log(Log.WARN, "ignoring --measurement-run-on-isolcpus, since cpulist is specified")
> +        msrcfg.cpulist = cpulist_utils.collapse_cpulist(cpulist)
> +        if ldcfg.cpulist:
>              ldcfg.cpulist = remove_offline(ldcfg.cpulist)
> -            msrcfg.cpulist = remove_offline(msrcfg.cpulist)
>          # if we only specified one set of cpus (loads or measurement)
>          # default the other to the inverse of the specified list
> -        if not ldcfg.cpulist and msrcfg.cpulist:
> +        if not ldcfg.cpulist and msrcfg_cpulist_present:
>              tmplist = cpulist_utils.expand_cpulist(msrcfg.cpulist)
>              tmplist = SysTopology().invert_cpulist(tmplist)
> -            ldcfg.cpulist = cpulist_utils.compress_cpulist(tmplist)
> -            msrcfg.cpulist = remove_offline(msrcfg.cpulist)
> -        if not msrcfg.cpulist and ldcfg.cpulist:
> +            tmplist = cpulist_utils.online_cpulist(tmplist)
> +            ldcfg.cpulist = cpulist_utils.collapse_cpulist(tmplist)
> +        if not msrcfg_cpulist_present and ldcfg.cpulist:
>              tmplist = cpulist_utils.expand_cpulist(ldcfg.cpulist)
>              tmplist = SysTopology().invert_cpulist(tmplist)
> -            msrcfg.cpulist = cpulist_utils.compress_cpulist(tmplist)
> -            ldcfg.cpulist = remove_offline(ldcfg.cpulist)
> +            tmplist = cpulist_utils.online_cpulist(tmplist)
> +            msrcfg.cpulist = cpulist_utils.collapse_cpulist(tmplist)
>  
>          if ldcfg.cpulist:
>              logger.log(Log.DEBUG, f"loads cpulist: {ldcfg.cpulist}")
>          # if --onlyload is specified msrcfg.cpulist is unused
> -        if msrcfg.cpulist and not rtevcfg.onlyload:
> +        if msrcfg_cpulist_present and not rtevcfg.onlyload:
>              logger.log(Log.DEBUG, f"measurement cpulist: {msrcfg.cpulist}")
>          logger.log(Log.DEBUG, f"workdir: {rtevcfg.workdir}")
>  
> diff --git a/rteval/cpulist_utils.py b/rteval/cpulist_utils.py
> index 402d579..7abc45a 100644
> --- a/rteval/cpulist_utils.py
> +++ b/rteval/cpulist_utils.py
> @@ -126,3 +126,36 @@ def nonisolated_cpulist(cpulist):
>      isolated_cpulist = sysread(cpupath, "isolated")
>      isolated_cpulist = expand_cpulist(isolated_cpulist)
>      return list(set(cpulist).difference(set(isolated_cpulist)))
> +
> +
> +def is_relative(cpulist):

a docstring here would be nice

> +    return cpulist.startswith("+") or cpulist.startswith("-")
> +
> +
> +def expand_relative_cpulist(cpulist):
> +    """
> +    Expand a relative cpulist into a tuple of lists.
> +    :param cpulist: Relative cpulist of form +1,2,3,-4,5,6
> +    :return: Tuple of two lists, one for added CPUs, one for removed CPUs
> +    """
> +    added_cpus = []
> +    removed_cpus = []
> +
> +    if not cpulist:
> +        return added_cpus, removed_cpus
> +
> +    cpus = None
> +
> +    for part in cpulist.split(','):
> +        if part.startswith('+') or part.startswith('-'):
> +            cpus = added_cpus if part[0] == '+' else removed_cpus
> +            part = part[1:]
> +        if '-' in part:
> +            a, b = part.split('-')
> +            a, b = int(a), int(b)
> +            cpus.extend(list(range(a, b + 1)))
> +        else:
> +            a = int(part)
> +            cpus.append(a)
> +
> +    return list(set(added_cpus)), list(set(removed_cpus))
> diff --git a/rteval/modules/measurement/__init__.py b/rteval/modules/measurement/__init__.py
> index 66dc9c5..11bd7b0 100644
> --- a/rteval/modules/measurement/__init__.py
> +++ b/rteval/modules/measurement/__init__.py
> @@ -5,7 +5,7 @@
>  
>  import libxml2
>  from rteval.modules import RtEvalModules, ModuleContainer
> -from rteval.systopology import SysTopology as SysTop
> +from rteval.systopology import parse_cpulist_from_config
>  import rteval.cpulist_utils as cpulist_utils
>  
>  class MeasurementProfile(RtEvalModules):
> @@ -183,12 +183,7 @@ measurement profiles, based on their characteristics"""
>          rep_n = libxml2.newNode("Measurements")
>          cpulist = self.__cfg.GetSection("measurement").cpulist
>          run_on_isolcpus = self.__cfg.GetSection("measurement").run_on_isolcpus
> -        if cpulist:
> -            # Convert str to list and remove offline cpus
> -            cpulist = cpulist_utils.expand_cpulist(cpulist)
> -            cpulist = cpulist_utils.online_cpulist(cpulist)
> -        else:
> -            cpulist = SysTop().online_cpus() if run_on_isolcpus else SysTop().default_cpus()
> +        cpulist = parse_cpulist_from_config(cpulist, run_on_isolcpus)
>          rep_n.newProp("measurecpus", cpulist_utils.collapse_cpulist(cpulist))
>  
>          for mp in self.__measureprofiles:
> diff --git a/rteval/modules/measurement/cyclictest.py b/rteval/modules/measurement/cyclictest.py
> index dcfca0b..87fa9d8 100644
> --- a/rteval/modules/measurement/cyclictest.py
> +++ b/rteval/modules/measurement/cyclictest.py
> @@ -16,8 +16,7 @@ import math
>  import libxml2
>  from rteval.Log import Log
>  from rteval.modules import rtevalModulePrototype
> -from rteval.systopology import cpuinfo
> -from rteval.systopology import SysTopology
> +from rteval.systopology import cpuinfo, parse_cpulist_from_config
>  import rteval.cpulist_utils as cpulist_utils
>  
>  
> @@ -192,39 +191,9 @@ class Cyclictest(rtevalModulePrototype):
>          self.__priority = int(self.__cfg.setdefault('priority', 95))
>          self.__buckets = int(self.__cfg.setdefault('buckets', 2000))
>          self.__numcores = 0
> -        self.__cpus = []
>          self.__cyclicdata = {}
> -        self.__sparse = False
> -        self.__run_on_isolcpus = bool(self.__cfg.setdefault('run-on-isolcpus', False))
> -
> -        if self.__cfg.cpulist:
> -            self.__cpulist = self.__cfg.cpulist
> -            self.__cpus = cpulist_utils.expand_cpulist(self.__cpulist)
> -            # Only include online cpus
> -            self.__cpus = cpulist_utils.online_cpulist(self.__cpus)
> -            # Reset cpulist from the newly calculated self.__cpus
> -            self.__cpulist = cpulist_utils.collapse_cpulist(self.__cpus)
> -            self.__cpus = [str(c) for c in self.__cpus]
> -            self.__sparse = True
> -            if self.__run_on_isolcpus:
> -                self._log(Log.WARN, "ignoring --measurement-run-on-isolcpus, since cpulist is specified")
> -        else:
> -            self.__cpus = SysTopology().online_cpus_str()
> -            # Get the cpuset from the environment
> -            cpuset = os.sched_getaffinity(0)
> -            # Convert the elements to strings
> -            cpuset = [str(c) for c in cpuset]
> -            # Get isolated CPU list
> -            isolcpus = [str(c) for c in SysTopology().isolated_cpus()]
> -            # Only include cpus that are in the cpuset and isolated CPUs if run_on_isolcpus is enabled
> -            self.__cpus = [c for c in self.__cpus if c in cpuset or self.__run_on_isolcpus and c in isolcpus]
> -            if self.__run_on_isolcpus:
> -                self.__sparse = True
> -                self.__cpulist = cpulist_utils.collapse_cpulist([int(c) for c in self.__cpus])
> -
> -        # Sort the list of cpus to align with the order reported by cyclictest
> -        self.__cpus.sort(key=int)
> -
> +        self.__cpulist = self.__cfg.cpulist
> +        self.__cpus = [str(c) for c in cpulist_utils.expand_cpulist(self.__cpulist)]
>          self.__numcores = len(self.__cpus)
>  
>          info = cpuinfo()
> @@ -241,10 +210,7 @@ class Cyclictest(rtevalModulePrototype):
>                                                logfnc=self._log)
>          self.__cyclicdata['system'].description = (f"({self.__numcores} cores) ") + info['0']['model name']
>  
> -        if self.__sparse:
> -            self._log(Log.DEBUG, f"system using {self.__numcores} cpu cores")
> -        else:
> -            self._log(Log.DEBUG, f"system has {self.__numcores} cpu cores")
> +        self._log(Log.DEBUG, f"system using {self.__numcores} cpu cores")
>          self.__started = False
>          self.__cyclicoutput = None
>          self.__breaktraceval = None
> @@ -279,12 +245,8 @@ class Cyclictest(rtevalModulePrototype):
>                        f'-h {self.__buckets}',
>                        f"-p{int(self.__priority)}",
>                        ]
> -        if self.__sparse:
> -            self.__cmd.append(f'-t{self.__numcores}')
> -            self.__cmd.append(f'-a{self.__cpulist}')
> -        else:
> -            self.__cmd.append('-t')
> -            self.__cmd.append('-a')
> +        self.__cmd.append(f'-t{self.__numcores}')
> +        self.__cmd.append(f'-a{self.__cpulist}')
>  
>          if 'threads' in self.__cfg and self.__cfg.threads:
>              self.__cmd.append(f"-t{int(self.__cfg.threads)}")
> diff --git a/rteval/systopology.py b/rteval/systopology.py
> index 9db1a80..40ac5ab 100644
> --- a/rteval/systopology.py
> +++ b/rteval/systopology.py
> @@ -241,6 +241,39 @@ class SysTopology:
>          """ return a list of online cpus in cpulist """
>          return [c for c in self.online_cpus() if c in cpulist]
>  
> +
> +def parse_cpulist_from_config(cpulist, run_on_isolcpus=False):
> +    """
> +    Generates a cpulist based on --*-cpulist argument given by user
> +    :param cpulist: Value of --*-cpulist argument
> +    :param run_on_isolcpus: Value of --*-run-on-isolcpus argument
> +    :return: Sorted list of CPUs as integers
> +    """
> +    if cpulist and not cpulist_utils.is_relative(cpulist):
> +        result = cpulist_utils.expand_cpulist(cpulist)
> +        # Only include online cpus
> +        result = cpulist_utils.online_cpulist(result)
> +    else:
> +        result = SysTopology().online_cpus()
> +        # Get the cpuset from the environment
> +        cpuset = os.sched_getaffinity(0)
> +        # Get isolated CPU list
> +        isolcpus = SysTopology().isolated_cpus()
> +        if cpulist and cpulist_utils.is_relative(cpulist):
> +            # Include cpus that are not removed in relative cpuset and are either in cpuset from affinity,
> +            # isolcpus (with run_on_isolcpus enabled, or added by relative cpuset
> +            added_cpus, removed_cpus = cpulist_utils.expand_relative_cpulist(cpulist)
> +            result = [c for c in result
> +                      if (c in cpuset or
> +                          c in added_cpus or
> +                          run_on_isolcpus and c in isolcpus) and
> +                      c not in removed_cpus]
> +        else:
> +            # Only include cpus that are in the cpuset and isolated CPUs if run_on_isolcpus is enabled
> +            result = [c for c in result if c in cpuset or run_on_isolcpus and c in isolcpus]
> +    return result
> +
> +
>  if __name__ == "__main__":
>  
>      def unit_test():
> -- 
> 2.39.3
> 
> 
> 

This is the big pay off! I like this, just if you make changes to patch 3 
you might need to modify a few things here, about ready to sign-off on it.

John