[PATCH 0/3] rteval: Offline NUMA node bugfix

[PATCH 0/3] rteval: Offline NUMA node bugfix

[Valentin Schneider]

Hi folks,

I wanted to try out rteval on a NUMA system with one node offlined and
got a few exceptions, here's my take on them. There's also a small
cleanup as an added bonus.

Cheers,
Valentin

Valentin Schneider (3):
  rteval: systopology: Fix offline NUMA node parsing
  rteval: kcompile: Fix offline node handling
  rteval: systopology: Slight CpuList.__expand_cpulist() cleanup

 rteval/modules/loads/kcompile.py |  5 ++++-
 rteval/systopology.py            | 15 +++++++++------
 2 files changed, 13 insertions(+), 7 deletions(-)

--
2.27.0

[PATCH 1/3] rteval: systopology: Fix offline NUMA node parsing

[Valentin Schneider]

An offline NUMA node will report in its cpulist an empty
string. Unfortunately, "".split(sep=x) with x != None returns a list
containing an empty string rather than an empty list, which causes
CpuList._expand_cpulist() to try to run int(''), which ends up in the
following exception:

  ValueError: invalid literal for int() with base 10: ''

Prevent this by adding an early empty-string check.

Signed-off-by: Valentin Schneider <vschneid@redhat.com>
---
 rteval/systopology.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/rteval/systopology.py b/rteval/systopology.py
index bf794ce..b2da7bb 100644
--- a/rteval/systopology.py
+++ b/rteval/systopology.py
@@ -103,6 +103,10 @@ class CpuList:
         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('-')
-- 
2.27.0

[PATCH 2/3] rteval: kcompile: Fix offline node handling

[Valentin Schneider]

Having an empty NumaNode but with CPUs attached to it (IOW they are all
offline) causes kcompile.py to raise the following exception:

  calc_jobs_per_cpu():
      ratio = float(mem) / float(len(self.node))
  ZeroDivisionError: float division by zero

Remove nodes that do have CPUs but none of which are online.

Signed-off-by: Valentin Schneider <vschneid@redhat.com>
---
 rteval/modules/loads/kcompile.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/rteval/modules/loads/kcompile.py b/rteval/modules/loads/kcompile.py
index 367f8dc..ac99964 100644
--- a/rteval/modules/loads/kcompile.py
+++ b/rteval/modules/loads/kcompile.py
@@ -211,7 +211,10 @@ class Kcompile(CommandLineLoad):
 
         # remove nodes with no cpus available for running
         for node, cpus in self.cpus.items():
-            if not cpus:
+            # If the intersection between the node CPUs and the cpulist is empty
+            # then either the cpulist exludes that node, or the CPUs allowed by
+            # the cpulist are actually offline
+            if not set(self.topology.nodes[node].cpus.cpulist) & set(cpus):
                 self.nodes.remove(node)
                 self._log(Log.DEBUG, "node %s has no available cpus, removing" % node)
 
-- 
2.27.0

[PATCH 3/3] rteval: systopology: Slight CpuList.__expand_cpulist() cleanup

[Valentin Schneider]

This method currently aggregates CPUs into a list, then converts this to
set and then back to list. The aggregation can instead be done directly
into a set.

(as an offside, it would make more sense for CpuList to have its storage be
a set in the first place as duplicate CPU ids don't make sense for it, but
that's a separate discussion :-))

The integer conversion of the "a-b" pattern can also be condensed into a
single map() expression.

Signed-off-by: Valentin Schneider <vschneid@redhat.com>
---
 rteval/systopology.py | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/rteval/systopology.py b/rteval/systopology.py
index b2da7bb..2a28f9c 100644
--- a/rteval/systopology.py
+++ b/rteval/systopology.py
@@ -102,20 +102,19 @@ class CpuList:
         """ expand a range string into an array of cpu numbers
         don't error check against online cpus
         """
-        result = []
-
         if not cpulist:
-            return result
+            return []
+
+        result = set()
 
         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)))
+                a, b = map(int, part.split('-'))
+                result |= set(range(a, b + 1))
             else:
                 a = int(part)
-                result.append(a)
-        return [int(i) for i in list(set(result))]
+                result |= {a}
+        return list(result)
 
     def getcpulist(self):
         """ return the list of cpus tracked """
-- 
2.27.0

Re: [PATCH 1/3] rteval: systopology: Fix offline NUMA node parsing

[John Kacur]

On Tue, 19 Apr 2022, Valentin Schneider wrote:

> An offline NUMA node will report in its cpulist an empty
> string. Unfortunately, "".split(sep=x) with x != None returns a list
> containing an empty string rather than an empty list, which causes
> CpuList._expand_cpulist() to try to run int(''), which ends up in the
> following exception:
> 
>   ValueError: invalid literal for int() with base 10: ''
> 
> Prevent this by adding an early empty-string check.
> 
> Signed-off-by: Valentin Schneider <vschneid@redhat.com>
> ---
>  rteval/systopology.py | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/rteval/systopology.py b/rteval/systopology.py
> index bf794ce..b2da7bb 100644
> --- a/rteval/systopology.py
> +++ b/rteval/systopology.py
> @@ -103,6 +103,10 @@ class CpuList:
>          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('-')
> -- 
> 2.27.0
> 
> 
Signed-off-by: John Kacur <jkacur@redhat.com>

Re: [PATCH 2/3] rteval: kcompile: Fix offline node handling

[John Kacur]

On Tue, 19 Apr 2022, Valentin Schneider wrote:

