diff mbox series

[v2,2/3] rteval: Minor improvements to CpuList class

Message ID 20231123072436.270267-3-tglozar@redhat.com
State Superseded
Headers show
Series None | expand

Commit Message

Tomas Glozar Nov. 23, 2023, 7:24 a.m. UTC
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 | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

Comments

John Kacur Nov. 24, 2023, 3:27 p.m. UTC | #1
On Fri, 24 Nov 2023, Tomas Glozar wrote:

> čt 23. 11. 2023 v 17:21 odesílatel John Kacur <jkacur@redhat.com> napsal:
> > The reason we check cpu1 is that on some hardware where you can't turn off
> > cpu0, the online file also doesn't exist even though it is available for
> > other cpus. We only want to learn whether we can use it in general or not.
> >
> > For example on my laptop
> >
> > [jkacur@fionn rteval]$ ls /sys/devices/system/cpu/cpu0/online
> > ls: cannot access '/sys/devices/system/cpu/cpu0/online': No such file or
> > directory
> > [jkacur@fionn rteval]$ ls /sys/devices/system/cpu/cpu1/online
> > /sys/devices/system/cpu/cpu1/online
> >
> > John
> >
> 
> Oh I've never encountered that, thanks for pointing that out, that's
> interesting. I'd say we should check for cpu0 || cpu1 then.
> 
Or just check cpu1 like we already do! 

John
Tomas Glozar Nov. 24, 2023, 4:51 p.m. UTC | #2
pá 24. 11. 2023 v 16:27 odesílatel John Kacur <jkacur@redhat.com> napsal:
> Or just check cpu1 like we already do!
>
> John

That won't work on machines that have only one CPU though.
gene heskett Nov. 24, 2023, 9:38 p.m. UTC | #3
On 11/24/23 11:52, Tomas Glozar wrote:
> pá 24. 11. 2023 v 16:27 odesílatel John Kacur <jkacur@redhat.com> napsal:
>> Or just check cpu1 like we already do!
>>
>> John
> 
> That won't work on machines that have only one CPU though.

Does that flavor actually exist? 4 seems to be pretty std now.

> 
> 
> .

Cheers, Gene Heskett.
Tomas Glozar Nov. 27, 2023, 7:52 a.m. UTC | #4
pá 24. 11. 2023 v 21:06 odesílatel John Kacur <jkacur@redhat.com> napsal:
>
> Sure it will, because if a machine only has one CPU we assume that it is
> online. :)
>

You are right, I thought I had an error when I run rteval in QEMU with
only one CPU (which I set for testing some unrelated issue originally)
but looking at it now I cannot reproduce it, maybe I remember it
wrong.

Thanks for review.

Tomas
diff mbox series

Patch

diff --git a/rteval/systopology.py b/rteval/systopology.py
index ea8e242..97d3dc4 100644
--- a/rteval/systopology.py
+++ b/rteval/systopology.py
@@ -82,12 +82,14 @@  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
+        return os.path.exists(os.path.join(CpuList.cpupath, "cpu0/online"))
 
     @staticmethod
     def isolated_file_exists():
@@ -147,14 +149,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 +160,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