[PATCH] rteval: Fix loads cpulist restriction

[PATCH] rteval: Fix loads cpulist restriction

[Valentin Schneider]

A recent batch of commits, one of them being:

  39115f0a826d ("rteval: Make use of systopology instead of misc in hackbench")

has made the loads modules use CpuList.expand_cpulist() (which produces a
list(int)) instead of misc.expand_cpulist() (which produces a list(str)).
However, the bits handling restricting CPU affinity based on a user
argument still expects to handle a list(str), which results in:

  [DEBUG] [kcompile] node 0 has no available cpus, removing
  [...]
  [DEBUG] [hackbench] node 0 has no available cpus, removing

Remove the leftover string casts.
Cyclictest is unaffected.

Signed-off-by: Valentin Schneider <vschneid@redhat.com>
---
Staring at this made me think that we might be better served by a single
Cpumask type backed by a set() and armed with string-returning methods - I
find it difficult to figure for a given CPU container if it's supposed to
be int or str-backed.

This opens up simplifying shrinking operations into e.g.:

  self.cpus[n] = self.cpus & self.cpulist

How much hate would I get for following up with something like that?
---
 rteval/modules/loads/hackbench.py | 2 +-
 rteval/modules/loads/kcompile.py  | 2 +-
 rteval/modules/loads/stressng.py  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/rteval/modules/loads/hackbench.py b/rteval/modules/loads/hackbench.py
index 538f60f..14e60d1 100644
--- a/rteval/modules/loads/hackbench.py
+++ b/rteval/modules/loads/hackbench.py
@@ -76,7 +76,7 @@ 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 str(c) in expand_cpulist(self.cpulist)]
+                self.cpus[n] = [c for c in self.cpus[n] if c in expand_cpulist(self.cpulist)]
 
             # track largest number of cpus used on a node
             node_biggest = len(sysTop.getcpus(int(n)))
diff --git a/rteval/modules/loads/kcompile.py b/rteval/modules/loads/kcompile.py
index a544fd9..8de00cf 100644
--- a/rteval/modules/loads/kcompile.py
+++ b/rteval/modules/loads/kcompile.py
@@ -235,7 +235,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 str(c) in expand_cpulist(self.cpulist)]
+                self.cpus[n] = [c for c in self.cpus[n] if c in expand_cpulist(self.cpulist)]
 
         # remove nodes with no cpus available for running
         for node, cpus in self.cpus.items():
diff --git a/rteval/modules/loads/stressng.py b/rteval/modules/loads/stressng.py
index 287f4e2..85cb473 100644
--- a/rteval/modules/loads/stressng.py
+++ b/rteval/modules/loads/stressng.py
@@ -68,7 +68,7 @@ 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 str(c) in expand_cpulist(self.cpulist)]
+                cpus[n] = [c for c in cpus[n] if c in expand_cpulist(self.cpulist)]
 
         # remove nodes with no cpus available for running
         for node, cpu in cpus.items():
-- 
2.31.1

Re: [PATCH] rteval: Fix loads cpulist restriction

[Valentin Schneider]

On 05/08/22 14:42, Valentin Schneider wrote:
> A recent batch of commits, one of them being:
>
>   39115f0a826d ("rteval: Make use of systopology instead of misc in hackbench")
>
> has made the loads modules use CpuList.expand_cpulist() (which produces a
> list(int)) instead of misc.expand_cpulist() (which produces a list(str)).
> However, the bits handling restricting CPU affinity based on a user
> argument still expects to handle a list(str), which results in:
>
>   [DEBUG] [kcompile] node 0 has no available cpus, removing
>   [...]
>   [DEBUG] [hackbench] node 0 has no available cpus, removing
>

This is lacking some context, so here's more:

This was triggered on an arm64 system (Ampere eMAG), any sort of affinity
restriction suffices, e.g.

 $ rteval -O -D -v --loads-cpulist=2-3

I can reproduce that on my x86 laptop:

  $ sudo ./rteval-cmd -O -D -v --loads-cpulist=2-3
  [DEBUG] [kcompile] systopology: 1 node system (8 cores per node)
  [DEBUG] [kcompile] node 0 has no available cpus, removing
  [DEBUG] [kcompile] node 0 has no available cpus, removing
  [DEBUG] [hackbench] node 0 has no available cpus, removing

Re: [PATCH] rteval: Fix loads cpulist restriction

[Clark Williams]

I think the recent slew of commit is John trying to clean up the mess
that David and I made, by removing duplicate functions.

I'm not against using a set instead of a list in the cpulist
structure, although at that point we might want to call it a cpuset
:).

