| Message ID | 20240818194216.299176-2-jkacur@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Changes to supply a customer kernel | expand |
On Sun, 2024-08-18 at 15:42 -0400, John Kacur wrote: > Commit 56c7cf63942d rteval: Allow arguments specific to module group > > intended to allow group options to the overall measurement modules without > applying to the load modules. It inadvertently disabled options for most > load modules such as hackbench and kcompile. How did that happen and how does this fix it? (not saying it doesn't, just that clarifying that would help with reviewing and not introducing something similar in the future) > > This patch reworks the overall group mechanism a little bit to restore > these menus. Menus? > + # Set up options for measurement modules only > + if self.__modtype == 'measurement': > + grparser.add_argument(f'--{self.__modtype}-run-on-isolcpus', > + dest = > f'{self.__modtype}___run_on_isolcpus', > + action = "store_true", > + default = > config.GetSection("measurement").setdefault("run-on-isolcpus", > "false").lower() == "true", > + help = "Include isolated CPUs in > default cpulist") You already know it's measurement in this section, so why {self.__modtype}? Why does this need to be here rather than in rteval/modules/measurement.py? > for (modname, mod) in list(self.__modsloaded.items()): > opts = mod.ModuleParameters() > if len(opts) == 0: > @@ -296,7 +310,7 @@ reference from the first import""" > # Ignore if a section is not found > cfg = None > > - modgrparser = parser.add_argument_group(f"Options for the > {shortmod} module") > + grparser = parser.add_argument_group(f"Options for the > {shortmod} module") > for (o, s) in list(opts.items()): > descr = 'descr' in s and s['descr'] or "" > metavar = 'metavar' in s and s['metavar'] or None > @@ -311,7 +325,7 @@ reference from the first import""" > default = 'default' in s and s['default'] or None > > > - modgrparser.add_argument(f'--{shortmod}-{o}', > + grparser.add_argument(f'--{shortmod}-{o}', > dest=f"{shortmod}___{o}", > action='store', > help='%s%s' % (descr, > @@ -319,8 +333,6 @@ reference from the first import""" > default=default, > metavar=metavar) Wouldn't "modparser" be more accurate, since this isn't for the entire group? Or we could reuse the mechanism for both group and module level (maybe s/ModuleParameters/Parameters/) if it's not introducing more complexity than it eliminates. > diff --git a/rteval/modules/measurement/__init__.py > b/rteval/modules/measurement/__init__.py > index 9314d1cb6bbc..44708ce0b035 100644 > --- a/rteval/modules/measurement/__init__.py > +++ b/rteval/modules/measurement/__init__.py > @@ -29,17 +29,7 @@ class MeasurementModules(RtEvalModules): > > def SetupModuleOptions(self, parser): > "Sets up all the measurement modules' parameters for the option > parser" > - grparser = super().SetupModuleOptions(parser) > - > - # Set up options specific for measurement module group > - grparser.add_argument("--measurement-run-on-isolcpus", > - dest="measurement___run_on_isolcpus", > - action="store_true", > - > default=self._cfg.GetSection("measurement").setdefault("run-on-isolcpus", > "false").lower() > - == "true", > - help="Include isolated CPUs in default > cpulist") > - grparser.add_argument('--idle-set', > dest='measurement___idlestate', metavar='IDLESTATE', > - default=None, help='Idle state depth to set on > cpus running measurement modules') > + super().SetupModuleOptions(parser) Why is this method still here if all it does is call super? -Crystal
diff --git a/rteval/modules/__init__.py b/rteval/modules/__init__.py index acd6330788e2..eb29db86ce7a 100644 --- a/rteval/modules/__init__.py +++ b/rteval/modules/__init__.py @@ -280,10 +280,24 @@ reference from the first import""" grparser = parser.add_argument_group(f"Group Options for {self.__modtype} modules") grparser.add_argument(f'--{self.__modtype}-cpulist', - dest=f'{self.__modtype}___cpulist', action='store', default="", + dest=f'{self.__modtype}___cpulist', action='store', + default="", help=f'CPU list where {self.__modtype} modules will run', metavar='CPULIST') + # Set up options for measurement modules only + if self.__modtype == 'measurement': + grparser.add_argument(f'--{self.__modtype}-run-on-isolcpus', + dest = f'{self.__modtype}___run_on_isolcpus', + action = "store_true", + default = config.GetSection("measurement").setdefault("run-on-isolcpus", "false").lower() == "true", + help = "Include isolated CPUs in default cpulist") + grparser.add_argument('--idle-set', + dest='measurement___idlestate', + metavar='IDLESTATE', + default=None, + help='Idle state depth to set on cpus running measurement modules') + for (modname, mod) in list(self.__modsloaded.items()): opts = mod.ModuleParameters() if len(opts) == 0: @@ -296,7 +310,7 @@ reference from the first import""" # Ignore if a section is not found cfg = None - modgrparser = parser.add_argument_group(f"Options for the {shortmod} module") + grparser = parser.add_argument_group(f"Options for the {shortmod} module") for (o, s) in list(opts.items()): descr = 'descr' in s and s['descr'] or "" metavar = 'metavar' in s and s['metavar'] or None @@ -311,7 +325,7 @@ reference from the first import""" default = 'default' in s and s['default'] or None - modgrparser.add_argument(f'--{shortmod}-{o}', + grparser.add_argument(f'--{shortmod}-{o}', dest=f"{shortmod}___{o}", action='store', help='%s%s' % (descr, @@ -319,8 +333,6 @@ reference from the first import""" default=default, metavar=metavar) - return grparser - def InstantiateModule(self, modname, modcfg, modroot=None): """Imports a module and instantiates an object from the modules create() function. diff --git a/rteval/modules/measurement/__init__.py b/rteval/modules/measurement/__init__.py index 9314d1cb6bbc..44708ce0b035 100644 --- a/rteval/modules/measurement/__init__.py +++ b/rteval/modules/measurement/__init__.py @@ -29,17 +29,7 @@ class MeasurementModules(RtEvalModules): def SetupModuleOptions(self, parser): "Sets up all the measurement modules' parameters for the option parser" - grparser = super().SetupModuleOptions(parser) - - # Set up options specific for measurement module group - grparser.add_argument("--measurement-run-on-isolcpus", - dest="measurement___run_on_isolcpus", - action="store_true", - default=self._cfg.GetSection("measurement").setdefault("run-on-isolcpus", "false").lower() - == "true", - help="Include isolated CPUs in default cpulist") - grparser.add_argument('--idle-set', dest='measurement___idlestate', metavar='IDLESTATE', - default=None, help='Idle state depth to set on cpus running measurement modules') + super().SetupModuleOptions(parser) def Setup(self, modparams):
Commit 56c7cf63942d rteval: Allow arguments specific to module group intended to allow group options to the overall measurement modules without applying to the load modules. It inadvertently disabled options for most load modules such as hackbench and kcompile. This patch reworks the overall group mechanism a little bit to restore these menus. Signed-off-by: John Kacur <jkacur@redhat.com> --- rteval/modules/__init__.py | 22 +++++++++++++++++----- rteval/modules/measurement/__init__.py | 12 +----------- 2 files changed, 18 insertions(+), 16 deletions(-)