On Fri, 2025-04-04 at 16:36 -0400, John Kacur wrote:
>
> On Thu, 13 Mar 2025, Crystal Wood wrote:
>
> > 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:
> > > > + "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/it\'s/its/
> >
> > 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.
>
> I agree that idle_set is not the ideal name since if you look at the
> cpupower tool, this is just one function, maybe cpupower would be a
> better name. It's a bit unfair to say we're reimplementing the tool.
> The functionality is in upstream kernel code and John Wyatt created
> python bindings for that, and that is what he is using here.
But (as of now) this isn't using the python bindings to implement
something higher-level; it's exposing them as a command line tool that
has very similar functionality to the existing one. I'm not sure what
being written in Python has to do with it, other than a motive for
reimplementing, to make future features easier.
> I think it is better to have all the tools for manipulating the
> environment through one tool and not make the user go search for
> multiple tools to get the job done.
"all the tools for manipulating the environment" is an extremely broad
mission... :-P
So far it doesn't seem to align very well with the rest of what tuna
does, which is managing processes/threads... is there a plan to tie
them together so that you can have it set idle states based on what the
tasks running on a given cpu need, and/or move tasks around based on
that?
> > 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?
>
> This is a different cpu_list than the instantiated one.
> This is which cpus the user wants the info from. I think
> the name is confusing here.
The only call (so far...) is literally "self.print_idle_info(self.__cpu_list)".
Where are you seeing a different cpu list? When (even with future
patches) would the instantiated cpu list be anything other than what
the user asked for, and does that say anything about whether the class
design makes sense?
> > > > + 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?
>
> valid, but I think John Wyatt was thinking the string could be
> useful for other purposes.
The json stuff I assume? Is this even in the right format for that?
Is there any plan to json-ize other tuna functionality?
It's really hard to review patches when there are vague or unstated
notions of "this could be useful in the future"... especially when
recent experience with rteval has highlighted the importance of
avoiding overengineering.
-Crystal
