On Wed, 2025-03-12 at 11:41 -0300, Wander Lairson Costa wrote:
> On Tue, Mar 11, 2025 at 04:30:19PM -0400, John B. Wyatt IV wrote:
> > Allows Tuna to control cpu idle-state functionality on the system,
> > including querying, enabling, disabling of cpu idle-states to control
> > power usage or to test functionality.
> >
> > This requires cpupower, a utility in the Linux kernel repository and
> > the cpupower Python bindings added in Linux 6.12 to control cpu
> > idle-states.
> >
> > This patch revision includes text snippet suggestions by Crystal Wood
> > (v2) and small Python suggestions by John Kacur (v3 and v4).
> >
> > Suggested-by: Crystal Wood <crwood@xxxxxxxxxx>
Eh, this tag makes it look like I suggested the overall idea, not that I
gave feedback on the patch...
> > @@ -115,8 +116,12 @@ def gen_parser():
> > "disable_perf": dict(action='store_true', help="Explicitly disable usage of perf in GUI for process view"),
> > "refresh": dict(default=2500, metavar='MSEC', type=int, help="Refresh the GUI every MSEC milliseconds"),
> > "priority": dict(default=(None, None), metavar="POLICY:RTPRIO", type=tuna.get_policy_and_rtprio, help="Set thread scheduler tunables: POLICY and RTPRIO"),
> > - "background": dict(action='store_true', help="Run command as background task")
> > - }
> > + "background": dict(action='store_true', help="Run command as background task"),
> > + "idle_info": dict(dest='idle_info', action='store_const', const=True, help='Print general idle information for the selected CPUs, including index values for IDLE-STATE.'),
> > + "idle_state_disabled_status": dict(dest='idle_state_disabled_status', metavar='IDLE-STATE', type=int, help='Print whether IDLE-STATE is enabled on the selected CPUs.'),
You have a tab between "selected" and "CPUs".
> > + "disable_idle_state": dict(dest='disable_idle_state', metavar='IDLE-STATE', type=int, help='Disable IDLE-STATE on the selected CPUs.'),
> > + "enable_idle_state": dict(dest='enable_idle_state', metavar='IDLE-STATE', type=int, help='Enable IDLE-STATE on the selected CPUs.')
> > + }
> >
> > parser = HelpMessageParser(description="tuna - Application Tuning Program")
> >
> > @@ -127,6 +132,9 @@ def gen_parser():
> >
> > subparser = parser.add_subparsers(dest='command')
> >
> > + idle_set = subparser.add_parser('idle_set',
> > + description='Manage CPU idle state disabling (requires libcpupower and it\'s Python bindings)',
> > + help='Set all idle states on a given CPU-LIST.')
s/Set all idle states/Manage idle states/
I still don't like the "idle_set" name. This does more than setting,
unlike the standalone utility that we're apparently reimplementing.
> > diff --git a/tuna/cpupower.py b/tuna/cpupower.py
> > new file mode 100755
> > index 0000000..7913027
> > --- /dev/null
> > +++ b/tuna/cpupower.py
> > @@ -0,0 +1,184 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +# Copyright (C) 2024 John B. Wyatt IV
> > +
> > +from typing import List
> > +import tuna.utils as utils
> > +
> > +cpupower_required_kernel = "6.12"
> > +have_cpupower = None
> > +
> > +try:
> > + import raw_pylibcpupower as lcpw
> > + lcpw.cpufreq_get_available_frequencies(0)
> > + have_cpupower = True
> > +except:
>
> Catch all statements in Python are bad IMO, because they catch syntax
> errors as well. We should at least print the exception.
The (expected) exception is for the case where the package is not
installed and thus we disable that feature. We don't want to print
anything in that case (unless the user tries to use these features).
But yeah, being more specific in the catch makes sense.
> > + def __init__(self, cpu_list=None):
> > + if cpu_list == None or cpu_list == []: # args.cpu_list == [] when the -c option is not used
>
> Nit: use the "is" operator to compare against None.
Or just use "if cpu_list:" like the existing code does.
That's also pretty obnoxious behavior of the code that sets
args.cpu_list...
> > + @staticmethod
> > + def print_idle_info(cpu_list=[0]):
This default is never used. cpu_list is only ever set to
self.__cpu_list. Why is this a static method?
> > + for cpu in cpu_list:
> > + idle_info = Cpupower.get_idle_info(cpu)
> > + print_str = f"""
> > +CPUidle driver: {idle_info["CPUidle-driver"]}
> > +CPUidle governor: {idle_info["CPUidle-governor"]}
> > +analyzing CPU {cpu}
> > +
> > +Number of idle states: {idle_info["idle-states-count"]}
> > +Available idle states: {idle_info["available-idle-states"]}
> > +"""
> > + for state in idle_info["cpu-states"]:
> > + print_str += f"""{state["Idle State Name"]}
> > +Flags/Description: {state["Flags/Description"]}
> > +Latency: {state["Latency"]}
> > +Usage: {state["Usage"]}
> > +Duration: {state["Duration"]}
> > +"""
> > + print(
> > + print_str
> > + )
>
> nit: I don't think we need three lines for this statement.
Why not just print each state inside the loop, instead of building one
big string?
> > + elif args.enable_idle_state != None:
>
> nit: Use the "is" operator to compare against None.
>
> > + cstate_index = args.enable_idle_state
> > + cstate_list, cstate_amt = self.get_idle_states(self.__cpu_list[0]) # Assuming all cpus have the same idle state
> > + if cstate_index < 0 or cstate_index >= cstate_amt:
> > + print(f"Invalid idle state range. Total for this cpu is {cstate_amt}")
>
> nit: Use the "is" operator to compare against None.
>
> > + return 1
> > + cstate_name = cstate_list[cstate_index]
> > + ret = self.disable_idle_state(cstate_index, 0)
> > + for e in ret:
> > + self.handle_common_lcpw_errors(e, self.LCPW_ERROR_THREE_CASE, cstate_name)
Why isn't self.disable_idle_state() doing the error handling (which is
still duplicated between enable and disable)? And as I mentioned
before, isn't libcpupower itself doing the same range checking?
It looks like the get_idle_states() call is just for that redundant
check, and to print the name in an error... You could wait until you
actually get an error, and then just call lcpw.cpuidle_state_name on
the particular cpu and state id. That would get rid of the assumption
about all cpus having the same idle states, and be simpler.
-Crystal
