Re: [PATCH 0/2] Add cpupower idle-state functionality

[Crystal Wood <crwood@redhat.com>]

On Mon, 2025-02-10 at 14:50 -0500, John Kacur wrote:
> 
> On Mon, 27 Jan 2025, John B. Wyatt IV wrote:
> 
> > This patch series adds idle-state functionality to control cpu power
> > usage and to test idle states.
> > 
> > The number of cpus was needed in the cpupower file; I extracted out the
> > previously local to tuna-cli.py functionality to a separate file so the
> > cpu code can be used in any file in Tuna and reduce duplications. The
> > nics code was similar so it was also extracted to reduce the number of
> > global variables.
> > 
> > Sincerely,
> > John Wyatt
> > Software Engineer, Core Kernel
> > Red Hat
> > 
> > John B. Wyatt IV (2):
> >   tuna: extract cpu and nics determination code into a utils.py file
> >   tuna: Add idle-state control functionality
> > 
> >  tuna-cmd.py      |  67 +++++++++-------
> >  tuna/cpupower.py | 202 +++++++++++++++++++++++++++++++++++++++++++++++
> >  tuna/utils.py    |  27 +++++++
> >  3 files changed, 267 insertions(+), 29 deletions(-)
> >  create mode 100755 tuna/cpupower.py
> >  create mode 100644 tuna/utils.py
> > 
> > -- 
> > 2.48.1
> > 
> > 
> > 
> ./tuna-cmd.py idle-set -h
> usage: tuna-cmd.py idle-set [-h] [-c CPU-LIST]
>                             (-s IDLESTATEDISABLEDSTATUS | -i | -d 
> IDLESTATEINDEX | -e IDLESTATEINDEX)
> 
> Query and set all idle states on a given CPU list. Requires libcpupower to 
> be
> installed
> 
> options:
>   -h, --help            show this help message and exit
>   -c CPU-LIST, --cpus CPU-LIST
>                         CPU-LIST affected by commands
>   -s IDLESTATEDISABLEDSTATUS, --status IDLESTATEDISABLEDSTATUS
>                         Print if cpu idle state of the cpus in CPU-LIST is
>                         enabled or disabled. If CPU-LIST is not specified,
>                         default to all cpus.
>   -i, --idle-info       Print general idle information on cpus in 
> CPU-LIST. If
>                         CPU-LIST is not specified, default to all cpus.
>   -d IDLESTATEINDEX, --disable IDLESTATEINDEX
>                         Disable cpus in CPU-LIST's cpu idle (cpu sleep 
> state).
>                         If CPU-LIST is not specified, default to all cpus.
>   -e IDLESTATEINDEX, --enable IDLESTATEINDEX
>                         Enable cpus in CPU-LIST's cpu idle (cpu sleep 
> state).
>                         If CPU-LIST is not specified, default to all cpus.
> 
> These names are kind of awkward, isn't IDLESTATE good enough, why 
> IDLESTATEINDEX?
> 
> For the -s option, why do we need to put the IDLESTATEDISABLEDSTATUS, if 
> we omit that can't we just get a result like running "cpupower idle-info"?
> Could you rename IDLESTATEDISABLEDSTATUS to just IDLESTATE_STATUS?

This was confusing to me too, but it looks like the argument is actually
a particular idle state, not the status of that state.  So it should be
"IDLESTATE" just like -d and -e.  Or better, "IDLE-STATE" like "CPU-
LIST".

-Crystal