On Fri, Aug 5, 2022 at 2:43 PM Valentin Schneider <vschneid@redhat.com> wrote:
>
> On 05/08/22 14:42, Valentin Schneider wrote:
> > A recent batch of commits, one of them being:
> >
> >   39115f0a826d ("rteval: Make use of systopology instead of misc in hackbench")
> >
> > has made the loads modules use CpuList.expand_cpulist() (which produces a
> > list(int)) instead of misc.expand_cpulist() (which produces a list(str)).
> > However, the bits handling restricting CPU affinity based on a user
> > argument still expects to handle a list(str), which results in:
> >
> >   [DEBUG] [kcompile] node 0 has no available cpus, removing
> >   [...]
> >   [DEBUG] [hackbench] node 0 has no available cpus, removing
> >
>
> This is lacking some context, so here's more:
>
> This was triggered on an arm64 system (Ampere eMAG), any sort of affinity
> restriction suffices, e.g.
>
>  $ rteval -O -D -v --loads-cpulist=2-3
>
> I can reproduce that on my x86 laptop:
>
>   $ sudo ./rteval-cmd -O -D -v --loads-cpulist=2-3
>   [DEBUG] [kcompile] systopology: 1 node system (8 cores per node)
>   [DEBUG] [kcompile] node 0 has no available cpus, removing
>   [DEBUG] [kcompile] node 0 has no available cpus, removing
>   [DEBUG] [hackbench] node 0 has no available cpus, removing
>

Re: [PATCH] rteval: Fix loads cpulist restriction

[John Kacur]

On Fri, 5 Aug 2022, Valentin Schneider wrote:

> On 05/08/22 14:42, Valentin Schneider wrote:
> > A recent batch of commits, one of them being:
> >
> >   39115f0a826d ("rteval: Make use of systopology instead of misc in hackbench")
> >
> > has made the loads modules use CpuList.expand_cpulist() (which produces a
> > list(int)) instead of misc.expand_cpulist() (which produces a list(str)).
> > However, the bits handling restricting CPU affinity based on a user
> > argument still expects to handle a list(str), which results in:
> >
> >   [DEBUG] [kcompile] node 0 has no available cpus, removing
> >   [...]
> >   [DEBUG] [hackbench] node 0 has no available cpus, removing
> >
> 
> This is lacking some context, so here's more:
> 
> This was triggered on an arm64 system (Ampere eMAG), any sort of affinity
> restriction suffices, e.g.
> 
>  $ rteval -O -D -v --loads-cpulist=2-3
> 
> I can reproduce that on my x86 laptop:
> 
>   $ sudo ./rteval-cmd -O -D -v --loads-cpulist=2-3
>   [DEBUG] [kcompile] systopology: 1 node system (8 cores per node)
>   [DEBUG] [kcompile] node 0 has no available cpus, removing
>   [DEBUG] [kcompile] node 0 has no available cpus, removing
>   [DEBUG] [hackbench] node 0 has no available cpus, removing
> 
> 
Thanks
Signed-off-by: John Kacur <jkacur@redhat.com>

re python sets for cpu (cpuset is an overloaded term)
The idea isn't bad it could work.

Right now I am trying reduce the number of duplicated interfaces.
Even a seemingly simple change to use lists of ints instead of lists of 
strings of ints can uncover problems. So, I probbaly would not be so open 
to python sets for cpus at the moment, but could be in the future.

It is probably more work than you realize though, and I would require a 
full solution, which means replacing the uses of the current interfaces in 
cyclictest (measurement module), kcompile, hackbench, stressng. You could 
create a personal fork and have a go at it though if you like!

John

Re: [PATCH] rteval: Fix loads cpulist restriction

[Valentin Schneider]

