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

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(-)

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
> 
> 
>

ú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

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>