> Having an empty NumaNode but with CPUs attached to it (IOW they are all
> offline) causes kcompile.py to raise the following exception:
> 
>   calc_jobs_per_cpu():
>       ratio = float(mem) / float(len(self.node))
>   ZeroDivisionError: float division by zero
> 
> Remove nodes that do have CPUs but none of which are online.
> 
> Signed-off-by: Valentin Schneider <vschneid@redhat.com>
> ---
>  rteval/modules/loads/kcompile.py | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/rteval/modules/loads/kcompile.py b/rteval/modules/loads/kcompile.py
> index 367f8dc..ac99964 100644
> --- a/rteval/modules/loads/kcompile.py
> +++ b/rteval/modules/loads/kcompile.py
> @@ -211,7 +211,10 @@ class Kcompile(CommandLineLoad):
>  
>          # remove nodes with no cpus available for running
>          for node, cpus in self.cpus.items():
> -            if not cpus:
> +            # If the intersection between the node CPUs and the cpulist is empty
> +            # then either the cpulist exludes that node, or the CPUs allowed by
> +            # the cpulist are actually offline
> +            if not set(self.topology.nodes[node].cpus.cpulist) & set(cpus):
>                  self.nodes.remove(node)
>                  self._log(Log.DEBUG, "node %s has no available cpus, removing" % node)
>  
> -- 
> 2.27.0
> 
> 

Sorry, this isn't quite right.

The cpulist in kcompile is the list of cpus where the load modules will 
run. The user can specify it like this
--loads-cpulist=LIST

If the user does not specify a list (because they want it to run 
everywhere) then the cpulist is empty. Your patch was working for you 
because the cpulist was empty, but that has nothing to do with whether the 
cpu is online or not.

systopology will fetch a list of cpus and consider whether they are online 
or not. So, I think the solution is to delete the method in kcompile and 
just use the one in systopology.

Sending another mail with the patch.

Thanks

John Kacur

Re: [PATCH 3/3] rteval: systopology: Slight CpuList.__expand_cpulist() cleanup

[John Kacur]

On Tue, 19 Apr 2022, Valentin Schneider wrote:

> This method currently aggregates CPUs into a list, then converts this to
> set and then back to list. The aggregation can instead be done directly
> into a set.
> 
> (as an offside, it would make more sense for CpuList to have its storage be
> a set in the first place as duplicate CPU ids don't make sense for it, but
> that's a separate discussion :-))
> 
> The integer conversion of the "a-b" pattern can also be condensed into a
> single map() expression.
> 
> Signed-off-by: Valentin Schneider <vschneid@redhat.com>
> ---
>  rteval/systopology.py | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/rteval/systopology.py b/rteval/systopology.py
> index b2da7bb..2a28f9c 100644
> --- a/rteval/systopology.py
> +++ b/rteval/systopology.py
> @@ -102,20 +102,19 @@ class CpuList:
>          """ expand a range string into an array of cpu numbers
>          don't error check against online cpus
>          """
> -        result = []
> -
>          if not cpulist:
> -            return result
> +            return []
> +
> +        result = set()
>  
>          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)))
> +                a, b = map(int, part.split('-'))
> +                result |= set(range(a, b + 1))
>              else:
>                  a = int(part)
> -                result.append(a)
> -        return [int(i) for i in list(set(result))]
> +                result |= {a}
> +        return list(result)
>  
>      def getcpulist(self):
>          """ return the list of cpus tracked """
> -- 
> 2.27.0
> 
> 

I'm guessing that the reason for the "set" was to remove any potential 
duplicates. Duplicates are not normally a problem in rteval, but if you 
want to ensure you handle any user input correctly, it makes sense to do 
that. I think your code is correct, but I'm not sure if it buys us 
anything.

John

Re: [PATCH 3/3] rteval: systopology: Slight CpuList.__expand_cpulist() cleanup

[Valentin Schneider]

On 29/04/22 16:53, John Kacur wrote:
> On Tue, 19 Apr 2022, Valentin Schneider wrote:
>
>> This method currently aggregates CPUs into a list, then converts this to
>> set and then back to list. The aggregation can instead be done directly
>> into a set.
>>
>> (as an offside, it would make more sense for CpuList to have its storage be
>> a set in the first place as duplicate CPU ids don't make sense for it, but
>> that's a separate discussion :-))
>>
>> The integer conversion of the "a-b" pattern can also be condensed into a
>> single map() expression.
>>
>> Signed-off-by: Valentin Schneider <vschneid@redhat.com>
>> ---
>>  rteval/systopology.py | 15 +++++++--------
>>  1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/rteval/systopology.py b/rteval/systopology.py
>> index b2da7bb..2a28f9c 100644
>> --- a/rteval/systopology.py
>> +++ b/rteval/systopology.py
>> @@ -102,20 +102,19 @@ class CpuList:
>>          """ expand a range string into an array of cpu numbers
>>          don't error check against online cpus
>>          """
>> -        result = []
>> -
>>          if not cpulist:
>> -            return result
>> +            return []
>> +
>> +        result = set()
>>
>>          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)))
>> +                a, b = map(int, part.split('-'))
>> +                result |= set(range(a, b + 1))
>>              else:
>>                  a = int(part)
>> -                result.append(a)
>> -        return [int(i) for i in list(set(result))]
>> +                result |= {a}
>> +        return list(result)
>>
>>      def getcpulist(self):
>>          """ return the list of cpus tracked """
>> --
>> 2.27.0
>>
>>
>
> I'm guessing that the reason for the "set" was to remove any potential
> duplicates. Duplicates are not normally a problem in rteval, but if you
> want to ensure you handle any user input correctly, it makes sense to do
> that. I think your code is correct, but I'm not sure if it buys us
> anything.
>

It doesn't really, it should be functionaly identical to the existing code
- it's just "drive-by cleanup" really :)

> John