| Message ID | 20250128014551.15058-1-jwyatt@redhat.com |
|---|---|
| Headers | show |
| Series | Add cpupower idle-state functionality | expand |
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? Thanks John Kacur
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
On Wed, Feb 12, 2025 at 02:53:35PM -0600, Crystal Wood wrote: > On Mon, 2025-02-10 at 14:50 -0500, John Kacur wrote: > > 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 > Apologies for not answering you earlier John. Did not see this email until Crystal's email. If there is no objections I will go with Crystal's suggestion. Here is the current printout for the patch series I will send (note I changed a few items to address confusion). Please let me know if this addresses everyone's concerns: options: -h, --help show this help message and exit -i, --idle-info Print general idle information on cpus in CPU-LIST. If CPU-LIST is not specified, default to all cpus. -s IDLE-STATE, --status IDLE-STATE Print the idle-state (cpu sleep state) of the cpus in CPU-LIST as enabled or disabled. The argument is the index of the idle-state of the cpu as reported by -i. If CPU-LIST is not specified, default to all cpus. -d IDLE-STATE, --disable IDLE-STATE Disable cpus in CPU-LIST's idle-state (cpu sleep state). The argument is the index of the idle-state of the cpu as reported by -i. If CPU-LIST is not specified, default to all cpus. -e IDLE-STATE, --enable IDLE-STATE Enable cpus in CPU-LIST's idle-state (cpu sleep state). The argument is the index of the idle-state of the cpu as reported by -i. If CPU-LIST is not specified, default to all cpus. -c CPU-LIST, --cpus CPU-LIST CPU-LIST affected by commands
On Wed, 12 Feb 2025, John B. Wyatt IV wrote: > On Wed, Feb 12, 2025 at 02:53:35PM -0600, Crystal Wood wrote: > > On Mon, 2025-02-10 at 14:50 -0500, John Kacur wrote: > > > 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 > > > > Apologies for not answering you earlier John. Did not see this email until > Crystal's email. > > If there is no objections I will go with Crystal's suggestion. No objection, just as long as it is legible, that looks a lot better! John > > Here is the current printout for the patch series I will send (note I changed > a few items to address confusion). Please let me know if this addresses > everyone's concerns: > > options: > -h, --help show this help message and exit > -i, --idle-info Print general idle information on cpus in CPU-LIST. If CPU-LIST is not specified, default to all cpus. > -s IDLE-STATE, --status IDLE-STATE > Print the idle-state (cpu sleep state) of the cpus in CPU-LIST as enabled or disabled. The argument is the index of the idle-state of the cpu as reported by -i. If > CPU-LIST is not specified, default to all cpus. > -d IDLE-STATE, --disable IDLE-STATE > Disable cpus in CPU-LIST's idle-state (cpu sleep state). The argument is the index of the idle-state of the cpu as reported by -i. If CPU-LIST is not specified, > default to all cpus. > -e IDLE-STATE, --enable IDLE-STATE > Enable cpus in CPU-LIST's idle-state (cpu sleep state). The argument is the index of the idle-state of the cpu as reported by -i. If CPU-LIST is not specified, > default to all cpus. > -c CPU-LIST, --cpus CPU-LIST > CPU-LIST affected by commands > > -- > Sincerely, > John Wyatt > Software Engineer, Core Kernel > Red Hat > > >
On Wed, 2025-02-12 at 16:24 -0500, John B. Wyatt IV wrote: > On Wed, Feb 12, 2025 at 02:53:35PM -0600, Crystal Wood wrote: > > On Mon, 2025-02-10 at 14:50 -0500, John Kacur wrote: > > > 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 > > > > Apologies for not answering you earlier John. Did not see this email until > Crystal's email. > > If there is no objections I will go with Crystal's suggestion. > > Here is the current printout for the patch series I will send (note I changed > a few items to address confusion). Please let me know if this addresses > everyone's concerns: > > options: > -h, --help show this help message and exit > -i, --idle-info Print general idle information on cpus in CPU-LIST. If CPU-LIST is not specified, default to all cpus. > -s IDLE-STATE, --status IDLE-STATE > Print the idle-state (cpu sleep state) of the cpus in CPU-LIST as enabled or disabled. The argument is the index of the idle-state of the cpu as reported by -i. If > CPU-LIST is not specified, default to all cpus. > -d IDLE-STATE, --disable IDLE-STATE > Disable cpus in CPU-LIST's idle-state (cpu sleep state). The argument is the index of the idle-state of the cpu as reported by -i. If CPU-LIST is not specified, > default to all cpus. > -e IDLE-STATE, --enable IDLE-STATE > Enable cpus in CPU-LIST's idle-state (cpu sleep state). The argument is the index of the idle-state of the cpu as reported by -i. If CPU-LIST is not specified, > default to all cpus. We could simplify the description as: -i, --idle-info Print general idle information for the selected CPUs, including index values for IDLE-STATE. -s IDLE-STATE, --status IDLE-STATE Print whether IDLE-STATE is enabled on the selected CPUs. -d IDLE-STATE, --disable IDLE-STATE Disable IDLE-STATE on the selected CPUs -e IDLE-STATE, --enable IDLE-STATE Enable IDLE-STATE on the selected CPUs -c CPU-LIST, --cpus CPU-LIST Specify the list of CPUs to act on. If omitted, act on all CPUs. -Crystal