On Wed, 29 Nov 2023, tglozar@xxxxxxxxxx wrote:
> From: Tomas Glozar <tglozar@xxxxxxxxxx>
>
> - 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@xxxxxxxxxx>
> ---
> 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
>
>
>