On 05/08/22 13:04, John Kacur wrote:
> On Fri, 5 Aug 2022, Valentin Schneider wrote:
>
>> On 05/08/22 14:42, Valentin Schneider wrote:
>> > A recent batch of commits, one of them being:
>> >
>> >   39115f0a826d ("rteval: Make use of systopology instead of misc in hackbench")
>> >
>> > has made the loads modules use CpuList.expand_cpulist() (which produces a
>> > list(int)) instead of misc.expand_cpulist() (which produces a list(str)).
>> > However, the bits handling restricting CPU affinity based on a user
>> > argument still expects to handle a list(str), which results in:
>> >
>> >   [DEBUG] [kcompile] node 0 has no available cpus, removing
>> >   [...]
>> >   [DEBUG] [hackbench] node 0 has no available cpus, removing
>> >
>>
>> This is lacking some context, so here's more:
>>
>> This was triggered on an arm64 system (Ampere eMAG), any sort of affinity
>> restriction suffices, e.g.
>>
>>  $ rteval -O -D -v --loads-cpulist=2-3
>>
>> I can reproduce that on my x86 laptop:
>>
>>   $ sudo ./rteval-cmd -O -D -v --loads-cpulist=2-3
>>   [DEBUG] [kcompile] systopology: 1 node system (8 cores per node)
>>   [DEBUG] [kcompile] node 0 has no available cpus, removing
>>   [DEBUG] [kcompile] node 0 has no available cpus, removing
>>   [DEBUG] [hackbench] node 0 has no available cpus, removing
>>
>>
> Thanks
> Signed-off-by: John Kacur <jkacur@redhat.com>
>
> re python sets for cpu (cpuset is an overloaded term)
> The idea isn't bad it could work.
>
> Right now I am trying reduce the number of duplicated interfaces.
> Even a seemingly simple change to use lists of ints instead of lists of
> strings of ints can uncover problems. So, I probbaly would not be so open
> to python sets for cpus at the moment, but could be in the future.
>
> It is probably more work than you realize though, and I would require a
> full solution, which means replacing the uses of the current interfaces in
> cyclictest (measurement module), kcompile, hackbench, stressng. You could
> create a personal fork and have a go at it though if you like!
>

I think I'll tinker with a new class and figure out what features it would
need, and then I'll sit on it and push it further down my todolist - like
you said, the primary ingredient here is gonna be time :)

Re: [PATCH] rteval: Fix loads cpulist restriction

[John Kacur]

On Fri, 5 Aug 2022, Valentin Schneider wrote:

> On 05/08/22 13:04, John Kacur wrote:
> > On Fri, 5 Aug 2022, Valentin Schneider wrote:
> >
> >> On 05/08/22 14:42, Valentin Schneider wrote:
> >> > A recent batch of commits, one of them being:
> >> >
> >> >   39115f0a826d ("rteval: Make use of systopology instead of misc in hackbench")
> >> >
> >> > has made the loads modules use CpuList.expand_cpulist() (which produces a
> >> > list(int)) instead of misc.expand_cpulist() (which produces a list(str)).
> >> > However, the bits handling restricting CPU affinity based on a user
> >> > argument still expects to handle a list(str), which results in:
> >> >
> >> >   [DEBUG] [kcompile] node 0 has no available cpus, removing
> >> >   [...]
> >> >   [DEBUG] [hackbench] node 0 has no available cpus, removing
> >> >
> >>
> >> This is lacking some context, so here's more:
> >>
> >> This was triggered on an arm64 system (Ampere eMAG), any sort of affinity
> >> restriction suffices, e.g.
> >>
> >>  $ rteval -O -D -v --loads-cpulist=2-3
> >>
> >> I can reproduce that on my x86 laptop:
> >>
> >>   $ sudo ./rteval-cmd -O -D -v --loads-cpulist=2-3
> >>   [DEBUG] [kcompile] systopology: 1 node system (8 cores per node)
> >>   [DEBUG] [kcompile] node 0 has no available cpus, removing
> >>   [DEBUG] [kcompile] node 0 has no available cpus, removing
> >>   [DEBUG] [hackbench] node 0 has no available cpus, removing
> >>
> >>
> > Thanks
> > Signed-off-by: John Kacur <jkacur@redhat.com>
> >
> > re python sets for cpu (cpuset is an overloaded term)
> > The idea isn't bad it could work.
> >
> > Right now I am trying reduce the number of duplicated interfaces.
> > Even a seemingly simple change to use lists of ints instead of lists of
> > strings of ints can uncover problems. So, I probbaly would not be so open
> > to python sets for cpus at the moment, but could be in the future.
> >
> > It is probably more work than you realize though, and I would require a
> > full solution, which means replacing the uses of the current interfaces in
> > cyclictest (measurement module), kcompile, hackbench, stressng. You could
> > create a personal fork and have a go at it though if you like!
> >
> 
> I think I'll tinker with a new class and figure out what features it would
> need, and then I'll sit on it and push it further down my todolist - like
> you said, the primary ingredient here is gonna be time :)
> 
> 
Also, CpuList was really just designed as a helper class to SysTopology 
and not really mean as a general interface. I do have plans to separate 
some of that functionality out into a more generic interace we can use.
It might make more sense at that time, to start thinking about whether 
sets would work better or not. So, like anything open source, you're 
welcome to play with it, but you might want to chat with me a little if 
you're serious about it, we could maybe hash out a coherent plan.

John