On Fri, 4 Apr 2025, Crystal Wood wrote:
> 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.
People seem to be interested in testing rt with some power savings
enabled, so we're trying to make it easier for people to do so with
the tools they are used to using. I'm not really open to debating this.
>
> > 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
:) how about all the tools for manipulating the environment that I think
are important.
>
> 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
All I meant is that self.__cpu_list is not the same as cpu_list.
Since the names are confusingly similar, if a different variable really
is needed, it would be best to rename the one local to the method /
function.
> 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?
No, there is no play to json anything in tuna. Look, agreed, the string
is messy, we'll fix it, I'm just saying he probably had something else
in mind when he coded it.
>
> 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.
rteval doesn't have anything to do with this, the original coders were
completely different people. However, rteval has stood the test of time.
I don't like the multiple layers of inheritance, but sometimes I find
suprising benefits to the original design. Anyway, you can discuss that
with me separately, that doesn't have anything to do with this.
What we are talking about here, is a single file that wraps the code in a
class, this is fine, it often results in cleaner code even when object
oriented features aren't used a lot.
Your suggestions are fine, now, give him some time to implement them.
John Kacur
